Use psygnal instead of EventEmitter (callbacks receive value directly). Add deprecation strategy

[tlambert03]

This PR removes events.py and uses psygnal instead... In general, I dislike the event object that gets passed around in callbacks (it makes sense for mouse events and stuff, but not so much for single value callbacks). This was originally raised by @berl in https://github.com/napari/magicgui/issues/173, and I agree with the sentiment there that requiring a user to make a callback that uses event.value is stranger than just using value directly. This supersedes and closes #182

This is still WIP, as I need to add deprecation warnings

update:

I believe this is now ready for some input! The major changes are described below (and will be added to https://github.com/napari/magicgui/issues/255, which is linked directly in the warning message).

👇

The big change here is that callbacks connected to widget.changed (and other event emitters) now receive the value(s) directly, instead of an event object:

the change

# before:
@widget.changed.connect
def my_callback(event):
    new_value = event.value

# now becomes
@widget.changed.connect
def my_callback(value):
    new_value = value

# note also that psygnal lets you accept _less_
# arguments than the emitter provides
# so this also works fine
@widget.changed.connect
def my_callback():
    # something that didn't need the value

for the few packages who were manually emitting change events:

# before:
widget.changed(value='whatever')

# now becomes
widget.changed.emit('whatever')

deprecation strategy

• Though this PR is designed not to break anything (just warn), I plan to make it a minor version bump (v0.3.0). v0.4.0 will then drop support for the "current" events mechanism.
• After this PR, callbacks that take a single un-annotated parameter, (like def my_callback(param):) will receive the Event object as they currently do, but will now see a FutureWarning explaining the upcoming change. (note, the Event object is now just a simple namedtuple that matches the API of the old Event object.)
• To silence the warning and "opt-in" to the new system (i.e. receive the value directly instead of the Event object) , you can annotate your parameter as anything other than a magicgui.events.Event. e.g. def my_callback(param: str): will receive the value directly, not a Event.
• callbacks that take no parameters will now work fine (they would have failed with the old emitter that expected an event object, so these are likely new functions, and psygnal is smart enough to not give them any params).
• callbacks that take more than 1 parameter will also now receive values instead of Event objects (they too would have broken before)
• anyone trying to emit a change event manually with widget.changed(value='whatever') will get a FutureWarning explaining that they should use widget.changed.emit('whatever') instead.

breaking:

• this removes callback positioning altogether (connect(..., position=...))... @hanjinliu, I see you're using that a bit in magic-class), so would be curious to hear how troublesome losing that will be.

I tested this PR with stardist-napari, (which I believe might be the package using the largest "surface" of the current events API), and it works (though of course emits a lot of warnings now). @uschmidt83, @maweigert, I can help with those updates as well.

@sofroniewn and @jni ... i know we've discussed this before, but just wanted to give you another heads up

[codecov]

Codecov Report

Merging #253 (35945bb) into main (8f6b5a7) will decrease coverage by 0.00%.
The diff coverage is 96.77%.

@@            Coverage Diff             @@
##             main     #253      +/-   ##
==========================================
- Coverage   89.96%   89.96%   -0.01%     
==========================================
  Files          29       29              
  Lines        3258     3257       -1     
==========================================
- Hits         2931     2930       -1     
  Misses        327      327              

Impacted Files	Coverage Δ
magicgui/widgets/_bases/categorical_widget.py	84.50% <50.00%> (ø)
magicgui/widgets/_bases/button_widget.py	96.42% <100.00%> (ø)
magicgui/widgets/_bases/container_widget.py	92.34% <100.00%> (-0.05%)	⬇️
magicgui/widgets/_bases/value_widget.py	96.87% <100.00%> (-0.05%)	⬇️
magicgui/widgets/_bases/widget.py	87.64% <100.00%> (ø)
magicgui/widgets/_concrete.py	84.27% <100.00%> (ø)
magicgui/widgets/_function_gui.py	96.89% <100.00%> (+0.01%)	⬆️
magicgui/widgets/_table.py	96.85% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f6b5a7...35945bb. Read the comment docs.

[hanjinliu]

Hi, @tlambert03 .
I use position=... because I want to close FunctionGui widget after the function call is finished (because we don't want to close the window by ourselves every time after loading data or something). If I just connect the callback without position argument, FunctionGui closes before the function is actually called. As a result, when the GUI is docked in napari, function will not be called (strangely, this problem does not happen without napari).

I think I can do without position argument by removing the call button and adding new one or... are there any better ways?

[tlambert03]

I think I can do without position argument by removing the call button and adding new one or... are there any better ways?

looks like you've already updated magic-class... actually manipulating (removing and re-adding) the callbacks on an emitter feels a little brittle to me. but this seems like a decent feature to add to core.

[tlambert03]

If I just connect the callback without position argument, FunctionGui closes before the function is actually called. As a result, when the GUI is docked in napari, function will not be called (strangely, this problem does not happen without napari).

ok, we should probably figure out why that's happening on the napari side then. This code works for me (you too?):

from magicgui import magicgui

@magicgui
def f(x: int):
    print(locals())

f.called.connect(lambda e: f.close())
f.show()  # clicking the run button prints locals before closing

edit: hmm... it seems to work for me on napari too. Can you make an issue either here or at napari with a minimal example showing the problem?

[hanjinliu]

Thank you, @tlambert03 , the called event is a very clear way to bind callbacks to push buttons, I like it!
Your example using called also worked for me both with FunctionGui alone and in napari.

Can you make an issue either here or at napari with a minimal example showing the problem?

OK, I'll open a issue in napari.

[tlambert03]

thanks!

[sofroniewn]

Nice work here. I think we should go with the change and then make the new 0.4.0 magicgui release. i looked through some of the deprecation approach - i looks good to me.

My next big question is - should we switch over the main napari repo to use pysgnal too? I'd be curious what @goanpeca thinks about that. I can't remember if we've discussed. I'd probably be in favour of it, but curious what he and others think too

[tlambert03]

My next big question is - should we switch over the main napari repo to use pysgnal too?

I'd like to eventually ... with the notable exception of mouse events and keyboard events. Those make a lot of sense as a single (possibly complicated) event object, and I can see why vispy used them (since it's almost all mouse/keyboard events). Basically, everything that has some sort of QEvent equivalent make sense to keep as is. But when objects want to emit that something about them have changed, (i.e. wherever Qt would use signals/slots instead of QEvents), then I think this pattern makes more sense. I'll open a task at napari

[jni]

@tlambert03 before I look at the code, I'll say I generally like the API, but since we are overhauling a lot of big things, should we rethink a few things?

• widget.changed.emit(value) -- is there a reason not to use __call__ as an alias for emit? I actually do prefer the widget.changed(value) syntax.
• In a few situations we've wanted to have access to the previous value as well as the new value. Can we somehow make this new framework do that as well? e.g. viewer.changed.emit(value, prev_value)?
• you mention that having a big event object makes sense for mouse and keyboard events, but couldn't that just be the value anyway? ie value can be an arbitrarily complex object so I'm not sure why this would prevent us from unifying our event system.
• does this still have the block() context manager? (probably reading the code will clarify but wanted to write down all my thoughts)

[jni]

does this still have the block() context manager? (probably reading the code will clarify but wanted to write down all my thoughts)

Yup, except it's "blocked". I would suggest renaming to "block"?

Anyway, I very much like the PR! See my comments above, they are very small, and I would consider none of them blockers.

[jni]

One more thought, could be dumb: it's kinda nice to have a decorator that shows you when an event is going to trigger a function. Could we also make a decorator for emitting when a function/method is called? e.g.:

@changed.emit_when
def fill(self, position, value):
    ...

[tlambert03]

• widget.changed.emit(value) -- is there a reason not to use __call__ as an alias for emit? I actually do prefer the widget.changed(value) syntax.

oh if you really want it i suppose there's no harm :)

• In a few situations we've wanted to have access to the previous value as well as the new value. Can we somehow make this new framework do that as well? e.g. viewer.changed.emit(value, prev_value)?

yes, the signature is up to the emitter... so you can add as many or as few positional params:

class T:
    changed = Signal(str, str)  # new, previous

t = T()
t.changed.emit('new', 'old')

the receiver, however, can accept as many (or as few) of the params as it wants:

@t.changed.connect
def i_take_them_all(new_val: str, old_val: str): ...

@t.changed.connect
def but_this_is_fine(): ...

@t.changed.connect
def so_is_this(new_val: str): ...

• you mention that having a big event object makes sense for mouse and keyboard events, but couldn't that just be the value anyway? ie value can be an arbitrarily complex object so I'm not sure why this would prevent us from unifying our event system.

I'm not sure either... just observing that Qt seems to reserve mouse and keyboard events for non signals. I don't know that it's just related to complexity. need to ponder that.

• does this still have the block() context manager? (probably reading the code will clarify but wanted to write down all my thoughts)... Yup, except it's "blocked". I would suggest renaming to "block"?

I dunno... I kind of favor past tense "blocked" in context managers cause it reads like a sentence:

with signal.blocked():
    do_something()

# this feels a little less "englishy" 🤪
with signal.block():
    do_something()

besides, signal.block() and signal.unblock() are already in use for the non-context managed (permanent) signal blocker

Could we also make a decorator for emitting when a function/method is called? e.g.
@changed.emit_when

interesting! I do kinda like it. two things to note though:

• probably the oddest issue here is that most of the time, the signal instance (changed) will be an object on an instance of a class... so really it would be self.changed.emit_when, which makes it harder to use as a class method decorator, since you won't have self yet. (but, for solo signal instance usage it would be fine)
• emit() needs arguments... so in your example, I guess the return value of fill(self, position, value): would have to match the signature expected by signal.emit?

could be handy!

[jni]

the receiver, however, can accept as many (or as few) of the params as it wants:

lovely.

I dunno... I kind of favor past tense "blocked" in context managers cause it reads like a sentence:

Interesting, I guess with open() has changed my way of thinking here, but ultimately I think you're right. And...

besides, signal.block() and signal.unblock() are already in use for the non-context managed (permanent) signal blocker

I guess that makes it pretty final 😂

which makes it harder to use as a class method decorator, since you won't have self yet.

Yeah, I guess I was hoping that some kind of magic could be done so that it would apply to the instance. But perhaps it's too hard/magicky to bother.

I guess the return value of fill(self, position, value): would have to match the signature expected by signal.emit?

Yes, that was my idea. Currently most/all of our in-place modifiers don't return anything, so I don't think that's a big ask!