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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cmer
Copy link

@cmer cmer commented May 24, 2021

No description provided.

@cantino
Copy link
Owner

cantino commented May 26, 2021

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

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

@cmer
Copy link
Author

cmer commented May 26, 2021

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
Copy link
Owner

cantino commented May 27, 2021

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
Copy link

jltml commented May 27, 2021

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
Copy link
Author

cmer commented May 27, 2021

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.

@cantino
Copy link
Owner

cantino commented May 30, 2021

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
Copy link
Author

cmer commented May 30, 2021

See my implementation here: 55554be

@cantino
Copy link
Owner

cantino commented Jun 5, 2021

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

@cmer
Copy link
Author

cmer commented Jun 8, 2021

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
Copy link
Owner

cantino commented Jun 20, 2021

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
Copy link
Contributor

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
Copy link

Seems that it does not work on the latest macos

image

@AllanOricil
Copy link

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

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.

5 participants