weisJ / auto-dark-mode

IDEA plugin to automatically apply system theme settings on macOS and Windows.
https://plugins.jetbrains.com/plugin/14076-auto-dark-mode
MIT License
53 stars 14 forks source link

Linux support (only Gnome for now) #8

Closed IncPlusPlus closed 4 years ago

IncPlusPlus commented 4 years ago

Hello! I've been using this plugin for a while now and it's been perfect. I dual-boot with Windows and Pop!_OS. When I'm on Windows, I have Armin2208/Windows-Auto-Night-Mode running. Once sunset is underway, it will change my Windows theme to dark mode. Because of this wonderful plugin, IntelliJ promptly follows suit and changes its theme to my desired dark theme. I use a similar utility, rmnvgr/nightthemeswitcher-gnome-shell-extension, on Pop!_OS.

Unfortunately, this plugin doesn't have Linux support. I'm hoping to help change this. This pull request is a proof of concept which demonstrates this plugin working when used in a Gnome environment. As such, this isn't entirely complete. There are a few areas (like determining whether the user is using a dark theme or whether it is a high contrast theme) that aren't in a state that I would call "production ready".

Goal

The goal of this PR is to determine whether you would be interested in Linux support as a fleshed-out feature. Please let me know if you would be interested in me completing this feature so it could become a part of Auto Dark Mode. I'd also like to know your thoughts on the code itself and whether it is at a quality level sufficient for your plugin.

Implementation note

This uses ProcessBuilder to make calls via CLI. I've done some manual tests and this method has been very stable. While it would be more ideal for the Gnome module to perform direct API calls to Gio.Settings much like what can be seen here, I couldn't find a way to pull this off for Java. According to the GObject-Introspection docs, there is a project that can create language bindings for Java known as JGIR. However, information on it is scarce and it seems to require build-time steps that would be difficult to integrate with this project's Gradle-based build system without creating a Gradle plugin solely for creating Java bindings. Therefore, I chose the simpler method of simply making CLI calls. If you would prefer that this feature be done through the proper API, I could try to make use of JGIR instead.

Below is the commit message for this PR which contains additional details

Commit message

This commit adds a Linux module to the plugin. Because Linux has a variety of desktop environments, this plugin uses a delegation strategy to allow a delegate ThemeMonitorService to be provided depending on the desktop environment being used. For now, there is only Gnome support.

It should be noted that this module accomplishes its task much differently than the Windows and Mac OS modules. Because Gnome (and others) support user themes, there is no centralized switch in the OS or its components for using "the" light theme or dark theme. It is common practice for user themes (at least for Gnome) to be provided in a light and dark form (along with a few others sometimes like "darker" and "darkest"). This is also explained in the JavaDoc for GtkVariants.

Instead of using native OS API calls to ask the OS whether it is using the "dark theme" or the "light theme", the Gnome ThemeMonitorService deduces (based on the name of the current theme) whether the user's current theme is a light theme or a dark theme. This is a temporary measure I am using simply as a proof-of-concept that Gnome (and others in the future) can be supported by this plugin. If the PR for this feature gets the go-ahead, I intend to add an additional area to the look and feel settings which allows the user to choose which of their themes is their light, dark, and high contrast theme. This would replace the guessing functionality.

weisJ commented 4 years ago

Thank you very much for the PR!

I did some research myself about adding support for Linux before and came to the same conclusion that there is no good way of detecting dark mode because there is no real concept for it. You definitely have green light from me to go forward with this PR!

Here are some thoughts regarding some of the points you mentioned:

This uses ProcessBuilder to make calls via CLI. I've done some manual tests and this method has been very stable. While it would be more ideal for the Gnome module to perform direct API calls to Gio.Settings much like what can be seen here, I couldn't find a way to pull this off for Java. According to the GObject-Introspection docs, there is a project that can create language bindings for Java known as JGIR. However, information on it is scarce and it seems to require build-time steps that would be difficult to integrate with this project's Gradle-based build system without creating a Gradle plugin solely for creating Java bindings. Therefore, I chose the simpler method of simply making CLI calls. If you would prefer that this feature be done through the proper API, I could try to make use of JGIR instead.

As to my knowledge there are pretty good c++ API bindings for Gio.Settings that could be used to accomplish it on the native level through JNI (which the project already has a framework to build). Preferably this would be the way to go, if we get any advantages by doing so (i.e. native hooks for listening to changes to the settings). I haven't read far into the API and am not sure whether this is the case.

...I intend to add an additional area to the look and feel settings which allows the user to choose which of their themes is their light, dark, and high contrast theme. This would replace the guessing functionality.

If the guessing is replaced by a configuration of the dark/light themes the approach the the plugin settings needs to be redone into being more modular. I'd like the base plugin to be agnostic about the details of the individual implementations for different Operating Systems. Exposing the theme configuration through the base plugin wouldn't be beneficial for this. For now I think the guessing is appropriate for this PR and further work should be done separately, as it also applies to the other implementations.

IncPlusPlus commented 4 years ago

Awesome! I'm really excited to contribute. Thanks for all the comments. They're all very helpful. I'll reply to each as I address them in my changes. Once I address all of them and get this working using JNI instead of CLI, I'll mark this PR as "ready for review".

...The note I made about how this uses CLI at the moment

As to my knowledge there are pretty good c++ API bindings for Gio.Settings that could be used to accomplish it on the native level through JNI (which the project already has a framework to build). Preferably this would be the way to go, if we get any advantages by doing so (i.e. native hooks for listening to changes to the settings). I haven't read far into the API and am not sure whether this is the case.

Your intuition is right. The GSettings class has the changed signal which I could subscribe to. I'll be using JNI much like the Mac OS and the Windows modules. For this change, I'll be adding an additional GitHub Action which builds the necessary artifacts (for the same reason the other actions exist).

For now, I think that it's okay to build the Linux JNI components much like the other modules. However, something we should start thinking about is what might happen when we need different native libraries for different environments (like how Gnome requires. We may need to split the Linux module into different submodules for the different environments. This would also mean a unique GitHub Action would be necessary for each of those environments as well. This is just a minor concern at the moment and I'm sure a solution will be within reach when this issue arises. I bring this up because I think it might be worth thinking about.

My C++ knowledge is limited so I'm hoping that when I build the JNI components, your review will point out mistakes and things I can learn from. :smile:

...I intend to add an additional area to the look and feel settings which allows the user to choose which of their themes is their light, dark, and high contrast theme. This would replace the guessing functionality.

If the guessing is replaced by a configuration of the dark/light themes the approach the the plugin settings needs to be redone into being more modular. I'd like the base plugin to be agnostic about the details of the individual implementations for different Operating Systems. Exposing the theme configuration through the base plugin wouldn't be beneficial for this. For now I think the guessing is appropriate for this PR and further work should be done separately, as it also applies to the other implementations.

I 100% agree with this. It would be better to get a complete implementation done in this PR first and then do the refactoring required for this feature afterwards.

IncPlusPlus commented 4 years ago

After much tinkering, I now have Gnome support using native components instead of clunky listeners and CLI usage as of 610a6ebc3ab8d8ecb73ba32593a245a2b4502cc8. It's proven to be just as stable.

This change does add some complexity to the build process as libsigc++ and glibmm are now required. However, people who aren't building on Linux shouldn't be affected so long as the additional workflow from c11e29248252f54fc3b0be425b0ee4692ba30e16 is functioning properly.

I think we should split the Linux module into submodules based on the desktop environment. I haven't looked into how to accomplish what I've done here with other environments but I would assume that the dependencies and methodology is likely significantly different. Maybe this should happen in a separate PR. Let me know what you think about this idea. I'm not all that familiar with Gradle or your project structure preferences so let me know what you would want the directory structure to look like (among other details) if we were to do that.

I still have a few more changes to make to address the comments you made above. I'll probably work on those tomorrow or Monday.

Note: I was unable to get a working 32-bit build. Some of the headers make assertions related to sizeof() calls involving various types which fail when trying to compile for a 32-bit system. Attempting to install the necessary i386 packages to allow this led me straight into dependency hell.

Implementation details for DarkModeGnome.cpp

DarkModeGnome.cpp loosely resembles DarkMode.cpp from the Windows module. However, instead of running a loop that employs a blocking function like WaitForSingleObject(), DarkModeGnome.cpp subscribes the settingChanged(const Glib::ustring &name) function to the signal Gio::Settings->signal_changed (const Glib::ustring& key) using connect (SlotType&& slot, bool after=true) by connecting settingChanged(const Glib::ustring &name) via sigc::mem_fun().

This is a lot of detail but this is nearly the same process as what is demonstrated in the signal handler tutorials .

weisJ commented 4 years ago

This change does add some complexity to the build process as libsigc++ and glibmm are now required. However, people who aren't building on Linux shouldn't be affected so long as the additional workflow from c11e292 is functioning properly.

If you enable Github actions on your fork the actions should be executed there after each commit. Otherwise as soon as the PR leaves the draft phase the action will get executed here. Though I don't see any reason why it shouldn't work.

I think we should split the Linux module into submodules based on the desktop environment. I haven't looked into how to accomplish what I've done here with other environments but I would assume that the dependencies and methodology is likely significantly different. Maybe this should happen in a separate PR. Let me know what you think about this idea. I'm not all that familiar with Gradle or your project structure preferences so let me know what you would want the directory structure to look like (among other details) if we were to do that.

I absolute agree. I image a project structure that looks something like this:

auto-dark-mode
| ...
| linux
    |  gnome
         | src
         | build.gradle.kts // Basically the current build file
    |  src // Only contains the source of the LinuxMonitorService
    | build.gradle.kts // Simply bundles all implementations using `java-library` plugin.

Then in the settings.gradle.kts:

include(
    ...,
    "linux",
    "linux/gnome"
)

Note: I was unable to get a working 32-bit build. Some of the headers make assertions related to sizeof() calls involving various types which fail when trying to compile for a 32-bit system. Attempting to install the necessary i386 packages to allow this led me straight into dependency hell.

Not a big deal. Most people aren't running 32-bit anymore anyway, so there is little to no benefit in dealing with the difficulties of providing support for it.

Implementation details for DarkModeGnome.cpp

DarkModeGnome.cpp loosely resembles DarkMode.cpp from the Windows module. However, instead of running a loop that employs a blocking function like WaitForSingleObject(), DarkModeGnome.cpp subscribes the settingChanged(const Glib::ustring &name) function to the signal Gio::Settings->signal_changed (const Glib::ustring& key) using connect (SlotType&& slot, bool after=true) by connecting settingChanged(const Glib::ustring &name) via sigc::mem_fun().

This is a lot of detail but this is nearly the same process as what is demonstrated in the signal handler tutorials .

Very nice. This follows more closely the implementation for macOS (In an ideal scenario a similar design would have been chosen for windows but there simply isn't an API provided for such a mechanism).

weisJ commented 4 years ago

Then in the settings.gradle.kts:

include(
    ...,
    "linux",
    "linux/gnome"
)

You will need to add .replace("/", "-") to p.name in settings.gradle.kts.

IncPlusPlus commented 4 years ago

I've marked several conversations as resolved. Most of them were obsolete after moving to using native code. The others were small fixes. I'm quite close to having this be ready for review (although this process has already been somewhat of a continuous review). I'm just stuck on one little issue.

IncPlusPlus commented 4 years ago

Oops! This isn't ready for merging. I just need to address one last thing.

IncPlusPlus commented 4 years ago

I don't know why I wasn't encountering this before but I'm having an issue building. During the task :auto-dark-mode-plugin:buildSearchableOptions, kotlin.jvm.KotlinReflectionNotSupportedError is thrown with the message Kotlin reflection implementation is not found at runtime. Make sure you have kotlin-reflect.jar in the classpath. This issue goes away when I add implementation("org.jetbrains.kotlin:kotlin-reflect") to the dependencies block in plugin/build.gradle.kts. Should I commit this change or is there an issue that I should fix with my local environment? build failure.txt

weisJ commented 4 years ago

Could you try to rebase? In the master branch kotlin configuration is automatically handled for all submodules. Only applying the kotlin("jvm") should be enough. If this doesn't help you can commit the dependency.

weisJ commented 4 years ago

Btw. you can ignore the macOS build failure. Github has updated the framework versions of their runners.

IncPlusPlus commented 4 years ago

After messing with Gradle for some time, it turns out that the culprit was actually being caused by a task I needn't include when building. I removed that task from a one-liner I recommended in the README with cb497fb4f7347c19e7a314effad2770cd8bf89a0.

weisJ commented 4 years ago

Looking good! Some last minor things. If these are addressed I'll merge the PR.