Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in _disconnect functions on QObjects #157

Open
tlambert03 opened this issue Nov 9, 2023 · 3 comments
Open

Bug in _disconnect functions on QObjects #157

tlambert03 opened this issue Nov 9, 2023 · 3 comments

Comments

@tlambert03
Copy link
Member

tlambert03 commented Nov 9, 2023

some more details ... ultimately, I think this bug is due to a failure in the disconnection strategy i mentioned above in #147 (comment).

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 #147 (comment)

@tlambert03
Copy link
Member Author

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

@lucyleeow
Copy link
Collaborator

lucyleeow commented Nov 10, 2023

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')

@tlambert03
Copy link
Member Author

tlambert03 commented Nov 10, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants