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

Add autodark plugin (for linux) #8840

Merged
merged 4 commits into from
Nov 25, 2023
Merged

Add autodark plugin (for linux) #8840

merged 4 commits into from
Nov 25, 2023

Conversation

smac89
Copy link
Contributor

@smac89 smac89 commented Oct 27, 2023

  • I'm the package's author and/or maintainer.
  • I have have read the docs.
  • I have tagged a release with a semver version number.
  • My package repo has a description and a README describing what it's for and how to use it.
  • My package doesn't add context menu entries. *
  • My package doesn't add key bindings. **
  • Any commands are available via the command palette.
  • Preferences and keybindings (if any) are listed in the menu and the command palette, and open in split view.
  • If my package is a syntax it doesn't also add a color scheme. ***
  • If my package is a syntax it is named after the language it supports (without suffixes like "syntax" or "highlighting").
  • I use .gitattributes to exclude files from the package: images, test files, sublime-project/workspace.

My package is Auto Dark

There are no packages like it in Package Control.

repository/a.json Outdated Show resolved Hide resolved
@braver
Copy link
Collaborator

braver commented Nov 19, 2023

Not sure you're just working around a bug in ST here? Have you reported that?

I can imagine things being not-so-easy on Linux, maybe it's specific to some subset of configurations. As for your implementation here, maybe let's discuss a little. I'm not so sure I would go with a deamon, they have a tendency of dying or otherwise misbehaving. Or you end up spawning multiple. Although I guess it's more elegant than having event listeners. What was your approach here?

There are some print statements in your code. Would be cleaner to have those disabled unless in a debug mode.

Finally, you need to tag a semver version, so v0.1.0 (3 digits) instead of v0.1.

@smac89
Copy link
Contributor Author

smac89 commented Nov 23, 2023

@braver I believe it is a bug. I'm not exactly sure what causes it, but another user (also on Linux) had reported something similar affecting them here. In their case, they are on Wayland (which is still fairly new). I notice the same issue with X11 which is still the standard window server.

The solution I implemented is based on xdg-desktop-portal, which is the standard on Linux to detect dark/light mode preference. The plugin so far works, and I made sure to test it well in various scenarios. It does behave like an event listener via the systemd-busctl client.

The only time I've noticed the thread (which is not a daemon thread btw, despite the name) lingering, is when sublime is not shutdown vs closed, i.e. when using the File > Quit menu. I could work around this by also starting another thread which detects when sublime quits and then stops the dbus client thread as well itself, but I think this something that should be implemented in sublime, not by package maintainers.

A better implementation would be to integrate the dbus support directly into sublime (assuming it doesn't do this already), or integrate a subset of the dbus api by using libdarkman, which is written in go.

@smac89
Copy link
Contributor Author

smac89 commented Nov 23, 2023

I also found another issue which was reported.

I think I'm starting to get a clearer picture as to why dark mode switching isn't fully working on Linux. As stated here: sublimehq/sublime_text#5194 (comment):

We grab the setting gtk-theme from org.gnome.desktop.interface, apply the theme to a GtkStyleContext and grab the selected background color. If the background color has all 3 components < 0.5 we switch to dark mode.

All I can say about this approach is that a solution that will work on "Linux", should not depend on a single desktop i.e. gnome. In the list of desktops that support xdg-desktop-portal, gnome is listed as one of several. Therefore, the right solution would one that is universal.

Furthermore, the nature of the bus interface makes it so that one does not even need to know exactly which backend implements what interface. So the devs can make certain assumptions and maybe use their current solution as a backup plan.

More related bug reports:

@smac89 smac89 requested a review from braver November 24, 2023 04:25
Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated testing result: SUCCESS

Repo link: Auto Dark mode for Linux

Packages added:
  - Auto Dark mode for Linux

Processing package "Auto Dark mode for Linux"
  - All checks passed

@braver
Copy link
Collaborator

braver commented Nov 25, 2023

Ah, that makes sense. At least the bug is reported, and until that's dealt with it would be good to have your package available for users.

Be sure to tag a new release with your latest changes.

@braver braver merged commit 3408ac5 into wbond:master Nov 25, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants