-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat: connection_token to remember and easily disconnect (later) a number of connections #208
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ 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
☔ View full report in Codecov by Sentry. |
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 |
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()
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
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? |
ok, fair enough :) I can understand the concerns 👍 I'll just use this code internally in projects when needed |
Maybe instead of hundreds of signals there could be one signal emitting |
not quite following, can you elaborate? |
(it's too hard/magic to retrieve the |
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 |
I frequently use a pattern where I connect a bunch of things in an
__init__
function, and then later disconnect them during adestroyed
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.There's also a
temporary_connections
context, which automatically disconnects@Czaki ?