Closed zwimer closed 1 year ago
In principle, I am not against this feature. My only concern is that it has to be clearly documented, so I would consider the PR only if its provides adequate documentation. At the moment, because the API was super simple, our only documentation was the README. With this feature, we might want to add something like readthedocs
, I am not sure.
One thing that should be retained is that Listener
should only work when the OS supports darkdetect
. In other words, the gates provided by __init__
should work also with these classes (e.g. returning BaseListener
or something equivalent added in _dummy
).
Regarding the code example, I think in _linux_detect.Listener
, the class should inherit from BaseListener
?
One small note about how we got here: originally, there were no listeners, then the Windows version was developed and I consider that as "the best" listener: based on ctypes
, no continuous polling, no subprocess, perfectly interruptible. Then the Linux listener was proposed. I am not a Linux user so I could not advise on a better implementation for Linux, and so I had to "blindly" accept the listener implementation for Linux, but I was aware of its limitations. For macOS, I tried quite hard to achieve a similar implementation to the Windows one, but in the end I did not manage, and now we merged #30 and basically propagated the problems of Linux to macOS (with also the addition of the extra dependencies).
To recap, I did not extend/improve the listener
API because I hoped implementing listeners similar to the Windows one would have been possible, some day. But now with #30 I basically accepted that is not possible, and so I believe an API extension along the lines you are proposing makes sense.
Since we are aligned on requirements, I would close this and keep the discussion in the PR.
Would a PR containing a listener class be acceptable? This would not have to remove old functionality, it could be purely additive.
Why?
subprocess.POpen
. If a user were to force quit (for example aSIGQUIT
on linux) the process, or a native component of a GUI library were to segfault, python may not clean up the child process it was listening to. Proving an API that promises to do so would be greatly appreciated. For example, a user could add a segfault handler to kill child subprocesses beforeos._exit(1)
. Right now I'm using a somewhat reimplemented version ofdarkdetect
due to the lack of this functionality.NotImpelementedError()
s are acceptable.Possible API + Example
In
_linux_detect.py
:This would allow users to ensure that cleanup has happened properly, interrupt a listener, or subclass to add / edit functionality. For listeners that subclass, really the important bit is having the subprocess saved into something that users can kill if the need should arise, but this class struct has other benefits as mentioned above.
Possible Reasons to Subclass
These are other possible benefits of allowing subclassing:
set_callback
, or have it set via a subclass methodListener
and their own class as part of a mixin design pattern or some other pattern.In the examples above,
darkdetect
still only provides an API that users can use, so unlike in https://github.com/albertosottile/darkdetect/issues/29 we don't need to do threading stuff. We simply provide a listener object for the user to use; and to support the existing API we simply just have the standardlistener(callback)
function invokeListener(callback).listen()
.If I were to make a PR to add such functionality for each supported OS, would it be considered or rejected?