Skip to content

Conversation

loichuder
Copy link
Member

Checklist:


Fix #4253

Screencast.from.2025-09-23.10-43-25.webm

@loichuder loichuder self-assigned this Sep 23, 2025
@loichuder loichuder requested a review from t20100 September 23, 2025 08:45
self.STATE[True, "icon"] = icons.getQIcon("plot-yup")
self.STATE[True, "state"] = "Y-axis is oriented upward"
self.STATE[True, "action"] = "Orient Y-axis upward"
class _AxisOriginToolButton(PlotToolButton):
Copy link
Member Author

Choose a reason for hiding this comment

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

I completely refactored YAxisOriginToolButton to have a base class to abstract the common behaviour.

The only change between the XAxisOriginToolButton and the YAxisOriginToolButton is the axis to act on (abstracted by getAxis) and the icons/labels (abstracted by getState).

Note that self.STATE was converted to TypedDict, which should be fine given these were introducted in Python 3.8.

Note also that STATE is no longer lazy-loaded. Is this a problem ?

Copy link
Member

Choose a reason for hiding this comment

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

Refactoring sounds good.
The state info is still lazy-loaded, however it's no longer cached.
It shouldn't be an issue: icons are already cached.

@loichuder
Copy link
Member Author

loichuder commented Sep 23, 2025

Sorry, went a bit too fast. I still have CI woes to solve.

@loichuder loichuder removed the request for review from t20100 September 23, 2025 09:11
@loichuder loichuder force-pushed the invert-x-action branch 2 times, most recently from 0b3058d to fe8bd76 Compare September 23, 2025 13:35
@loichuder loichuder requested a review from t20100 September 23, 2025 13:50
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.

Looks good!

Main concern is about circular references induced by using lambda: self.method.

upwardAction.triggered.connect(self.setYAxisUpward)
upwardAction.setIconVisibleInMenu(True)
disableInversionAction = self._createAction(False)
disableInversionAction.triggered.connect(lambda: self.setAxisInverted(False))
Copy link
Member

Choose a reason for hiding this comment

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

This creates a mixed python/qt circular reference (lambda -> self -> menu -> disableInversionAction -> signal -> lambda) which may delay proper deletion, so best to avoid.

WeakMethod can be used here:

Suggested change
disableInversionAction.triggered.connect(lambda: self.setAxisInverted(False))
inversionTriggered = WeakMethod(self.setAxisInverted)
disableInversionAction.triggered.connect(lambda: inversionTriggered(False))

or wrapper methods since signal.connect(self.slot) is handled correctly.

Illustration of the problem:

>>> import weakref
>>> from silx import sx
>>> class A(sx.qt.QObject):
...     def __init__(self):
...         super().__init__()
...         self.objectNameChanged.connect(self.changed)
...     def changed(self, name):
...         print(name)
... 
>>> a = A()
>>> r = weakref.ref(a, lambda ref: print('dead', ref))
>>> del a
dead <weakref at 0x7f48eff53ef0; dead>

>>> class A(sx.qt.QObject):
...     def __init__(self):
...         super().__init__()
...         self.objectNameChanged.connect(lambda n: self.changed(n))
...     def changed(self, name):
...         print(name)
... 
>>> a = A()
>>> r = weakref.ref(a, lambda ref: print('dead', ref))
>>> del a
>>> 
>>> r()
<__main__.A object at 0x7f48fe72eb80>

Copy link
Member Author

Choose a reason for hiding this comment

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

In the end, I used wrapper functions (108c45f) instead of weakref.

It seems to have fixed the elusive segfault in the CI !

Copy link
Member

Choose a reason for hiding this comment

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

Good!

IMO we should prevent using lambda functions, functools.partial or anything keeping references to self in signals even if that requires some boilerplate methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. It looks to be recipes for disaster 😞

self.STATE[True, "icon"] = icons.getQIcon("plot-yup")
self.STATE[True, "state"] = "Y-axis is oriented upward"
self.STATE[True, "action"] = "Orient Y-axis upward"
class _AxisOriginToolButton(PlotToolButton):
Copy link
Member

Choose a reason for hiding this comment

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

Refactoring sounds good.
The state info is still lazy-loaded, however it's no longer cached.
It shouldn't be an issue: icons are already cached.

@t20100
Copy link
Member

t20100 commented Sep 26, 2025

Looks like my suggestions breaks the CI 😅 , I'll have a look

@loichuder
Copy link
Member Author

Looks like my suggestions breaks the CI 😅 , I'll have a look

Missed a renaming from xAxisInvertedAction to _xAxisInvertedAction in PlotWindow. Should be good now 🤞

@loichuder
Copy link
Member Author

Maaan, there is a segfault in the CI that I cannot reproduce locally 😮‍💨

@loichuder loichuder merged commit 5ecded4 into main Oct 1, 2025
4 of 8 checks passed
@loichuder loichuder deleted the invert-x-action branch October 1, 2025 09:10
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.

[GUI] Offer the opportunity to flip the image horizontally in addition to vertically using the pull-down menu

2 participants