albertosottile / darkdetect

Detect OS Dark Mode from Python
Other
171 stars 18 forks source link

Implement a macOS listener #30

Closed zwimer closed 1 year ago

zwimer commented 1 year ago

Implements: https://github.com/albertosottile/darkdetect/issues/25

UPDATED: This PR:

  1. Adds a macOS listener; this listener requires pip install darkdetect[mac_listener]; if the extra mac_listener is not installed, the listener raises NotImpelementedError
  2. Bumps minor version to 0.8.0 as a feature has been added.
  3. Drops python2 support, as per: https://github.com/albertosottile/darkdetect/pull/28#issuecomment-1339961348
albertosottile commented 1 year ago

Hi, thanks for having worked on this. I was not aware of PyObjCTools.AppHelper.runConsoleEventLoop, this might be the piece I was missing during my experiments.

Could you provide a usage example? I tried the one reported in the README

import darkdetect
import threading

# def listener(callback: typing.Callable[[str], None]) -> None: ...

t = threading.Thread(target=darkdetect.listener, args=(print,))
t.daemon = True
t.start()

but it does not work, the thread is not printing when the theme is changed.

In addition, because this implementation of the listener depends on an external package (pyobjc-framework-Cocoa) I'd rather put this dependency as as an extra (see the discussion here)

zwimer commented 1 year ago

@albertosottile Concerning extras, I could, but I will point out that the way I added it to requires is such that it only gets downloaded on macOS and only for python3.0+. With those edits in place, I feel this should stay as required rather than an extra, as this is part of the core functionality advertised on the README. If users really want to ignore dependencies via pip they could always do pip install --no-deps darkdetect. If you disagree with this though, let me know.

As for the README.md update: It works in the main thread, I'm having trouble getting it to work in a worker thread- I'll look into this.

albertosottile commented 1 year ago

If you disagree with this though, let me know.

One of the key propositions of darkdetect is that it does not depend on external packages. If you think about it, pyobjc can easily replicate the functionality of this package in a one liner.

That being said, I understand there might be cases in which having an extra dependency could be desirable, e.g. for having a listener, but I would confine these to an extra.

I'm having trouble getting it to work in a worker thread

This was also my main struggle when I tried to implement this on my own. Perhaps you will benefit from reading the recap of my findings here: https://github.com/albertosottile/darkdetect/issues/25#issuecomment-1188297721

zwimer commented 1 year ago

This PR now requires python3, which I've taken is ok given: https://github.com/albertosottile/darkdetect/pull/28#issuecomment-1339961348

Right now it uses multiprocessing, instead of subprocess which may have side effects if a user forgets if __name__ == '__main__': and their code is run in spawn instead of fork mode (as in, the child process may re-import things / the main file's code). It could be edited to use subprocess, but that would require mimicking an IPC queue; I think this is preferring multiprocessing is beneficial.

zwimer commented 1 year ago

I'll update the PR description with all the changes.

albertosottile commented 1 year ago

I am sorry for all the effort you are investing in this, perhaps I should have been clearer with the requirements. The goal of listener is to provide a function that continuously monitors the GUI theme and calls a callback function whenever there are changes. It is then left to the user how to run this, whether in the main thread, or in a separate thread, or in a separate process or subprocess. This was the original idea behind the listener API, and is also part of the "contract" reported in the README. To be honest, I would prefer to have no macOS listener, than a listener that does not allow to fulfill this contract.

In addition to this general principle, I would especially avoid using multiprocessing in darkdetect, as it could have complex and unpredictable effects on the users' code. You mention forgetting if __name__ == '__main__', but this is just the tip of the iceberg. The truth is that multiprocessing completely reinitializes all the import chain, and this can cause issues with a lot of GUI libraries (e.g. PySide2 and wx) that the user can simply not control, and darkdetect is actually meant to be used in conjunction with such libraries.

In summary, I would rather not merge the PR as it is. I will adapt the text in #25 to clarify the requirements for this function.

albertosottile commented 1 year ago

To be honest, I would prefer to have no macOS listener, than a listener that does not allow to fulfill this contract.

As further clarification, I would like to emphasize that most GUI bindings actually provide native methods to continuously detect theme changes, see e.g. an implementation for Qt here: https://github.com/albertosottile/darkdetect/issues/14#issuecomment-945090847

(the same is not true for determining dark vs light, hence the need for darkdetect).

zwimer commented 1 year ago

I can adapt this to use subprocess then that waits on and reads stdout; just like how the linux listener works. That should satisfy every requirement listed:

def listener(callback):
    with subprocess.Popen(
        ('gsettings', 'monitor', 'org.gnome.desktop.interface', 'gtk-theme'),
        stdout=subprocess.PIPE,
        universal_newlines=True,
    ) as p:
        for line in p.stdout:
            callback('Dark' if '-dark' in line.strip().removeprefix("gtk-theme: '").removesuffix("'").lower() else 'Light')

Except, instead of ('gsettings', 'monitor', 'org.gnome.desktop.interface', 'gtk-theme'), it'll be ('sys.executable', '-c', 'import darkdetect; darkdetect._macos_listener._listen_child()') or something similar.

As for QT, that is actually the reason I've found this problem, since that signal was insufficient for determining if the theme changed when used with custom palletes. Unfortunately we've found no native way to truly detect light vs dark mode in QT.

zwimer commented 1 year ago

The latest commit removes multiprocessing and updates the listener to be basically as the above, just like the linux listener.

zwimer commented 1 year ago

Rebased onto master because of the pyproject.toml update.

albertosottile commented 1 year ago

I can adapt this to use subprocess then that waits on and reads stdout; just like how the linux listener works. That should satisfy every requirement listed:

I am fine with this approach, provided that it fulfills all the requirements listed in #25.

As for QT, that is actually the reason I've found this problem, since that signal was insufficient for determining if the theme changed when used with custom palletes. Unfortunately we've found no native way to truly detect light vs dark mode in QT.

True, but Qt can notify you when there is a change, then you can call darkdetect.theme() to determine the new appearance.

The latest commit removes multiprocessing and updates the listener to be basically as the above, just like the linux listener.

I tried your implementation on my MacBook Air, but it does not work, not even in the main thread: nothing is printed when I switch from Dark to Light and vice versa. Did you test this code?

zwimer commented 1 year ago

Did you test this

I did. What code did you use to test this? I can run it myself

(test3) zwimer@Lotus ~/D/W/darkdetect> python
Python 3.10.8 (main, Oct 13 2022, 09:48:40) [Clang 14.0.0 (clang-1400.0.29.102)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import threading
>>> import darkdetect
>>>
>>> # def listener(callback: typing.Callable[[str], None]) -> None: ...
>>>
>>> t = threading.Thread(target=darkdetect.listener, args=(print,))
>>> t.daemon = True
>>> t.start()
>>> Dark
Light
Dark
Light
[1]    21169 quit       python3
albertosottile commented 1 year ago

Same code as you, but also darkdetect.listener(print) or darkdetect._mac_detect._listen_chld() from the main thread: the callback is never called when I change theme. I am on macOS 12.6.1

zwimer commented 1 year ago

Hmm, if you send over your alternative I shall try integrating it.

zwimer commented 1 year ago

The last commit should address the requested changes.

zwimer commented 1 year ago

True, but Qt can notify you when there is a change, then you can call darkdetect.theme() to determine the new appearance.

Unfortunately for my application, since the pallete is customizeable it can be manually set so this wouldn't work either. This is a failing on QT's part, other frameworks do implement theme detection notifications. That's why I intend to fall back to darkdetect.

zwimer commented 1 year ago

@albertosottile That should use the listener you provided. I've tested it on my end; let me know if it works for you.

albertosottile commented 1 year ago

That should use the listener you provided. I've tested it on my end; let me know if it works for you.

Works perfectly both from the main and from a secondary thread. Thanks!

zwimer commented 1 year ago

The last commit:

  1. Properly ignores Ctrl-C
  2. On death, if the parent doesn't clean up the child, the child will be silent when it later dies. (This is the catching of IOError, in case the parent closes stdout either intentionally or much more likely via death).
albertosottile commented 1 year ago

Thanks for having worked on this. I will make a release with this feature when I have some spare time.