Add Wayland support by providing evdev and libinput backends #71
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 47a5f10 in 2 minutes and 4 seconds
More details
- Looked at
770lines of code in7files - Skipped
1files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. Makefile:14
- Draft comment:
Ensure 'make typecheck' is intentional in test target; consider whether typechecking should block tests. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. aw_watcher_afk/evdev_devices.py:9
- Draft comment:
Consider documenting the rationale behind the chosen required keys for keyboard identification. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. aw_watcher_afk/listeners.py:1
- Draft comment:
Remove duplicate module docstring; only one docstring is needed at the top. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. aw_watcher_afk/listeners_libinput.py:37
- Draft comment:
Hardcoding 'seat0' in udev_assign_seat may not be flexible; consider parameterizing or documenting the assumption. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
This is a new file, so the comment is about changed code. The suggestion to parameterize 'seat0' could be valid, but without understanding libinput's seat system and whether other values are common/possible, I can't be certain this is actually an issue. The code may work fine with 'seat0' in 99% of cases. Making it configurable adds complexity that may not be needed.
I don't have enough context about libinput's seat system to know if this is actually a problem. The current implementation might be the standard approach.
While parameterization could add flexibility, we shouldn't suggest changes without strong evidence that the current approach is problematic.
Delete the comment since we don't have strong evidence that hardcoding 'seat0' is actually problematic in practice.
5. Makefile:9
- Draft comment:
Consider conditionally including the '--extras' flag only when extras are non‐empty; passing an empty string may cause unexpected behavior in some poetry versions. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment raises a reasonable concern about poetry's behavior with empty extras. However, poetry's CLI is well-designed and should handle empty extras gracefully. Without specific evidence of which poetry versions have this issue or concrete examples of the "unexpected behavior", this feels speculative. The current code is simple and clear.
I could be wrong about poetry's handling of empty extras - there might be real issues in some versions that I'm not aware of.
While possible, we shouldn't make code more complex based on speculative issues. The comment doesn't provide evidence of actual problems.
Delete this comment as it suggests a change based on speculative issues without concrete evidence of problems.
6. aw_watcher_afk/evdev_devices.py:1
- Draft comment:
Consider adding docstrings to functions (e.g. is_keyboard, is_mouse) to improve code readability and maintainability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
7. aw_watcher_afk/listeners.py:1
- Draft comment:
Remove duplicate module docstring to avoid redundancy. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
8. aw_watcher_afk/listeners_pynput.py:87
- Draft comment:
Consider ensuring thread safety for the mutable 'self.pos' used in on_move; although pynput listeners are often single-threaded, clarifying concurrency assumptions may prevent future issues. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
9. pyproject.toml:22
- Draft comment:
Consider adding documentation (e.g., in the README) about the optional 'use_evdev' extra dependency and how to enable it. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_O50oMvMhQYWqukxJ
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| def _create_tasks(self, loop): | ||
| """Schedule a read loop for all relevant devices""" | ||
| devices = list(self._find_devices()) | ||
| assert len(devices), "You may need to add your user to the 'input' group" |
There was a problem hiding this comment.
The assert on device list length is helpful; consider handling no-device case more gracefully than assertion in production.
|
|
||
| def start(self): | ||
| from pynput import keyboard | ||
| def use_evdev(): |
There was a problem hiding this comment.
For consistency, consider using the provided 'env_true' helper function in use_evdev() and use_libinput() instead of directly comparing os.getenv values.
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 47a5f10 in 2 minutes and 4 seconds
More details
- Looked at
770lines of code in7files - Skipped
1files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. Makefile:14
- Draft comment:
Ensure 'make typecheck' is intentional in test target; consider whether typechecking should block tests. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. aw_watcher_afk/evdev_devices.py:9
- Draft comment:
Consider documenting the rationale behind the chosen required keys for keyboard identification. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. aw_watcher_afk/listeners.py:1
- Draft comment:
Remove duplicate module docstring; only one docstring is needed at the top. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. aw_watcher_afk/listeners_libinput.py:37
- Draft comment:
Hardcoding 'seat0' in udev_assign_seat may not be flexible; consider parameterizing or documenting the assumption. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
This is a new file, so the comment is about changed code. The suggestion to parameterize 'seat0' could be valid, but without understanding libinput's seat system and whether other values are common/possible, I can't be certain this is actually an issue. The code may work fine with 'seat0' in 99% of cases. Making it configurable adds complexity that may not be needed.
I don't have enough context about libinput's seat system to know if this is actually a problem. The current implementation might be the standard approach.
While parameterization could add flexibility, we shouldn't suggest changes without strong evidence that the current approach is problematic.
Delete the comment since we don't have strong evidence that hardcoding 'seat0' is actually problematic in practice.
5. Makefile:9
- Draft comment:
Consider conditionally including the '--extras' flag only when extras are non‐empty; passing an empty string may cause unexpected behavior in some poetry versions. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment raises a reasonable concern about poetry's behavior with empty extras. However, poetry's CLI is well-designed and should handle empty extras gracefully. Without specific evidence of which poetry versions have this issue or concrete examples of the "unexpected behavior", this feels speculative. The current code is simple and clear.
I could be wrong about poetry's handling of empty extras - there might be real issues in some versions that I'm not aware of.
While possible, we shouldn't make code more complex based on speculative issues. The comment doesn't provide evidence of actual problems.
Delete this comment as it suggests a change based on speculative issues without concrete evidence of problems.
6. aw_watcher_afk/evdev_devices.py:1
- Draft comment:
Consider adding docstrings to functions (e.g. is_keyboard, is_mouse) to improve code readability and maintainability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
7. aw_watcher_afk/listeners.py:1
- Draft comment:
Remove duplicate module docstring to avoid redundancy. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
8. aw_watcher_afk/listeners_pynput.py:87
- Draft comment:
Consider ensuring thread safety for the mutable 'self.pos' used in on_move; although pynput listeners are often single-threaded, clarifying concurrency assumptions may prevent future issues. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
9. pyproject.toml:22
- Draft comment:
Consider adding documentation (e.g., in the README) about the optional 'use_evdev' extra dependency and how to enable it. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_O50oMvMhQYWqukxJ
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| def _create_tasks(self, loop): | ||
| """Schedule a read loop for all relevant devices""" | ||
| devices = list(self._find_devices()) | ||
| assert len(devices), "You may need to add your user to the 'input' group" |
There was a problem hiding this comment.
The assert on device list length is helpful; consider handling no-device case more gracefully than assertion in production.
|
|
||
| def start(self): | ||
| from pynput import keyboard | ||
| def use_evdev(): |
There was a problem hiding this comment.
For consistency, consider using the provided 'env_true' helper function in use_evdev() and use_libinput() instead of directly comparing os.getenv values.
|
|
||
| async for event in dev.async_read_loop(): | ||
| # logger.debug(f"Evdev event ({dev.name}): {evdev.categorize(event)}") | ||
| if event.type == ecodes.EV_KEY and event.value == 1: |
There was a problem hiding this comment.
Consider adding error handling (or logging) for device disconnections inside the async read loop to prevent silent failures.
aw_watcher_afk/listeners_libinput.py
Outdated
| threading.Thread(target=self._listen, daemon=True).start() | ||
|
|
||
| def _listen(self): | ||
| for event in self.libinput_context.get_event(): |
There was a problem hiding this comment.
Wrap event processing in a try/except block to safeguard against unexpected libinput errors that could terminate the listening thread.
Game4Move78
force-pushed
the
evdev-support
branch
from
47a5f10 to
f0c080a
Compare
Game4Move78
force-pushed
the
evdev-support
branch
from
f0c080a to
fb0571e
Compare
(Pulled in by pynput on Linux anyway)
Game4Move78
force-pushed
the
evdev-support
branch
from
fb0571e to
184bff2
Compare
|
Why use this method of getting idle status, instead of using the staging Wayland protocol ext-idle-notify, which popular and proven Wayland watchers like awatcher and aw-watcher-window-wayland use? |
|
@johan-bjareholt I am not sure why input events are currently tracked here in aw-watcher-afk rather than aw-watcher-input and the idle watcher for ActivityWatch on Linux currently uses input tracking rather than display protocols for tracking idle. As a result this PR will also cause the current implementation of aw-watcher-afk to support Wayland, even if it is only really needed to support aw-watcher-input. It's good to have implementations with Wayland support for both aw-watcher-input and aw-watcher-afk in the activitywatch repo, even if there are better solutions for aw-watcher-afk in other repos. If they are merged in future (awatcher is very good!) then this (and the existing pynput backend ) can be moved to aw-watcher-input. |
Fixed circular import.
Game4Move78
force-pushed
the
evdev-support
branch
from
826ce39 to
512f2af
Compare
This PR introduces Wayland support by adding
evdevandlibinputbackends as alternatives to pynput, which only supports X11 and XWayland (docs). These alternatives should provide Wayland support if run as root or with the appropriate group permissions (inputgroup).Changes:
Related to issue ActivityWatch/aw-watcher-input#25.
Important
Add Wayland support with
evdevandlibinputbackends, updating build and configuration files for optionalevdevsupport.evdevandlibinputbackends inlisteners_evdev.pyandlisteners_libinput.py.evdevbackend is functional and tested;libinputbackend is based on an unmaintained wrapper.listeners.pyupdated to select backend based onUSE_EVDEVandUSE_LIBINPUTenvironment variables.Makefileupdated to includePOETRY_EXTRASforevdevsupport.pyproject.tomlupdated with optionalevdevdependency and extras configuration.evdev_devices.pyfor device detection.listeners_pynput.pyto separatepynputbackend logic.This description was created by
for 47a5f10. It will automatically update as commits are pushed.