Conversation
Added `tag_focus_history` to `Monitor` to maintain the last focused window for each tag combination. When switching views, `resolve_focus_target` now checks this history before falling back to the topmost window in the visible stack. This prevents focus from resetting to the top window every time the user switches back to a previously visited tag. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideTracks and restores the last focused window per tag on each monitor by storing focus history and consulting it when resolving focus targets, falling back to the existing top-of-stack behavior when necessary. Sequence diagram for resolving and updating focus with tag historysequenceDiagram
actor User
participant WindowManager
participant CoreCtx
participant Monitor
participant ClientsStore
User->>WindowManager: switch_tag(new_tag_mask)
WindowManager->>CoreCtx: resolve_focus_target(core, None)
CoreCtx->>Monitor: get_selected_tags()
Monitor-->>CoreCtx: selected_tag_mask
Note over CoreCtx,Monitor: Resolve focus target
CoreCtx->>Monitor: lookup tag_focus_history[selected_tag_mask]
alt history entry exists
Monitor-->>CoreCtx: hist_win
CoreCtx->>ClientsStore: get(hist_win)
ClientsStore-->>CoreCtx: client
alt client visible_on_tags(selected_tag_mask) and not hidden
CoreCtx-->>WindowManager: FocusTargetResult(target = hist_win)
else client invalid
CoreCtx->>Monitor: first_visible_client(clients_map)
Monitor-->>CoreCtx: top_win or None
CoreCtx-->>WindowManager: FocusTargetResult(target = top_win or None)
end
else no history entry
CoreCtx->>Monitor: first_visible_client(clients_map)
Monitor-->>CoreCtx: top_win or None
CoreCtx-->>WindowManager: FocusTargetResult(target = top_win or None)
end
Note over WindowManager,Monitor: Update focus state
WindowManager->>CoreCtx: update_focus_state(core, result)
CoreCtx->>Monitor: set sel = result.target
alt result.target is Some(t)
CoreCtx->>Monitor: tag_focus_history.insert(selected_tags(), t)
end
Monitor-->>User: last focused window on tag is restored
Class diagram for updated Monitor focus historyclassDiagram
class Monitor {
+Vec~u32~ tags
+Vec~WindowId~ clients
+Option~WindowId~ sel
+HashMap~u32, WindowId~ tag_focus_history
+Option~WindowId~ overlay
+Vec~WindowId~ stack
+Option~WindowId~ fullscreen
+selected_tags() u32
+first_visible_client(clients_map) Option~WindowId~
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughImplements tag-based focus history for monitors. When resolving a focus target, the system now checks the monitor's focus history for the current tag mask before falling back to the first visible client. Focus state updates record the target window to this history, enabling retention of window focus across tag switches. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider using a single consistent source for the tag mask key (e.g., always
selected_tags()or alwaysselected.bits()) in bothresolve_focus_targetandupdate_focus_stateto avoid subtle mismatches if the representation changes. - It might be worth pruning or updating
tag_focus_historyentries when a window is removed or no longer matches a tag mask so the map does not accumulate stale window IDs over time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using a single consistent source for the tag mask key (e.g., always `selected_tags()` or always `selected.bits()`) in both `resolve_focus_target` and `update_focus_state` to avoid subtle mismatches if the representation changes.
- It might be worth pruning or updating `tag_focus_history` entries when a window is removed or no longer matches a tag mask so the map does not accumulate stale window IDs over time.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/types/monitor.rs (1)
117-117: Add cleanup for staletag_focus_historyentries when clients are destroyed.The
tag_focus_historymap accumulates entries pointing to dead windows as clients are closed. While the lookup inresolve_focus_targetvalidates that windows still exist (preventing crashes), entries for destroyed windows remain in the map. The pattern for this cleanup already exists inunmanage()—overlay and fullscreen references are explicitly cleared for destroyed windows. Extend this totag_focus_historyby removing entries that reference the destroyed window, or periodically prune entries for non-existent windows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/monitor.rs` at line 117, When a client is destroyed unmanage() should remove any entries in tag_focus_history that point to that client's window to avoid accumulating stale references; update unmanage() to iterate tag_focus_history (the HashMap) and remove keys whose value == client.win (or otherwise reference the destroyed window), similar to how overlay and fullscreen are cleared, or add a helper prune_tag_focus_history(window) and call it from unmanage(); ensure resolve_focus_target still validates existence after this cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/types/monitor.rs`:
- Line 117: When a client is destroyed unmanage() should remove any entries in
tag_focus_history that point to that client's window to avoid accumulating stale
references; update unmanage() to iterate tag_focus_history (the HashMap) and
remove keys whose value == client.win (or otherwise reference the destroyed
window), similar to how overlay and fullscreen are cleared, or add a helper
prune_tag_focus_history(window) and call it from unmanage(); ensure
resolve_focus_target still validates existence after this cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1516927c-ecb3-4ae1-850e-2e25c8d66929
📒 Files selected for processing (2)
src/focus.rssrc/types/monitor.rs
1 participant
Fixes an issue where switching back to a tag would always select the window at the top of the stack, rather than the window that was previously active when the user left the tag.
tag_focus_history: HashMap<u32, WindowId>toMonitor.update_focus_stateto store the newly focused window into the active tag mask's history.resolve_focus_targetto consulttag_focus_historybefore usingfirst_visible_clientas a fallback.PR created automatically by Jules for task 1143635075217860583 started by @paperbenni
Summary by Sourcery
Maintain per-tag focus history so returning to a tag restores the previously focused window instead of always selecting the top of the stack.
Bug Fixes:
Enhancements:
Summary by CodeRabbit