pyapp-kit / superqt

Missing widgets and components for Qt-python
https://pyapp-kit.github.io/superqt/
BSD 3-Clause "New" or "Revised" License
207 stars 36 forks source link

feat: connection_token to remember and easily disconnect (later) a number of connections #208

Closed tlambert03 closed 1 year ago

tlambert03 commented 1 year ago

I frequently use a pattern where I connect a bunch of things in an __init__ function, and then later disconnect them during a destroyed callback or something. Napari does this a lot as well, for example here... and that's error prone if you forget one of the connections in the other method.

This provides a connection_token object that remembers a set of connections (optionally within a context manager) and can disconnect them all later.

from qtpy.QtCore import QObject, Signal
from superqt.utils import connection_token

class Thing(QObject):
    sig = Signal(int)

t = Thing()
with connection_token() as token:
    t.sig.connect(lambda v: print("called with", v))
    t.sig.connect(lambda: print("hey!"))
    t.sig.connect(lambda: print("anyone there?"))

# you can also use append
token.append(t.sig.connect(print))

t.sig.emit(2)  # prints a bunch of stuff

token.disconnect()
t.sig.emit(4)  # nothing happens

There's also a temporary_connections context, which automatically disconnects

from superqt.utils import temporary_connections

with temporary_connections():
    obj.some_signal.connect(some_callback)
    obj.some_signal.emit()  # some_callback is called
obj.some_signal.emit()  # some_callback is not called

@Czaki ?

codecov[bot] commented 1 year ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

Comparison is base (d821149) 86.90% compared to head (26792e0) 87.46%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #208 +/- ## ========================================== + Coverage 86.90% 87.46% +0.55% ========================================== Files 45 45 Lines 3346 3375 +29 ========================================== + Hits 2908 2952 +44 + Misses 438 423 -15 ``` | [Files](https://app.codecov.io/gh/pyapp-kit/superqt/pull/208?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyapp-kit) | Coverage Δ | | |---|---|---| | [src/superqt/utils/\_\_init\_\_.py](https://app.codecov.io/gh/pyapp-kit/superqt/pull/208?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyapp-kit#diff-c3JjL3N1cGVycXQvdXRpbHMvX19pbml0X18ucHk=) | `100.00% <100.00%> (ø)` | | | [src/superqt/utils/\_misc.py](https://app.codecov.io/gh/pyapp-kit/superqt/pull/208?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyapp-kit#diff-c3JjL3N1cGVycXQvdXRpbHMvX21pc2MucHk=) | `100.00% <100.00%> (ø)` | | ... and [3 files with indirect coverage changes](https://app.codecov.io/gh/pyapp-kit/superqt/pull/208/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyapp-kit)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Czaki commented 1 year ago

The description needs to be clarified for me. Is the problem connected with lambda functions? Or also with regular methods?

In general, I do not like the global nature of connection_token as it may impact some inner connection creations. I would prefer to provide obj as connection_token argument to impact only these object connections.

tlambert03 commented 1 year ago

Is the problem connected with lambda functions? Or also with regular methods?

the problem is partially about bookkeeping ... look at this code in napari:

    def _connect_theme(self, theme):
        # connect events to update theme. Here, we don't want to pass the event
        # since it won't have the right `value` attribute.
        theme.events.background.connect(self._update_theme_no_event)
        theme.events.foreground.connect(self._update_theme_no_event)
        theme.events.primary.connect(self._update_theme_no_event)
        theme.events.secondary.connect(self._update_theme_no_event)
        theme.events.highlight.connect(self._update_theme_no_event)
        theme.events.text.connect(self._update_theme_no_event)
        theme.events.warning.connect(self._update_theme_no_event)
        theme.events.current.connect(self._update_theme_no_event)
        theme.events.icon.connect(self._update_theme_no_event)
        theme.events.canvas.connect(
            lambda _: self._qt_viewer.canvas._set_theme_change(
                get_settings().appearance.theme
            )
        )
        # connect console-specific attributes only if QtConsole
        # is present. The `console` is called which might slow
        # things down a little.
        if self._qt_viewer._console:
            theme.events.console.connect(self._qt_viewer.console._update_theme)
            theme.events.syntax_style.connect(
                self._qt_viewer.console._update_theme
            )

    def _disconnect_theme(self, theme):
        theme.events.background.disconnect(self._update_theme_no_event)
        theme.events.foreground.disconnect(self._update_theme_no_event)
        theme.events.primary.disconnect(self._update_theme_no_event)
        theme.events.secondary.disconnect(self._update_theme_no_event)
        theme.events.highlight.disconnect(self._update_theme_no_event)
        theme.events.text.disconnect(self._update_theme_no_event)
        theme.events.warning.disconnect(self._update_theme_no_event)
        theme.events.current.disconnect(self._update_theme_no_event)
        theme.events.icon.disconnect(self._update_theme_no_event)
        theme.events.canvas.disconnect(
            lambda _: self._qt_viewer.canvas._set_theme_change(
                get_settings().appearance.theme
            )
        )

this would allow:

token = connection_token()
token.append(theme.events.background.connect(self._update_theme_no_event))
token.append(theme.events.foreground.connect(self._update_theme_no_event))
token.append(theme.events.primary.connect(self._update_theme_no_event))

# or
with token():
    theme.events.background.connect(self._update_theme_no_event)
    theme.events.foreground.connect(self._update_theme_no_event)
    theme.events.primary.connect(self._update_theme_no_event)

and then later:

def _disconnect_theme(self, theme):
    token.disconnect()    

In general, I do not like the global nature of connection_token as it may impact some inner connection creations.

thats' why I created the context manager to keep it local to a specific set of connections. if you don't want that, you could just use append

Czaki commented 1 year ago

thats' why I created the context manager to keep it local to a specific set of connections. if you don't want that, you could just use append

But some additional connections may happen during connecting (for example caused by __getattr__), and the user may not know this as it is a dependency. That may lead to hard-to-understand and debug errors.

the problem is partially about bookkeeping ... look at this code in napari:

This is an example of code that is not Qt and never will be. Destroying also should be handled appropriately (until someone uses lambda). But the example of adding/removing an object from some collection is left?

tlambert03 commented 1 year ago

ok, fair enough :) I can understand the concerns 👍

I'll just use this code internally in projects when needed

Czaki commented 1 year ago

Maybe instead of hundreds of signals there could be one signal emitting (str, object) that will be dispatched on handler side?

tlambert03 commented 1 year ago

not quite following, can you elaborate?

tlambert03 commented 1 year ago

(it's too hard/magic to retrieve the QtCore.QMetaObject.Connection for a signal in PySide2 anyway)

Czaki commented 1 year ago

The problem happen when you have element that is addded/removed from collection, that have multiple signals that need to be handled (in example just evented model).

But if you control this object it could be something like:

class Model(QObject):
    attr1_signal = Signal(int)
    attr2_signal = Signal(float)

    attr_changed = Signal(object)

    @attr1.setter
    def attr1(self, val):
        ...
        self.attr1_signal.emit(val)
        self.attr_changed.emit({"attr1": val})

    ...

and then handle it by attr_changed signal.