created LegendIconWidget a similar LegendIcon #4404

abmajith · 2025-09-11T12:06:29Z

In this pull request, created a unique LegendIcon, and LegendItem widget that is listed in vertical in qt.MainWindow,

the respective example are provided, but so far no unit test,

also I still not sure, whether I added some what generic use case to the LegendItem widget,

loichuder · 2025-09-11T14:31:27Z

@abmajith We introduced black and flake8 in the CI in the mean time. Please reformat and check the code, thanks 🙂

t20100

First round of feedbacks, here are the 2 main ones:

LegendItemList should make minimal changes (add/remove) on the list rather than re-creating it on changes.
It seems LegendItemIcon and LegendItemWidget are having similar logic, maybe best not to inherit from LegendIconWidget and implement the update logic at a single place in LegendItemWidget?

t20100 · 2025-09-12T13:50:25Z

src/silx/gui/plot/LegendItem.py

+        self.setFixedWidth(200)
+        self._layout = qt.QVBoxLayout(self)
+        self._plot: PlotWidget | None = None
+        self._binding(parent)


The PlotWidget that is "observed" by the list of legends is not the "parent": "parent" is Qt widget tree structure, the plot can be anywhere in it.
Best to provide get|setPlotWidget methods to set it separately, and eventually pass it in __init__:

def __init__( self, parent: qt.QWidget | None = None, plotWidget: PlotWidget | None = None, ):

t20100 · 2025-09-12T13:51:19Z

src/silx/gui/plot/LegendItem.py

+        if hasattr(parent, "sigVisibilityChanged"):
+            parent.sigVisibilityChanged.connect(self._setVisibility)
+
+    def _setVisibility(self, status: bool) -> None:
+        if status:
+            self.show()
+        else:
+            self.hide()


This widget shouldn't need to care about this, Qt does it already:

Suggested change

if hasattr(parent, "sigVisibilityChanged"):

parent.sigVisibilityChanged.connect(self._setVisibility)

def _setVisibility(self, status: bool) -> None:

if status:

self.show()

else:

self.hide()

t20100 · 2025-09-12T13:58:49Z

src/silx/gui/plot/LegendItem.py

+        self._plot.sigActiveCurveChanged.connect(self._onActiveItemChanged)
+        self._plot.sigActiveImageChanged.connect(self._onActiveItemChanged)
+        self._plot.sigActiveScatterChanged.connect(self._onActiveItemChanged)


The "active" state of the items is not used in _onActiveItemChanged, so no need to handle those signals

Suggested change

self._plot.sigActiveCurveChanged.connect(self._onActiveItemChanged)

self._plot.sigActiveImageChanged.connect(self._onActiveItemChanged)

self._plot.sigActiveScatterChanged.connect(self._onActiveItemChanged)

t20100 · 2025-09-12T13:59:47Z

src/silx/gui/plot/LegendItem.py

+        self._plot.sigItemAdded.connect(self._onContentChanged)
+        self._plot.sigItemRemoved.connect(self._onContentChanged)


No need for wrapper method:

Suggested change

self._plot.sigItemAdded.connect(self._onContentChanged)

self._plot.sigItemRemoved.connect(self._onContentChanged)

self._plot.sigItemAdded.connect(self._updateAllItemsList)

self._plot.sigItemRemoved.connect(self._updateAllItemsList)

But here, we don't want to redo the whole list each time an item is added/removed, just add/remove the corresponding LegendItemWidget

t20100 · 2025-09-12T14:02:07Z

src/silx/gui/plot/LegendItem.py

+        self._updateHeigth()
+
+    def _updateHeigth(self):
+        _totalHeight = (self._layout.count() - 1) * self._layout.spacing()
+        _totalHeight += self._layout.contentsMargins().top()
+        _totalHeight += self._layout.contentsMargins().bottom()
+        if self._layout.count() > 0:
+            item = self._layout.itemAt(0)
+            _totalHeight += item.widget().sizeHint().height() * self._layout.count()
+        self.setFixedHeight(_totalHeight)


If there is issue with layout, it should be dealt otherwise.
It's always risky to play with this, for instance, the text of the legend is clipped on my laptop.

t20100 · 2025-09-12T14:26:18Z

src/silx/gui/plot/LegendItem.py

+    def __init__(self, parent: PlotWidget):
+        super().__init__(parent, qt.Qt.Window)
+        self.setFixedWidth(200)
+        self._layout = qt.QVBoxLayout(self)


No need to keep a reference on the layout, it can be retrieved with self.layout()

t20100 · 2025-09-12T14:27:14Z

src/silx/gui/plot/LegendItem.py

+
+class LegendItemList(qt.QWidget):
+    def __init__(self, parent: PlotWidget):
+        super().__init__(parent, qt.Qt.Window)


Either pass flags from __init__ or use the default:

Suggested change

super().__init__(parent, qt.Qt.Window)

super().__init__(parent)

t20100 · 2025-09-12T14:29:10Z

src/silx/gui/plot/LegendItem.py

+class LegendItemList(qt.QWidget):
+    def __init__(self, parent: PlotWidget):
+        super().__init__(parent, qt.Qt.Window)
+        self.setFixedWidth(200)


Do use fixed size, best to let Qt handles it: rendering changes depending on OS & screen...

t20100 · 2025-09-12T14:29:36Z

src/silx/gui/plot/LegendItem.py

+        super().__init__(parent, qt.Qt.Window)
+        self.setFixedWidth(200)
+        self._layout = qt.QVBoxLayout(self)
+        self._plot: PlotWidget | None = None


Best to keep a weakreference

the weekref not pointing to the item attribute class, it throws error, for example when i call ite,getName(), throws no methods called getName() something

Can you be more explicit?
Without the code and the complete error message, it is impossible to help.

python3 examples/plotLegendLists.py Traceback (most recent call last): File "/home/noordhee/DAU_Projects/silx/src/silx/gui/plot/LegendItem.py", line 162, in _updateAllItemsList self._onItemAdded(item) File "/home/noordhee/DAU_Projects/silx/src/silx/gui/plot/LegendItem.py", line 171, in _onItemAdded legendIcon = LegendItemWidget(None, item) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/noordhee/DAU_Projects/silx/src/silx/gui/plot/LegendItem.py", line 29, in __init__ self._label = qt.QLabel(self._item.getName()) ^^^^^^^^^^^^^^^^^^ AttributeError: 'weakref.ReferenceType' object has no attribute 'getName' Traceback (most recent call last): File "/home/noordhee/DAU_Projects/silx/src/silx/gui/plot/LegendItem.py", line 162, in _updateAllItemsList self._onItemAdded(item) File "/home/noordhee/DAU_Projects/silx/src/silx/gui/plot/LegendItem.py", line 171, in _onItemAdded legendIcon = LegendItemWidget(None, item) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/noordhee/DAU_Projects/silx/src/silx/gui/plot/LegendItem.py", line 29, in __init__ self._label = qt.QLabel(self._item.getName()) ^^^^^^^^^^^^^^^^^^ AttributeError: 'weakref.ReferenceType' object has no attribute 'getName' Traceback (most recent call last): File "/home/noordhee/DAU_Projects/silx/src/silx/gui/plot/LegendItem.py", line 162, in _updateAllItemsList self._onItemAdded(item) File "/home/noordhee/DAU_Projects/silx/src/silx/gui/plot/LegendItem.py", line 171, in _onItemAdded legendIcon = LegendItemWidget(None, item) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/noordhee/DAU_Projects/silx/src/silx/gui/plot/LegendItem.py", line 29, in __init__ self._label = qt.QLabel(self._item.getName()) ^^^^^^^^^^^^^^^^^^ AttributeError: 'weakref.ReferenceType' object has no attribute 'getName'

when i store the item with weakref, this is the error i got

weakref.ref cannot be accessed as the item it references.
You have to "call" it to get back the item:

myweakref = weakref.ref(myitem) ... item = myweakref() if item is not None: # It returns None if the item has been deleted item.getName()

An alternative is to use weakref.proxy, but best IMO to know if the object still exists

