cursive icon indicating copy to clipboard operation
cursive copied to clipboard

menubar should bubble up events which are not handled

Open JoshMcguigan opened this issue 4 years ago • 4 comments

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 avatar Aug 21 '20 21:08 JoshMcguigan

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?

JoshMcguigan avatar Aug 22 '20 13:08 JoshMcguigan

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.

gyscos avatar Aug 24 '20 17:08 gyscos

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.

JoshMcguigan avatar Aug 25 '20 03:08 JoshMcguigan

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.

gyscos avatar Feb 05 '21 18:02 gyscos