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

connecting event vs value outputs #173

Closed
berl opened this issue Mar 2, 2021 · 5 comments
Closed

connecting event vs value outputs #173

berl opened this issue Mar 2, 2021 · 5 comments

Comments

@berl
Copy link

berl commented Mar 2, 2021

I found the lambda e: callback(e.value.value) here a little confusing, especially given the current docs where the callback is shown receiving the event from the called signal and then pulling out the event.value to print it (or whatever).

Idea: in addition to the lovely selection of signals, could magicgui provide a bare value returned by the decorated function? My feeling here is that someone using magicgui should have some way of defining what gets passed to a callback without having to make a lambda or debug how many times to get a .value attribute. Side benefit is that downstream code used as callbacks wouldn't have to modify the inputs either.

@tlambert03
Copy link
Member

tlambert03 commented Mar 2, 2021

totally agree, that's a strange case! ... I honestly can't event remember now why you need a double .value.value there 🤔 probably because the FileEdit is a compound widget (both a line edit and a button). but that's no excuse, that should (and can) definitely just be event.value

Idea: in addition to the lovely selection of signals, could magicgui provide a bare value returned by the decorated function?

I agree, that that one additional level is kind of annoying, and we could consider just calling callbacks with the new value (rather than the event object itself) ... but a fair amount of additional information about the event that triggered the change is unfortunately lost in that case. I'm not saying it's not a worthwhile loss, but it doesn't come without a cost to flexibility.

someone using magicgui should have some way of defining what gets passed to a callback without having to make a lambda or debug how many times to get a .value attribute.

outside of the strange FileEdit bug, it should should just always be a single .value:

def my_callback(event):
	new_widget_value = event.value

widget.changed.connect(my_callback)

as for the user indicating what the callback should get passed... that seems rather complicated actually. It's pretty typical for a callback to be required to accept a standard argument. As mentioned above, we can definitely discuss & change what that argument(s) should be... but I'm not sure it should be user-adjustable.

As for the lambda, that's purely optional syntax. No users have to make lambdas if they don't want to, they can always use a named function as shown above.

So, thanks for opening this! I do think it's worth considering changing what the callback is given. That would be a relatively big breaking change though, so would require a major version change. In the meantime, I'll fix the FileEdit and you can generally assume (with this one exception) that the new value is at event.value

@berl
Copy link
Author

berl commented Mar 3, 2021

thanks for the discussion and the tips!

My reading of magicgui's goal in life is to simplify gui programming so much that python programmers and scientists can hook up a quick gui without a bunch of pyqt syntax or specialized knowledge. In that spirit, I'm imagining a scenario where magicgui-decorated function returns a thing and the user could just write a function expecting a thing as an argument and the whole changed.connect() -> callback(event) ->event.value would be hidden from the user. maybe call it my_magicgui_function.output.connect? The regular changed and called events would still be available of course.

anyway, I appreciate your work on magicgui and I'll close this out

@berl berl closed this as completed Mar 3, 2021
@tlambert03 tlambert03 reopened this Mar 3, 2021
@tlambert03
Copy link
Member

gonna leave this open because I think it's worth thinking about some more. I was already kinda leaning towards callbacks being given the thing directly, rather than needing to access it at event.value. Your reading of magicgui's goal in life is spot on, and I agree that having to receive an event object (instead of thing itself), is added complexity. Maybe those who want the full event object (likely a minority) can use a different connect function, like my_function.called.connect_event_callback(). It's verbose, but that's fine. Thoughts?

@berl
Copy link
Author

berl commented Mar 4, 2021

I like the idea of different names for the connecting the full event callback vs the thing callback!

I agree that the people who want the full event will eventually be a pretty small subset of users, but keeping it around like you propose seems like it covers both use cases. Isn't there someone's law that states that if a design change satisfies two opposing use cases, it must also be a hassle to implement?

@tlambert03
Copy link
Member

with 0.3.0, I think we can finally put this issue to bed.

the old approach ...

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

now becomes ...

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

see #253 for details. Thanks for the original motivation @berl ...

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

No branches or pull requests

2 participants