Repository navigation
Conversation
mmaher88
left a comment
There was a problem hiding this comment.
Thanks for the substantial work. The ring itself looks polished and it's well tested. At 156 files and +3.6k lines it's too much to review and land as one PR, and master has moved under it. #174 made button routing per-device: the dispatcher takes PhysicalDevice*, has no ActiveDeviceResolver, and its state is keyed by serial. So the activeDevice() == device gating in AppRoot and the m_selection usage in the dispatcher would undo that design; they need to be reworked, not just rebased.
Could you split this up? Suggested order:
- The
launchAppsplitCommand fix on its own, since it's a real bug fix. - The core binding and overlay, with cursor lookup and overlay placement behind
IDesktopIntegration. KDeDesktop already has KWin script infrastructure; Hyprland and GNOME need their own cursor source, or the ring should be hidden there via a capability flag. - The editor and icons.
- The app catalog, reusing
LinuxDesktopBase's .desktop scanning. - Folders, adjustments and presets.
Also:
- Please add LayerShellQt (build) and gio (runtime) to the Arch, Fedora and Debian packaging, or official packages will silently fall back.
- Please store the ring in INI keys rather than a JSON blob, and don't reset the whole ring when one entry fails validation.
- Please get the full test suite green without depending on link order.
- Please say where the icons come from and under what licence (ideally one SVG per glyph, tinted, instead of light and dark copies).
Happy to review the first slices quickly.
|
Thank you very much for the detailed review and for pointing out these issues. I appreciate the suggested breakdown and will address the problems you identified. Work on splitting the feature has already started in the proposed order. The launchApp / splitCommand fix has been separated into its own PR (#177), and I am currently reworking the core binding and overlay on top of the latest master. This includes adapting it to per-device routing from #174, moving cursor lookup and placement behind IDesktopIntegration, using capability gating for unsupported desktops, switching the configuration to INI keys, and updating packaging dependencies and tests. The editor, icon set, application catalog, folders, adjustments, and presets will follow as separate slices. Regarding the SVGs: the glyphs currently in the Action Ring branch were created specifically for this implementation and were not copied from an external icon pack. They are contributed under the same GPL-3.0-only license as the project. I agree that this should be stated explicitly. For the icon slice, I will also replace the separate light and dark copies with one tintable SVG per glyph and add a clear licensing note. Thanks again — the feedback is very helpful, and I will keep the next PRs small and focused. |
Summary
Behavior
Validation
ctest: 782/786 passed; the four remaining failures are environment-sensitive existing tests inSettingsModelTestandHyprlandDesktopFocus, outside the files changed by this PRNotes