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

Replace termion with crossterm #296

Merged
merged 20 commits into from
Mar 4, 2023
Merged

Conversation

jtschuster
Copy link
Collaborator

This isolates the changes from #280 that are related to crossterm.

@cantino Hopefully this makes it easier to review and test yourself, and PowerShell/Windows support can be added separately.

@jtschuster jtschuster marked this pull request as draft November 7, 2022 16:47
@cantino
Copy link
Owner

cantino commented Nov 27, 2022

Is this ready for testing @jtschuster?

@jtschuster jtschuster marked this pull request as ready for review November 30, 2022 20:35
@jtschuster
Copy link
Collaborator Author

@cantino Sorry for the delay, it should be ready to look at now.

Copy link
Owner

@cantino cantino left a comment

Choose a reason for hiding this comment

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

I get

 $ thread 'main' panicked at 'McFly error: failed to read input IoError(Custom { kind: Other, error: "Failed to initialize input reader" })', src/interface.rs:413:43
 git st                                             note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace                                                                                           2h 11m

On my Mac when I run this.

src/interface.rs Show resolved Hide resolved
@jtschuster
Copy link
Collaborator Author

It looks like there is a known issue with crossterm on mac when /dev/tty is used instead of stdin. Since it looks like zsh's zle uses the tty, that seems to be causing issues. It's working for me on linux with bash and zsh, and mac with bash, but not zsh.
There's a couple PR's looking to address the issue, one stale and one new:
crossterm-rs/crossterm#407
crossterm-rs/crossterm#711

@jtschuster
Copy link
Collaborator Author

I've also just set the crossterm dependency to reference the in-progress pr crossterm-rs/crossterm#711 instead of the released version and it seems to work in zsh on mac now (at least doesn't panic).

@jtschuster
Copy link
Collaborator Author

@cantino Sorry for letting this get stale, but it should be ready to take another look at now. The crashing on zsh issue was fixed in crossterm v0.26, and it seems to be working for me. Please take a look when you can.

@cantino
Copy link
Owner

cantino commented Feb 26, 2023

Thank you for your hard work on this @jtschuster. Have you been using this personally without issue?

I've tested on fish, zsh, and bash on my mac and zsh on Ubuntu. Seems to work great!

Minor: The highlight color is a bit hard to read on my Mac on iTerm2:

image

Is now a good time to allow color customization?

@jtschuster
Copy link
Collaborator Author

Thank you for your hard work on this @jtschuster. Have you been using this personally without issue?

I use windows primarily, but I've been using #280 without issues so far, and haven't hit issues testing this branch yet.

Minor: The highlight color is a bit hard to read on my Mac on iTerm2:

I just pushed a commit that changes the highlight color to DarkGreen and it's much more readable.

Is now a good time to allow color customization?

I wouldn't want to add too much to this PR, but it should be easy to rebase #156 once this is in.

@cantino
Copy link
Owner

cantino commented Feb 27, 2023

Makes sense!

In light mode, the text you've typed isn't visible when selected:

image

While it is visible in dark mode:

image

And it is on master:
image

@jtschuster
Copy link
Collaborator Author

Good catch! I've pushed a commit that makes it match the behavior in master.

@praveenperera
Copy link
Collaborator

Thanks for the or @jtschuster, i’m going to test this out locally tomorrow. For now would you find fixing the last clippy warnings?

@praveenperera
Copy link
Collaborator

praveenperera commented Mar 3, 2023

@jtschuster @cantino I've been using this branch, its been good

edit: please run cargo fmt

@cantino
Copy link
Owner

cantino commented Mar 4, 2023

One more merge and I think we're good to go on this.

@cantino cantino merged commit 85e024f into cantino:master Mar 4, 2023
@cantino
Copy link
Owner

cantino commented Mar 4, 2023

Thanks @jtschuster! Awesome work.

@cantino
Copy link
Owner

cantino commented Mar 7, 2023

Released in v0.8.0.

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.

4 participants