fix: Do not use lambda in QMenuItemAction.destroyed callback

[Czaki]

In napari tests I got segfaults, after local debugs it points to lambda that are modified in this PR.

In general, using the method is safer, as qt will not try to trigger the method of destroyed objects.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (f8b7406) to head (c9a2b33).

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #183   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           31        31           
  Lines         1786      1785    -1     
=========================================
- Hits          1786      1785    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Czaki]

I do not understand this test fail

[tlambert03]

Thanks for the fix @Czaki. that test fail was definitely a weird one. Ultimately, I was able to track it down to this line

action = QMenuItemAction(item, app=app, parent=menu)

and a few other things:

1. The removal of the lambda means that the QMenuItemAction will not be removed from the cache when the object is destroyed; because, just like you said qt will not try to trigger the method of destroyed objects. (side note: I'm not 100% sure that this is the behavior we want, but if if the lambda was causing you segfaults, then lets go with it and see if we run into any other unexpected side effects of leaving these items in the cache.). So, this PR made it so that when the menu is cleared during rebuilding, it no longer clears the cache, and that call to QMenuItemAction() uses a cached QMenuItemAction instead of making a new one.
2. Oddly enough, it was the providing of parent=menu that caused the issue here. That caused the rebuilding of the menu to hang, so that when the test asserted that the menu items were there, they actually weren't yet there because the menu rebuild never completed. Exactly why it causes the rebuild to hang, I'm not sure... but I suspect it has something to do with doing too much with a single QObject within a callback: in other words, while rebuilding a QMenu, if we try to both clear() that menu of all it's QActions, and then add back one of those QActions which never got deleted then stuff goes poorly.

So, simply changing QMenuItemAction(item, app=app, parent=menu) to QMenuItemAction(item, app=app) was enough to fix it (and I don't think that not passing the parent is a big deal here... since we're about to add it to the menu anyway).

---

you'll note I made a bunch of other changes to the caching pattern in 1fb13fb. While trying to debug, I got rid of __new__ in favor of a create() classmethod. and I think it's probably a safer pattern so stuck with it

[tlambert03]

i'll leave this here for the day in case you want to have another look, and then will merge this evening

[tlambert03]

interesting. are both of these required to be able to use create(parent=...) here? (i.e. not using clear as well as using setParent() inside of create?)

happy to go with whatever works best. just curious

[Czaki]

We need to keep reference to actions to prevent them being garbage collected, but we need to remove them from the menu before rebuild.

[tlambert03]

aha... good one

[tlambert03]

thanks!

[Czaki]

Could I request for bug fix release after merge this?

[tlambert03]

absolutely. i prefer to keep releases as close to main as possible and avoid lengthy delays :)

[lucyleeow]

Just a question, I did not follow:

The removal of the lambda means that the QMenuItemAction will not be removed from the cache when the object is destroyed; because, just like you said qt will not try to trigger the method of destroyed objects.

The first part makes sense but I do not understand the 2nd part. What does "qt will not try to trigger the method of destroyed objects" cause in this context ? Thanks

[Czaki]

The first part makes sense but I do not understand the 2nd part. What does "qt will not try to trigger the method of destroyed objects" cause in this context ? Thanks

Qt (And same for psygnal), when you connect a method to a signal, before calling callback, try first to validate if the object is still alive. And if it is already deleted then callback is not triggered.

When lamda is used, then there is no option to easy check if all referred objects still exist.