ENH: ctkSettingsDialog: Add removePanel function. - #1423
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a ctkSettingsDialog::removePanel(...) API (by label or by panel pointer) to support use cases like reloadable plugins where settings panels need to be unregistered from the dialog (Fixes #1422).
Changes:
- Added two
Q_INVOKABLEoverloads forctkSettingsDialog::removePanelin the public header. - Implemented recursive removal logic to delete the corresponding
QTreeWidgetItemsubtree and detach panels from theQStackedWidget.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
Libs/Widgets/ctkSettingsDialog.h |
Declares the new removePanel public API overloads. |
Libs/Widgets/ctkSettingsDialog.cpp |
Implements recursive panel removal and the two new removePanel entry points. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void ctkSettingsDialog::removePanel(const QString& label) | ||
| { | ||
| Q_D(ctkSettingsDialog); | ||
|
|
||
| QTreeWidgetItem* item = d->item(label); | ||
|
|
||
| if (item != d->SettingsTreeWidget->invisibleRootItem()) | ||
| { | ||
| d->removePanelRecursive(item); | ||
| } | ||
|
|
||
| this->adjustTreeWidgetToContents(); | ||
| } | ||
|
|
||
| // -------------------------------------------------------------------------- | ||
| void ctkSettingsDialog::removePanel(ctkSettingsPanel* panel) | ||
| { | ||
| Q_D(ctkSettingsDialog); | ||
|
|
||
| QTreeWidgetItem* item = d->item(panel); | ||
|
|
||
| if (item != d->SettingsTreeWidget->invisibleRootItem()) | ||
| { | ||
| d->removePanelRecursive(item); | ||
| } | ||
|
|
||
| this->adjustTreeWidgetToContents(); | ||
| } |
There was a problem hiding this comment.
@lassoan should I try to write tests or should we do it the same way we did with the last PR?
…re removed and handle the case when the last settings panel is removed.
78af6f3 to
37ecd56
Compare
|
@lassoan Does this make sense to merge without the tests or should I/you write tests for this to be merged. |
Fixes #1422