fix: set parent for NetItem to prevent JS release#526
Merged
18202781743 merged 1 commit intolinuxdeepin:masterfrom Mar 24, 2026
Merged
fix: set parent for NetItem to prevent JS release#52618202781743 merged 1 commit intolinuxdeepin:masterfrom
18202781743 merged 1 commit intolinuxdeepin:masterfrom
Conversation
18202781743
requested review from
BLumia,
fly602,
mhduiy,
robertkill,
wjyrich and
yixinshark
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnsures NetItem instances maintain proper Qt parent-child relationships to prevent premature deletion by JavaScript bindings by setting parents for root/delete items in NetManagerPrivate and for child items in NetItemPrivate::addChild, alongside updating copyright years. Sequence diagram for NetItemPrivate::addChild parent-child setupsequenceDiagram
participant Caller
participant NetItemPrivate_parent as NetItemPrivate_parent
participant NetItem_parent as NetItem_parent
participant NetItemPrivate_child as NetItemPrivate_child
participant NetItem_child as NetItem_child
Caller->>NetItemPrivate_parent: addChild(NetItemPrivate_child, index)
NetItemPrivate_parent->>NetItem_parent: childAboutToBeAdded(NetItem_parent, index)
NetItemPrivate_parent->>NetItemPrivate_child: item()
NetItemPrivate_child-->>NetItemPrivate_parent: NetItem_child
NetItemPrivate_parent->>NetItem_parent: insert child in m_children at index
NetItemPrivate_parent->>NetItem_child: setParent(NetItem_parent)
NetItemPrivate_parent->>NetItemPrivate_child: set m_parent = NetItem_parent
NetItemPrivate_parent->>NetItem_parent: childAdded(NetItem_child)
NetItemPrivate_parent-->>Caller: return true
Class diagram for updated NetManagerPrivate and NetItemPrivate parent relationshipsclassDiagram
class QObject
class NetManagerPrivate {
- bool m_supportWireless
- NetItem* m_root
- NetItem* m_deleteItem
- NetManager* q_ptr
+ NetManagerPrivate(NetManager* manager)
}
class NetItem {
+ void setParent(QObject* parent)
+ QObject* parent()
+ void childAboutToBeAdded(NetItem* parent, int index)
+ void childAdded(NetItem* child)
}
class NetItemPrivate {
- NetItem* m_item
- NetItem* m_parent
- vector~NetItem*~ m_children
+ bool addChild(NetItemPrivate* child, int index)
+ NetItem* item()
}
NetItemPrivate --> NetItem : owns m_item
NetItemPrivate --> NetItem : m_parent
NetItemPrivate --> NetItem : m_children
NetManagerPrivate --> NetItem : m_root
NetManagerPrivate --> NetItem : m_deleteItem
QObject <|-- NetItem
QObject <|-- NetManagerPrivate
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 - I've found 1 issue, and left some high level feedback:
- In
NetManagerPrivate's constructor, double-check thatthisis the correctQObjectowner form_root->item()andm_deleteItem->item()—if the publicNetManager(q_ptr) is the QObject, you may want to parent these items toq_ptrinstead to keep ownership consistent with the public API. - In
NetItemPrivate::addChild, consider guardingchild->m_item->setParent(m_item)against re-parenting an item that already has a parent, or explicitly handling the re-parenting case, to avoid unexpected lifetime or signal/slot side effects.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `NetManagerPrivate`'s constructor, double-check that `this` is the correct `QObject` owner for `m_root->item()` and `m_deleteItem->item()`—if the public `NetManager` (`q_ptr`) is the QObject, you may want to parent these items to `q_ptr` instead to keep ownership consistent with the public API.
- In `NetItemPrivate::addChild`, consider guarding `child->m_item->setParent(m_item)` against re-parenting an item that already has a parent, or explicitly handling the re-parenting case, to avoid unexpected lifetime or signal/slot side effects.
## Individual Comments
### Comment 1
<location path="net-view/operation/private/netitemprivate.cpp" line_range="124" />
<code_context>
Q_EMIT m_item->childAboutToBeAdded(m_item, index);
m_children.insert(m_children.begin() + index, child->item());
+ child->m_item->setParent(m_item);
child->m_parent = m_item;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Keep QObject parent and logical `m_parent` in sync or derive one from the other to avoid inconsistencies.
Here you now update `QObject` parent (`child->m_item->setParent(m_item)`) and logical parent (`child->m_parent = m_item`) separately, which risks them getting out of sync if any future reparenting only updates one. Please either derive `m_parent` from `QObject::parent()` and drop the manual pointer, or centralize all parent changes in a single helper that updates both together to avoid subtle hierarchy inconsistencies.
Suggested implementation:
```cpp
Q_EMIT m_item->childAboutToBeAdded(m_item, index);
m_children.insert(m_children.begin() + index, child->item());
child->setParentItem(m_item);
Q_EMIT m_item->childAdded(child->item());
```
To fully implement the centralization and keep `QObject` parent and logical parent in sync, you should also:
1. In the corresponding header for `NetItemPrivate` (likely `netitemprivate.h`), declare a helper method, for example:
```cpp
void setParentItem(QObject *parent);
```
or, if `m_parent` is a more specific type, use that type:
```cpp
void setParentItem(NetItem *parent);
```
2. In `net-view/operation/private/netitemprivate.cpp`, **define** this helper so that it updates both the `QObject` parent and the logical `m_parent` in one place, e.g.:
```cpp
void NetItemPrivate::setParentItem(QObject *parent)
{
m_item->setParent(parent);
m_parent = parent;
}
```
Adjust the parameter type and assignment as needed to match the actual type of `m_parent`.
3. Search for any other places in the codebase where both `m_item->setParent(...)` and `m_parent = ...` (or equivalent) are called separately, and replace those paired operations with a single call to `setParentItem(...)` so that all reparenting goes through this centralized helper.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| Q_EMIT m_item->childAboutToBeAdded(m_item, index); | ||
| m_children.insert(m_children.begin() + index, child->item()); | ||
| child->m_item->setParent(m_item); |
There was a problem hiding this comment.
suggestion (bug_risk): Keep QObject parent and logical m_parent in sync or derive one from the other to avoid inconsistencies.
Here you now update QObject parent (child->m_item->setParent(m_item)) and logical parent (child->m_parent = m_item) separately, which risks them getting out of sync if any future reparenting only updates one. Please either derive m_parent from QObject::parent() and drop the manual pointer, or centralize all parent changes in a single helper that updates both together to avoid subtle hierarchy inconsistencies.
Suggested implementation:
Q_EMIT m_item->childAboutToBeAdded(m_item, index);
m_children.insert(m_children.begin() + index, child->item());
child->setParentItem(m_item);
Q_EMIT m_item->childAdded(child->item());
To fully implement the centralization and keep QObject parent and logical parent in sync, you should also:
-
In the corresponding header for
NetItemPrivate(likelynetitemprivate.h), declare a helper method, for example:void setParentItem(QObject *parent);
or, if
m_parentis a more specific type, use that type:void setParentItem(NetItem *parent);
-
In
net-view/operation/private/netitemprivate.cpp, define this helper so that it updates both theQObjectparent and the logicalm_parentin one place, e.g.:void NetItemPrivate::setParentItem(QObject *parent) { m_item->setParent(parent); m_parent = parent; }
Adjust the parameter type and assignment as needed to match the actual type of
m_parent. -
Search for any other places in the codebase where both
m_item->setParent(...)andm_parent = ...(or equivalent) are called separately, and replace those paired operations with a single call tosetParentItem(...)so that all reparenting goes through this centralized helper.
caixr23
previously approved these changes
Mar 24, 2026
1. Added setParent() calls for root and delete items in NetManagerPrivate constructor 2. Added setParent() call when adding child items in NetItemPrivate::addChild() 3. These changes ensure proper parent-child relationships are established 4. Prevents potential issues where JavaScript could release NetItem objects prematurely Influence: 1. Test network item hierarchy remains stable during operations 2. Verify parent-child relationships are correctly maintained 3. Test JavaScript interactions with network items 4. Ensure no memory leaks or premature object destruction 5. Verify network view operations work correctly after these changes fix: 为NetItem设置父对象防止JS释放 1. 在NetManagerPrivate构造函数中为根项目和删除项目添加setParent()调用 2. 在NetItemPrivate::addChild()中添加子项目时调用setParent() 3. 这些更改确保建立正确的父子关系 4. 防止JavaScript可能提前释放NetItem对象的问题 Influence: 1. 测试网络项目层次结构在操作期间保持稳定 2. 验证父子关系是否正确维护 3. 测试JavaScript与网络项目的交互 4. 确保没有内存泄漏或对象过早销毁 5. 验证网络视图操作在这些更改后正常工作
caixr23
approved these changes
Mar 24, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, caixr23 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
NetManagerPrivate constructor
NetItemPrivate::addChild()
established
objects prematurely
Influence:
fix: 为NetItem设置父对象防止JS释放
Influence:
Summary by Sourcery
Ensure NetItem instances are correctly parented to prevent premature destruction in JavaScript integrations.
Bug Fixes: