Listener class PR acceptable?

[zwimer]

Would a PR containing a listener class be acceptable? This would not have to remove old functionality, it could be purely additive.

Why?

1. In my opinion, the most important reason, is allowing guaranteed cleanup. Consider a listener which uses subprocess.POpen. If a user were to force quit (for example a SIGQUIT 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 before os._exit(1). Right now I'm using a somewhat reimplemented version of darkdetect due to the lack of this functionality.
2. Right now, the listeners are uninterruptible. For some listeners, like the Windows listener, this may make sense, but for others that poll processes, it is avoidable as these processes can be killed. In this case, NotImpelementedError()s are acceptable.
3. Allowing subclassing will allow editing of functionality

Possible API + Example

class BaseListener:

  def __init__(self, callback):
      self.set_callback(callback)

  def set_callback(self, callback):
      self._callback = callback

  def listen(self):
     raise NotImpelementedError()

  def stop(self):
     raise NotImpelementedError()

In _linux_detect.py:

class Listener(BaseListener):
  def __init__(self, callback):
    super().__init__(callback)
    self._proc = None
  
  def listen(self):
    self._proc = subprocess.Popen(
        ('gsettings', 'monitor', 'org.gnome.desktop.interface', 'gtk-theme'),
        stdout=subprocess.PIPE,
        universal_newlines=True,
    ) 
    with self._proc:
        for line in p.stdout:
            self._callback('Dark' if '-dark' in line.strip().removeprefix("gtk-theme: '").removesuffix("'").lower() else 'Light')

  def stop(self):
    if self._proc is not None:
      self._proc.kill()
      self._proc.wait()
      self._proc = None

# For those who want to use the old API        
def listener(callback): 
  Listener(callback).listen()

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:

1. A user wants to use an alternative listener for a specific OS. Perhaps their linux distro doesn't use gnome; but they still want the macOS and Windows listeners to work as is.
2. A user wants to edit the windows listener so that interrupts are possible, they can override the listen function and perhaps poll once per second if they desire
3. They want to easily swap out the callback function for some reason; they can use the base classes' set_callback, or have it set via a subclass method
4. A user may want to subclass Listener 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 #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 standard listener(callback) function invoke Listener(callback).listen().

If I were to make a PR to add such functionality for each supported OS, would it be considered or rejected?

[albertosottile]

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.

[zwimer]

PR: #32

[albertosottile]

Since we are aligned on requirements, I would close this and keep the discussion in the PR.