feat: unify status text behavior to exclusively use status command or IPC#201
Conversation
…d or IPC - Removes reading `WM_NAME` from the root window for updating the status bar text - In X11 backend, updates status by defaulting to `instantwm-<VERSION>` if empty and allows updates purely via IPC or spawned `status_command`. - Spawns the `status_command` in the Wayland DRM backend, matching behavior in Wayland winit and X11 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 GuideUnifies status text handling so that both X11 and Wayland backends rely solely on status_command or IPC, removing root-window WM_NAME as a source and ensuring a default status text is set only when none is provided, while also spawning the status command at DRM startup. Sequence diagram for unified status text initialization and updatesequenceDiagram
participant DRMRun as StartupDrm_run
participant BarStatus as BarStatus_module
participant StatusCmd as StatusCommandProcess
participant Core as CoreGlobalState
participant BarX11 as BarX11_update_status
DRMRun->>BarStatus: spawn_status_command(cmd)
BarStatus-->>StatusCmd: start process with cmd
StatusCmd-->>Core: send status text via IPC
BarX11->>Core: update_status(core, x11, x11_runtime, systray)
alt status_text is empty
BarX11->>Core: set status_text to instantwm-VERSION
else status_text already set
BarX11-->>Core: keep existing status_text
end
Class diagram for updated status handling componentsclassDiagram
class BarX11 {
+update_status(core, x11, x11_runtime, systray)
}
class WmCtxX11 {
+property_notify(e)
-x11
-x11_runtime
-core
-systray
}
class StartupDrm {
+run()
}
class BarStatus {
+spawn_status_command(cmd)
}
class CoreGlobal {
+g
}
class GlobalState {
+status_text
+cfg
+selected_monitor_id()
}
class Config {
+status_command
}
CoreGlobal *-- GlobalState
GlobalState *-- Config
StartupDrm --> BarStatus : uses
StartupDrm --> CoreGlobal : initializes
BarX11 --> CoreGlobal : updates status_text
WmCtxX11 --> BarX11 : previously triggered updates
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis PR refactors status text handling in the window manager by removing WM_NAME-based extraction from the event loop and adding explicit status command spawning during startup. Status initialization is simplified to a guard pattern, and event property notifications no longer trigger status updates for root window WM_NAME changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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:
- Consider centralizing the
status_commandspawning logic in a shared startup/helper function so that X11/Wayland/DRM backends all follow the same code path and don’t drift in behavior over time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider centralizing the `status_command` spawning logic in a shared startup/helper function so that X11/Wayland/DRM backends all follow the same code path and don’t drift in behavior over time.
## Individual Comments
### Comment 1
<location path="rust/src/bar/x11.rs" line_range="15" />
<code_context>
+pub fn update_status(
</code_context>
<issue_to_address>
**suggestion:** Update `update_status` signature or implementation to avoid unused parameters.
The function no longer uses `x11`, `x11_runtime`, or `systray`, yet still requires them. If `update_status` is now only responsible for initializing `status_text` when empty, consider removing these parameters or extracting this logic into a smaller helper. Unused parameters make call sites misleading by implying side effects that no longer exist.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -14,19 +14,8 @@ pub fn update_status( | |||
| x11_runtime: &mut X11RuntimeConfig, | |||
| systray: Option<&mut crate::types::Systray>, | |||
There was a problem hiding this comment.
suggestion: Update update_status signature or implementation to avoid unused parameters.
The function no longer uses x11, x11_runtime, or systray, yet still requires them. If update_status is now only responsible for initializing status_text when empty, consider removing these parameters or extracting this logic into a smaller helper. Unused parameters make call sites misleading by implying side effects that no longer exist.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/src/bar/x11.rs (1)
150-173:⚠️ Potential issue | 🟡 MinorRemove unused
get_text_propfunction.This function is no longer called anywhere in the codebase after the WM_NAME reading logic was removed. It should be deleted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/bar/x11.rs` around lines 150 - 173, Remove the now-unused helper function get_text_prop (signature: fn get_text_prop(x11: &X11BackendRef, win: Window, atom: u32) -> Option<String>) from the file since WM_NAME reading logic was removed and nothing calls it; delete the entire function definition and any related unused imports or references (e.g., X11BackendRef, Window usage in that function) to avoid dead code warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@rust/src/bar/x11.rs`:
- Around line 150-173: Remove the now-unused helper function get_text_prop
(signature: fn get_text_prop(x11: &X11BackendRef, win: Window, atom: u32) ->
Option<String>) from the file since WM_NAME reading logic was removed and
nothing calls it; delete the entire function definition and any related unused
imports or references (e.g., X11BackendRef, Window usage in that function) to
avoid dead code warnings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e4e3a2e-a9ab-497d-8dce-0842b4e65b04
📒 Files selected for processing (3)
rust/src/bar/x11.rsrust/src/events.rsrust/src/startup/drm/mod.rs
💤 Files with no reviewable changes (1)
- rust/src/events.rs
|
when the status command is none, then the current time should be used, as well as the "instantWM" and the instantwm version. Verify if this is already the case, and implement if it is not |
I have reviewed the code in |
- Ensures the DRM backend also spawns the configured status command (i3status-rs), aligning with the Wayland winit and X11 backends. - Removes legacy X11 `WM_NAME` logic for status bar updates in the Rust codebase so that it entirely relies on `status_command` or IPC updates. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
1 participant
Closes the issue where status text in the X11 backend was also driven by the root window
WM_NAMEproperty. Now both X11 and Wayland backends exclusively usestatus_commandor IPC to receive status text.PR created automatically by Jules for task 2906740452463338987 started by @paperbenni
Summary by Sourcery
Unify status text handling so that both X11 and DRM-based backends rely solely on configured status text sources instead of X11 root window properties.
Enhancements:
Summary by CodeRabbit
Release Notes
Improvements
New Features