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

[Linux] Support org.freedesktop.appearance.color-scheme to retreive system dark mode #2258

Closed
vimpostor opened this issue Jul 13, 2022 · 8 comments
Labels
enhancement New feature or request fixed-in-nightly This is (or is assumed to be) fixed in the nightly builds. Wayland X11

Comments

@vimpostor
Copy link
Contributor

The current dark mode detection in wezterm for Linux is unreliable. Luckily, there is now a desktop environment independent way to query the system dark mode: The org.freedesktop.appearance.color-scheme key in the XDG desktop portal.

This works both on X11 and Wayland and it is also possible to be notified during runtime as soon as the dark mode preference changes.
Example desktop environments that have support for this key are:

It would be great if wezterm could support reading this key to get the preferred appearance and also to react to runtime changes of the key. The advantage (next to being independent of DE) is that one would not need to poll for dark mode changes but would immediately get an interrupt once the system dark mode changes.

Related issue: #806

More Info

Example apps and implementations that already support this key

@vimpostor vimpostor added the enhancement New feature or request label Jul 13, 2022
wez added a commit that referenced this issue Jul 13, 2022
There are some other settings in there that could also help with
things like the cursor theme on Wayland.

Note that we don't currently subscribe to the settings changed
signal: that can be done in a follow up.

refs: #2258
refs: #1742
@wez
Copy link
Owner

wez commented Jul 13, 2022

Thanks for this suggestion! I've pushed a first-pass implementation of this: reading the appearance will read from this interface.
I still need to hook up subscribing to the settings-changed signal.

wez added a commit that referenced this issue Jul 14, 2022
We use this to detect changes in dark mode

refs: #2258
@wez wez added the fixed-in-nightly This is (or is assumed to be) fixed in the nightly builds. label Jul 14, 2022
@wez
Copy link
Owner

wez commented Jul 14, 2022

Change notifications are now also hooked up!

This should be fixed now in main.

It typically takes about an hour before fixes are available as nightly builds for all platforms. Linux builds are the fastest to build and are often available within about 20 minutes. Windows and macOS builds take a bit longer.

Please take a few moments to try out the fix and let me know how that works out. You can find the nightly downloads for your system in the wezterm installation docs.

If you prefer to use packages provided by your distribution or package manager of choice and don't want to replace that with a nightly download, keep in mind that you can download portable packages (eg: a .dmg file on macOS, a .zip file on Windows and an .AppImage file on Linux) that can be run without permanently installing or replacing an existing package, and can then simply be deleted once you no longer need them.

If you are eager and can build from source then you may be able to try this out more quickly.

@vimpostor
Copy link
Contributor Author

Wow thanks, you are an absolute legend and it works beautifully!

wezterm_dark_mod.mp4

I think this makes wezterm the very first terminal that supports this setting out of the box. 🚀

@vimpostor
Copy link
Contributor Author

vimpostor commented Jul 14, 2022

Right now wezterm flickers very shortly on startup: It starts with whatever colorscheme is set in color_scheme in the config and then receives the window-config-reloaded a few ms later, setting the correct color_scheme. This causes a short flicker if the new color_scheme is a different one.

Is there any way to fix this or to already set the color_scheme based on appearance on startup?

I tried the following, but there is no window defined in the config return obviously. Do you know how to get the appearance setting in the return statement at the very bottom? That would probably fix the flickering.

function scheme_for_appearance(appearance)
	if appearance:find("Dark") then
		return "Breeze"
	else
		return "PencilLight"
	end
end

wezterm.on("window-config-reloaded", function(window, pane)
	local overrides = window:get_config_overrides() or {}
	local scheme = scheme_for_appearance(window:get_appearance())
	if overrides.color_scheme ~= scheme then
		overrides.color_scheme = scheme
		window:set_config_overrides(overrides)
	end
end)

return {
-- HERE: Unfortunately window is not defined here and we can't get appearance
	color_scheme = scheme_for_appearance(window:get_appearance()),
}

wez added a commit that referenced this issue Jul 14, 2022
This simplifies the "change scheme based on dark mode" example
a lot.  This was previously impossible to do because we didn't
have a lua module associated with the gui until recently, so
the only way to reference a gui-related object was via an
event callback.

refs: #2258
@wez
Copy link
Owner

wez commented Jul 14, 2022

Oh yeah, that's annoying.

The appearance stuff was only tied to a window method because we didn't have a way to reference GUI functions outside of a gui-initiated event callbacks until recently. Now that we have wezterm.gui as an available lua module, I've added the function there. That means that the config to react to dark mode changes is now a lot simpler:

local wezterm = require 'wezterm'

function scheme_for_appearance(appearance)
  if appearance:find("Dark") then
    return "Builtin Solarized Dark"
  else
    return "Builtin Solarized Light"
  end
end

return {
  color_scheme = scheme_for_appearance(wezterm.gui.get_appearance()),
}

I just pushed support for this, so give it 20+ minutes to show up in a nightly build!

@wez
Copy link
Owner

wez commented Jul 14, 2022

@vimpostor
Copy link
Contributor Author

Thanks, works like a charm!

vimpostor added a commit to vimpostor/wezterm that referenced this issue Jul 15, 2022
Right now the initial appearance retrieval uses the x11 connection
interface, which completely circumvents the already existing more
complete implementation in x_and_wayland.rs.
The latter implementation is strictly better, because it first attempts
getting the appearance from the XDG desktop portal and then falls back
to the X11 interface, whereas right now we are only using the X11
interface.

Before this patch there was a very weird issue for folks using the OS
system dark mode with the following config snippet:
```
color_scheme = scheme_for_appearance(wezterm.gui.get_appearance())
```

The color_scheme on startup would be correct, but there would be a very
weird problem where sometimes wezterm ignores the first time that the
portal notifies about a colorscheme change. All following colorscheme
change notifications would be processed correctly.

The source of the bug was an inconsistent retrieval of the appearance
setting:
- The Lua API used the XDG desktop portal
- The internal appearance used the X11 specific connection at startup

For example due to this, the internal appearance variable could have
stored "Dark" from the X11 connection, but the actual appearance from
the XDG desktop portal was "Light".
If then the XDG desktop portal changes to "Dark", the
appearance_changed() method would dismiss the update with the following
check because self.appearance was "Dark":
```
if appearance != self.appearance
```
It is only after that, that the internal inconsistency would have been
solved and following appearance changes would succeed and update the
colorscheme.

To fix this problem, we now read the appearance setting from the generic
connection which is consistent with the Lua wezterm.gui.get_appearance()
API.

refs: wez#2258
vimpostor added a commit to vimpostor/wezterm that referenced this issue Jul 15, 2022
Right now the initial appearance retrieval uses the x11 connection
interface, which completely circumvents the already existing more
complete implementation in x_and_wayland.rs.
The latter implementation is strictly better, because it first attempts
getting the appearance from the XDG desktop portal and then falls back
to the X11 interface, whereas right now we are only using the X11
interface.

Before this patch there was a very weird issue for folks using the OS
system dark mode with the following config snippet:

```
color_scheme = scheme_for_appearance(wezterm.gui.get_appearance())
```

The color_scheme on startup would be correct, but there would be a very
weird problem where sometimes wezterm ignores the first time that the
portal notifies about a colorscheme change. All following colorscheme
change notifications would be processed correctly.

The source of the bug was an inconsistent retrieval of the appearance
setting:
- The Lua API used the XDG desktop portal
- The internal appearance used the X11 specific connection at startup

For example due to this, the internal appearance variable could have
stored "Dark" from the X11 connection, but the actual appearance from
the XDG desktop portal was "Light".
If then the XDG desktop portal changes to "Dark", the
appearance_changed() method would dismiss the update because
self.appearance was already "Dark".

It is only after that, that the internal inconsistency would have been
solved and following appearance changes would succeed and update the
colorscheme.

To fix this problem, we now read the appearance setting from the generic
connection which is consistent with the Lua wezterm.gui.get_appearance()
API.

