cantino / mcfly

Fly through your shell history. Great Scott!
MIT License
6.76k stars 176 forks source link

Replace termion with crossterm #296

Closed jtschuster closed 1 year ago

jtschuster commented 1 year ago

This isolates the changes from https://github.com/cantino/mcfly/pull/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.

cantino commented 1 year ago

Is this ready for testing @jtschuster?

jtschuster commented 1 year ago

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

jtschuster commented 1 year ago

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: https://github.com/crossterm-rs/crossterm/pull/407 https://github.com/crossterm-rs/crossterm/pull/711

jtschuster commented 1 year ago

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

jtschuster commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/cantino/mcfly/pull/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 https://github.com/cantino/mcfly/pull/156 once this is in.

cantino commented 1 year ago

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 commented 1 year ago

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

praveenperera commented 1 year ago

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 commented 1 year ago

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

edit: please run cargo fmt

cantino commented 1 year ago

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

cantino commented 1 year ago

Thanks @jtschuster! Awesome work.

cantino commented 1 year ago

Released in v0.8.0.