-
-
Notifications
You must be signed in to change notification settings - Fork 561
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
crossterm support #331
crossterm support #331
Conversation
A curiosity, I've decided to try partial upgrade (tui 0.18 + termion) and it's still eating my arrow key inputs, but otherwise still responsive. So I'm not sure what's going on there |
Do you know what mode your terminal is in? Different modes can send different escape sequences, and it could be listening for the wrong ones EG we switch to cursor mode here: https://github.com/ellie/atuin/blob/main/src/shell/atuin.zsh#L34 and perhaps termion doesn't like that |
Yeah, I was debugging and termion/crossterm never get the up/down. Which made me try again with our current version. When running |
Yup I had this when I was writing the initial implementation. Some applications can switch terminal mode and then don't switch it back, leaving other programs in a broken state Terminal stuff has a lot of baggage 😂 But otherwise it all works? search -i is only really supposed to run from the binding anyway 🤔 |
I'm rebasing this branch now, I'll let you know |
6405958
to
ce87487
Compare
Ok, final conclusion since I've spent a while debugging this now. It's a combination of quite a few problems:
mio said they won't fix this specific behaviour, tbh I agree: tokio-rs/mio#1377 There's an open PR for crossterm to manually use So far, this patch fixed it for me |
ce87487
to
82021bf
Compare
3f48063
to
dca7652
Compare
82021bf
to
15b5976
Compare
@conradludgate looks like there's been some motion on that crossterm PR, and now this has been opened |
Yep. I've been following it. Hopefully it gets merged at some point |
15b5976
to
ae6bf4d
Compare
Just waiting for a new crossterm release after the merge ⏳ |
Crossterm 0.26 is out 🎉 |
I have updated to 0.26 crossterm and started vendoring a minimal subset of tui (now that it's maintenance is in question). It can be trimmed down further, but I don't want to go too minimal incase there are features we end up using. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!! Super excited we are finally ready to do this hahaha, thank you for working on this for so long.
@@ -0,0 +1,5 @@ | |||
# tui-rs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense that we are vendoring this, but it would be nice to be able to remove it someday in The Future™️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, slowly we can either find a replacement or slim it down as much as possible to make the API as good for us as possible
widgets::{StatefulWidget, Widget}, | ||
}; | ||
use std::io; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @conradludgate @ellie,
I started to try and rebase my crossterm/inline PR #648, and figured that the version of tui-rs that was copied here is missing the bits needed from https://github.com/fdehau/tui-rs/pull/552/files#diff-554109cedfab40c7397c98d70e7a7449266358293b29ad5aa8db2f79e958122bR11 to make inline viewport possible.
Should I try and backport fdehau/tui-rs#552 on this local copy?
Any reason why git history from upstream was not preserved using git subtree?
Also, wouldn't maintaining a fork of tui-rs make things easier, at least until it is not needed anymore at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A full fork WG is already in the works and I'm taking part as a stakeholder. It seems we won't be vendoring it for long.
I'm not an expert in all the git features so I didn't think to use subtree. It sounds like it would be awkward though because we want it as a module and not a crate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I guess should I copy the parts needed from the upstream inline PR then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok let's do it 🚀
For #282, fixes #283.
Todo