Document modules for supporting Wayland more clearly #146
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 48dd40c in 1 minute and 13 seconds
More details
- Looked at
79lines of code in3files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. src/getting-started.rst:56
- Draft comment:
Good addition of a Wayland note. Consider clarifying which replacement modules to use (or linking to a section detailing supported desktop environments) for extra clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The note is already concise and links to detailed information. The suggestion to add more detail here would make the note longer and potentially duplicate information that's already in the linked section. The current format follows good documentation practices - brief warning with a link to details.
The comment might have a point about user experience - some users prefer having immediate access to key information without following links.
Documentation best practices favor concise, well-organized content with clear navigation over duplicating information. The current structure achieves this.
The comment should be deleted as it suggests a change that would likely make the documentation less maintainable without significant benefit.
2. src/installing-from-source.rst:32
- Draft comment:
Useful note for Wayland users. It might help to emphasize that the default X11 watchers won't work on Wayland and to refer users to the specific replacement module info. - Reason this comment was not posted:
Marked as duplicate.
3. src/watchers.rst:24
- Draft comment:
The update listing Wayland support is clearer now. Consider ensuring consistent naming for desktop environments (e.g., listing supported ones explicitly) so users understand which DEs are supported and which are not. - Reason this comment was not posted:
Marked as duplicate.
4. src/getting-started.rst:36
- Draft comment:
Good addition of the 'Source' tab to highlight building from source. - Reason this comment was not posted:
Confidence changes required:0%
None
5. src/getting-started.rst:56
- Draft comment:
Clear note for Wayland users; consider optionally specifying that the default watchers (aw-watcher-afk and aw-watcher-window) are X11-only. - Reason this comment was not posted:
Confidence changes required:33%
None
6. src/installing-from-source.rst:31
- Draft comment:
Consistent note for Wayland systems helps users during the source build process. - Reason this comment was not posted:
Confidence changes required:0%
None
7. src/watchers.rst:6
- Draft comment:
Explicitly mentioning '(X11 only)' for the default watchers is a useful clarification for non-X11 users. - Reason this comment was not posted:
Confidence changes required:0%
None
8. src/watchers.rst:24
- Draft comment:
The updated description for aw-watcher-window-wayland clearly indicates its support (Posh, Sway). Consider noting any limitations if applicable. - Reason this comment was not posted:
Confidence changes required:33%
None
9. src/watchers.rst:25
- Draft comment:
Listing the supported desktop environments for 2e3s/awatcher is very helpful. For consistency, you might consider listing them in alphabetical order. - Reason this comment was not posted:
Confidence changes required:33%
None
Workflow ID: wflow_XZYu1mCGLfsFgsmZ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
Yeah, I just got burned as described - let's merge this please |
ErikBjare
reviewed
May 15, 2025
src/watchers.rst
Outdated
Comment on lines
19
to
20
| Window and idle watchers for Wayland | ||
| ------------------------------------------- |
There was a problem hiding this comment.
Suggested change
| Window and idle watchers for Wayland | |
| ------------------------------------------- | |
| Window watchers | |
| --------------- |
I prefer the old headings and description, but good that you link to it and make the Wayland things clear.
There was a problem hiding this comment.
Could maybe make the Wayland watchers a subsection instead, that way you can still have your wayland-watchers ref.
There was a problem hiding this comment.
I believe making Wayland watchers its own section is the best way. Makes it clear they also offer idle watcher support. Clearly separates the officially bundled watchers from the ones the ones that are needed to solve Linux compatibility issues.
ErikBjare
reviewed
May 15, 2025
ErikBjare
reviewed
May 15, 2025
src/watchers.rst
Outdated
|
|
||
| - :gh-aw:`aw-watcher-afk` - Watches for mouse & keyboard activity to detect if the user is active (by default, an inactive period of at least 3 minutes is flagged as AFK: away from keyboard). | ||
| - :gh-aw:`aw-watcher-window` - Watches the active window, its title, and it's url (on Chrome-based browsers & Safari). | ||
|
|
||
| These default watchers are collecting some of the most important data. | ||
| But there is more to collect, so here are some other watchers that let you do so. | ||
|
|
||
| .. _window-watchers: | ||
| .. note:: | ||
| For Wayland, see :ref:`wayland-watchers`. |
There was a problem hiding this comment.
Suggested change
| For Wayland, see :ref:`wayland-watchers`. | |
| For watchers supporting Wayland, see :ref:`window-watchers`. |
There was a problem hiding this comment.
A bit weird to have a link that only goes to the next section.
ErikBjare
reviewed
May 15, 2025
src/getting-started.rst
Outdated
| @@ -48,6 +52,9 @@ If you want more advanced ways to run ActivityWatch (including running it withou | |||
| .. note:: | |||
| If you are running GNOME 3 or another desktop environment that does not support system trays, or if for some reason Qt can't be used on your machine, read `Running on GNOME`. | |||
|
|
|||
| .. note:: | |||
| If your Linux system is using Wayland rather than X11, the default watchers will not work. Read :ref:`window and idle watchers for Wayland<wayland-watchers>`. | |||
There was a problem hiding this comment.
Suggested change
| If your Linux system is using Wayland rather than X11, the default watchers will not work. Read :ref:`window and idle watchers for Wayland<wayland-watchers>`. | |
| If your Linux system is using Wayland rather than X11, the default watchers will not work. See :ref:`window watchers <window-watchers>`. |
ErikBjare
reviewed
May 15, 2025
src/installing-from-source.rst
Outdated
Comment on lines
31
to
33
| .. note:: | ||
| If your Linux system is using Wayland rather than X11, the default watchers will not work. Read :ref:`window and idle watchers for Wayland<wayland-watchers>` for replacement modules supporting Wayland. | ||
|
|
There was a problem hiding this comment.
Suggested change
| .. note:: | |
| If your Linux system is using Wayland rather than X11, the default watchers will not work. Read :ref:`window and idle watchers for Wayland<wayland-watchers>` for replacement modules supporting Wayland. |
I think we've made it clear enough elsewhere in docs.
There was a problem hiding this comment.
I think users will click the "install from source" link without scrolling down and expect complete instructions to be provided there. Having an additional step for Wayland users in the "usage section" of the "getting started" page is less intuitive I think. I removed it for now and if you agree with my rationale I'll add it back.
ErikBjare
reviewed
May 15, 2025
Comment on lines
+36
to
+38
| .. group-tab:: Source | ||
|
|
||
| If you prefer to build ActivityWatch from source, check out :doc:`this guide <installing-from-source>` instead. |
ErikBjare
reviewed
May 15, 2025
…atch/docs): ActivityWatch#146 Restructured references and text, and removed the note from installing from source page.
ErikBjare
reviewed
May 15, 2025
Comment on lines
+6
to
+12
|
|
||
| .. _window-watchers: | ||
|
|
||
| Window watchers | ||
| ------------------------------------------- | ||
|
|
||
| ActivityWatch comes with two watchers enabled by default supporting Windows, macOS and Linux (X11 only): |
There was a problem hiding this comment.
Hmm, a bit confusing to list aw-watcher-afk under this heading now (doesn't watch windows).
I figured we'd keep the intro and simple nest the Wayland watchers under a Window watchers subheading (underline with ~~~ to create a subheading).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The following information is easy to miss:
Wayland users are likely to burn themselves once or twice before realising the default watchers do not support their system. Currently this is hinted with
(X11 only)in the "Window Watchers" section ofwatchers.rst, which users may not think to check, so I added a note on the "Getting Started" and "Installing from Source" pages. Information thataw-watcher-window-waylanddoesn't work on KDE or Gnome is also not clear, so I added a list of the supported DE in brackets.Also I linked to "building from source" as an installation option. At the moment it is only linked to from "Usage", which is less intuitive.
Important
Improves documentation clarity on Wayland support and installation options in ActivityWatch.
getting-started.rstandinstalling-from-source.rstabout Wayland not being supported by default watchers, with links to Wayland-compatible watchers.watchers.rstto clarify default watcher support (Windows, macOS, Linux X11) and list Wayland-compatible watchers (aw-watcher-window-wayland,2e3s/awatcher).getting-started.rstfor better visibility.This description was created by
for 48dd40c. It will automatically update as commits are pushed.