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 Listener Class #32

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

zwimer
Copy link
Contributor

@zwimer zwimer commented Dec 8, 2022

Implements: #31

Changes:

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

Before merging:

Required

  • Test on macOS with extras installed
  • Test on macOS without extras installed
  • Test on Linux that uses Gnome (like Ubuntu 22.04)
  • Test on Windows
  • 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
Copy link
Contributor Author

zwimer commented Dec 8, 2022

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

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.

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.

I did a first round of review, but I'd like to test this myself on all the platforms as well.

darkdetect/__init__.py Outdated Show resolved Hide resolved
darkdetect/__init__.py Outdated Show resolved Hide resolved
darkdetect/base.py Outdated Show resolved Hide resolved
darkdetect/base.py Outdated Show resolved Hide resolved
darkdetect/_mac_detect.py Outdated Show resolved Hide resolved
darkdetect/_linux_detect.py Outdated Show resolved Hide resolved
darkdetect/_windows_detect.py Outdated Show resolved Hide resolved
@albertosottile
Copy link
Owner

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?

@albertosottile albertosottile added the enhancement New feature or request label Dec 9, 2022
@zwimer
Copy link
Contributor Author

zwimer commented Dec 12, 2022

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

darkdetect/base.py Outdated Show resolved Hide resolved
@zwimer
Copy link
Contributor Author

zwimer commented Dec 13, 2022

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 zwimer marked this pull request as ready for review December 13, 2022 01:59
@zwimer
Copy link
Contributor Author

zwimer commented Dec 16, 2022

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

@zwimer
Copy link
Contributor Author

zwimer commented Dec 17, 2022

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.

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.

I have some requested changes and I will probably apply further editing to the README after this PR is merged.

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.

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

zwimer commented Dec 22, 2022

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

zwimer commented Jan 9, 2023

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

@zwimer
Copy link
Contributor Author

zwimer commented Jan 17, 2023

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

@zwimer
Copy link
Contributor Author

zwimer commented Jan 23, 2023

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants