Using a more generic Gtk detection method. #54

zliuva · 2022-04-28T04:05:42Z

Using Gtk::Settings that reads the Net/ThemeName key from XSettings.

https://www.freedesktop.org/wiki/Specifications/XSettingsRegistry/

XSettings are respected by most Gtk desktops and this allows us to
support a wider range of desktops.

Theoretically any desktop with an XSettings daemon should be supported.

Tested:

i3 + xfsettingsd (XSettings daemon from Xfce 4)
i3 + gsd-xsettings (Gnome Settings Daemon)

zliuva · 2022-04-28T04:10:21Z

Hi there!

I understand there is a pending PR #50 using the FreeDesktop dark mode preference.

However, as a LTS user on stable distros, I do not see myself able to use this on my work computer any time soon.

However, I did discover a more generic way that should work with many GTK desktops that implements the XSettings protocol.

Thought this'd be useful for other folks stuck in the same situation.

plugin/src/test/kotlin/com/github/weisj/darkmode/platform/linux/gnome/GnomeNativeTest.kt

weisJ

I think it would be good to rename all Gnome** classes/files to read GTK**. Same goes for the subproject.

weisJ · 2022-04-28T14:00:16Z

base/src/main/java/com/github/weisj/darkmode/platform/LibraryUtil.java

+    // really any GTK would do (`isGnome` named for historical reasons)
+    public static final boolean isGnome = SystemInfo.isGNOME || SystemInfo.isXfce || SystemInfo.isI3;


No need to keep the old name. Please rename to isGTK

weisJ · 2022-04-28T14:01:52Z

plugin/src/test/kotlin/com/github/weisj/darkmode/platform/linux/gnome/GnomeNativeTest.kt

@@ -43,9 +43,10 @@ class GnomeNativeTest {

    private fun currentGtkTheme(): String {
        val theme = "gsettings get $settingsPath $settingsKey".runCommand()
+        //val theme = "xfconf-query -c xsettings -p /Net/ThemeName".runCommand()


Why is this commented out?

on my computer I use xfconf-query from XFCE but I don't see the need to change the default test based on gnome.

I left it as a comment to show that alternative utilities would also work.

As these comments are removed in the folowing commit please drop them from this one :)

plugin/src/test/kotlin/com/github/weisj/darkmode/platform/linux/gnome/GnomeNativeTest.kt

zliuva · 2022-04-28T14:55:23Z

I think it would be good to rename all Gnome** classes/files to read GTK**

I'm ok with that. Hopefully the diff is still obvious of the actual functional change.

weisJ · 2022-04-28T16:42:02Z

I think it would be good to rename all Gnome** classes/files to read GTK**

I'm ok with that. Hopefully the diff is still obvious of the actual functional change.

As long as you keep the renaming of files in a separate commit it should be fine.

weisJ · 2022-04-28T21:02:58Z

Looks like the CI is failing because the build actions need to be updated

zliuva · 2022-04-29T03:15:45Z

Looks like the CI is failing because the build actions need to be updated

lol I used ripgrep to find Gnome references but I just learned it ignores dot directories by default!

weisJ

Some last comment. Otherwise this looks good to me. Very good work. Thank for tackling this!

weisJ · 2022-05-02T18:43:06Z

plugin/build.gradle.kts

+        v1.7.0
+        <ul>
+            <li>Generic GTK desktops support. (XSettings based)</li>
+        </ul>


Please make this its own commit

weisJ · 2022-05-02T18:44:54Z

gradle.properties

@@ -11,7 +11,7 @@ systemProp.org.gradle.internal.publish.checksums.insecure = true
 org.gradle.jvmargs                                        = -noverify

 # Version
-auto-dark-mode.version                                    = 1.6.2-2022.1
+auto-dark-mode.version                                    = 1.7.0-2022.1


This should also go into the release note commit.

weisJ · 2022-05-02T18:47:41Z

linux/gtk/src/main/java/com/github/weisj/darkmode/platform/linux/gtk/GtkSettings.kt


 data class GtkTheme(val name: String) : Comparable<GtkTheme> {
    override fun compareTo(other: GtkTheme): Int = name.compareTo(other.name)
 }

-object GnomeSettings : DefaultSettingsContainer(identifier = "gnome_settings") {
+object GtkSettings : DefaultSettingsContainer(identifier = "gtk_settings") {


This will invalidate existing stored settings. Either keep the identifier the old one with a comment explaining why or advance the settings version in https://github.com/weisJ/auto-dark-mode/blob/master/plugin/src/main/java/com/github/weisj/darkmode/AutoDarkModeOptions.kt

ah i was not aware of this side effect. I think keeping the name with a comment is the least disruptive solution.

weisJ · 2022-05-02T18:49:08Z

plugin/src/test/kotlin/com/github/weisj/darkmode/platform/linux/gnome/GnomeNativeTest.kt

@@ -43,9 +43,10 @@ class GnomeNativeTest {

    private fun currentGtkTheme(): String {
        val theme = "gsettings get $settingsPath $settingsKey".runCommand()
+        //val theme = "xfconf-query -c xsettings -p /Net/ThemeName".runCommand()


As these comments are removed in the folowing commit please drop them from this one :)

weisJ · 2022-05-02T18:50:50Z

README.md

@@ -8,7 +8,7 @@ operating system settings. The plugin distinguishes between `Light`, `Dark` and
 the theme used for each mode can be customized.
 This plugin currently works for Windows and macOS.

-Linux support is both limited and experimental. At the moment, the only Linux desktop environment supported by this plugin is Gnome.
+Linux support is both limited and experimental. At the moment, Linux desktop environments based on Gtk and has an [XSettings](https://www.freedesktop.org/wiki/Specifications/xsettings-spec/) daemon are supported.


This sounds odd. How about "Linux desktop environments based on Gtk, which have an XSettings daemon are supported."

I wonder if I should expand on that:

An XSettings daemon (e.g. gsd-xsettings for Gnome; xfsettingsd for Xfce; or even standalone ones like https://github.com/derat/xsettingsd) is required because it is the one that actually sends out the notifications when a theme (amont other settings) changes;

Conversely, you can run a desktop environment without XSettings daemon (e.g. to my knowledge LXDE does not have one by default), in which case theme changes are not "real time" (i.e. they usually takes effect only for new apps launched, since running apps do not get notified)

I haven't verified it, but I think even non-Gtk native environments like KDE should work with this, if it runs such a daemon (e.g. https://github.com/KDE/xsettings-kde)

so really the dominating factor is not Gtk (we just use it to get notified), but rather the XSettings daemon (who actually sends out the notifications)

Thank you for the clarification. Nonetheless the setence as it is isn't grammatically correct. I also think it would be worth while to elaborate a bit on what is needed to use the plugin. This could also be added to https://github.com/weisJ/auto-dark-mode/blob/master/plugin/src/main/resources/META-INF/plugin.xml

Using Gtk::Settings that reads the `Net/ThemeName` key from XSettings. https://www.freedesktop.org/wiki/Specifications/XSettingsRegistry/ XSettings are respected by most Gtk desktops and this allows us to support a wider range of desktops. Theoretically any desktop with an XSettings daemon should be supported. Tested: * i3 + xfsettingsd (XSettings daemon from Xfce 4) * i3 + gsd-xsettings (Gnome Settings Daemon)

zliuva · 2022-05-09T00:52:58Z

hi @weisJ thanks for your comments. I've rebased the PR and:

reverted the settings' identifier
squashed implementations into a single commit
left version bumps into its own commit

while implementing this change I also learned quite a bit about desktop environments (vs window managers) and the XSettings daemon. the main take-away I have is that as long as there is such a daemon running and it's signaling the changes according to the spec, this new method of detecting current theme (and change of themes) should work.

I've updated README.md to summarize such learnings in the most technically accurate way possible, while keeping it short (enough).

weisJ · 2022-05-09T18:23:31Z

Thanks a lot for tackling this 👍🏼

while implementing this change I also learned quite a bit about desktop environments (vs window managers) and the XSettings daemon. the main take-away I have is that as long as there is such a daemon running and it's signaling the changes according to the spec, this new method of detecting current theme (and change of themes) should work.

In this case it could be possible to allow users to enable monitoring on their own risk (i.e. they make sure that a compatible daemon is installed). But this is something for another time :)

zliuva · 2022-05-09T18:32:09Z

Thank you for your feedback :)

Thanks a lot for tackling this 👍🏼

while implementing this change I also learned quite a bit about desktop environments (vs window managers) and the XSettings daemon. the main take-away I have is that as long as there is such a daemon running and it's signaling the changes according to the spec, this new method of detecting current theme (and change of themes) should work.

In this case it could be possible to allow users to enable monitoring on their own risk (i.e. they make sure that a compatible daemon is installed). But this is something for another time :)

Yeah that's what I'm thinking as a potential future improvement.

zliuva commented Apr 28, 2022

View reviewed changes

plugin/src/test/kotlin/com/github/weisj/darkmode/platform/linux/gnome/GnomeNativeTest.kt Show resolved Hide resolved

weisJ reviewed Apr 28, 2022

View reviewed changes

zliuva force-pushed the gtk-settings branch from fb3597c to f30bc50 Compare April 29, 2022 14:36

weisJ reviewed May 2, 2022

View reviewed changes

zliuva added 2 commits May 8, 2022 17:39

Releasing 1.7.0.

05f17e7

zliuva force-pushed the gtk-settings branch from f30bc50 to 05f17e7 Compare May 9, 2022 00:47

weisJ merged commit 5ac4662 into weisJ:master May 9, 2022

telnoratti mentioned this pull request Oct 4, 2022

gnome-settings-daemon detection doesn't work outside of i3, GNOME, or XFCE #61

Closed

weisJ mentioned this pull request Mar 28, 2023

Not working in 2023.1 EAP #65

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using a more generic Gtk detection method. #54

Using a more generic Gtk detection method. #54

zliuva commented Apr 28, 2022

zliuva commented Apr 28, 2022

weisJ left a comment

weisJ Apr 28, 2022

weisJ Apr 28, 2022

zliuva Apr 28, 2022

weisJ May 2, 2022

zliuva commented Apr 28, 2022

weisJ commented Apr 28, 2022

weisJ commented Apr 28, 2022

zliuva commented Apr 29, 2022

weisJ left a comment

weisJ May 2, 2022

weisJ May 2, 2022

weisJ May 2, 2022

zliuva May 2, 2022

weisJ May 2, 2022

weisJ May 2, 2022

zliuva May 2, 2022

weisJ May 3, 2022

zliuva commented May 9, 2022

weisJ commented May 9, 2022

zliuva commented May 9, 2022

		// really any GTK would do (`isGnome` named for historical reasons)
		public static final boolean isGnome = SystemInfo.isGNOME \|\| SystemInfo.isXfce \|\| SystemInfo.isI3;

Using a more generic Gtk detection method. #54

Using a more generic Gtk detection method. #54

Conversation

zliuva commented Apr 28, 2022

zliuva commented Apr 28, 2022

weisJ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zliuva commented Apr 28, 2022

weisJ commented Apr 28, 2022

weisJ commented Apr 28, 2022

zliuva commented Apr 29, 2022

weisJ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zliuva commented May 9, 2022

weisJ commented May 9, 2022

zliuva commented May 9, 2022