refs: wez#2258
vimpostor added a commit to vimpostor/wezterm that referenced this issue Jul 15, 2022
Right now the initial appearance retrieval uses the x11 connection
interface, which completely circumvents the already existing more
complete implementation in x_and_wayland.rs.
The latter implementation is strictly better, because it first attempts
getting the appearance from the XDG desktop portal and then falls back
to the X11 interface, whereas right now we are only using the X11
interface.

Before this patch there was a very weird issue for folks using the OS
system dark mode with the following config snippet:

```
color_scheme = scheme_for_appearance(wezterm.gui.get_appearance())
```

The color_scheme on startup would be correct, but there would be a very
weird problem where sometimes wezterm ignores the first time that the
portal notifies about an appearance update.

The source of the bug was an inconsistent retrieval of the appearance
setting:
- The Lua API used the XDG desktop portal
- The internal appearance used the X11 specific connection at startup

For example due to this, the internal appearance variable could have
stored "Dark" from the X11 connection, but the actual appearance from
the XDG desktop portal was "Light".
If then the XDG desktop portal changes to "Dark", the
appearance_changed() method would dismiss the update because
self.appearance was already "Dark".

It is only after that, that the internal inconsistency would have been
solved and following appearance changes would succeed and update the
colorscheme correctly.

To fix this problem, we now read the appearance setting from the generic
connection which is consistent with the Lua wezterm.gui.get_appearance()
API.

refs: wez#2258
vimpostor added a commit to vimpostor/wezterm that referenced this issue Jul 15, 2022
Right now the initial x11 appearance retrieval uses the specific
connection interface, which completely circumvents the already existing
more complete implementation in x_and_wayland.rs.
The latter implementation is strictly better, because it first attempts
getting the appearance from the XDG desktop portal and then falls back
to the X11 interface.

Before this patch there was a very weird issue for folks using the OS
system dark mode with the following config snippet:

```
color_scheme = scheme_for_appearance(wezterm.gui.get_appearance())
```

The color_scheme on startup would be correct, but there would be a very
weird problem where sometimes wezterm ignores the first time that the
portal notifies about an appearance update.

The source of the bug was an inconsistent retrieval of the appearance
setting:
- The Lua API used the XDG desktop portal
- The internal appearance used the X11 specific connection at startup

For example due to this, the internal appearance variable could have
stored "Dark" from the X11 connection, but the actual appearance from
the XDG desktop portal was "Light".
If then the XDG desktop portal changes to "Dark", the
appearance_changed() method would dismiss the update because
self.appearance was already "Dark".

It is only after that, that the internal inconsistency would have been
solved and following appearance changes would succeed and update the
colorscheme correctly.

To fix this problem, we now use the portal directly in both the x11 and
wayland connections,  which is consistent with the Lua
wezterm.gui.get_appearance() API.

refs: wez#2258
vimpostor added a commit to vimpostor/wezterm that referenced this issue Jul 15, 2022
Right now the initial x11 appearance retrieval uses the specific
connection interface, which completely circumvents the already existing
more complete implementation in x_and_wayland.rs.
The latter implementation is strictly better, because it first attempts
getting the appearance from the XDG desktop portal and then falls back
to the X11 interface.

Before this patch there was a very weird issue for folks using the OS
system dark mode with the following config snippet:

```
color_scheme = scheme_for_appearance(wezterm.gui.get_appearance())
```

The color_scheme on startup would be correct, but there would be a very
weird problem where sometimes wezterm ignores the first time that the
portal notifies about an appearance update.

The source of the bug was an inconsistent retrieval of the appearance
setting:
- The Lua API used the XDG desktop portal
- The internal appearance used the X11 specific connection at startup

For example due to this, the internal appearance variable could have
stored "Dark" from the X11 connection, but the actual appearance from
the XDG desktop portal was "Light".
If then the XDG desktop portal changes to "Dark", the
appearance_changed() method would dismiss the update because
self.appearance was already "Dark".

It is only after that, that the internal inconsistency would have been
solved and following appearance changes would succeed and update the
colorscheme correctly.

To fix this problem, we now use the portal directly in both the x11 and
wayland connections,  which is consistent with the Lua
wezterm.gui.get_appearance() API.

