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

Stack event handlers #304

Merged
merged 3 commits into from
Feb 7, 2024
Merged

Conversation

jrmoulton
Copy link
Collaborator

No description provided.

@@ -96,41 +96,47 @@ pub trait Decorators: View + Sized {
///
/// NOTE: View should have `.keyboard_navigable()` in order to receive keyboard events
fn on_key_down(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on_key_down and on_key_up are already stackable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the handlers from view data and made them just work like all of the other event handlers. Is there a reason to leave it this way?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. They have less startup cost as they directly mutate ViewData and don't need to send an update message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should any other events be implemented with handlers in ViewData?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move as many as possible to use ViewData.event_handlers. Just have to be careful to not change behavior. I'd probably add a ViewData::add_event_handler method which boxes and pushes the handler for convenience.

Keep that in a separate PR if you give it a shot.

src/context.rs Outdated Show resolved Hide resolved
@dzhou121 dzhou121 merged commit a035cfd into lapce:main Feb 7, 2024
7 checks passed
@jrmoulton jrmoulton deleted the add-stacking-handlers branch September 18, 2024 02:36
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.

3 participants