feat(ipc): add opt-in complete workspace state and monitor-targeted switching - #118
Mihir-Null wants to merge 10 commits into
Conversation
jcardama
left a comment
There was a problem hiding this comment.
The three suggestions below are nice-to-have improvements, not blockers. The rebase and IPC-version reconciliation noted separately still need to be completed before landing.
The Rust and Python subscription examples in agent_docs/ipc-events.md still describe an empty filter as all event kinds, but it now delivers only legacy kinds. A consumer copying either example will not receive workspace snapshots. I suggest updating both comments to say that workspace_state requires explicit opt-in. The README stream paragraph and the subscription round-trip test comment need the same distinction.
The opt-in design and monitor-targeted command fit the intended integration. Before landing, please rebase onto current main and reconcile the IPC version with #110, which now owns version 3. I recommend version 4 for this addition while retaining the acknowledged-filter capability check and older-client compatibility.
|
These are my recommendations for this PR. They do not establish a complete policy for future IPC extensions. 1. Opt-in and default behavior I recommend keeping explicit I do not see a need for another default-migration mechanism in this PR. Consumers should check the acknowledged event filter before expecting workspace snapshots. Sending the filter alone does not prove support. 2. Commands and monitor focus I recommend keeping Keep command indices 1-based. Keep snapshot indices 0-based. Document this distinction in the maintained IPC contract. An explicit monitor target should switch the workspace on that monitor and transfer focus to it. Omitting 3. Protocol version and capability detection #110 has merged with IPC version 3. I recommend rebasing this PR onto current main and assigning IPC version 4 to this addition. Keep the version history clear: version 3 adds hotkey queries, and version 4 adds the workspace IPC capability. The personal combined build is not part of this review. Retain the acknowledged-filter capability check alongside the protocol version. Preserve older-client compatibility. For future extensions, document the version change and the capability check together. 4. Snapshot contents and publication The compact monitor, workspace, and HWND membership model fits this PR. I recommend keeping all nine workspace slots for each connected monitor. I do not propose additional fields or naming changes at this stage. Keep geometry, titles, and icons outside this snapshot contract. Consumer presentation should remain in YASB. Complete replacement snapshots are a reasonable starting point. Consumers should apply a snapshot only after they receive its complete begin/chunk/end transaction. They should discard an incomplete transaction and reconnect for a fresh snapshot after lag or a stream failure. The existing broadcaster and end-of-event publication approach fit this design. Publish a new revision when the represented state changes. Geometry-only changes should not produce a new revision. Keep the 64 KiB frame limit, including the newline. The separate review notes the cost of building the snapshot after animation-frame events. That is a nonblocking measurement suggestion, not a request to redesign the stream. Verification limits This was a source-only review. I did not run the PR or test its integration with current main. I did not review or run the companion YASB PR. These recommendations do not establish end-to-end compatibility. |
0260233 to
14d9328
Compare
|
Rebased onto current main and reconciled IPC history: v3 remains hotkey queries and workspace IPC is v4. Documentation now distinguishes legacy empty filters, 1-based commands versus 0-based snapshots, explicit monitor focus transfer, and acknowledged-filter capability detection. The historical proposal links to the maintained contract. I also added a large-membership projection benchmark and skipped end-of-event projection when no stream subscribers exist while preserving forced query/subscription synchronization. CI run 401 passes completely. |
8061172 to
1b17a13
Compare
jcardama
left a comment
There was a problem hiding this comment.
The protocol-version conflict and documentation comments are resolved. I found two remaining issues: floating-only focus restoration when the target workspace is already active, and workspace snapshot traffic consuming legacy subscribers' buffer capacity. Please fix those before landing. The client timeout suggestion is optional.
CI is green for 1b17a13. This was a source review, not local execution or live-desktop verification. Main has since advanced with #120's wry update, so integration with that newer base still needs verification.
Summary
Status bars need complete workspace membership and monitor-targeted switching. This extends the existing IPC commands and subscription transport so consumers can render every monitor's workspaces, including inactive/empty slots and floating windows.
lwm subscribe --events workspace_statelwm subscribe/ empty wire filterlwm query workspaceslwm workspace N --monitor DEVICEContract and implementation
workspace_statein the acknowledged filter.agent_docs/ipc-events.md; the original proposal is marked historical.Review 5225970035
mainated0aa13, including chore(deps): bump wry from 0.56.1 to 0.57.0 #120's wry update and chore(deps): bump toml from 1.1.5+spec-1.1.0 to 1.1.6+spec-1.1.0 #121's toml update.Validation
git diff --check: passed. Full-workspace formatting was checked before the final follow-up; daemon and CLI module trees were checked again on the final changes.233491dd2e22d39ff16f486f9e8bc3d7e468f59d: release MSVC build, GUI-subsystem verification,cargo test --all(1,549 passed, 0 failed, 13 ignored, including both CLI binaries and doc tests), 26 offline desktop acceptance helper tests, andcargo clippy --all -- -D warnings.AI assistance
Implementation, regression tests, and documentation updates were developed with OpenAI Codex assistance. Automated checks and source review do not substitute for maintainer review or live-desktop verification.