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 (only Gnome for now) #8

Merged
merged 25 commits into from
Jul 26, 2020

Conversation

IncPlusPlus
Copy link
Contributor

@IncPlusPlus IncPlusPlus commented Jul 13, 2020

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.

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

weisJ commented Jul 14, 2020

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

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. 😄

...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.

See PR weisJ#8 for remarks on its internals. This commit also adds some additional details to the README.
As it turns out, there's no need to run a [Main Event Loop](https://developer.gnome.org/glib/stable/glib-The-Main-Event-Loop.html#glib-The-Main-Event-Loop.description) via [`Glib::MainLoop::create(true)`](https://developer.gnome.org/glibmm/stable/classGlib_1_1MainLoop.html#a37a1dcf3cb167cf02260111f533d0b5b).

This was a holdover from when I was experimenting with using signals _outside_ of the context of JNI in my own sandbox C++ project. For the project to not exit when the main method was done executing, I found that the most reliable way was to use a GLib concept called the [main event loop](https://developer.gnome.org/glib/stable/glib-The-Main-Event-Loop.html#glib-The-Main-Event-Loop.description). Spawning this in a separate thread and then using `pthread_exit(nullptr)` in the main thread would allow for the `changed` signal of Gio::Settings to fire. Otherwise, the program would exit as soon as the main method had executed all of its lines successfully.
@IncPlusPlus
Copy link
Contributor Author

IncPlusPlus commented Jul 19, 2020

After much tinkering, I now have Gnome support using native components instead of clunky listeners and CLI usage as of 610a6eb. 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 c11e292 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
Copy link
Owner

weisJ commented Jul 19, 2020

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).

README.md Show resolved Hide resolved
linux/src/main/cpp/DarkModeGnome.cpp Outdated Show resolved Hide resolved
linux/src/main/cpp/DarkModeGnome.cpp Outdated Show resolved Hide resolved
linux/src/main/cpp/DarkModeGnome.cpp Outdated Show resolved Hide resolved
.github/workflows/libs.yml Outdated Show resolved Hide resolved
@weisJ
Copy link
Owner

weisJ commented Jul 19, 2020

Then in the settings.gradle.kts:

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

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

Update the branch I'm using for my PR.
DelegatingThemeMonitorService has been refactored from LinuxEnvironmentDelegateHelper to be OS-agnostic. Both it and NullThemeMonitorService have been moved to the base module as they are no longer OS-specific.
While I could have just replaced "/*/build/" with "/**/build/", I figured this was avoiding being greedy for a reason. So, I made it so that all sub-modules of the linux module have their build folder ignored.
- Use JNI_OK instead of 0
- Extract various usages of "gtk-theme" and "org.gnome.desktop.interface" into `constexpr auto` global variables
- Move Gio::Settings to the global scope such that it can be referenced outside of EventHandler, making creation of a one-off RefPtr unnecessary
- Moved Gio::init() to a new init() method such that it will only be called once
- Added init() native method to GnomeNative and added method call to GnomeThemeMonitorService.install
isSupported better describes the purpose of this method.
@IncPlusPlus
Copy link
Contributor Author

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.

@weisJ weisJ mentioned this pull request Jul 22, 2020
@IncPlusPlus IncPlusPlus marked this pull request as ready for review July 23, 2020 17:11
@IncPlusPlus
Copy link
Contributor Author

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

@IncPlusPlus
Copy link
Contributor Author

IncPlusPlus commented Jul 23, 2020

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

weisJ commented Jul 23, 2020

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

weisJ commented Jul 23, 2020

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

@IncPlusPlus
Copy link
Contributor Author

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 cb497fb.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
linux/gnome/build.gradle.kts Outdated Show resolved Hide resolved
@weisJ
Copy link
Owner

weisJ commented Jul 24, 2020

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

Per review suggestions

Co-authored-by: Jannis Weis <31143295+weisJ@users.noreply.github.com>
As it turns out, prefixing `clean` and `build` with a colon makes those tasks run only on the root project instead of building everything. Can you tell I'm new to Gradle? :P
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.

2 participants