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

Addition of support for the Kitty keyboard protocol. #5

Merged
merged 2 commits into from
Jun 5, 2024

Conversation

cxreiff
Copy link
Collaborator

@cxreiff cxreiff commented Jun 5, 2024

Changes

  • Supplies enable_kitty_protocol method that uses crossterm helper methods to enable the Kitty keyboard protocol.
  • Cleans up kitty protocol flags when leaving ratatui alternate view.
  • New example showing how to use the method and one example of what it supplies (keyboard "release" events).
  • EDIT: Also added the cursor::Show during cleanup, necessary in some terminals.

Justification

One could argue that kitty protocol support is somewhat orthogonal to what an integration between bevy and ratatui should be responsible for, but I think there are two good arguments for inclusion:

  • Bevy users will be accustomed to more keyboard events than terminals supply by default, and especially if the eventual plan is to convert crossterm events into the standard bevy ones, this protocol will be required for better parity.
  • If the kitty protocol flags are pushed before the ratatui alternate view is created it can cause issues with using the protocol (or if the alternate view is exited before the flags are removed the terminal might not be left in the proper state). For this reason, handling the protocol here might cause less user frustration than pointing them elsewhere or making them add the flags themselves.

Alternative

Alternatively, for an even more seamless setup we could just push the flags as part of the context initialization and store whether the protocol was enabled as part of the RatatuiContext struct. Opinion here?

Testing

Tested with Alacritty, Kitty, iTerm, and WezTerm on macOS.

modified crossterm event handler to pass along all EventKind variants.
added example showing how to enable the kitty protocol and listen for
"release" events.
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 17 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (0e57e80) to head (f2bd462).

Files Patch % Lines
src/terminal.rs 0.00% 12 Missing ⚠️
src/event.rs 0.00% 5 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main      #5   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files          4       4           
  Lines        116     129   +13     
=====================================
- Misses       116     129   +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joshka
Copy link
Owner

joshka commented Jun 5, 2024

EDIT: Also added the cursor::Show during cleanup, necessary in some terminals.

In theory that's handled by Terminal::drop - https://github.com/ratatui-org/ratatui/blob/main/src/terminal/terminal.rs#L91-L95, but I suppose that it likely suffers the same problems that happen when panics happen (maybe - I don't 100% recall if drop gets skipped). There was some discussion on a bevy issue about adding a shutdown / teardown system similar to the way the startup systems run, but it petered out due to lack of interest. I was thinking that this might be a good reason to go back to that idea.

One could argue that kitty protocol support is somewhat orthogonal to what an integration between bevy and ratatui should be responsible for, but I think there are two good arguments for inclusion:

I had a vague thought that perhaps it's worth splitting bevy_ratatui and bevy_crossterm perhaps, but for now it's useful to think of them as paired (and there's an existing https://crates.io/crates/bevy_crossterm that seems to be abandoned - it uses a custom runner approach rather than just using the existing systems / schedules like I have).

For now, I'm fine with keeping this sort of thing in the lib, then we can work out where it goes when evolving. (get it working, get it right, get it performing)

Bevy users will be accustomed to more keyboard events than terminals supply by default, and especially if the eventual plan is to convert crossterm events into the standard bevy ones, this protocol will be required for better parity.

Yup - I want that - you're thinking the same thing as me there.

Alternatively, for an even more seamless setup we could just push the flags as part of the context initialization and store whether the protocol was enabled as part of the RatatuiContext struct. Opinion here?

I'll think about this as I look through the code. I ran into a similar problem with mouse capture implementing the crossterm / mouse events earlier today.

Copy link
Owner

@joshka joshka left a comment

Choose a reason for hiding this comment

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

LGTM

@joshka joshka merged commit 3176694 into joshka:main Jun 5, 2024
11 checks passed
@github-actions github-actions bot mentioned this pull request Jun 5, 2024
@cxreiff
Copy link
Collaborator Author

cxreiff commented Jun 5, 2024

Thank you for fixing that up!

@cxreiff cxreiff deleted the kitty_support branch June 5, 2024 15:23
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