refs: wez#2258
wez pushed a commit that referenced this issue Jul 15, 2022
Right now the initial x11 appearance retrieval uses the specific
connection interface, which completely circumvents the already existing
more complete implementation in x_and_wayland.rs.
The latter implementation is strictly better, because it first attempts
getting the appearance from the XDG desktop portal and then falls back
to the X11 interface.

Before this patch there was a very weird issue for folks using the OS
system dark mode with the following config snippet:

```
color_scheme = scheme_for_appearance(wezterm.gui.get_appearance())
```

The color_scheme on startup would be correct, but there would be a very
weird problem where sometimes wezterm ignores the first time that the
portal notifies about an appearance update.

The source of the bug was an inconsistent retrieval of the appearance
setting:
- The Lua API used the XDG desktop portal
- The internal appearance used the X11 specific connection at startup

For example due to this, the internal appearance variable could have
stored "Dark" from the X11 connection, but the actual appearance from
the XDG desktop portal was "Light".
If then the XDG desktop portal changes to "Dark", the
appearance_changed() method would dismiss the update because
self.appearance was already "Dark".

It is only after that, that the internal inconsistency would have been
solved and following appearance changes would succeed and update the
colorscheme correctly.

To fix this problem, we now use the portal directly in both the x11 and
wayland connections,  which is consistent with the Lua
wezterm.gui.get_appearance() API.

refs: #2258
vimpostor added a commit to vimpostor/wezterm that referenced this issue Aug 13, 2022
This improves the startup time.

Right now we query the portal appearance value again over dbus every
time that we access it, for example every time that the user calls
wezterm.gui.get_appearance() from the Lua interface.

Queries over dbus are slow, they usually take a few milliseconds to
complete, for example on my system a portal query over dbus takes around
2 milliseconds to complete.

Wezterm also automatically calls the portal during its own internal
x11/wayland connection initialization, thus right now wezterm queries
the appearance portal setting n+1 times on startup, where n is the
number of times that the user calls get_appearance() from the config.

To fix this problem, we simply cache the portal appearance.

Thus this patch decreases the startup time by 2ms for users that
configure wezterm to follow the global system theme and potentially by
more for users that call get_appearance() in inflational amounts.

Of course in order to prevent our cached value from going invalid, we
have to track the appearance value by subscribing to the SettingChanged
signal which we did already anyway.

refs: wez#2258
vimpostor added a commit to vimpostor/wezterm that referenced this issue Aug 13, 2022
This improves the startup time.

Right now we query the portal appearance value again over dbus every
time that we access it, for example every time that the user calls
wezterm.gui.get_appearance() from the Lua interface.

Queries over dbus are slow, they usually take a few milliseconds to
complete, for example on my system a portal query over dbus takes around
2 milliseconds to complete.

Wezterm also automatically calls the portal during its own internal
x11/wayland connection initialization, thus right now wezterm queries
the appearance portal setting n+1 times on startup, where n is the
number of times that the user calls get_appearance() from the config.

To fix this problem, we simply cache the portal appearance.

Thus this patch decreases the startup time by 2ms for users that
configure wezterm to follow the global system theme and potentially by
more for users that call get_appearance() in inflational amounts.

Of course in order to prevent our cached value from going invalid, we
have to track the appearance value by subscribing to the SettingChanged
signal which we did already anyway.

refs: wez#2258
vimpostor added a commit to vimpostor/wezterm that referenced this issue Aug 17, 2022
This slightly improves the startup time of wezterm.

Right now we query the portal appearance value again over dbus every
time that we access it, for example every time that the user calls
wezterm.gui.get_appearance() from the Lua interface.

Queries over dbus are slow, they usually take a few milliseconds to
complete, for example on my system a portal query over dbus takes around
2 milliseconds to complete.

Wezterm also automatically calls the portal during its own internal
x11/wayland connection initialization, thus right now wezterm queries
the appearance portal setting n+1 times on startup, where n is the
number of times that the user calls get_appearance() from the config.

To fix this problem, we simply cache the portal appearance.

Thus this patch decreases the startup time by 2ms for users that
configure wezterm to follow the global system theme and potentially by
more for users that call get_appearance() in inflational amounts.

