Fix fullscreen window styling and stacking order#234
Merged
paperbenni merged 3 commits intomainfrom Mar 15, 2026
Merged
Conversation
- Exclude true fullscreen windows from tiling calculations in `Client::is_tiled`. - Add `apply_fullscreen` helper in `arrange_monitor` to resize fullscreen windows to the entire monitor. - Update `apply_border_widths` to correctly strip borders from true fullscreen windows. - In `restack`, push true fullscreen windows to a new `fullscreen_stack` positioned above `bar_win` and `floating_stack`, guaranteeing they obscure everything else on screen. 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 GuideAdjusts tiling logic, border handling, geometry, and restacking so true fullscreen windows are excluded from tiling, rendered borderless at monitor size, and always stacked above the bar and floating windows, plus adds lightweight project docs and a test helper script. Sequence diagram for arranging and restacking true fullscreen windowssequenceDiagram
actor User
participant WaylandClient
participant WaylandState
participant Globals
participant WmCtx
participant LayoutManager
User->>WaylandClient: Request fullscreen
WaylandClient->>WaylandState: fullscreen_request
WaylandState->>Globals: set client.is_fullscreen = true
WaylandState->>Globals: set monitor.fullscreen = Some(win)
WaylandState->>Globals: set layout_dirty = true
loop On next arrange
WmCtx->>LayoutManager: arrange(monitor_id)
LayoutManager->>WmCtx: show_hide
LayoutManager->>LayoutManager: arrange_monitor(monitor_id)
rect rgb(230,230,255) Fullscreen aware layout
LayoutManager->>LayoutManager: apply_border_widths
Note right of LayoutManager: strip border if client.is_true_fullscreen()
LayoutManager->>LayoutManager: run_layout
Note right of LayoutManager: exclude clients where is_true_fullscreen() from tiling via is_tiled
LayoutManager->>LayoutManager: apply_fullscreen
Note right of LayoutManager: resize all is_true_fullscreen clients to monitor_rect
LayoutManager->>LayoutManager: place_overlay
end
LayoutManager->>LayoutManager: restack(monitor_id)
Note right of LayoutManager: build tiled_stack, floating_stack, fullscreen_stack
LayoutManager->>WmCtx: restack(stack = tiled + bar + floating + fullscreen)
end
Updated class diagram for client tiling and fullscreen handlingclassDiagram
class Client {
bool is_floating
bool is_fullscreen
bool isfakefullscreen
bool is_hidden
Geo geo
Geo old_geo
bool is_visible_on_tags(u32 selected_tags)
bool is_true_fullscreen()
bool is_tiled(u32 selected_tags)
}
class ClientMethodsChanges {
+bool is_true_fullscreen()
+bool is_tiled(u32 selected_tags)
}
Client <|-- ClientMethodsChanges
class LayoutManager {
+arrange(WmCtx ctx, MonitorId monitor_id)
+arrange_monitor(WmCtx ctx, MonitorId monitor_id)
+apply_border_widths(WmCtx ctx, MonitorId monitor_id)
+run_layout(WmCtx ctx, MonitorId monitor_id)
+apply_fullscreen(WmCtx ctx, MonitorId monitor_id)
+place_overlay(WmCtx ctx, MonitorId monitor_id)
+restack(WmCtx ctx, MonitorId monitor_id)
}
class Monitor {
MonitorId id
Rect monitor_rect
u32 clientcount
WindowId[] clients
WindowId[] stack
u32 selected_tags()
u32 tiled_client_count(HashMap_WindowId_Client clients)
Vec_Client collect_tiled(HashMap_WindowId_Client clients)
}
class StackingLogic {
WindowId[] tiled_stack
WindowId[] floating_stack
WindowId[] fullscreen_stack
WindowId bar_win
WindowId selected_window
}
LayoutManager --> Monitor : uses
LayoutManager --> Client : layouts and fullscreen
LayoutManager --> StackingLogic : builds and merges stacks
class TilingFilterChange {
+bool is_tiled(u32 selected_tags)
}
TilingFilterChange : now requires !is_true_fullscreen()
Client <|-- TilingFilterChange
class BorderWidthLogic {
+apply_border_widths(WmCtx ctx, MonitorId monitor_id)
}
BorderWidthLogic : strip_border if info.is_true_fullscreen()
LayoutManager <|-- BorderWidthLogic
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds true-fullscreen handling: detects and resizes true-fullscreen windows to the monitor rect during layout, strips their borders, separates them into a fullscreen stack for restacking/z-order, and updates client tiling logic to exclude true-fullscreen clients. Changes
Sequence DiagramsequenceDiagram
participant AM as arrange_monitor
participant AF as apply_fullscreen
participant M as Monitor
participant C as Client
participant RS as Restack
AM->>AF: invoke with monitor & selected tags
AF->>M: query monitor rect
AF->>C: detect true_fullscreen windows on tags
AF->>C: set geometry to monitor rect
AM->>AM: run layout
AM->>RS: build stacks from monitor.clients
RS->>RS: collect tiled_stack, floating_stack, fullscreen_stack
RS->>RS: promote selected fullscreen if present
RS->>RS: assemble final z-order and restack
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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 left some high level feedback:
- The detailed analysis in
plan.mdlooks more like design notes than source and may not belong in the repo long term; consider either converting relevant parts into comments near the changed code or removing the file before merging. run_test.shis a very thin wrapper aroundcargo test; if there isn’t a repo-specific need for this script (e.g., extra flags or setup), consider dropping it to avoid redundant maintenance.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The detailed analysis in `plan.md` looks more like design notes than source and may not belong in the repo long term; consider either converting relevant parts into comments near the changed code or removing the file before merging.
- `run_test.sh` is a very thin wrapper around `cargo test`; if there isn’t a repo-specific need for this script (e.g., extra flags or setup), consider dropping it to avoid redundant maintenance.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@jules |
- Exclude true fullscreen windows from tiling calculations in `Client::is_tiled`. - Add `apply_fullscreen` helper in `arrange_monitor` to resize fullscreen windows to the entire monitor. - Update `apply_border_widths` to correctly strip borders from true fullscreen windows. - In `restack`, push true fullscreen windows to a new `fullscreen_stack` positioned above `bar_win` and `floating_stack`, guaranteeing they obscure everything else on screen. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/layouts/manager.rs (2)
56-57: Avoid duplicate monitor lookup andunwrap()in fullscreen pass.Line 56 re-queries monitor state and uses
unwrap(). You can keep this single-pass and panic-free by extractingselected_tagsin the first match.♻️ Proposed refactor
-fn apply_fullscreen(ctx: &mut WmCtx<'_>, monitor_id: MonitorId) { - let (mon_rect, clients) = match ctx.g().monitor(monitor_id) { - Some(m) => (m.monitor_rect, m.clients.clone()), +fn apply_fullscreen(ctx: &mut WmCtx<'_>, monitor_id: MonitorId) { + let (mon_rect, selected_tags, clients) = match ctx.g().monitor(monitor_id) { + Some(m) => (m.monitor_rect, m.selected_tags(), m.clients.clone()), None => return, }; - - let selected_tags = ctx.g().monitor(monitor_id).unwrap().selected_tags(); let fullscreen_windows: Vec<_> = clients .into_iter()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/layouts/manager.rs` around lines 56 - 57, The fullscreen pass re-queries the monitor and calls unwrap() on ctx.g().monitor(monitor_id) — instead, reuse the monitor already matched earlier (e.g. the variable used in the surrounding match) and extract selected_tags once there, then pass that value into the fullscreen branch; remove the duplicate ctx.g().monitor(monitor_id).unwrap() call and avoid unwrap() by using the existing monitor reference (or handle the Option/Result) when obtaining selected_tags so the code is single-pass and panic-free.
221-227: Update the z-order comment to include fullscreen layer.The comment describes only tiled/bar/floating order, but Line 227 now appends fullscreen windows too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/layouts/manager.rs` around lines 221 - 227, Update the z-order comment above the stack assembly to reflect that fullscreen windows are also included: mention the final order is tiled clients, then the bar, then floating clients, and finally fullscreen clients (since the code pushes bar_win, then extends floating_stack, then extends fullscreen_stack). Edit the comment near the let mut stack = tiled_stack; block (referencing tiled_stack, bar_win, floating_stack, fullscreen_stack) to describe the full ordering including the fullscreen layer.
🤖 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/layouts/manager.rs`:
- Around line 56-57: The fullscreen pass re-queries the monitor and calls
unwrap() on ctx.g().monitor(monitor_id) — instead, reuse the monitor already
matched earlier (e.g. the variable used in the surrounding match) and extract
selected_tags once there, then pass that value into the fullscreen branch;
remove the duplicate ctx.g().monitor(monitor_id).unwrap() call and avoid
unwrap() by using the existing monitor reference (or handle the Option/Result)
when obtaining selected_tags so the code is single-pass and panic-free.
- Around line 221-227: Update the z-order comment above the stack assembly to
reflect that fullscreen windows are also included: mention the final order is
tiled clients, then the bar, then floating clients, and finally fullscreen
clients (since the code pushes bar_win, then extends floating_stack, then
extends fullscreen_stack). Edit the comment near the let mut stack =
tiled_stack; block (referencing tiled_stack, bar_win, floating_stack,
fullscreen_stack) to describe the full ordering including the fullscreen layer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4c7986f-6dfa-4bb7-b7f1-4ec068afded0
📒 Files selected for processing (2)
src/layouts/manager.rssrc/types/client.rs
Member
Author
|
@jules Nitpick comments:
|
Contributor
I have addressed both nitpicks:
Changes have been made and tests run successfully. |
- Exclude true fullscreen windows from tiling calculations in `Client::is_tiled`. - Add `apply_fullscreen` helper in `arrange_monitor` to resize fullscreen windows to the entire monitor. - Update `apply_border_widths` to correctly strip borders from true fullscreen windows. - In `restack`, push true fullscreen windows to a new `fullscreen_stack` positioned above `bar_win` and `floating_stack`, guaranteeing they obscure everything else on screen. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
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.
This PR addresses issues where fullscreen windows, particularly on Wayland, continued to render with visible borders and allowed the bar or floating windows to appear above them.
src/types/client.rs:is_tilednow checks!self.is_true_fullscreen(), avoiding layout algorithms inadvertently shifting or resizing true fullscreen windows as if they were tiled.src/layouts/manager.rs:apply_fullscreenfunction to explicitly map all fullscreen windows to themonitor_rect, and included a call to this function inarrange_monitor.apply_border_widthsto strip borders forinfo.is_true_fullscreen().restackto maintain a dedicatedfullscreen_stack. This stack is appended after thebar_winandfloating_stack, ensuring fullscreen windows retain top-most z-index priority.PR created automatically by Jules for task 17069478896616846344 started by @paperbenni
Summary by Sourcery
Add planning notes for fullscreen layout and border handling changes and introduce a basic test runner script.
Build:
cargo test.Chores:
Summary by CodeRabbit