Make the default macOS theme depend on Dark Mode #2197
Conversation
Enselic
force-pushed
the
macos-default-theme
branch
2 times, most recently
from
e00ff33
to
3c08042
Compare
|
Some tests are failing. Will investigate some other day. |
Enselic
force-pushed
the
macos-default-theme
branch
from
3c08042
to
6551690
Compare
We frequently get complaints from macOS users that bat does not work on their default macOS terminal background, which is white. Pay the price of slightly increased startup time to get a better default on macOS. To avoid the slightly increased startup time, simply specify a theme explicitly via `--theme`, `BAT_THEME`, or `~/.config/bat`. Note that if there is an error when we check if Dark Mode is enabled, we behave the same as on Windows and Linux; assume that the terminal background is dark. This harmonizes behavior across platforms, and makes bat behave the same as before, when Dark Mode was always assumed to be enabled.
Enselic
force-pushed
the
macos-default-theme
branch
from
6551690
to
ddb93fe
Compare
|
Thanks for review @keith-hall 🙏 The tests were failing because it seems like the CI runner uses a light system theme by default on macOS. Making the tests explicitly set a theme fixed the failures. I have contemplated to add automated tests for this, but since the test must also pass on systems that uses Dark Mode, and since I don't want our testing code to change macOS system theme, I don't currently plan on adding automated tests for this. If we get problems with regressions going ahead, I will reconsider. We could make use of dependency injection here, for example. But as it stands, this PR is fundamentally ready to be merged. I want to keep it open for at least a few days though, maybe a week, to give time for more feedback. |
|
Just realised I also need to add an entry to CHANGELOG.md ;) |
|
Thanks for review! Seems unlikely that more feedback will come in, so I will merge this now. |
The comment summarizes it pretty well, but basically: - bat (as of 0.22 <sharkdp/bat#2197>) defaults to switching theme based on macOS' light/dark mode - That isn't the same thing as whether the terminal theme is light or dark - There are ways to do that, both in Apple's terminal and in iTerm2 (in the 2.5 beta), but until then/if it's not desired, this works - One day there will [hopefully](sharkdp/bat#1746) be a way to set both light and dark themes for `bat` - None of this makes much of any difference for `delta`
| if macos_dark_mode_active() { | ||
| Self::default_dark_theme() | ||
| } else { | ||
| Self::default_light_theme() | ||
| } |
There was a problem hiding this comment.
could this logic be extended to other OSes too?
In my opinion, the macos_dark_mode_active function should be renamed to os_dark_mode_active and then handle the specific logic of several OSes (including macos).
We frequently get complaints from macOS users that bat does not work on
their default macOS terminal background, which is white.
Pay the price of slightly increased startup time to get a better default
on macOS. To avoid the slightly increased startup time, simply
specify a theme explicitly via
--theme,BAT_THEME, or~/.config/bat.I did some quick testing, and it seems to work. With Dark Mode:
With default system theme:
Startup time is increased slightly due to this check:
./target/release/bat -f examples/simple.rsbat-v0.21.0 -f examples/simple.rsBut there is no difference if an explicit theme is selected:
./target/release/bat -f --theme "Monokai Extended" examples/simple.rsbat-v0.21.0 -f --theme "Monokai Extended" examples/simple.rsNote that if there is an error when we check if Dark Mode is enabled, we
behave the same as on Windows and Linux; assume that the terminal
background is dark. This harmonizes behavior across platforms, and makes
bat behave the same as before, when Dark Mode was always assumed to be
enabled.