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

menubar should bubble up events which are not handled #493

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JoshMcguigan
Copy link

Hello, and thanks again for all your work on this crate.

I found an issue where global callbacks were not being called while the menubar was focused. This PR fixes that by ensuring that events bubble up if they are not handled while the menu bar is focused.

I'm not sure this is the most direct way to accomplish this, since it seems if the menubar is focused we could somehow directly call the global events rather than relying on View::on_event(&mut self.root, event.relativized((0, offset))) to do it. But I wasn't sure exactly how to accomplish that. This PR does seem to work as-is.

@JoshMcguigan
Copy link
Author

Might be worth noting here that the whole reason I noticed this was I wanted vim style navigation, so I mapped hjkl to the left-down-up-right events using global callbacks. But then my navigation didn't work when the menubar was focused (although it did work once I selected one of the top level menu items).

I'm not sure if there is a better way to handle these types of low level keyboard remappings?

@gyscos
Copy link
Owner

gyscos commented Aug 24, 2020

Hi, and thanks for the PR!

I see the problem - since we moved global callbacks to the root view, it's hard to have an event trigger a global callback without sending it to the root view, where it could potentially be caught by the regular views - which would be weird when the menubar is focused (for example you could still write a message in an input box with a menu open).

The ideal long-term solution is probably to push the menubar into a regular view, and put that behind the root-level OnEventView. But we're not there yet.

A workaround would be to somehow send the event to the root OnEventView, but only for the callbacks, not for the view itself - but right now OnEventView doesn't really let you trigger the registered callbacks without poking the child view.

None of these two solutions are trivial, but the workaround is probably less up-front cost.

@JoshMcguigan
Copy link
Author

I think you are suggesting adding some mechanism to allow OnEventView to expose the registered callbacks? Perhaps something like self.root.execute_callbacks_on(event)?

Would there be any restrictions on running the BeforeChild vs AfterChild? I think in practice for this use case we could get away with only running AfterChild since that is how the global callbacks are registered, but for simplicity I think it makes sense to run all callbacks.

@gyscos
Copy link
Owner

gyscos commented Feb 5, 2021

I think you are suggesting adding some mechanism to allow OnEventView to expose the registered callbacks? Perhaps something like self.root.execute_callbacks_on(event)?

Sorry for the delay - yes, this is exactly what I was thinking about.

Would there be any restrictions on running the BeforeChild vs AfterChild? I think in practice for this use case we could get away with only running AfterChild since that is how the global callbacks are registered, but for simplicity I think it makes sense to run all callbacks.

Yeah we should run all callbacks here. Global callbacks default to AfterChild, but there's still Cursive::set_on_pre_event to register BeforeChild events.

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 this pull request may close these issues.

2 participants