Implement Listener Class

zwimer commented 1 year ago

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

Changes:

_mac_detect.py and other files now only need to expose only two objects, theme and some BaseListener subclass.
__all__ added to _*_detect.py files so that only the main objects are dumped when doing import *
py.typed added to allow better type annotation checking via tools such as mypy
General code quality improvements, mostly stuff pylint complained about
Move isDark(), isLight(), and def listener out into __init__.py to avoid code duplication.

Before merging:

Required

[x] Test on macOS with extras installed
[x] Test on macOS without extras installed
[x] Test on Linux that uses Gnome (like Ubuntu 22.04)
[x] Test on Windows
[x] Documentation about new API

Optional

[ ] If possible have WindowsListener .stop(0) be more than a no-op

After merging:

New Issues:

[ ] Feature Request: Support .stop(None) for Windows by improving Windows stop function

zwimer commented 1 year ago

@albertosottile How exactly would you accept documentation? There isn't much to document, just the BaseListener class really. I can put this in the README.md easily. This is my preference.

If readthedocs is a must, this requires setup on your side to hook github to readthedocs and such. I've never personally set this up, but found this guide: https://docs.readthedocs.io/en/stable/tutorial/index.html

I can edit .rst files for a readthedocs. If you setup the auto-building on PRs I can easily verify them, otherwise I will use a too like pip install rst2pdf to test.

albertosottile commented 1 year ago

For me it is ok to put the documentation in the README, as long as it is clear and it does not clutter it too much. I only suggested readthedocs in case you wanted to do this more extensively and you felt too much constrained by the README.

albertosottile commented 1 year ago

General comment, if you do not mind I would release 0.8.0 from the current codebase, and then eventually land this change in 0.9.0. Would that be ok for you?

zwimer commented 1 year ago

General comment, if you do not mind I would release 0.8.0 from the current codebase, and then eventually land this change in 0.9.0. Would that be ok for you?

I have 0 issues with this, presuming that there is nothing that forces you to obey some arbitrary delay between releases. I.e. if 0.9.0 can be released once this PR merges instead of "at least 3 months after 0.8.0" then I'd prefer to wait; but baring something restricting you like that then releasing 0.8.0 sounds like a good idea to me. :)

zwimer commented 1 year ago

With the last 2 commits, when calling .stop() on Windows, the listener itself will continue listening until the next theme change is detected, but the callback will not be invoked. That is:

> listener.stop() 
> # Listener alive and listening, callbacks will probably no longer be called
> set_theme("Dark")
> # Listener is now dead, `.wait()` will return instantly. callback was not called

Note: probably because there is a race condition, but that is ok. .stop() does not promise not to call callbacks, it just initiates the stop sequence. It is .wait()'s job to wait until callbacks and such are complete. I just felt that callback should not be invoked 9 hours after the initial .stop() when the theme changes again; so I added in a check before calling the callback to ensure the Listener should actively be listening.

zwimer commented 1 year ago

Actually, I will add in one more tweak, some extra error checking!

zwimer commented 1 year ago

macOS Ventura and Ubuntu 22.04. I do not have a windows machine. I may be able to borrow one next week to test, though if you have access to one that would be appreciated.

zwimer commented 1 year ago

One side note: I am on holiday until the second week of January, so I cannot test this. We could either finish the review during this timeframe and merge it after, or merge it during the holidays and test it after, I have no strong feelings. Nevertheless, a release will have to wait for proper testing.

I have no issues waiting; I prefer having something fully tested before merging. I have tested this myself, but I lack windows and know you had some other tests to run.

zwimer commented 1 year ago

@albertosottile I've addressed every message post I believe.

zwimer commented 1 year ago

@albertosottile Is there something I can do to help get this merged?

zwimer commented 1 year ago

@albertosottile Once this is merged, perhaps one way to solve the sys.executable on macos on pyinstaller builds not always exposing -c issue could be having _mac_detect subprocess an osascript -e process which uses Objective-C to install the listener. It's likely safe to assume any mac has osascript installed since it's pre-installed on macOS. It might also remove our extra's dependency for macOS. I might look into it after this is merged, then make a PR if I do.

albertosottile commented 1 year ago

I apology, but I do not have any free time to work on this at the moment and I am not sure regarding when the situation will improve. This is definitely not a small PR and so it is not something I can hack in right before going to sleep. I will try to review it and maybe to test it on Windows ASAP, but I am afraid you cannot expect it in the next days.

albertosottile / darkdetect