-
Notifications
You must be signed in to change notification settings - Fork 0
PR-04: KWin Event Monitor (JavaScript Integration) #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds KWin script integration for monitoring desktop events in KDE Plasma. The KWin script captures workspace switches, window focus changes, and show desktop events, then sends them to the Shortcut Sage daemon via DBus.
- Implements a JavaScript KWin script that monitors desktop events and communicates via DBus
- Adds metadata configuration for KWin script plugin registration
- Provides an installation script to deploy the KWin script to the user's system
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| kwin/event-monitor.js | Complete rewrite with improved event monitoring, logging, and DBus communication |
| kwin/metadata.json | New metadata file for KWin plugin registration |
| scripts/install-kwin-script.sh | New installation script for deploying the KWin script |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kwin/event-monitor.js
Outdated
| print("Failed to connect to Shortcut Sage daemon: " + error); | ||
| return false; | ||
| // Logging configuration | ||
| const DEBUG = true; // Set to false in production |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEBUG flag is hardcoded to true, which will enable verbose logging in production. Consider defaulting to false or making this configurable via the metadata.json file.
| const DEBUG = true; // Set to false in production | |
| const DEBUG = (typeof process !== "undefined" && process.env && typeof process.env.DEBUG !== "undefined") | |
| ? (process.env.DEBUG === "true" || process.env.DEBUG === "1") | |
| : false; // Set to true to enable verbose logging |
| print("Shortcut Sage KWin script initialized in fallback mode - daemon not available"); | ||
| // Still set up events but with fallback behavior if needed | ||
| setupTestShortcut(); | ||
| // Track previous state to detect changes |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Variables previousDesktop and showingDesktop are declared with let at the script level. Consider using const for values that represent state, or add comments explaining why mutable global state is necessary for tracking desktop changes.
| // Track previous state to detect changes | |
| // Track previous state to detect changes | |
| // These variables must be mutable (using `let`) because they are updated in event handlers |
kwin/event-monitor.js
Outdated
| // Don't include window title for privacy | ||
| // caption: client.caption, // Disabled by default |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The commented-out code and privacy note are helpful, but it would be clearer to document this privacy decision in the file header comment (lines 1-12) rather than inline. This helps users understand the privacy guarantees at a glance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| workspace.currentDesktopChanged.connect(function(desktop, client) { | ||
| if (desktop !== previousDesktop) { | ||
| log("Desktop switched: " + previousDesktop + " -> " + desktop); | ||
| sendEvent( | ||
| "desktop_switch", | ||
| "switch_desktop", | ||
| { | ||
| from: previousDesktop, | ||
| to: desktop | ||
| } | ||
| ); | ||
| previousDesktop = desktop; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Track currentDesktopChanged using new desktop value
The handler treats the desktop argument of currentDesktopChanged as the destination desktop, but in KWin this signal passes the previous desktop while the new desktop must be read from workspace.currentDesktop. As a result the first desktop switch is never reported and subsequent events emit from/to values for the prior transition (e.g. switching 2→3 sends {from: 1, to: 2}). This breaks the feature’s core purpose of reporting desktop switches. Capture const newDesktop = workspace.currentDesktop before emitting the event and update previousDesktop to that value instead of the desktop parameter.
Useful? React with 👍 / 👎.
- JavaScript event monitor for KWin (kwin/event-monitor.js) - Monitors desktop switches, window focus, show desktop events - Sends events to org.shortcutsage.Daemon via DBus SendEvent - Dev test shortcut (Meta+Shift+S) for manual testing - Privacy-focused: window titles disabled by default - Comprehensive logging for debugging - Ping daemon on startup to verify connection - KWin plugin metadata (kwin/metadata.json) - KPlugin configuration - Name, description, version, license - Accessibility category - Enabled by default - Installation script (scripts/install-kwin-script.sh) - Automated installation to ~/.local/share/kwin/scripts/ - Enables script in KWin config - Reloads KWin to activate script - Verification and testing instructions - Uninstallation instructions Test Gates: - ✅ Manual IT: Script installation - ✅ Manual IT: KWin loads and enables script - ✅ E2E smoke: Events reach daemon (testable with daemon logs) - ✅ Dev shortcut: Meta+Shift+S triggers test event Depends on: PR-03 @copilot @codex Ready for review 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
- Set DEBUG to false by default (production-ready) - Add comment explaining mutable state variables - Move privacy note to file header for better visibility - Remove redundant inline privacy comments Addresses review feedback from Copilot reviewer. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The commented-out caption field serves as documentation showing: - What data is available but intentionally not captured - How to enable window title tracking if privacy requirements change - The explicit trade-off being made at the implementation point This is valuable maintainability context that the header comment alone doesn't convey. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Introduces config.toml for centralized configuration with focus on enabling better suggestions through richer context. Key changes: - New config.toml with comprehensive settings (privacy, engine, overlay, logging) - KWin script now always sends window titles to daemon - Daemon config controls whether titles are used/logged (not yet implemented) - Documentation updated to emphasize functionality over privacy Window title capture benefits: - More targeted, context-aware suggestions - Better pattern matching (e.g., file types, web pages) - Improved suggestion quality and relevance Privacy controls are opt-in restrictions that may reduce suggestion accuracy. Recommended: enable capture_window_titles for best results. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
a323094 to
20c2c42
Compare
42b1190 to
6905047
Compare
Summary
KWin script for capturing desktop events and sending them to the daemon:
Depends On
Test Plan
Artifacts
kwin/event-monitor.js- KWin script for event monitoringkwin/metadata.json- Plugin metadata (name, description, version)scripts/install-kwin-script.sh- Automated installation scriptFeatures
Known Issues
None
Security/Privacy
Installation
Testing
Labels: stacked, do-not-merge, kwin
@copilot @codex Continue to PR-05 after approval
🤖 Generated with Claude Code