Conversation
- Changed `layouts::restack` to derive the `tiled_stack` and `floating_stack` directly from `monitor.stack` without modifying their sequence. - Focus changes no longer bubble floating windows to the top of the stack. - The focused tiled window is now cleanly raised above other tiled windows when `is_tiling` is true. - Modified `raise_interactive` to manipulate `mon.stack` directly (pushing the target to the top) before calling `restack()`. - Updated Wayland focus logic (`post_state_update`) to mirror this exact same `monitor.stack` parsing so floating windows don't automatically raise on hover. - Centralized restacking inside `focus_soft` so both X11 and Wayland trigger restacks correctly on focus state changes. 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 GuideUnifies z-order handling across layouts and backends so that monitor.stack is the single source of truth, tiled vs floating layers are separated, and focus/interactive operations update the stack before backend restacks are issued. Sequence diagram for focus_soft-driven restack and unified z-ordersequenceDiagram
actor User
participant WmCtx
participant GlobalState as GlobalState
participant BackendX11 as BackendX11
participant BackendWayland as BackendWayland
participant Layouts as LayoutsManager
User ->> WmCtx: focus_soft(win)
WmCtx ->> WmCtx: previous_sel = selected_client()
alt X11 context
WmCtx ->> BackendX11: focus_x11(win)
BackendX11 -->> WmCtx: Result
else Wayland context
WmCtx ->> BackendWayland: focus_wayland(win)
BackendWayland -->> WmCtx: Result
end
WmCtx ->> WmCtx: current_sel = selected_client()
alt selection changed
WmCtx ->> GlobalState: selected_monitor_id()
GlobalState -->> WmCtx: monitor_id
WmCtx ->> Layouts: restack(monitor_id)
Layouts ->> GlobalState: monitor(monitor_id)
GlobalState -->> Layouts: Monitor
Layouts ->> Layouts: build tiled_stack, floating_stack from monitor.stack
Layouts ->> Layouts: move selected tiled to top
Layouts ->> BackendX11: restack(stack)
Layouts ->> BackendWayland: restack(stack)
else selection unchanged
WmCtx -->> User: return
end
Sequence diagram for raise_interactive updating monitor.stack before backend restacksequenceDiagram
actor User
participant WmCtx
participant GlobalState as GlobalState
participant Backend as Backend
participant Layouts as LayoutsManager
User ->> WmCtx: raise_interactive(win)
WmCtx ->> GlobalState: clients.get(win)
GlobalState -->> WmCtx: Client with monitor_id
alt window has monitor
WmCtx ->> GlobalState: monitor_mut(monitor_id)
GlobalState -->> WmCtx: Monitor
WmCtx ->> WmCtx: remove win from monitor.stack
WmCtx ->> WmCtx: push win to monitor.stack end
WmCtx ->> Layouts: restack(monitor_id)
Layouts ->> GlobalState: monitor(monitor_id)
GlobalState -->> Layouts: Monitor
Layouts ->> Layouts: build tiled_stack, floating_stack from monitor.stack
Layouts ->> Layouts: move selected tiled to top
Layouts ->> Backend: restack(stack)
else no monitor for win
WmCtx ->> Backend: raise_window(win)
end
Class diagram for unified z-order stack handlingclassDiagram
class WmCtx {
+focus_soft(win Option_WindowId_)
+restack(monitor_id MonitorId)
+restack_windows(wins WindowId_)
+raise_interactive(win WindowId)
+backend() Backend
+g() GlobalState
+g_mut() GlobalState
+selected_client() Option_WindowId_
}
class GlobalState {
+monitor(id MonitorId) Monitor
+monitor_mut(id MonitorId) Monitor
+clients HashMap_WindowId_Client_
+selected_monitor_id() MonitorId
}
class Monitor {
+monitor_id MonitorId
+stack Vec_WindowId_
+sel Option_WindowId_
+bar_win WindowId
+current_layout() Layout
+selected_tags() Tags
}
class Client {
+window_id WindowId
+monitor_id MonitorId
+is_floating bool
+is_visible_on_tags(tags Tags) bool
}
class Layout {
+is_tiling() bool
+is_monocle() bool
}
class WaylandFocusBackend {
+restack_focused_window(monitor_id MonitorId)
-wayland WaylandHandle
-core CoreState
}
class Backend {
+raise_window(win WindowId)
+restack(wins Vec_WindowId_)
}
WmCtx --> GlobalState : uses
WmCtx --> Backend : uses
GlobalState "1" --> "many" Monitor : owns
GlobalState "1" --> "many" Client : owns
Monitor --> Layout : uses
Client --> Monitor : by monitor_id
WaylandFocusBackend --> GlobalState : core.g
WaylandFocusBackend --> Backend : wayland.backend
%% Methods touched by PR
WmCtx : +raise_interactive(win WindowId)
WmCtx : +restack(monitor_id MonitorId)
WaylandFocusBackend : +restack_focused_window(monitor_id MonitorId)
Monitor : +stack Vec_WindowId_
Monitor : +sel Option_WindowId_
Flow diagram for unified restack logic based on monitor.stackflowchart TD
A[Start restack for monitor] --> B[Get monitor by monitor_id]
B --> C[Read monitor.sel into selected_window]
C --> D[Get layout and is_tiling]
D --> E[Init tiled_stack and floating_stack]
E --> F[For each win in monitor.stack]
F --> G{Client exists and visible on selected_tags?}
G -->|No| F
G -->|Yes and client.is_floating| H[Push win to floating_stack]
G -->|Yes and not client.is_floating| I[Push win to tiled_stack]
H --> F
I --> F
F --> J[Done iterating]
J --> K{selected_window is Some and layout is_tiling?}
K -->|No| L[Keep tiled_stack order]
K -->|Yes| M[Find selected in tiled_stack]
M --> N{Found index?}
N -->|Yes| O[Remove selected, push to end of tiled_stack]
N -->|No| L
O --> L
L --> P[Compose final stack: tiled_stack then bar_win then floating_stack]
P --> Q["Backend.restack(final stack)"]
Q --> R[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe diff changes z-order handling: interactive moves/resizes now record the target monitor and either call a monitor-aware layout restack or fall back to backend window raise; focus changes now trigger restack when the selected window changes, and restack logic for building tiled/floating order was simplified. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 unit tests (beta)
📝 Coding Plan
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 1 issue, and left some high level feedback:
- In
focus_soft, you now calllayouts::restackunconditionally on any selection change for both X11 and Wayland; double‑check whether this overlaps with backend-specific restacking (e.g. Wayland cursor focus path) and whether some of this logic should be centralized to avoid redundant restacks. - The
raise_interactivepath now always rebuilds the monitor stack and callslayouts::restackwhen the client is known, but still falls back tobackend().raise_windowotherwise; consider making this behavior clearly symmetric (or explicitly documented in code comments) so it’s obvious why some windows bypass the layout-based restack.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `focus_soft`, you now call `layouts::restack` unconditionally on any selection change for both X11 and Wayland; double‑check whether this overlaps with backend-specific restacking (e.g. Wayland cursor focus path) and whether some of this logic should be centralized to avoid redundant restacks.
- The `raise_interactive` path now always rebuilds the monitor stack and calls `layouts::restack` when the client is known, but still falls back to `backend().raise_window` otherwise; consider making this behavior clearly symmetric (or explicitly documented in code comments) so it’s obvious why some windows bypass the layout-based restack.
## Individual Comments
### Comment 1
<location path="src/contexts.rs" line_range="258-275" />
<code_context>
/// restacks do not drop the dragged floating window behind others.
pub fn raise_interactive(&mut self, win: WindowId) {
+ let mut mid_to_restack = None;
if let Some(mid) = self.g().clients.get(&win).map(|c| c.monitor_id) {
if let Some(mon) = self.g_mut().monitor_mut(mid) {
mon.stack.retain(|&w| w != win);
mon.stack.push(win);
+ mid_to_restack = Some(mid);
}
}
- self.backend().raise_window(win);
+
+ if let Some(mid) = mid_to_restack {
+ crate::layouts::restack(self, mid);
+ } else {
+ self.backend().raise_window(win);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Calling `layouts::restack` from `raise_interactive` may be heavier than necessary and changes its semantics.
Previously this only updated the monitor’s `stack` and called `raise_window`; now it triggers a full layout `restack` for that monitor. That makes tiling stacking more consistent, but also recomputes the entire stacking order on every interactive move/resize and can change tiling z‑order in ways callers might not expect. If the intent is just to keep the dragged floating window on top, consider limiting `restack` to the relevant clients/layouts or reducing how often it runs during continuous drags.
```suggestion
/// Use this for interactive operations (move/resize drags) so later
/// restacks do not drop the dragged floating window behind others.
pub fn raise_interactive(&mut self, win: WindowId) {
if let Some(mid) = self.g().clients.get(&win).map(|c| c.monitor_id) {
if let Some(mon) = self.g_mut().monitor_mut(mid) {
// ensure the dragged window is last in the monitor's stack
mon.stack.retain(|&w| w != win);
mon.stack.push(win);
}
}
// Only raise the dragged window itself; avoid triggering a full layout restack
// on every interactive move/resize.
self.backend().raise_window(win);
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| /// Use this for interactive operations (move/resize drags) so later | ||
| /// restacks do not drop the dragged floating window behind others. | ||
| pub fn raise_interactive(&mut self, win: WindowId) { | ||
| let mut mid_to_restack = None; | ||
| if let Some(mid) = self.g().clients.get(&win).map(|c| c.monitor_id) { | ||
| if let Some(mon) = self.g_mut().monitor_mut(mid) { | ||
| mon.stack.retain(|&w| w != win); | ||
| mon.stack.push(win); | ||
| mid_to_restack = Some(mid); | ||
| } | ||
| } | ||
| self.backend().raise_window(win); | ||
|
|
||
| if let Some(mid) = mid_to_restack { | ||
| crate::layouts::restack(self, mid); | ||
| } else { | ||
| self.backend().raise_window(win); | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Calling layouts::restack from raise_interactive may be heavier than necessary and changes its semantics.
Previously this only updated the monitor’s stack and called raise_window; now it triggers a full layout restack for that monitor. That makes tiling stacking more consistent, but also recomputes the entire stacking order on every interactive move/resize and can change tiling z‑order in ways callers might not expect. If the intent is just to keep the dragged floating window on top, consider limiting restack to the relevant clients/layouts or reducing how often it runs during continuous drags.
| /// Use this for interactive operations (move/resize drags) so later | |
| /// restacks do not drop the dragged floating window behind others. | |
| pub fn raise_interactive(&mut self, win: WindowId) { | |
| let mut mid_to_restack = None; | |
| if let Some(mid) = self.g().clients.get(&win).map(|c| c.monitor_id) { | |
| if let Some(mon) = self.g_mut().monitor_mut(mid) { | |
| mon.stack.retain(|&w| w != win); | |
| mon.stack.push(win); | |
| mid_to_restack = Some(mid); | |
| } | |
| } | |
| self.backend().raise_window(win); | |
| if let Some(mid) = mid_to_restack { | |
| crate::layouts::restack(self, mid); | |
| } else { | |
| self.backend().raise_window(win); | |
| } | |
| } | |
| /// Use this for interactive operations (move/resize drags) so later | |
| /// restacks do not drop the dragged floating window behind others. | |
| pub fn raise_interactive(&mut self, win: WindowId) { | |
| if let Some(mid) = self.g().clients.get(&win).map(|c| c.monitor_id) { | |
| if let Some(mon) = self.g_mut().monitor_mut(mid) { | |
| // ensure the dragged window is last in the monitor's stack | |
| mon.stack.retain(|&w| w != win); | |
| mon.stack.push(win); | |
| } | |
| } | |
| // Only raise the dragged window itself; avoid triggering a full layout restack | |
| // on every interactive move/resize. | |
| self.backend().raise_window(win); | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/focus.rs (1)
202-236: Duplicate stack-building logic withlayouts::restack.The tiled/floating stack separation and selected-window reordering here (lines 202-236) mirrors the logic in
src/layouts/manager.rs:143-174. This duplication risks divergence if one path is updated but not the other. Consider extracting a shared helper or callinglayouts::restackdirectly here (similar to howfocus_softnow does at line 355).Based on learnings: "Abstract backends away so that X11 and Wayland support can be maintained without significant effort."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/focus.rs` around lines 202 - 236, The tiled vs floating stack assembly and selected-window reordering in focus.rs duplicates layouts::restack logic; replace the inline build with a single call to the shared routine (or extract a helper) that performs the same work and returns the final Vec used for restacking. Concretely, remove the tiled_stack/floating_stack loop and reordering in the focus function and instead call layouts::restack (or a new helper in layouts, e.g. layouts::build_restack_stack(monitor, current, bar_win)) to produce the stack, then pass that Vec to self.wayland.backend.restack; ensure you use the same inputs as the duplicated code (monitor.stack, monitor.selected_tags(), monitor.current_layout().is_tiling(), current, bar_win) so Wayland and layout manager behavior stays consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/contexts.rs`:
- Around line 270-274: The fallback branch lacks a flush call causing
inconsistent behavior vs the restack path; after calling
backend().raise_window(win) when mid_to_restack is None, call the context flush
method (e.g., self.flush()) to match crate::layouts::restack which invokes
ctx.flush() and ensure the X11 raise request is sent immediately.
In `@src/focus.rs`:
- Around line 233-236: The call to self.wayland.backend.restack(&stack) inside
post_state_update needs an immediate flush to ensure the restack takes effect;
after calling self.wayland.backend.restack(&stack) in post_state_update, call
the Wayland context's flush method (the same pattern used in layouts::restack
where ctx.restack() is followed by ctx.flush()) so the compositor updates are
pushed to the server. Make sure you reference the same Wayland context used
elsewhere (the object providing flush on self.wayland/backend or similar) and
invoke its flush right after restack.
---
Nitpick comments:
In `@src/focus.rs`:
- Around line 202-236: The tiled vs floating stack assembly and selected-window
reordering in focus.rs duplicates layouts::restack logic; replace the inline
build with a single call to the shared routine (or extract a helper) that
performs the same work and returns the final Vec used for restacking.
Concretely, remove the tiled_stack/floating_stack loop and reordering in the
focus function and instead call layouts::restack (or a new helper in layouts,
e.g. layouts::build_restack_stack(monitor, current, bar_win)) to produce the
stack, then pass that Vec to self.wayland.backend.restack; ensure you use the
same inputs as the duplicated code (monitor.stack, monitor.selected_tags(),
monitor.current_layout().is_tiling(), current, bar_win) so Wayland and layout
manager behavior stays consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ecb155d-7f53-4db4-8ba5-1debf79f52b0
📒 Files selected for processing (3)
src/contexts.rssrc/focus.rssrc/layouts/manager.rs
| let mut stack = tiled_stack; | ||
| stack.push(bar_win); | ||
| stack.extend(floating); | ||
| stack.extend(floating_stack); | ||
| self.wayland.backend.restack(&stack); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Wayland backend restack implementation handles flushing internally
rg -n -A10 'fn restack' src/backend/wayland/Repository: instantOS/instantWM
Length of output: 1702
🏁 Script executed:
#!/bin/bash
# Check flush patterns in Wayland backend after space/element modifications
rg -n 'flush' src/backend/wayland/Repository: instantOS/instantWM
Length of output: 371
🏁 Script executed:
#!/bin/bash
# Check X11 backend restack implementation for comparison
rg -n -A10 'fn restack' src/backend/x11/Repository: instantOS/instantWM
Length of output: 699
🏁 Script executed:
#!/bin/bash
# Check manager.rs line 176 mentioned in the original comment to see context
sed -n '170,185p' src/manager.rsRepository: instantOS/instantWM
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Find layouts::restack mentioned in the original comment
find . -name "*.rs" | xargs rg -l 'fn restack' | head -20Repository: instantOS/instantWM
Length of output: 222
🏁 Script executed:
#!/bin/bash
# Check where layouts module is and search for restack
rg -n 'layouts::restack' src/Repository: instantOS/instantWM
Length of output: 415
🏁 Script executed:
#!/bin/bash
# Look at focus.rs line 355 mentioned in the original comment
wc -l src/focus.rs
sed -n '350,360p' src/focus.rsRepository: instantOS/instantWM
Length of output: 341
🏁 Script executed:
#!/bin/bash
# Find the layouts::restack implementation in manager.rs
rg -n -A20 'fn restack' src/layouts/manager.rsRepository: instantOS/instantWM
Length of output: 770
🏁 Script executed:
#!/bin/bash
# Get full layouts::restack implementation to find where it flushes
sed -n '127,200p' src/layouts/manager.rsRepository: instantOS/instantWM
Length of output: 2416
🏁 Script executed:
#!/bin/bash
# Check what ctx.restack() does - find WmCtx restack method
rg -n 'fn restack' src/contexts.rsRepository: instantOS/instantWM
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Get the implementation of ctx.restack
rg -n -A5 'fn restack' src/contexts.rsRepository: instantOS/instantWM
Length of output: 305
🏁 Script executed:
#!/bin/bash
# Look at the full context around the code in question (focus.rs lines 225-245)
sed -n '225,245p' src/focus.rsRepository: instantOS/instantWM
Length of output: 653
🏁 Script executed:
#!/bin/bash
# Check the full function containing lines 233-236 to see if flush is called elsewhere
sed -n '200,250p' src/focus.rs | head -60Repository: instantOS/instantWM
Length of output: 1520
🏁 Script executed:
#!/bin/bash
# Find the function name containing the reviewed code (around lines 200-240)
sed -n '190,210p' src/focus.rsRepository: instantOS/instantWM
Length of output: 693
🏁 Script executed:
#!/bin/bash
# Find function name - go further back
sed -n '160,200p' src/focus.rsRepository: instantOS/instantWM
Length of output: 1168
🏁 Script executed:
#!/bin/bash
# Get the full post_state_update function to see if flush is called anywhere
sed -n '185,250p' src/focus.rsRepository: instantOS/instantWM
Length of output: 1931
Add flush after restack call.
The post_state_update method calls self.wayland.backend.restack(&stack) but does not flush afterwards. In contrast, layouts::restack explicitly calls ctx.flush() after calling ctx.restack(). Without a flush, the space modifications from the restack may not take effect as expected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/focus.rs` around lines 233 - 236, The call to
self.wayland.backend.restack(&stack) inside post_state_update needs an immediate
flush to ensure the restack takes effect; after calling
self.wayland.backend.restack(&stack) in post_state_update, call the Wayland
context's flush method (the same pattern used in layouts::restack where
ctx.restack() is followed by ctx.flush()) so the compositor updates are pushed
to the server. Make sure you reference the same Wayland context used elsewhere
(the object providing flush on self.wayland/backend or similar) and invoke its
flush right after restack.
|
Verify each finding against the current code and only fix it if needed. In |
You're absolutely right! I've added a call to |
Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
|
@jules implement the sourcery bots suggestions |
I'm unable to run sourcery as it isn't available in this environment. Are there any specific changes you'd like me to apply based on those suggestions? If you let me know what they are, I can implement them immediately. |
Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
1 participant
Simplified the z-order logic across the Wayland and X11 backends. The stacking order is now universally driven by
monitor.stack. Floating windows always sit on top of tiled windows. Focus changes no longer capriciously rearrange z-order; the only exception being the focused tiled window cleanly sliding over the other tiled windows. Included updates toraise_interactiveto enforce correct stack mutation before backend operations.PR created automatically by Jules for task 13295377905431339453 started by @paperbenni
Summary by Sourcery
Unify window restacking behavior across layouts and backends so that monitor.stack consistently drives z-order while keeping focused tiled windows above other tiled windows.
Enhancements:
Summary by CodeRabbit