Add Kitty protocol keyboard enhancement support #607
Conversation
|
Thanks for implementing it and adding the check before enabling. If everything works fine when playing with the demo we can land this |
|
I found some issue when connecting to SSH. It seems the terminal still use Kitty protocol even after connecting to SSH. I'm not sure how to solve this. |
|
@Abdillah |
src/engine.rs
Outdated
| } | ||
|
|
||
| /// Enable keyboard enhancement to disambiguate escape code | ||
| pub fn enable_kitty_protocol(&mut self) -> Result<()> { |
There was a problem hiding this comment.
maybe a dumb question here 😌
do enable_kitty_protocol and disable_kitty_protocol need to return a Result<()>?
nothing in the function can fail, right?
There was a problem hiding this comment.
Ah yes, I followed prior patterns. It might be best to just.. no return?
There was a problem hiding this comment.
Ah yes, I followed prior patterns. It might be best to just.. no return?
this is why i said it might be a dumb question because i've never had a look at the source base of this repo so far...
looking at what the function does, it looks to me it could just be
pub fn enable_kitty_protocol(&mut self) {
self.use_kitty_protocol = true;
}and if it's not an implementation of some trait 🤔
again, not sure if it should be removed or not, just looking locally at this particular function 😌
There was a problem hiding this comment.
I think I will stick with the Result despite some itch 😆
There was a problem hiding this comment.
I think we should only do this if necessary. The other commands directly write to STDOUT to perform their operation and are just fallible. As long as this is just setting an internal flag I think it should be infallible.
There may be an argument if we were to move the check for the flags here but as long we don't do it, let's not make preemptive API decisions. reedline is still quite liberal with making breaking changes if necessary. Until then lets keep the API as simple as possible.
Not sure, @amtoine, I haven't filled any issue nor looking for it. My initial guess is, it just because the terminal recognize the running program as between enable and disable kitty protocol:
|
Abdillah
force-pushed
the
feature/kittyproto
branch
2 times, most recently
from
bc0cab8
to
8cc8c4e
Compare
|
The |
no worries, i see quite a lot of issues, i might just have hallucinated this one 😉 |
src/engine.rs
Outdated
| if self.use_kitty_protocol { | ||
| let _ = execute!(io::stdout(), event::PopKeyboardEnhancementFlags); | ||
| } |
There was a problem hiding this comment.
This is probably the wrong place to handle this, we have the split into read_line and read_line_helper to make sure that we disable extended terminal modes when leaving read_line independent of whether it succeeded or exited with an Err.
There was a problem hiding this comment.
Got it, I'll move out from the _helper as is, since the pop operation is perceived as infallible.
src/engine.rs
Outdated
| if self.use_kitty_protocol { | ||
| let _ = execute!(io::stdout(), event::PopKeyboardEnhancementFlags); | ||
| } |
There was a problem hiding this comment.
To be defensive we could keep this in case the application hosting reedline panics.
There was a problem hiding this comment.
Oh that's a good idea
src/engine.rs
Outdated
| } | ||
|
|
||
| /// Enable keyboard enhancement to disambiguate escape code | ||
| pub fn enable_kitty_protocol(&mut self) -> Result<()> { |
There was a problem hiding this comment.
I think we should only do this if necessary. The other commands directly write to STDOUT to perform their operation and are just fallible. As long as this is just setting an internal flag I think it should be infallible.
There may be an argument if we were to move the check for the flags here but as long we don't do it, let's not make preemptive API decisions. reedline is still quite liberal with making breaking changes if necessary. Until then lets keep the API as simple as possible.
|
May I |
|
Feel free to rebase |
Abdillah
force-pushed
the
feature/kittyproto
branch
from
448c668
to
b7bc8ef
Compare
|
Hello @sholderbach, is there anything else I can do to make this PR ready to merge? |
Background
Supporting Kitty keyboard enhancement protocol to enable vi action on other keyboard layout (e.g. ctrl+i on Colemak). Without this, ctrl+i will equivalent to Tab and cannot be remapped to anything else.
Change Summary
Add config
use_kitty_protocolto determine whether to enable kitty protocol or not. Then if the terminal support this, send the request to enable this terminal feature. Also disable it back whenthe engine droppedthe event handler finished.