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 and make colors configurable #156

Merged
merged 3 commits into from
Dec 3, 2023

Conversation

dmfay
Copy link
Contributor

@dmfay dmfay commented Jun 3, 2021

This started because I wanted to customize colors, and turned into something of a rabbit hole. To summarize:

  • dynamically setting colors in termion is awkward (cf broot)
  • the relevant feature request is two years old with minimal activity
  • in the mean time, broot has replaced termion with crossterm, which is not quite a drop-in API replacement, improves on it in the colors department, and opens up the possibility of supporting Windows since termion is *nix-only

Outside the terminal code, mcfly now reads colors and other settings from ~/.mcfly/mcfly.toml, defaulting to the original styling and config if the file doesn't exist. Environment variables are still supported and there's a self-describing sample config file. I don't have a Windows machine to test compatibility, so there are likely as not more obstacles, but it's a big step closer.

I did remove MCFLY_LIGHT entirely, so this conflicts with #151 (sorry!). If we want multiple colorschemes, figment's switchable profiles may be useful.

It also conflicts with #149, but that I can rebase.

@cantino
Copy link
Owner

cantino commented Jun 5, 2021

Thanks @dmfay for working on this! Since some users use MCFLY_LIGHT, how easy do you think it would be to add it for backwards compatability?

Also, please note that my wife and I just had our first child this week and so I will be slow to reply for a while...

@dmfay
Copy link
Contributor Author

dmfay commented Jun 5, 2021

Congratulations to both of you!

Detecting MCFLY_LIGHT and supplying alternate default colors turned out to be trivial, and would clear the way for #151 too -- updated.

@cantino
Copy link
Owner

cantino commented Jun 6, 2021

Nice!

@dmfay
Copy link
Contributor Author

dmfay commented Jun 6, 2021

Here's the rebase, for whenever's convenient! This version also prints a full-width cursor, but enough else changed over time in interface.rs that squashing was the better part of valor.

@cantino
Copy link
Owner

cantino commented Jun 20, 2021

Hey @dmfay, are you up for another rebase? I think the only change is moving ctrl-d to delete a character instead of exit from the interface.

I'm getting this error when I try to build this PR:

error[E0658]: the `#[track_caller]` attribute is an experimental feature
  --> /Users/andrew/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/figment-0.10.5/src/providers/serialized.rs:75:5
   |
75 |     #[track_caller]
   |     ^^^^^^^^^^^^^^^
   |
   = note: see issue #47809 <https://github.com/rust-lang/rust/issues/47809> for more information

error[E0658]: the `#[track_caller]` attribute is an experimental feature
  --> /Users/andrew/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/figment-0.10.5/src/providers/serialized.rs:91:5
   |
91 |     #[track_caller]
   |     ^^^^^^^^^^^^^^^
   |
   = note: see issue #47809 <https://github.com/rust-lang/rust/issues/47809> for more information

Do we have to use experimental features now?

@dmfay
Copy link
Contributor Author

dmfay commented Jun 20, 2021

Updated! As for experimental features, you might need to update Rust? I'm not seeing that issue with 1.53 (and track_caller has made it to stable).

@cantino
Copy link
Owner

cantino commented Jun 21, 2021

I updated rust and it builds successfully! However, now I'm getting:

thread 'main' panicked at 'McFly error: failed to read input IoError(Custom { kind: Other, error: "Failed to initialize input reader" })', src/interface.rs:340:43

When I try to launch mcfly.

@dmfay
Copy link
Contributor Author

dmfay commented Jun 22, 2021

I can replicate that with osx + zsh + ctrl-r -- in linux it's fine, in bash it's fine, running mcfly search works. Nothing jumps out at me in the zsh scripting though, and the full backtrace I cleaned up doesn't offer any obvious leads either.

thread 'main' panicked at 'McFly error: failed to read input IoError(Custom { kind: Other, error: "Failed to initialize input reader" })', src/interface.rs:340:43
stack backtrace:
0: 0x10d0ce754 - std::backtrace_rs::backtrace::libunwind::trace::h67b1783e93f517c7
      at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/../../backtrace/src/backtrace/libunwind.rs:90:5
1: 0x10d0ce754 - std::backtrace_rs::backtrace::trace_unsynchronized::h6642b95f229b3f12
      at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5
2: 0x10d0ce754 - std::sys_common::backtrace::_print_fmt::h0a233062426578e9
      at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/sys_common/backtrace.rs:67:5
3: 0x10d0ce754 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h5b5b7de1c79b4b45
      at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/sys_common/backtrace.rs:46:22
4: 0x10d0f04ce - core::fmt::write::h65ec8c8a6aa7b549
      at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/fmt/mod.rs:1094:17
