fix: fix repeater child object crash without parent #2822
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: caixr23 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Reviewer's GuideThis PR adds a compile-time memory management flag to control C++ ownership and safe deletion, introduces a DccQuickRepeater to auto-assign parent relationships for QML repeater items, replaces standard Repeaters in QML modules, and refines object ownership and cleanup in DccObject and DccManager to prevent crashes when children lack parents. Sequence diagram for DccQuickRepeater item addition and parent assignmentsequenceDiagram
participant QML as QML Engine
participant DccQuickRepeater
participant QQuickItem
QML->>DccQuickRepeater: Add new item to repeater
DccQuickRepeater->>QQuickItem: onItemAdded(index, item)
alt Item has no parent
DccQuickRepeater->>QQuickItem: setParent(DccQuickRepeater)
end
ER diagram for DccObject child-parent relationships and ownershiperDiagram
DCCOBJECT {
id int
parent_id int
ownership enum
}
DCCOBJECT ||--o{ DCCOBJECT : has_children
DCCOBJECT }o--|| DCCOBJECT : has_parent
Class diagram for new DccQuickRepeater and related ownership changesclassDiagram
class DccQuickRepeater {
+DccQuickRepeater(QQuickItem *parent)
+~DccQuickRepeater()
+onItemAdded(int index, QQuickItem *item)
}
DccQuickRepeater --|> QQuickRepeater
class DccObject {
+addChild(DccObject *child, bool updateParent)
+getSectionItem(QObject *parent)
+deleteSectionItem(bool later)
}
DccObject o-- "1..*" DccObject : children
class DccManager {
+clearData()
-QPointer<QWindow> m_window
}
class QPointer_QWindow
DccManager o-- QPointer_QWindow : m_window
QQuickRepeater <|-- DccQuickRepeater
QQuickItem <|-- DccQuickRepeater
DccObject "1" o-- "1..*" DccObject : children
DccManager o-- DccObject : m_root
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/dde-control-center/frame/plugin/src/dccqmlplugin.cpp:25` </location>
<code_context>
qmlRegisterType<dccV25::DccModel>(uri, 1, 0, "DccModel");
qmlRegisterType<dccV25::DccQuickDBusInterface>(uri, 1, 0, "DccDBusInterface");
- QQmlEngine::setObjectOwnership(dccV25::DccApp::instance(),QQmlEngine::CppOwnership);
+ qmlRegisterType<dccV25::DccQuickRepeater>(uri, 1, 0, "Repeater");
+ QQmlEngine::setObjectOwnership(dccV25::DccApp::instance(), QQmlEngine::CppOwnership);
qmlRegisterSingletonInstance(uri, 1, 0, "DccApp", dccV25::DccApp::instance());
</code_context>
<issue_to_address>
**issue:** Registering DccQuickRepeater as 'Repeater' may shadow QtQuick's Repeater.
Consider renaming DccQuickRepeater to avoid potential conflicts or confusion with QtQuick's built-in Repeater type in QML.
</issue_to_address>
### Comment 2
<location> `src/dde-control-center/frame/dccquickrepeater.cpp:17-18` </location>
<code_context>
+
+void DccQuickRepeater::onItemAdded(int, QQuickItem *item)
+{
+ if (!item->parent()) {
+ item->setParent(this);
+ }
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Setting parent in onItemAdded may interfere with QML's item hierarchy.
Check that manually setting the parent does not disrupt QML's automatic handling, which could affect rendering or event flow.
</issue_to_address>
### Comment 3
<location> `CMakeLists.txt:23` </location>
<code_context>
option(BUILD_TESTING "UNIT test" OFF)
set(BUILD_EXAMPLES OFF)
+set(DCC_MEMORY_MANAGEME ON)
# dcc-v23
set(BUILD_DCC_OLD OFF)
</code_context>
<issue_to_address>
**issue (typo):** Typo in variable name 'DCC_MEMORY_MANAGEME'; should likely be 'DCC_MEMORY_MANAGEMENT'.
Please update the variable name to 'DCC_MEMORY_MANAGEMENT' for consistency.
Suggested implementation:
```
set(DCC_MEMORY_MANAGEMENT ON)
```
```
if(DCC_MEMORY_MANAGEMENT)
add_definitions(-DDCC_MEMORY_MANAGEMENT)
endif()
```
</issue_to_address>
### Comment 4
<location> `CMakeLists.txt:105` </location>
<code_context>
add_definitions(-DLOCALE_I18N_PATH="${LOCALE_I18N_PATH}")
+if(DCC_MEMORY_MANAGEME)
+ add_definitions(-DDCC_MEMORY_MANAGEME)
+endif()
</code_context>
<issue_to_address>
**issue (typo):** Typo in conditional variable 'DCC_MEMORY_MANAGEME'; should likely be 'DCC_MEMORY_MANAGEMENT'.
Please correct all instances of the typo to 'DCC_MEMORY_MANAGEMENT'.
Suggested implementation:
```
set(DCC_MEMORY_MANAGEMENT ON)
```
```
if(DCC_MEMORY_MANAGEMENT)
add_definitions(-DDCC_MEMORY_MANAGEMENT)
endif()
```
</issue_to_address>
### Comment 5
<location> `CMakeLists.txt:106` </location>
<code_context>
add_definitions(-DLOCALE_I18N_PATH="${LOCALE_I18N_PATH}")
+if(DCC_MEMORY_MANAGEME)
+ add_definitions(-DDCC_MEMORY_MANAGEME)
+endif()
# 增加安全编译参数
</code_context>
<issue_to_address>
**issue (typo):** Typo in macro definition '-DDCC_MEMORY_MANAGEME'; should likely be '-DDCC_MEMORY_MANAGEMENT'.
Please update the macro to use the correct spelling to ensure consistency and prevent potential build errors.
</issue_to_address>
### Comment 6
<location> `include/dccquickrepeater.h:10` </location>
<code_context>
+#include "private/qquickrepeater_p.h"
+
+namespace dccV25 {
+class DccQuickRepeater : public QQuickRepeater
+{
+ Q_OBJECT
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the custom C++ subclass with QML-only logic to handle item parenting.
You don’t actually need a C++ subclass just to catch `onItemAdded` and reparent – you can do it all in QML. Removing `DccQuickRepeater` will save you the header/private include and boilerplate. For example, instead of:
```cpp
// DccQuickRepeater.h
class DccQuickRepeater : public QQuickRepeater {
Q_OBJECT
QML_ELEMENT
protected:
void onItemAdded(int index, QQuickItem *item) override {
item->setParentItem(someContainer);
}
};
```
you can use a plain Repeater in QML:
```qml
import QtQuick 2.15
Item {
id: someContainer
Repeater {
id: repeater
model: myModel
delegate: MyDelegate { /* ... */ }
onItemAdded: {
// exactly the same reparent logic
item.parent = someContainer
}
}
}
```
Or even simpler, push the parent into the delegate itself:
```qml
import QtQuick 2.15
Item {
id: someContainer
Repeater {
model: myModel
delegate: MyDelegate {
parent: someContainer
/* ... */
}
}
}
```
Both keep all existing behavior, remove the extra C++ class and private Qt header include, and are easy to maintain.
</issue_to_address>
### Comment 7
<location> `src/dde-control-center/frame/dccquickrepeater.cpp:6` </location>
<code_context>
+
+#include "private/qquickrepeater_p.h"
+
+namespace dccV25 {
+class DccQuickRepeater : public QQuickRepeater
+{
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the C++ subclass with a QML-only implementation to simplify the codebase.
```suggestion
Consider removing the entire C++ subclass and handling `itemAdded` in QML. You can keep the same API surface by providing a lightweight QML wrapper instead of custom C++:
Create DccQuickRepeater.qml:
```qml
// DccQuickRepeater.qml
import QtQuick 2.15
Repeater {
onItemAdded: {
// reparent if delegate forgot to set parent
if (!item.parent) item.parent = parent
}
}
```
– Delete `dccquickrepeater.h`/`.cpp` and its qmlRegisterType.
– In your QML imports, replace all `DccQuickRepeater { … }` usages with this new QML type.
This preserves the exact behavior with zero C++ overhead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
issue: Registering DccQuickRepeater as 'Repeater' may shadow QtQuick's Repeater.
Consider renaming DccQuickRepeater to avoid potential conflicts or confusion with QtQuick's built-in Repeater type in QML.
| option(BUILD_TESTING "UNIT test" OFF) | ||
| set(BUILD_EXAMPLES OFF) | ||
|
|
||
| set(DCC_MEMORY_MANAGEME ON) |
There was a problem hiding this comment.
issue (typo): Typo in variable name 'DCC_MEMORY_MANAGEME'; should likely be 'DCC_MEMORY_MANAGEMENT'.
Please update the variable name to 'DCC_MEMORY_MANAGEMENT' for consistency.
Suggested implementation:
set(DCC_MEMORY_MANAGEMENT ON)
if(DCC_MEMORY_MANAGEMENT)
add_definitions(-DDCC_MEMORY_MANAGEMENT)
endif()
|
|
||
| add_definitions(-DLOCALE_I18N_PATH="${LOCALE_I18N_PATH}") | ||
|
|
||
| if(DCC_MEMORY_MANAGEME) |
There was a problem hiding this comment.
issue (typo): Typo in conditional variable 'DCC_MEMORY_MANAGEME'; should likely be 'DCC_MEMORY_MANAGEMENT'.
Please correct all instances of the typo to 'DCC_MEMORY_MANAGEMENT'.
Suggested implementation:
set(DCC_MEMORY_MANAGEMENT ON)
if(DCC_MEMORY_MANAGEMENT)
add_definitions(-DDCC_MEMORY_MANAGEMENT)
endif()
| add_definitions(-DLOCALE_I18N_PATH="${LOCALE_I18N_PATH}") | ||
|
|
||
| if(DCC_MEMORY_MANAGEME) | ||
| add_definitions(-DDCC_MEMORY_MANAGEME) |
There was a problem hiding this comment.
issue (typo): Typo in macro definition '-DDCC_MEMORY_MANAGEME'; should likely be '-DDCC_MEMORY_MANAGEMENT'.
Please update the macro to use the correct spelling to ensure consistency and prevent potential build errors.
| #include "private/qquickrepeater_p.h" | ||
|
|
||
| namespace dccV25 { | ||
| class DccQuickRepeater : public QQuickRepeater |
There was a problem hiding this comment.
issue (complexity): Consider replacing the custom C++ subclass with QML-only logic to handle item parenting.
You don’t actually need a C++ subclass just to catch onItemAdded and reparent – you can do it all in QML. Removing DccQuickRepeater will save you the header/private include and boilerplate. For example, instead of:
// DccQuickRepeater.h
class DccQuickRepeater : public QQuickRepeater {
Q_OBJECT
QML_ELEMENT
protected:
void onItemAdded(int index, QQuickItem *item) override {
item->setParentItem(someContainer);
}
};you can use a plain Repeater in QML:
import QtQuick 2.15
Item {
id: someContainer
Repeater {
id: repeater
model: myModel
delegate: MyDelegate { /* ... */ }
onItemAdded: {
// exactly the same reparent logic
item.parent = someContainer
}
}
}Or even simpler, push the parent into the delegate itself:
import QtQuick 2.15
Item {
id: someContainer
Repeater {
model: myModel
delegate: MyDelegate {
parent: someContainer
/* ... */
}
}
}Both keep all existing behavior, remove the extra C++ class and private Qt header include, and are easy to maintain.
| // SPDX-License-Identifier: GPL-3.0-or-later | ||
| #include "dccquickrepeater.h" | ||
|
|
||
| namespace dccV25 { |
There was a problem hiding this comment.
issue (complexity): Consider replacing the C++ subclass with a QML-only implementation to simplify the codebase.
Suggested change
| namespace dccV25 { | |
| Consider removing the entire C++ subclass and handling `itemAdded` in QML. You can keep the same API surface by providing a lightweight QML wrapper instead of custom C++: | |
| Create DccQuickRepeater.qml: | |
| ```qml | |
| // DccQuickRepeater.qml | |
| import QtQuick 2.15 | |
| Repeater { | |
| onItemAdded: { | |
| // reparent if delegate forgot to set parent | |
| if (!item.parent) item.parent = parent | |
| } | |
| } |
– Delete dccquickrepeater.h/.cpp and its qmlRegisterType.
– In your QML imports, replace all DccQuickRepeater { … } usages with this new QML type.
This preserves the exact behavior with zero C++ overhead.
Add memory management feature flag DCC_MEMORY_MANAGEME to control object ownership and deletion behavior. Implement DccQuickRepeater to ensure child items have proper parent relationships. Fix potential crashes when repeater child objects lack parent objects by setting ownership and parent relationships correctly. Changes include: 1. Add DCC_MEMORY_MANAGEME compile flag to control memory management features 2. Create DccQuickRepeater class that automatically sets parent for child items 3. Replace standard Repeater with DccQuickRepeater in QML files 4. Improve object ownership management in DccObject child handling 5. Add conditional memory cleanup in DccManager with safety checks Log: Fixed crash issue when repeater child objects have no parent objects Influence: 1. Test repeater functionality in various modules (accounts, datetime, display, etc.) 2. Verify no crashes when dynamically adding/removing repeater items 3. Check memory usage and cleanup during application exit 4. Test object parent relationships in QML components 5. Verify timezone dialog and other repeater-based components work correctly fix: 修复Repeater子对象无父对象导致的崩溃问题 添加内存管理功能标志DCC_MEMORY_MANAGEME来控制对象所有权和删除行为。实现 DccQuickRepeater确保子项具有正确的父子关系。通过正确设置所有权和父子关系 修复当Repeater子对象缺少父对象时可能发生的崩溃问题。 变更包括： 1. 添加DCC_MEMORY_MANAGEME编译标志控制内存管理功能 2. 创建DccQuickRepeater类自动为子项设置父对象 3. 在QML文件中将标准Repeater替换为DccQuickRepeater 4. 改进DccObject子项处理中的对象所有权管理 5. 在DccManager中添加带安全检查的条件内存清理 Log: 修复Repeater子对象无父对象导致的崩溃问题 Influence: 1. 测试各模块中的Repeater功能（账户、时间日期、显示等） 2. 验证动态添加/删除Repeater项时不会崩溃 3. 检查应用程序退出时的内存使用和清理情况 4. 测试QML组件中的对象父子关系 5. 验证时区对话框和其他基于Repeater的组件正常工作 PMS: BUG-307037
deepin pr auto review我来对这段代码进行审查：
建议改进的地方：
// 在 DccQuickRepeater::onItemAdded 中
void DccQuickRepeater::onItemAdded(int, QQuickItem *item)
{
if (!item->parent()) {
// 建议使用 setParentItem 而不是 setParent
item->setParentItem(this);
// 同时设置 QObject 父对象
item->setParent(this);
}
}
// 在 dccmanager.cpp 中
if (m_window) {
delete m_window;
m_window = nullptr; // 添加这行以防止悬空指针
}
// 在 DccObject::getSectionItem 中
QQmlEngine::setObjectOwnership(p_ptr->m_sectionItem, QQmlEngine::JavaScriptOwnership);
// 建议在设置所有权之前检查对象是否有效
if (p_ptr->m_sectionItem) {
QQmlEngine::setObjectOwnership(p_ptr->m_sectionItem, QQmlEngine::JavaScriptOwnership);
}
// 在 DccManager::eventFilter 中
QQuickWindow *w = qobject_cast<QQuickWindow*>(m_window.get()); // 使用 qobject_cast 替代 static_cast
if (!w) {
return false;
}这些改进将使代码更加健壮和安全，同时保持良好的性能。建议在实施这些更改后进行充分的测试，特别是在内存管理和对象生命周期方面。 |
|
TAG Bot New tag: 6.1.59 |
|
TAG Bot New tag: 6.1.60 |
|
TAG Bot New tag: 6.1.61 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add memory management feature flag DCC_MEMORY_MANAGEME to control object ownership and deletion behavior. Implement DccQuickRepeater to ensure child items have proper parent relationships. Fix potential crashes when repeater child objects lack parent objects by setting ownership and parent relationships correctly.
Changes include:
Log: Fixed crash issue when repeater child objects have no parent objects
Influence:
fix: 修复Repeater子对象无父对象导致的崩溃问题
添加内存管理功能标志DCC_MEMORY_MANAGEME来控制对象所有权和删除行为。实现
DccQuickRepeater确保子项具有正确的父子关系。通过正确设置所有权和父子关系
修复当Repeater子对象缺少父对象时可能发生的崩溃问题。
变更包括：
Log: 修复Repeater子对象无父对象导致的崩溃问题
Influence:
PMS: BUG-307037
Summary by Sourcery
Introduce a memory management feature flag and a custom QuickRepeater to enforce parent-child relationships, update ownership and cleanup logic to prevent crashes when repeater items lack parents.
New Features:
Bug Fixes:
Enhancements:
Build: