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

Implement a macOS listener #30

Merged
merged 10 commits into from
Dec 8, 2022

Conversation

zwimer
Copy link
Contributor

@zwimer zwimer commented Dec 6, 2022

Implements: #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: Prefer setup.cfg to setup.py #28 (comment)

@albertosottile
Copy link
Owner

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

zwimer commented Dec 6, 2022

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

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: #25 (comment)

@zwimer
Copy link
Contributor Author

zwimer commented Dec 7, 2022

This PR now requires python3, which I've taken is ok given: #28 (comment)

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

zwimer commented Dec 7, 2022

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

@albertosottile
Copy link
Owner

albertosottile commented Dec 7, 2022

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

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: #14 (comment)

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

@zwimer
Copy link
Contributor Author

zwimer commented Dec 7, 2022

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

zwimer commented Dec 7, 2022

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

@zwimer zwimer force-pushed the macos_listener_objc branch from 96a5a27 to e37d0ac Compare December 7, 2022 20:07
@zwimer zwimer force-pushed the macos_listener_objc branch from e37d0ac to db5f950 Compare December 7, 2022 20:29
@zwimer
Copy link
Contributor Author

zwimer commented Dec 7, 2022

Rebased onto master because of the pyproject.toml update.

@albertosottile
Copy link
Owner

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?

darkdetect/__init__.py Outdated Show resolved Hide resolved
darkdetect/_mac_detect.py Outdated Show resolved Hide resolved
darkdetect/_mac_detect.py Outdated Show resolved Hide resolved
darkdetect/_mac_detect.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
darkdetect/_mac_detect.py Outdated Show resolved Hide resolved
@zwimer
Copy link
Contributor Author

zwimer commented Dec 7, 2022

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

albertosottile commented Dec 7, 2022

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

zwimer commented Dec 7, 2022

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

@zwimer
Copy link
Contributor Author

zwimer commented Dec 7, 2022

The last commit should address the requested changes.

README.md Outdated Show resolved Hide resolved
darkdetect/_mac_detect.py Outdated Show resolved Hide resolved
darkdetect/_mac_detect.py Outdated Show resolved Hide resolved
@zwimer
Copy link
Contributor Author

zwimer commented Dec 7, 2022

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

zwimer commented Dec 7, 2022

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

@albertosottile
Copy link
Owner

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

zwimer commented Dec 7, 2022

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

@zwimer zwimer requested a review from albertosottile December 7, 2022 23:00
Copy link
Owner

@albertosottile albertosottile left a comment

Choose a reason for hiding this comment

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

LGTM, I tested it with my KeyboardInterrupt script and it worked perfectly.

@albertosottile albertosottile merged commit 3347df3 into albertosottile:master Dec 8, 2022
@albertosottile
Copy link
Owner

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

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