mcfly icon indicating copy to clipboard operation
mcfly copied to clipboard

Automatically detect light terminal on initialization. Implements #150.

Open cmer opened this issue 3 years ago • 13 comments

cmer avatar May 24 '21 18:05 cmer

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

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

cantino avatar May 26 '21 00:05 cantino

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?

cmer avatar May 26 '21 11:05 cmer

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?

cantino avatar May 27 '21 05:05 cantino

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!

jltml avatar May 27 '21 06:05 jltml

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:

  • treating MCFLY_LIGHT as "auto" when it is not set,
  • forcing light mode when set to TRUE
  • forcing dark mode when set to FALSE

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

cmer avatar May 27 '21 10:05 cmer

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?

cantino avatar May 30 '21 06:05 cantino

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

cmer avatar May 30 '21 12:05 cmer

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

cantino avatar Jun 05 '21 21:06 cantino

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!

cmer avatar Jun 08 '21 14:06 cmer

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.)

cantino avatar Jun 20 '21 22:06 cantino

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.

SafariMonkey avatar Jun 20 '21 23:06 SafariMonkey

Seems that it does not work on the latest macos

image

AllanOricil avatar Nov 10 '23 12:11 AllanOricil

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

AllanOricil avatar Nov 10 '23 12:11 AllanOricil