Refactor: eliminate redundant client map lookups#202
Merged
paperbenni merged 1 commit intomainfrom Mar 9, 2026
Merged
Conversation
Refactored `rust/src/` to eliminate inefficient "double lookup" anti-patterns on the global `clients` map.
Specifically removed cases where client state was queried via `.get().unwrap_or(false)` and then subsequently retrieved again via `.get_mut()` for the same window.
Instead, we now use a single `.get_mut()` with early returns (`let Some(client) = ... else { return; }`) and combine queries and mutations efficiently. For read-only queries with defaults, switched to cleaner `.is_some_and(|c| ...)` syntax.
Affected files:
- `rust/src/backend/x11/lifecycle.rs`: `configure_client_border`
- `rust/src/scratchpad.rs`: `scratchpad_toggle`, `scratchpad_status`, `scratchpad_hide_name`
- `rust/src/toggles.rs`: `set_border_width`
- `rust/src/systray.rs`: `update_systray_icon_state`
Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
Contributor
|
👋 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 GuideRefactors several window-management functions to avoid redundant lookups into the global clients map by reusing a single mutable client reference and using more idiomatic option handling, while preserving existing behavior for border, scratchpad, snapping, and systray logic. Sequence diagram for updated change_snap client accesssequenceDiagram
actor User
participant WmCtx
participant Globals
participant ClientsMap as Clients
participant Client
User->>WmCtx: change_snap(win, direction)
WmCtx->>WmCtx: g_mut()
WmCtx->>Globals: get mutable Globals
Globals->>ClientsMap: get_mut(win)
alt client exists
ClientsMap-->>Globals: Some(Client)
Globals-->>WmCtx: mutable Client reference
WmCtx->>Client: read snap_status
WmCtx->>WmCtx: compute row, col
WmCtx->>WmCtx: look up new_snap in SNAP_MATRIX
alt snap_status == SnapPosition.None and client.is_floating
WmCtx->>Client: set float_geo = geo
end
WmCtx->>Client: set snap_status = new_snap
WmCtx->>Client: read monitor_id and previous snap_status
WmCtx-->>User: return with monitor_id and previous snap_status
else no client
ClientsMap-->>Globals: None
Globals-->>WmCtx: None
WmCtx-->>User: early return
end
Class diagram for client map access refactorclassDiagram
class Globals {
+HashMap<WindowId, Client> clients
}
class CoreCtx {
+Globals g
+g_mut() Globals
}
class WmCtx {
+Globals g
+g() Globals
+g_mut() Globals
+client(win WindowId) Client
}
class Client {
+i32 border_width
+MonitorId monitor_id
+Rect geo
+Rect float_geo
+bool is_floating
+bool issticky
+u32 tags
+SnapPosition snap_status
}
class Rect {
+i32 x
+i32 y
+i32 w
+i32 h
}
class SnapPosition {
<<enum>>
None
Other
}
class SnapDir {
<<enum>>
Left
Right
Up
Down
}
class TogglesModule {
+set_border_width(core CoreCtx, win WindowId, width i32)
}
class FloatingSnapModule {
+change_snap(ctx WmCtx, win WindowId, direction SnapDir)
}
class ScratchpadModule {
+scratchpad_toggle(ctx WmCtx, name Option~str~)
+scratchpad_status(g Globals, name str) String
+scratchpad_hide_name(ctx WmCtx, name str)
}
class X11LifecycleModule {
+configure_client_border(g Globals, x11_runtime X11Runtime, w WindowId, mon_monitor_rect Rect, borderpx i32, is_monocle bool)
}
class SystrayModule {
+update_systray_icon_state(core CoreCtx, icon_win WindowId, systray Option~Systray~)
}
Globals "1" o-- "*" Client : clients
CoreCtx "1" o-- "1" Globals : g
WmCtx "1" o-- "1" Globals : g
TogglesModule ..> CoreCtx
TogglesModule ..> Client
FloatingSnapModule ..> WmCtx
FloatingSnapModule ..> Client
FloatingSnapModule ..> SnapPosition
FloatingSnapModule ..> SnapDir
ScratchpadModule ..> WmCtx
ScratchpadModule ..> Globals
ScratchpadModule ..> Client
X11LifecycleModule ..> Globals
X11LifecycleModule ..> Client
X11LifecycleModule ..> Rect
SystrayModule ..> CoreCtx
SystrayModule ..> Client
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAll five modules consolidate client state mutations by replacing separate immutable-then-mutable access patterns with direct Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![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 found 3 issues, and left some high level feedback:
- In
floating/snap.rs::change_snap, the new(monitor_id, snap_status)binding now keepssnap_statusas the old value (status) instead of the newly computednew_snap; double-check any later use ofsnap_statusin this function to ensure it still represents the intended state (you may want a separateold_snap_statusvariable instead). - In places like
systray.rs::update_systray_icon_state, you now useclients.get_muteven though the client is only read; consider keepinggetfor read-only access to avoid unnecessarily broad mutable borrows and keep future borrow interactions simpler.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `floating/snap.rs::change_snap`, the new `(monitor_id, snap_status)` binding now keeps `snap_status` as the *old* value (`status`) instead of the newly computed `new_snap`; double-check any later use of `snap_status` in this function to ensure it still represents the intended state (you may want a separate `old_snap_status` variable instead).
- In places like `systray.rs::update_systray_icon_state`, you now use `clients.get_mut` even though the client is only read; consider keeping `get` for read-only access to avoid unnecessarily broad mutable borrows and keep future borrow interactions simpler.
## Individual Comments
### Comment 1
<location path="rust/src/scratchpad.rs" line_range="172-175" />
<code_context>
- .get(&found)
- .map(|c| c.issticky)
- .unwrap_or(false);
+ let Some(client) = ctx.g().clients.get(&found) else {
+ return;
+ };
+ let is_sticky = client.issticky;
if is_sticky {
</code_context>
<issue_to_address>
**question (bug_risk):** Early-return on missing client changes behavior compared to treating it as non-sticky.
Previously, a missing client was treated as `is_sticky == false`, so the code would continue into the non-sticky path (show/create scratchpad). With the early return, a missing client now results in no action. If that behavior change is intended, please call it out in the PR description; if not, consider preserving the old semantics (e.g., keep `unwrap_or(false)` and only early-return when a client reference is strictly required).
</issue_to_address>
### Comment 2
<location path="rust/src/backend/x11/lifecycle.rs" line_range="204-213" />
<code_context>
- .unwrap_or((false, 0, 0));
- let border_width = if !isfloating
+ let Some(client) = g.clients.get_mut(&w) else {
+ return;
+ };
+
+ let border_width = if !client.is_floating
&& is_monocle
- && client_width > mon_monitor_rect.w - 30
- && client_height > mon_monitor_rect.h - 30 - bar_height
+ && client.geo.w > mon_monitor_rect.w - 30
+ && client.geo.h > mon_monitor_rect.h - 30 - bar_height
{
0
} else {
borderpx
};
- if let Some(client) = g.clients.get_mut(&w) {
- client.border_width = border_width;
- }
+ client.border_width = border_width;
</code_context>
<issue_to_address>
**question:** Early return when the client is missing skips any X11-side border configuration for that window.
Previously, a missing client fell back to `(false, 0, 0)` and still allowed a configure request to reach X11. Now, missing clients cause an early return, so X11 is never updated for that window. If there are cases where X11 still has the window but it has been removed from `g.clients`, this may leave borders out of sync. Please confirm we never call `configure_client_border` in that situation, or add explicit handling if we do.
</issue_to_address>
### Comment 3
<location path="rust/src/systray.rs" line_range="143-144" />
<code_context>
let (current_tags, _has_systray) = {
- if let Some(client) = core.g.clients.get(&icon_win) {
+ if let Some(client) = core.g.clients.get_mut(&icon_win) {
(client.tags, systray.is_some())
} else {
return;
</code_context>
<issue_to_address>
**suggestion:** Use an immutable borrow instead of `get_mut` when only reading `tags`.
Since `client` is only read here, `get` is enough and avoids an unnecessary mutable borrow on `clients`, which keeps borrowing more flexible for any future code in this scope that may need mutable access.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+172
to
+175
| let Some(client) = ctx.g().clients.get(&found) else { | ||
| return; | ||
| }; | ||
| let is_sticky = client.issticky; |
There was a problem hiding this comment.
question (bug_risk): Early-return on missing client changes behavior compared to treating it as non-sticky.
Previously, a missing client was treated as is_sticky == false, so the code would continue into the non-sticky path (show/create scratchpad). With the early return, a missing client now results in no action. If that behavior change is intended, please call it out in the PR description; if not, consider preserving the old semantics (e.g., keep unwrap_or(false) and only early-return when a client reference is strictly required).
Comment on lines
+204
to
213
| let Some(client) = g.clients.get_mut(&w) else { | ||
| return; | ||
| }; | ||
|
|
||
| let border_width = if !client.is_floating | ||
| && is_monocle | ||
| && client_width > mon_monitor_rect.w - 30 | ||
| && client_height > mon_monitor_rect.h - 30 - bar_height | ||
| && client.geo.w > mon_monitor_rect.w - 30 | ||
| && client.geo.h > mon_monitor_rect.h - 30 - bar_height | ||
| { | ||
| 0 |
There was a problem hiding this comment.
question: Early return when the client is missing skips any X11-side border configuration for that window.
Previously, a missing client fell back to (false, 0, 0) and still allowed a configure request to reach X11. Now, missing clients cause an early return, so X11 is never updated for that window. If there are cases where X11 still has the window but it has been removed from g.clients, this may leave borders out of sync. Please confirm we never call configure_client_border in that situation, or add explicit handling if we do.
Comment on lines
+143
to
144
| if let Some(client) = core.g.clients.get_mut(&icon_win) { | ||
| (client.tags, systray.is_some()) |
There was a problem hiding this comment.
suggestion: Use an immutable borrow instead of get_mut when only reading tags.
Since client is only read here, get is enough and avoids an unnecessary mutable borrow on clients, which keeps borrowing more flexible for any future code in this scope that may need mutable access.
Merged
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Refactored
rust/src/to eliminate inefficient "double lookup" anti-patterns on the globalclientsmap.Specifically removed cases where client state was queried via
.get().unwrap_or(false)and then subsequently retrieved again via.get_mut()for the same window.Instead, we now use a single
.get_mut()with early returns (let Some(client) = ... else { return; }) and combine queries and mutations efficiently. For read-only queries with defaults, switched to cleaner.is_some_and(|c| ...)syntax.Affected files:
rust/src/backend/x11/lifecycle.rs:configure_client_borderrust/src/scratchpad.rs:scratchpad_toggle,scratchpad_status,scratchpad_hide_namerust/src/toggles.rs:set_border_widthrust/src/systray.rs:update_systray_icon_statePR created automatically by Jules for task 12851667626280172314 started by @paperbenni
Summary by Sourcery
Refactor client state handling to avoid redundant map lookups and streamline geometry/border updates across window management components.
Enhancements:
Summary by CodeRabbit