Look at: https://docs.python.org/3/library/weakref.html#weak-reference-objects

t20100 · 2025-09-12T14:33:56Z

src/silx/gui/plot/LegendItem.py

+        self.layout().addStretch()
+
+        self.label.setToolTip("Click to toggle visibility")
+        self.label.mousePressEvent = self._onLabelClicked


Not a good idea to "monkey patch" a Qt method.
Since it would make sense to also accept the press on the icon, you can implement mousePressEvent in this class.

abmajith · 2025-09-16T08:22:00Z

First round of feedbacks, here are the 2 main ones:

* `LegendItemList` should make minimal changes (add/remove) on the list rather than re-creating it on changes.

* It seems `LegendItemIcon` and `LegendItemWidget` are having similar logic, maybe best not to inherit from `LegendIconWidget` and implement the update logic at a single place in `LegendItemWidget`?

Yes, i think add/removal should be minimum, and also we the second point, making single class for icon and widget,

…dget (that shows name and legen item symbol in a row)

t20100

Second round of comments:
Much simpler with a single LegendItemWidget, ch and update only on add/remove!

There's something to check about removing/clearing widget, this needs to be robust: it should not let widgets alive that are no longer displayed.

There's a few comments from last review that where not taken into account nor commented. Before requesting a new review, can you address all comments, either by applying them if you agree or by commenting if you don't agree or need clarifications.

t20100 · 2025-09-19T10:13:39Z

src/silx/gui/plot/LegendItem.py

+
+
+class LegendItemWidget(qt.QWidget):
+    def __init__(self, parent, item: items):


items is a module, not a class

Suggested change

def __init__(self, parent, item: items):

def __init__(self, parent, item: items.Item):

t20100 · 2025-09-19T10:17:35Z

src/silx/gui/plot/LegendItem.py

+    def _clearLayout(self, layout):
+        """Helper to clear all widgets from a layout."""
+        if layout is not None:


No need to pass the layout as an argument:

Suggested change

def _clearLayout(self, layout):

"""Helper to clear all widgets from a layout."""

if layout is not None:

def _clear(self):

"""Helper to clear all widgets from a layout."""

layout = self.layout()

if layout is not None:

this does not clear self._itemWidgets

t20100 · 2025-09-19T10:23:52Z

src/silx/gui/plot/LegendItem.py

+        if self._plot is None:
+            return
+        self._clearLayout(self.layout())
+        _activeItem: tuple = self._plot.getItems()