Of course in order to prevent our cached value from going invalid, we
have to track the appearance value by subscribing to the SettingChanged
signal which we did already anyway.

refs: wez#2258
vimpostor added a commit to vimpostor/wezterm that referenced this issue Aug 19, 2022
This slightly improves the startup time of wezterm.

Right now we query the portal appearance value again over dbus every
time that we access it, for example every time that the user calls
wezterm.gui.get_appearance() from the Lua interface.

Queries over dbus are slow, they usually take a few milliseconds to
complete, for example on my system a portal query over dbus takes around
2 milliseconds to complete.

Wezterm also automatically calls the portal during its own internal
x11/wayland connection initialization, thus right now wezterm queries
the appearance portal setting n+1 times on startup, where n is the
number of times that the user calls get_appearance() from the config.

To fix this problem, we simply cache the portal appearance.

Thus this patch decreases the startup time by 2ms for users that
configure wezterm to follow the global system theme and potentially by
more for users that call get_appearance() in inflational amounts.

Of course in order to prevent our cached value from going invalid, we
have to track the appearance value by subscribing to the SettingChanged
signal which we did already anyway.

refs: wez#2258
vimpostor added a commit to vimpostor/wezterm that referenced this issue Aug 20, 2022
This slightly improves the startup time of wezterm.

Right now we query the portal appearance value again over dbus every
time that we access it, for example every time that the user calls
wezterm.gui.get_appearance() from the Lua interface.

Queries over dbus are slow, they usually take a few milliseconds to
complete, for example on my system a portal query over dbus takes around
2 milliseconds to complete.

Wezterm also automatically calls the portal during its own internal
x11/wayland connection initialization, thus right now wezterm queries
the appearance portal setting n+1 times on startup, where n is the
number of times that the user calls get_appearance() from the config.

To fix this problem, we simply cache the portal appearance.

Thus this patch decreases the startup time by 2ms for users that
configure wezterm to follow the global system theme and potentially by
more for users that call get_appearance() in inflational amounts.

With the naive implementation wezterm would be subject to the following
race condition:

1. wezterm calls get_appearance() and caches the value
2. System-wide dark mode changes
3. wezterm subscribes to portal notifications

In that scenario wezterm would miss the dark mode switch entirely and
would cache the wrong value until the dark mode switches again after
wezterm subscribed.

To fix this race condition we call read_setting() again **after** we
have subscribed just to be on the safe side.

Note that while this still introduces a second "redundant" dbus query
for the same value, this time it does not actually block start up since
it happens in another thread.

refs: wez#2258
wez pushed a commit that referenced this issue Aug 20, 2022
This slightly improves the startup time of wezterm.

Right now we query the portal appearance value again over dbus every
time that we access it, for example every time that the user calls
wezterm.gui.get_appearance() from the Lua interface.

Queries over dbus are slow, they usually take a few milliseconds to
complete, for example on my system a portal query over dbus takes around
2 milliseconds to complete.

Wezterm also automatically calls the portal during its own internal
x11/wayland connection initialization, thus right now wezterm queries
the appearance portal setting n+1 times on startup, where n is the
number of times that the user calls get_appearance() from the config.

To fix this problem, we simply cache the portal appearance.

Thus this patch decreases the startup time by 2ms for users that
configure wezterm to follow the global system theme and potentially by
more for users that call get_appearance() in inflational amounts.

With the naive implementation wezterm would be subject to the following
race condition:

1. wezterm calls get_appearance() and caches the value
2. System-wide dark mode changes
3. wezterm subscribes to portal notifications

In that scenario wezterm would miss the dark mode switch entirely and
would cache the wrong value until the dark mode switches again after
wezterm subscribed.

To fix this race condition we call read_setting() again **after** we
have subscribed just to be on the safe side.

Note that while this still introduces a second "redundant" dbus query
for the same value, this time it does not actually block start up since
it happens in another thread.

refs: #2258
@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2023

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request fixed-in-nightly This is (or is assumed to be) fixed in the nightly builds. Wayland X11
Projects
None yet
Development

No branches or pull requests

2 participants