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

WidgetClassExt::add_binding's callback returns a bool instead of ControlFlow #1505

Closed
A6GibKm opened this issue Oct 1, 2023 · 4 comments · Fixed by #1512
Closed

WidgetClassExt::add_binding's callback returns a bool instead of ControlFlow #1505

A6GibKm opened this issue Oct 1, 2023 · 4 comments · Fixed by #1512
Milestone

Comments

@A6GibKm
Copy link
Contributor

A6GibKm commented Oct 1, 2023

Normally one would return gdk::EVENT_PROPAGATE or gdk::EVENT_STOP in these, but you can't since the later are u32 instead of bool. I think this should retun a glib::ControlFlow.

https://gtk-rs.org/gtk4-rs/git/docs/gtk4/subclass/widget/trait.WidgetClassExt.html#method.add_binding

@sdroege
Copy link
Member

sdroege commented Oct 2, 2023

@A6GibKm Want to provide a PR?

@bilelmoussaoui
Copy link
Member

It would be nice to check all the subclassing code for cases where we return a boolean as well

@bilelmoussaoui bilelmoussaoui added this to the 0.8.0 milestone Oct 6, 2023
@bilelmoussaoui
Copy link
Member

The callback you pass ends up being used for creating a CallbackAction, https://gtk-rs.org/gtk4-rs/git/docs/gtk4/struct.CallbackAction.html#method.new. The callback is a https://docs.gtk.org/gtk4/callback.ShortcutFunc.html and as you can see, TRUE means the action was successful but i am not sure if that maps correctly. Continue doesn't convey the action was handled

@bilelmoussaoui
Copy link
Member

I would have closed the issue as I don't think we should change the type here but I have noticed few cases in the manual code where it would make more sense to switch to using ControlFlow/Propagation.

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

Successfully merging a pull request may close this issue.

3 participants