No _ prefix for local variables (there's others in this PR).
Already mentionned in #4404 (comment)

t20100 · 2025-09-19T10:27:25Z

src/silx/gui/plot/LegendItem.py

+
+    def _onItemRemoved(self, item: items.Item):
+        if item in self._itemWidgets:
+            self.layout().removeWidget(self._itemWidgets[item])


Is this enough to fully remove the widget? What is it's parent after the removal?
If it's the LegendItemList then it's not deleted.

Either this is enough and there is no need for deletedLater in _clearLayout or it's fully destroying the widget and something like in _clearLayout should be enough.

okay for the consistency, I will follow the same mechanism as in _clearLayout

t20100 · 2025-09-19T10:29:29Z

src/silx/gui/plot/LegendItem.py

+        self.layout().addStretch()
+
+        self._label.setToolTip("Click to toggle visibility")
+        self._label.mousePressEvent = self._onLabelClicked


Already mentioned in #4404 (comment)
This is not a good pattern. you can implement mousePressEvent in this class instead

t20100 · 2025-09-19T10:29:53Z

src/silx/gui/plot/LegendItem.py

+
+        self._label.setToolTip("Click to toggle visibility")
+        self._label.mousePressEvent = self._onLabelClicked
+        self._label.setCursor(qt.Qt.PointingHandCursor)


You can set it for the whole widget

Suggested change

self._label.setCursor(qt.Qt.PointingHandCursor)

self.setCursor(qt.Qt.PointingHandCursor)

t20100 · 2025-09-19T10:30:18Z

src/silx/gui/plot/LegendItem.py

+        if isinstance(item, items.SymbolMixIn):
+            self._icon.setSymbol(item.getSymbol())
+        if isinstance(item, items.LineMixIn):
+            self._icon.setLineWidth(item.getLineWidth())
+            self._icon.setLineStyle(item.getLineStyle())
+        if isinstance(item, items.ColorMixIn):
+            color_rgba = item.getColor()
+            color = qt.QColor.fromRgbF(
+                color_rgba[0],
+                color_rgba[1],
+                color_rgba[2],
+                color_rgba[3],
+            )
+            self._icon.setLineColor(color)
+            self._icon.setSymbolColor(color)
+        if isinstance(item, items.ColormapMixIn):
+            self._icon.setColormap(item.getColormap())


t20100 · 2025-09-19T10:34:03Z

src/silx/gui/plot/LegendItem.py

+            items.ItemChangedType.VISIBLE,
+            items.ItemChangedType.HIGHLIGHTED,
+            items.ItemChangedType.NAME,


Quite a few change events seems to be missing here: line, symbol, color...

abmajith · 2025-09-29T10:05:17Z

Hi @t20100

I worked on your review,

I have a problem with weakref, (provided the error message),

abmajith · 2025-10-06T09:39:39Z

Hi @t20100,

I tested the LegendItem gui,

there is one discomfort right now, When I am closing plot window that linked to LegendItem (as in this example/plotLegendItem), the legendItem has to be closed separately, Is this a problem, or we have to set up the parent widget to plot window so that it will automatically closed?

abmajith added 2 commits September 11, 2025 14:04

created LegendIconWidget a similar LegendIcon

5cb1434

Merge branch 'silx-kit:main' into legendVerticalList

12f7af4

loichuder requested a review from t20100 September 11, 2025 14:29

black, flake8 formated

51dbb7e

loichuder mentioned this pull request Sep 11, 2025

[plot] Add a widget to display Scatter legends #4345

Open

abmajith added 3 commits September 12, 2025 09:39

Merge branch 'silx-kit:main' into legendVerticalList

34d8c24

trimmed the legendItem widget methods

f4b3f22

visibility of legend box synched with plotwidget visibility

8363a49

abmajith force-pushed the legendVerticalList branch from d1d333b to 8363a49 Compare September 12, 2025 09:49

t20100 reviewed Sep 12, 2025

View reviewed changes

removed LegendItemIcon, and everything handled inside the LegenItemWi…

3116cb3

…dget (that shows name and legen item symbol in a row)

abmajith requested a review from t20100 September 16, 2025 08:37

t20100 reviewed Sep 19, 2025

View reviewed changes

avoided monkey patching, added eventFilter instead

adf519e

abmajith added 2 commits October 1, 2025 12:08

added weekref to LegendItem _item reference

ba21828

added weekref to LegendItem _item reference

0963403

	self._plot.sigActiveCurveChanged.connect(self._onActiveItemChanged)
	self._plot.sigActiveImageChanged.connect(self._onActiveItemChanged)
	self._plot.sigActiveScatterChanged.connect(self._onActiveItemChanged)

		self._plot.sigItemAdded.connect(self._onContentChanged)
		self._plot.sigItemRemoved.connect(self._onContentChanged)

	super().__init__(parent, qt.Qt.Window)
	super().__init__(parent)



		class LegendItemWidget(qt.QWidget):
		def __init__(self, parent, item: items):

	def __init__(self, parent, item: items):
	def __init__(self, parent, item: items.Item):

	self._label.setCursor(qt.Qt.PointingHandCursor)
	self.setCursor(qt.Qt.PointingHandCursor)

created LegendIconWidget a similar LegendIcon #4404

Are you sure you want to change the base?

created LegendIconWidget a similar LegendIcon #4404

Uh oh!

Conversation

abmajith commented Sep 11, 2025

Uh oh!

loichuder commented Sep 11, 2025

Uh oh!

t20100 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abmajith commented Sep 16, 2025

Uh oh!

t20100 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abmajith commented Sep 29, 2025

Uh oh!

abmajith commented Oct 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants