Skip to content

Comments

common api to add/remove items#4479

Merged
t20100 merged 4 commits intosilx-kit:mainfrom
abmajith:publicAPILegendWidget
Feb 11, 2026
Merged

common api to add/remove items#4479
t20100 merged 4 commits intosilx-kit:mainfrom
abmajith:publicAPILegendWidget

Conversation

@abmajith
Copy link
Contributor

In the LegendWidgets, we are moving from default add/remove Items (based on the internal Items trigger action from the plotwidget), to application explicit add/remove Items calls

Copy link
Member

@t20100 t20100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous behavior was fine and should not change IMO.
The manual add/remove can co-exist with the "automatic" sync with the plot: We just need to make sure in work in all conditions

else:
plot.sigItemAdded.connect(self._onItemAdded)
plot.sigItemRemoved.connect(self._onItemRemoved)
plot.sigItemRemoved.connect(self._onItemRemovedFromPlot)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why removing sigItemAdded connection?

The idea is to have 2 ways to manage the items:

  • Automatically from a PlotWidget through sigItemAdded, sigItemRemoved
  • Manually with explicit addItem, removeItem

Both mode should co-exist and work in corner cases e.g.:

  • remove an item added automatically and then remove it from the plot
  • add an item manually that was already added automatically (in this case it should not be added)
  • setPlotWidget(None) should not remove item added manually that do not belong to the previous plot

Note: We could remove setPlotWidget and have users do the connections sigItemAdded -> addItem, sigItemRemoved -> removeItem, but quite a few widgets provides an automatic update through a setPlotWidget (or setPlot) method, so it is worth keeping it IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, Now I understand features, by default it should be like previous way,

additionally, if the user wants to remove an item, then we could remove from legend and from plot widget,

if user wants to addItem to legend, it should also add it into plot.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, if we implement this feature, we dont have to go for the sorting of items based on the Items.type,

user can manually arange Items using multiple LegendWidgets, and order them as they want,

because, if we try to do manually, then it complicates the code, may be we need to find some clever sorting insertion, deletion, data structure, ?

@abmajith
Copy link
Contributor Author

As mentioned, made clear, add,remove Items as public, only delete the items belongs to the previous plot, when it changes from plot to another.

added orphan items interaction check, and added manual/automated items interference check

@abmajith abmajith requested a review from t20100 February 10, 2026 13:31
Copy link
Member

@t20100 t20100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the updates!

I put one minor change of the boolean asserts.

Not for this PR, but provided how much the tests are accessing _itemWidgets keys, we may add a getItems method to LegendsWidget as the one of PlotWidget

Comment on lines 93 to 101
assert orphan_item.isVisible() is True
assert item_widget._label.isEnabled() is True
qapp_utils.mouseClick(item_widget, qt.Qt.LeftButton)
qapp.processEvents()
assert orphan_item.isVisible() is False
assert item_widget._label.isEnabled() is False
qapp_utils.mouseClick(item_widget, qt.Qt.LeftButton)
assert orphan_item.isVisible() is True
assert item_widget._label.isEnabled() is True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for is True and is False is not:

Suggested change
assert orphan_item.isVisible() is True
assert item_widget._label.isEnabled() is True
qapp_utils.mouseClick(item_widget, qt.Qt.LeftButton)
qapp.processEvents()
assert orphan_item.isVisible() is False
assert item_widget._label.isEnabled() is False
qapp_utils.mouseClick(item_widget, qt.Qt.LeftButton)
assert orphan_item.isVisible() is True
assert item_widget._label.isEnabled() is True
assert orphan_item.isVisible()
assert item_widget._label.isEnabled()
qapp_utils.mouseClick(item_widget, qt.Qt.LeftButton)
qapp.processEvents()
assert not orphan_item.isVisible()
assert not item_widget._label.isEnabled()
qapp_utils.mouseClick(item_widget, qt.Qt.LeftButton)
assert orphan_item.isVisible()
assert item_widget._label.isEnabled()

@abmajith abmajith requested a review from t20100 February 10, 2026 15:54
@t20100 t20100 merged commit cfc5a5e into silx-kit:main Feb 11, 2026
5 of 8 checks passed
@abmajith abmajith deleted the publicAPILegendWidget branch February 11, 2026 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants