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 15 forks source link

[GTK] Fixing freeze when stopping the event handler #59

Closed zliuva closed 2 years ago

zliuva commented 2 years ago

https://github.com/weisJ/auto-dark-mode/issues/57 https://github.com/weisJ/auto-dark-mode/pull/58

Refactored the code to have all operations (most importantly, stop) related to the dummy Gtk app running in the same thread.

zliuva commented 2 years ago

Hi @weisJ I finally got some time to finish the refactoring idea.

I'm running into a kinda weird issue on CI that the linux tests were skipped: https://github.com/weisJ/auto-dark-mode/runs/7509434055?check_suite_focus=true#step:6:322

wondering if you can think of anything that might caused this (looks like Windows and mac have their tests running in CI jobs)

weisJ commented 2 years ago

Thanks for working on this. I highly appreciate it! I suppose that it may be reporting false in assumeTrue(LibraryUtil.isGtk && LibraryUtil.isX64), which results in the test being skipped. You can try to replace it with

assertTrue(LibraryUtil.isGtk, "Not Gtk")
assertTrue(LibraryUtil.isX64, "Not x86")
weisJ commented 2 years ago

After trying it out locally I can confirm that it is indeed because LibraryUtil.isGtk returns false. I'm not sure how to provide a desktop environment on github actions, so this is something to deail with at a later point. I suppose you checked on your side that it still works with your changes?

zliuva commented 2 years ago

@weisJ ahh thanks I did not notice it was assume not assert.

I re-run the unit tests locally and fixed a small issue (now that the runloop is managed by the callback handler, I have to move some assertions to after we created it)

> Task :auto-dark-mode-plugin:test
          0.2sec,    2 completed,   0 failed,   0 skipped, com.github.weisj.darkmode.platform.linux.gtk.GtkNativeTest
WARNING   0.0sec,    1 completed,   0 failed,   1 skipped, com.github.weisj.darkmode.platform.macos.MacOSNativeTest
WARNING   0.0sec,    1 completed,   0 failed,   1 skipped, com.github.weisj.darkmode.platform.windows.WindowsNativeTest
WARNING   0.9sec,    4 completed,   0 failed,   2 skipped, Gradle Test Run :auto-dark-mode-plugin:test
weisJ commented 2 years ago

Great! Does it work for you when running through the runIde task?

And more importantly I am a bit weary about passing an id string to Gtk::Application::create: When running IntelliJ twice what will the interaction will be. From the documentation I of Gtk::Application::create is suppose it will return the same instance each time called. But then both native listeners will try to run it on separate threads. Moreover what will happen if one IntelliJ instance gets closed and stops the gtk application.

I think the best thing would be to just call Gtk::Application::create() with no argument and create a unique gtk application each time. Makes reasoning about it a lot easier.

zliuva commented 2 years ago

yeah I did test it out in IDE. re the two "instances", I've yet able to actually launch two different instances of IntelliJ, I did test with opening/closing multiple projects in different windows. (I do believe they are still one instance of idea and hence one copy of the plugin)

weisJ commented 2 years ago

I think you should be able to start two instance by running gradlew runIde --no-daemon twice.

zliuva commented 2 years ago

the second invocation actually gave out a Already running and exit. that was what I was referring to re running two instances.

that said, I don't see the harm on not passing in an explict app id. updated