Fix scratchpad window visibility toggle and mapping#240
Conversation
The scratchpad logic incorrectly attempted to hide windows by merely modifying their tags and invoking `arrange()`. On the Wayland backend, this led to the window state terminating early or leaving the window on screen without mapping/unmapping correctly since it bypassed the necessary logic inside `crate::client::hide()` and `crate::client::show()`. This patch updates `scratchpad_make`, `scratchpad_hide_name`, `scratchpad_show_name`, and `scratchpad_unmake` to explicitly use `crate::client::hide` and `crate::client::show` to effectively update visibility on both X11 and Wayland backends. 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 GuideThis PR fixes scratchpad window visibility and mapping by consistently delegating show/hide behavior to the client layer and adjusting when focus/arrange/warp are invoked so hidden scratchpads are actually unmapped and remapped correctly across X11 and Wayland. Sequence diagram for scratchpad_show_name visibility handlingsequenceDiagram
actor User
participant WmCtx
participant Scratchpad as ScratchpadModule
participant Client
participant Backend
User->>WmCtx: trigger scratchpad_show_name(name)
WmCtx->>Scratchpad: scratchpad_show_name(name)
Scratchpad->>WmCtx: lookup client by name
WmCtx->>Client: get is_hidden for found
Client-->>WmCtx: is_hidden
alt scratchpad is hidden
Scratchpad->>Client: crate::client::show(ctx, found)
Client->>Backend: map_window(found)
else scratchpad is visible
Scratchpad->>WmCtx: selected_monitor_id()
Scratchpad->>WmCtx: crate::focus::focus_soft(ctx, Some(found))
Scratchpad->>WmCtx: arrange(ctx, Some(monitor_id))
WmCtx->>Backend: update_layout(monitor_id)
end
Scratchpad->>WmCtx: check focus_follows_mouse
alt focus_follows_mouse enabled
WmCtx->>Backend: warp_cursor_to_client(found)
end
Scratchpad-->>User: Ok("shown scratchpad name")
Sequence diagram for scratchpad hide and unmake lifecyclesequenceDiagram
actor User
participant WmCtx
participant Scratchpad as ScratchpadModule
participant Client
participant Backend
rect rgb(235,235,255)
Note over User,WmCtx: Hiding a visible scratchpad by name
User->>WmCtx: trigger scratchpad_hide_name(name)
WmCtx->>Scratchpad: scratchpad_hide_name(name)
Scratchpad->>WmCtx: find client by name
WmCtx->>Client: get mutable client(found)
Client-->>WmCtx: client reference
Scratchpad->>Client: set issticky = false
Scratchpad->>Client: set tags = SCRATCHPAD_MASK
Scratchpad->>Client: crate::client::hide(ctx, found)
Client->>Backend: unmap_window(found)
end
rect rgb(235,255,235)
Note over User,WmCtx: Unmaking a scratchpad (restoring normal window)
User->>WmCtx: trigger scratchpad_unmake(window_id)
WmCtx->>Scratchpad: scratchpad_unmake(ctx, window_id)
Scratchpad->>WmCtx: resolve selected_window
WmCtx->>Client: get mutable client(selected_window)
Client-->>WmCtx: client reference
Scratchpad->>Client: read is_hidden into was_hidden
Scratchpad->>Client: clear scratchpad_name
Scratchpad->>Client: set issticky = false
Scratchpad->>Client: restore tags or use monitor active tags
Scratchpad->>Client: clear scratchpad_restore_tags
alt was_hidden
Scratchpad->>Client: crate::client::show(ctx, selected_window)
Client->>Backend: map_window(selected_window)
else was visible
Scratchpad->>WmCtx: arrange(ctx, Some(monitor_id))
WmCtx->>Backend: update_layout(monitor_id)
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe scratchpad functions in Changes
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)
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:
- In
scratchpad_show_name, the guard againstmonitors.is_empty()was removed butselected_monitor_id()andarrangeare still called; if those assume at least one monitor this could panic in headless or transitional states, so consider preserving or reworking the empty-monitor handling. - When
scratchpad_show_nameencounters a hidden client, it now callsclient::showwithout thefocus_soft/restackpath that non-hidden clients take; verify whether the shown scratchpad should still be explicitly focused/restacked to keep behavior consistent with the non-hidden case.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `scratchpad_show_name`, the guard against `monitors.is_empty()` was removed but `selected_monitor_id()` and `arrange` are still called; if those assume at least one monitor this could panic in headless or transitional states, so consider preserving or reworking the empty-monitor handling.
- When `scratchpad_show_name` encounters a hidden client, it now calls `client::show` without the `focus_soft`/`restack` path that non-hidden clients take; verify whether the shown scratchpad should still be explicitly focused/restacked to keep behavior consistent with the non-hidden case.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@jules Overall Comments
|
Thank you for your feedback! I've updated |
The scratchpad logic incorrectly attempted to hide windows by merely modifying their tags and invoking `arrange()`. On the Wayland backend, this led to the window state terminating early or leaving the window on screen without mapping/unmapping correctly since it bypassed the necessary logic inside `crate::client::hide()` and `crate::client::show()`. This patch updates `scratchpad_make`, `scratchpad_hide_name`, `scratchpad_show_name`, and `scratchpad_unmake` to explicitly use `crate::client::hide` and `crate::client::show` to effectively update visibility on both X11 and Wayland backends. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
1 participant
Fix scratchpad creation and toggle issues where windows would not hide or show correctly. By explicitly invoking
crate::client::hideandcrate::client::show, both X11 and Wayland backends handle mapping and unmapping correctly as intended.PR created automatically by Jules for task 6972193914646323620 started by @paperbenni
Summary by Sourcery
Improve scratchpad window visibility handling by delegating to client-level hide/show operations instead of manual focus and arrangement logic.
Bug Fixes:
Summary by CodeRabbit