5: 0x10d0cb1da - std::io::Write::write_fmt::hc52e560ab9e57db4
      at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/io/mod.rs:1584:153c2-1590682
6: 0x10d0d058f - std::sys_common::backtrace::_print::h27e4664250aa8eaf
      at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/sys_common/backtrace.rs:49:5
7: 0x10d0d058f - std::sys_common::backtrace::print::hb149f667a3c579ca
      at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/sys_common/backtrace.rs:36:9
8: 0x10d0d058f - std::panicking::default_hook::{{closure}}::h0974624c30a6e2b5
      at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/panicking.rs:208:50
9: 0x10d0d0099 - std::panicking::default_hook::hda3cfa32e0773aac
     at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/panicking.rs:225:9
10: 0x10d0d0d55 - std::panicking::rust_panic_with_hook::h1cdaa4b48d8ea4a1
      at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/panicking.rs:591:17
11: 0x10d0d0865 - std::panicking::begin_panic_handler::{{closure}}::h51157b4dbc4c5d91
   at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/panicking.rs:497:13
12: 0x10d0cebe8 - std::sys_common::backtrace::__rust_end_short_backtrace::h8f7fee8b4c5a0c05
      at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/sys_common/backtrace.rs:141:18
13: 0x10d0d07ca - rust_begin_unwind
     at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/panicking.rs:493:5
14: 0x10d0d077b - std::panicking::begin_panic_fmt::h2d14b6e3309decb9
  at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/panicking.rs:435:5
15: 0x10cb1e3fd - mcfly::interface::Interface::select::{{closure}}::h90d8e7250ab1ab3c
    at /Users/dian/work/mcfly/src/interface.rs:340:43
16: 0x10ca507af - core::result::Result<T,E>::unwrap_or_else::h9fb4708dbefaa746
    at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/result.rs:821:23
17: 0x10cb1dd3d - mcfly::interface::Interface::select::h42cf7b2130881907
    at /Users/dian/work/mcfly/src/interface.rs:340:17
18: 0x10cb18c03 - mcfly::interface::Interface::display::h5541174453fd958a
     at /Users/dian/work/mcfly/src/interface.rs:111:9
19: 0x10ca0d907 - mcfly::handle_search::h408c96b2c1212afb
       at /Users/dian/work/mcfly/src/main.rs:46:18
20: 0x10ca0e325 - mcfly::main::h8d16aec9d2e01a04
  at /Users/dian/work/mcfly/src/main.rs:107:13
21: 0x10ca0e55e - core::ops::function::FnOnce::call_once::h0f631598e64ddf41
      at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/ops/function.rs:227:5
22: 0x10ca0edd1 - std::sys_common::backtrace::__rust_begin_short_backtrace::hec7b2049f2174e7a
  at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/sys_common/backtrace.rs:125:18
23: 0x10ca0ebc4 - std::rt::lang_start::{{closure}}::h44dd4ec2cae74050
     at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/rt.rs:49:18
24: 0x10d0d10c1 - core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once::hc43f50f6c262e503
     at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/ops/function.rs:259:13
25: 0x10d0d10c1 - std::panicking::try::do_call::ha2a21b2645967438
    at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/panicking.rs:379:40
26: 0x10d0d10c1 - std::panicking::try::h1ca18252e93fb057
  at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/panicking.rs:343:19
27: 0x10d0d10c1 - std::panic::catch_unwind::h64441306bfaea78f
    at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/panic.rs:431:14
28: 0x10d0d10c1 - std::rt::lang_start_internal::h37583e1fa93097c4
       at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/rt.rs:34:21
29: 0x10ca0eb9e - std::rt::lang_start::h94acaba22ce781a2
  at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/rt.rs:48:5
30: 0x10ca0e3b6 - _main

crossterm-rs/crossterm#500 may be relevant -- here's another crossterm-using project with an osx-only problem. I disabled the terminfo changes and tty pipe in mcfly-history-widget and that didn't avert a panic, though.

@bahlo
Copy link

bahlo commented Jul 21, 2021

Hey, I just want to say I'd love to have configurable colors, this looks broken in Unikitty Light as well. 👍 I tried your branch @dmfay and I get the same results.

load and apply config from ~/.mcfly/mcfly.toml

sample config file

remove lightmode in favor of color customization

interface cleanup and formatting

correct example key_scheme

supply alternate default colors for MCFLY_LIGHT
@MitMaro
Copy link

MitMaro commented Jan 11, 2023

👋🏼 Another Crossterm user here, that just happened to have one of my projects linked in a comment. Crossterm just pushed a fix for crossterm-rs/crossterm#500, so it's possible that the problem blocking this PR has been addressed.

I haven't tried the fix in my project yet, but figured a ping here wouldn't hurt.

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