app-model icon indicating copy to clipboard operation
app-model copied to clipboard

Bug in _disconnect functions on QObjects

Open tlambert03 opened this issue 2 years ago • 3 comments

some more details ... ultimately, I think this bug is due to a failure in the disconnection strategy i mentioned above in https://github.com/pyapp-kit/app-model/issues/147#issuecomment-1801076340.

If you add self.sample_menu.destroyed.connect(lambda: print("DESTROY")) somewhere, you'll see that the destroyed event IS indeed emitted, but the intended _disconnect callback is never getting called (and so self._app.menus.menus_changed.disconnect(self._on_registry_changed) is never getting called either). I think this probably means that the strategy of having a QObject connect its own method to its destroyed signal isn't going to work (presumably that callback method itself is cleaned up and no longer called).

If you try to outsmart it by using a locally defined function closing on the QObject:

        @self.destroyed.connect
        def _disconnect() -> None:
            self._app.menus.menus_changed.disconnect(self._on_registry_changed)

then the _disconnect function does get called, but then psygnal has an issue actually executing the disconnection cause it can no longer find the callback to disconnect it:

Traceback (most recent call last):
  File "/Users/talley/dev/self/app-model/src/app_model/backends/qt/_qmenu.py", line 58, in _disconnect
    self._app.menus.menus_changed.disconnect(self._on_registry_changed)
  File "src/psygnal/_signal.py", line 827, in disconnect
  File "src/psygnal/_signal.py", line 799, in _slot_index
  File "src/psygnal/_weak_callback.py", line 130, in weak_callback
  File "src/psygnal/_weak_callback.py", line 363, in __init__
  File "src/psygnal/_weak_callback.py", line 190, in __init__
  File "src/psygnal/_weak_callback.py", line 258, in object_key
RuntimeError: wrapped C/C++ object of type QModelSubmenu has been deleted

It's possible we could catch and special case that in psygnal itself (to handle the case where essentially all attribute access is no longer permitted on the previously connected callback)... but I need to look into that.

Originally posted by @tlambert03 in https://github.com/pyapp-kit/app-model/issues/147#issuecomment-1803898923

tlambert03 avatar Nov 09 '23 14:11 tlambert03

oh! It looks like that issue in disconnection is already fixed by https://github.com/pyapp-kit/psygnal/pull/236 (which hasn't been released yet)

tlambert03 avatar Nov 09 '23 14:11 tlambert03

that issue in disconnection is already fixed

~you mean "the intended _disconnect callback is never getting called" problem?~ Nevermind, I worked it out.

So this can be closed then? (we could amend the comment in #151 to say the fix is only it's for pysygnal versions <'x')

lucyleeow avatar Nov 10 '23 02:11 lucyleeow

I'm still working on it, will close soon! And will update with more explanation/detail soon

tlambert03 avatar Nov 10 '23 03:11 tlambert03