cantino / mcfly

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

Automatically detect light terminal on initialization. Implements #150. #151

Open cmer opened 3 years ago

cantino commented 3 years ago

Does this need something like from @jltml's comment?

  if [[ $(defaults read -g AppleInterfaceStyle) != 'Dark' ]]; then
    export MCFLY_LIGHT=TRUE
  fi
cmer commented 3 years ago

I also implemented Apple Terminal light mode detection.

One thing to keep in mind, though, is that this method sets the color scheme when the shell is first initialized. If dark mode changes after the shell has been initialized, Mcfly will display the wrong color scheme. This is partly why I had first suggested implementing this logic directly in the Rust codebase so that the checks can be made on each invocation of Mcfly.

This situation can happen if people's color scheme changes automatically at day/night time, for example.

Would you consider implementing this in the main codebase, @cantino?

cantino commented 3 years ago

That's a good point. I guess it's a bit annoying to have to call things like defaults read -g AppleInterfaceStyle from within the Rust code, but maybe it's okay. What do you think?

jltml commented 3 years ago

Totally up to you — I think it would be really nice to have it within the Rust codebase so it's dynamic/set upon each invocation (I have a tendency to leave Terminal windows open for days, so this would be really nice), but I agree with the annoyingness of having it in the Rust codebase too. Also, I just timed how long defaults read -g AppleInterfaceStyle takes with Hyperfine, and on my 2017 i5 MacBook Pro it took ≈ 8.5 ms ± 0.8 ms… so I suppose it would make startup that little bit slower too. It's definitely a trade-off, but maybe it could be controlled by an environment variable — something like MCFLY_LIGHT=auto? — so that there's a choice between saving the 8ms and setting the theme dynamically? Just an idea; it could very well be a terrible one :)

I'd love to implement this myself, but unfortunately I have yet to learn Rust!

cmer commented 3 years ago

It'd be great if we could only rely on env variables but it's not the case. However, that slight slow down would only happen on Apple Terminal, and if MCFLY_LIGHT is not set. It might be worth the tradeoff.

I suggest:

The "magic" would only happen when MCFLY_LIGHT is not set at all. This holds true for all terminals, not just Apple Terminal.

cantino commented 3 years ago

slight slow down would only happen on Apple Terminal, and if MCFLY_LIGHT is not set

How do we know when we're in that terminal and need to shell out to defaults?

cmer commented 3 years ago

See my implementation here: https://github.com/cantino/mcfly/pull/151/commits/55554beb8fd94a12edb3c28ca9ff682b87f59ef8

cantino commented 3 years ago

What do you think about this in relation to #156?

cmer commented 3 years ago

In my opinion, Mcfly should always try to do the right thing automatically without the need for any configuration, which is why I think it'd be wise to have something directly in Mcfly to try to automatically detect light mode, especially since it's rather trivial to do it effectively in 95% of cases.

As an example, I don't need to configure ls for it to work. It just does. Just my 2 cents!

cantino commented 3 years ago

I think having this in the shell scripts is simpler for the time being, but eventually could be pulled into the Rust code. Is this PR ready to merge? (@dmfay & @SafariMonkey appreciate a review on this too if you can.)

SafariMonkey commented 3 years ago

I would suggest keeping the previous value (if set) using something like export MCFLY_LIGHT="${MCFLY_LIGHT:-TRUE}" (bash , zsh), which seems to be roughly equivalent to changing to test -z MCFLY_LIGHT; and set -gx MCFLY_LIGHT TRUE in fish.

Otherwise, it looks fine to me, but I'm far from an expert.

AllanOricil commented 9 months ago

Seems that it does not work on the latest macos

image
AllanOricil commented 9 months ago

there should exist a standard for all OS to return if it is using dark/light mode