Skip to content

feat: Unified IPC-based status bar updates with i3status-rs support#198

Merged
paperbenni merged 1 commit intomainfrom
feat/unified-ipc-status-bar-10398353351146917924
Mar 8, 2026
Merged

feat: Unified IPC-based status bar updates with i3status-rs support#198
paperbenni merged 1 commit intomainfrom
feat/unified-ipc-status-bar-10398353351146917924

Conversation

@paperbenni
Copy link
Copy Markdown
Member

@paperbenni paperbenni commented Mar 8, 2026

  • Added UpdateStatus command to IPC Enum.
  • Wired up instantwmctl update-status "text" in CLI.
  • Extended parse_status_items to gracefully read i3status-rs standard output line format (JSON block array format) or fallback.
  • Ensured X11 updates trigger draw_bars_x11 for all monitors instead of just selmon_idx.
  • Ensured Wayland updates trigger redrawing properly by using bar.mark_dirty() method to bump layout seq invalidation natively.

PR created automatically by Jules for task 10398353351146917924 started by @paperbenni

Summary by Sourcery

Introduce IPC-driven status bar updates and broaden status parsing to support i3bar/i3status-rs output while ensuring bars redraw correctly on X11 and Wayland.

New Features:

  • Add an UpdateStatus IPC command and CLI subcommand to update the bar status text from external clients.
  • Support parsing i3bar-compatible JSON status lines (e.g. from i3status-rs) into status items for the bar.

Enhancements:

  • Trigger full bar redraws across all monitors on X11 and mark bars dirty on Wayland when status text is updated.
  • Align X11 property handling with explicit atom ID comparisons and add serde_json as a dependency for status parsing.

Summary by CodeRabbit

  • New Features

    • Added i3bar JSON format support for status bar parsing and rendering
    • Added UpdateStatus command to dynamically update status text via IPC/CLI with automatic bar redraw
  • Chores

    • Added serde_json dependency

This introduces the `UpdateStatus` IPC command in the rust backend
replacing the outdated `xsetroot` method to set status bar text.
It now seamlessly triggers both X11 (updating all monitors) and
Wayland bar redraws using the existing `bar_update_seq` invalidation
mechanism.

Additionally, this allows parsing native `i3status-rs` JSON format
directly, translating it into internal string rendering opcodes for
instantWM to seamlessly use its features. If the JSON is invalid or
absent, it falls back to parsing regular instantWM bar opcode string.

Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
@google-labs-jules
Copy link
Copy Markdown
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 @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@sourcery-ai
Copy link
Copy Markdown

sourcery-ai Bot commented Mar 8, 2026

Reviewer's Guide

Implements a new IPC-based mechanism for updating the status bar text (including i3status-rs/i3bar JSON support) and ensures both X11 and Wayland bars are properly redrawn on status changes, while cleaning up some X11 property handling.

Sequence diagram for IPC-based status bar update

sequenceDiagram
    actor User
    participant Instantwmctl
    participant IpcSocket
    participant Wm
    participant BackendX11
    participant BackendWayland
    participant Bar

    User->>Instantwmctl: instantwmctl update-status text
    Instantwmctl->>IpcSocket: send IpcCommand.UpdateStatus(text)
    IpcSocket->>Wm: deliver UpdateStatus(text)
    Wm->>Wm: set g.status_text = text
    Wm->>Bar: bar.mark_dirty()

    alt backend is X11
        Wm->>BackendX11: ctx = wm.ctx()
        BackendX11->>BackendX11: match WmCtx.X11(ctx)
        BackendX11->>Bar: draw_bars_x11(core, x11, x11_runtime, systray)
    else backend is Wayland
        Wm->>BackendWayland: bar.mark_dirty() triggers redraw
    end

    Bar-->>User: updated status visible on all monitors
Loading

Class diagram for status item parsing with i3bar JSON support

classDiagram
    class StatusItem {
        <<enum>>
        +Text(String)
        +SetFg(String)
        +SetBg(String)
        +ResetColors
    }

    class StatusParser {
        +parse_status_items(bytes: U8Slice) Vec_StatusItem
        +parse_i3bar_json(bytes: U8Slice) Option_Vec_StatusItem
    }

    class SerdeJson {
        +from_str(json_str: String) Vec_Value
    }

    StatusParser ..> StatusItem : produces
    StatusParser ..> SerdeJson : uses
    SerdeJson ..> StatusItem : values_mapped_to
Loading

File-Level Changes

Change Details Files
Add i3bar/i3status-rs JSON parsing for status bar text and integrate it into existing status parsing.
  • Introduce a helper that parses i3bar-style JSON block arrays into StatusItem sequences, handling color, background, and text fields and inserting reset/spacing tokens.
  • Short-circuit the existing status parser to first attempt i3bar JSON parsing and fall back to the legacy byte parser when JSON parsing fails.
rust/src/bar/status.rs
Introduce an UpdateStatus IPC command and wire it through CLI, IPC handler, and backend-specific redraw logic.
  • Extend the IpcCommand enum with an UpdateStatus(String) variant for remote status updates.
  • Add an instantwmctl subcommand UpdateStatus { text } that sends the new IPC command.
  • Handle UpdateStatus in the IPC dispatcher by updating wm.g.status_text, forcing a full X11 bar redraw via draw_bars_x11 for all monitors when on X11, and marking the bar dirty so Wayland redraws correctly.
rust/src/ipc_types.rs
rust/src/bin/instantwmctl.rs
rust/src/ipc.rs
Adjust X11 property event handling comparisons to use explicit u32 conversions and preserve bar/title updates.
  • Change WM_NAME, WM_NORMAL_HINTS, and WM_HINTS atom comparisons to use u32::from(AtomEnum::...) instead of implicit conversions for better type correctness.
  • Ensure status updates from root WM_NAME still trigger bar updates and title changes via update_status and update_title_x11.
rust/src/events.rs
Add serde_json dependency required for i3bar JSON parsing and update lockfile/patch artifacts.
  • Declare serde_json as a runtime dependency in Cargo.toml for JSON parsing support.
  • Regenerate Cargo.lock and add patch-related files (patch.diff, events.rs.orig/patch/rej) produced during development.
rust/Cargo.toml
rust/Cargo.lock
patch.diff
rust/src/events.rs.orig
rust/src/events.rs.patch
rust/src/events.rs.rej

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 8, 2026

📝 Walkthrough

Walkthrough

This PR introduces i3bar JSON parsing support for the status bar and adds a new UpdateStatus IPC command that allows dynamic status text updates via CLI and IPC, while adding serde_json as a dependency and refactoring atom comparisons for consistency.

Changes

Cohort / File(s) Summary
Dependencies
rust/Cargo.toml
Added serde_json 1.0.149 dependency to support JSON parsing.
i3bar JSON Parsing
rust/src/bar/status.rs
Introduced parse_i3bar_json function to parse i3bar JSON blocks into StatusItem variants (Text, SetFg, SetBg, ResetColors with spacing). parse_status_items now short-circuits to JSON parsing when available.
IPC UpdateStatus Command
rust/src/ipc_types.rs, rust/src/ipc.rs
Added UpdateStatus(String) variant to IpcCommand enum. IPC dispatcher assigns text to wm.g.status_text, triggers X11 bar redraw, and marks bar as dirty.
CLI UpdateStatus Support
rust/src/bin/instantwmctl.rs
Added UpdateStatus { text: String } subcommand variant to CommandKind enum and routed dispatch to IpcCommand::UpdateStatus.
Atom Comparison Refactoring
rust/src/events.rs
Standardized atom comparisons in property_notify to use explicit u32 conversions (e.g., u32::from(AtomEnum::WM_NAME)) for consistency.

Sequence Diagram(s)

sequenceDiagram
    participant CLI as instantwmctl CLI
    participant IPC as IPC Server
    participant WM as Window Manager
    participant X11 as X11 Backend

    CLI->>IPC: UpdateStatus(text)
    IPC->>WM: Dispatch UpdateStatus
    WM->>WM: wm.g.status_text = text
    WM->>X11: draw_bars_x11()
    X11->>X11: Render status bar
    X11-->>WM: Rendering complete
    WM->>IPC: Return OK
    IPC-->>CLI: Command completed
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A status bar sings in JSON's sweet refrain,
With i3bar blocks dancing through the pane,
UpdateStatus hops from CLI to IPC's door,
Atom comparisons aligned forevermore! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'feat: Unified IPC-based status bar updates with i3status-rs support' directly aligns with the main changes: adding UpdateStatus IPC command, CLI support, i3status-rs JSON parsing, and unified X11/Wayland bar update handling.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/unified-ipc-status-bar-10398353351146917924

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 2 issues, and left some high level feedback:

  • In parse_i3bar_json, you always return Some(items) even when items is empty, which prevents the legacy parser from running; consider returning None when no StatusItems were produced so the fallback path still works.
  • The comparison e.atom == u32::from(u32::from(AtomEnum::WM_NAME)) in events.rs looks like an accidental double conversion and should likely be simplified to a single u32::from(AtomEnum::WM_NAME) for clarity and correctness.
  • It looks like temporary patch artifacts (patch.diff, events.rs.orig, events.rs.patch, events.rs.rej) were committed; these should be removed from the repo before merging.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `parse_i3bar_json`, you always return `Some(items)` even when `items` is empty, which prevents the legacy parser from running; consider returning `None` when no `StatusItem`s were produced so the fallback path still works.
- The comparison `e.atom == u32::from(u32::from(AtomEnum::WM_NAME))` in `events.rs` looks like an accidental double conversion and should likely be simplified to a single `u32::from(AtomEnum::WM_NAME)` for clarity and correctness.
- It looks like temporary patch artifacts (`patch.diff`, `events.rs.orig`, `events.rs.patch`, `events.rs.rej`) were committed; these should be removed from the repo before merging.

## Individual Comments

### Comment 1
<location path="rust/src/bar/status.rs" line_range="77-79" />
<code_context>
+        if let Some(full_text) = obj.get("full_text").and_then(Value::as_str) {
+            items.push(StatusItem::Text(full_text.to_string()));
+        }
+        if !items.is_empty() {
+            items.push(StatusItem::ResetColors);
+            items.push(StatusItem::Text(" ".to_string()));
+        }
+    }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using the global `items` emptiness to decide on `ResetColors`/spacing can misfire when earlier blocks have already added items.

Because this check is based on the global `items` list rather than the current `block`, later blocks that add no items will still append `ResetColors` and a space as long as any earlier block added something. That can introduce separators/resets with no corresponding content. Consider tracking whether the current block added items (e.g., a `block_had_items` flag) and using that for this condition instead.

Suggested implementation:

```rust
    let mut items = Vec::new();
    for block in array {
        let obj = block.as_object()?;
        let mut block_had_items = false;

```

```rust
        if let Some(bg) = obj.get("background").and_then(Value::as_str) {
            if bg.starts_with('#') {
                block_had_items = true;
                items.push(StatusItem::SetBg(bg.to_string()));
            }
        }
        if let Some(full_text) = obj.get("full_text").and_then(Value::as_str) {
            block_had_items = true;
            items.push(StatusItem::Text(full_text.to_string()));
        }

```

```rust
        if let Some(full_text) = obj.get("full_text").and_then(Value::as_str) {
            block_had_items = true;
            items.push(StatusItem::Text(full_text.to_string()));
        }
        if block_had_items {
            items.push(StatusItem::ResetColors);
            items.push(StatusItem::Text(" ".to_string()));
        }

```

```rust

```

I only see part of the block-processing logic. For the behavior to be fully correct, every place inside the `for block in array` loop that pushes a `StatusItem` for that block (e.g., foreground color, underline, icons, short_text, etc.) should also set `block_had_items = true` right before or after the `items.push(...)`. That ensures the `ResetColors`/spacing is only added when the current block contributed at least one item.
</issue_to_address>

### Comment 2
<location path="rust/src/events.rs" line_range="582" />
<code_context>
+
+         let net_wm_name = ctx.x11_runtime.netatom.wm_name;
+-        if e.atom == u32::from(AtomEnum::WM_NAME as u32) || e.atom == net_wm_name {
++        if e.atom == u32::from(u32::from(AtomEnum::WM_NAME)) || e.atom == net_wm_name {
+             update_title_x11(&mut ctx.core, &ctx.x11, ctx.x11_runtime, win);
+         }
</code_context>
<issue_to_address>
**issue:** There appears to be an unnecessary double `u32::from` conversion on `AtomEnum::WM_NAME`.

`u32::from(u32::from(AtomEnum::WM_NAME))` converts the atom enum twice and is likely an accidental change. Please simplify to a single `u32::from(AtomEnum::WM_NAME)` (or match the pattern used for other atoms in this file) to keep the intent clear and avoid any potential type/trait resolution quirks.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread rust/src/bar/status.rs
Comment on lines +77 to +79
if !items.is_empty() {
items.push(StatusItem::ResetColors);
items.push(StatusItem::Text(" ".to_string()));
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (bug_risk): Using the global items emptiness to decide on ResetColors/spacing can misfire when earlier blocks have already added items.

Because this check is based on the global items list rather than the current block, later blocks that add no items will still append ResetColors and a space as long as any earlier block added something. That can introduce separators/resets with no corresponding content. Consider tracking whether the current block added items (e.g., a block_had_items flag) and using that for this condition instead.

Suggested implementation:

    let mut items = Vec::new();
    for block in array {
        let obj = block.as_object()?;
        let mut block_had_items = false;
        if let Some(bg) = obj.get("background").and_then(Value::as_str) {
            if bg.starts_with('#') {
                block_had_items = true;
                items.push(StatusItem::SetBg(bg.to_string()));
            }
        }
        if let Some(full_text) = obj.get("full_text").and_then(Value::as_str) {
            block_had_items = true;
            items.push(StatusItem::Text(full_text.to_string()));
        }
        if let Some(full_text) = obj.get("full_text").and_then(Value::as_str) {
            block_had_items = true;
            items.push(StatusItem::Text(full_text.to_string()));
        }
        if block_had_items {
            items.push(StatusItem::ResetColors);
            items.push(StatusItem::Text(" ".to_string()));
        }

I only see part of the block-processing logic. For the behavior to be fully correct, every place inside the for block in array loop that pushes a StatusItem for that block (e.g., foreground color, underline, icons, short_text, etc.) should also set block_had_items = true right before or after the items.push(...). That ensures the ResetColors/spacing is only added when the current block contributed at least one item.

Comment thread rust/src/events.rs

let net_wm_name = ctx.x11_runtime.netatom.wm_name;
if e.atom == AtomEnum::WM_NAME.into() || e.atom == net_wm_name {
if e.atom == u32::from(u32::from(AtomEnum::WM_NAME)) || e.atom == net_wm_name {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: There appears to be an unnecessary double u32::from conversion on AtomEnum::WM_NAME.

u32::from(u32::from(AtomEnum::WM_NAME)) converts the atom enum twice and is likely an accidental change. Please simplify to a single u32::from(AtomEnum::WM_NAME) (or match the pattern used for other atoms in this file) to keep the intent clear and avoid any potential type/trait resolution quirks.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (2)
rust/Cargo.toml (1)

49-49: Consider using semver-compatible version specification.

The dependency is pinned to a specific patch version 1.0.149. Consider using "1.0" to allow compatible updates, unless strict pinning is intentional for reproducibility.

♻️ Suggested change
-serde_json = "1.0.149"
+serde_json = "1.0"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@rust/Cargo.toml` at line 49, The Cargo.toml currently pins serde_json to an
exact patch "1.0.149"; change the version spec for the serde_json dependency to
a semver-compatible string like "1.0" (in Cargo.toml, the serde_json dependency
entry) so Cargo can pull compatible patch/minor updates while preserving the 1.0
API compatibility unless strict pinning is required.
rust/src/bar/status.rs (1)

63-63: Early return on non-object blocks may be too strict.

Using ? on block.as_object() causes the entire JSON parsing to fail if any single block is not an object. Consider whether a malformed block should skip to fallback parsing or just skip that block.

♻️ Alternative: skip non-object blocks instead of failing
     for block in array {
-        let obj = block.as_object()?;
+        let Some(obj) = block.as_object() else {
+            continue;
+        };
         if let Some(color) = obj.get("color").and_then(Value::as_str) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@rust/src/bar/status.rs` at line 63, The current use of the try-operator on
block.as_object() (let obj = block.as_object()?) causes the whole parse to fail
when a single block is not an object; change the logic in the function handling
blocks to treat non-object blocks as non-fatal by replacing the try with an
explicit check (e.g., if let Some(obj) = block.as_object() { ... } else {
continue; }) so malformed/non-object blocks are skipped and fallback parsing or
subsequent blocks can proceed; update any downstream uses of obj inside that
scope accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@rust/src/bar/status.rs`:
- Around line 55-83: In parse_i3bar_json, the code appends
StatusItem::ResetColors and a space whenever the cumulative items Vec is
non-empty, causing empty blocks to spuriously add separators; fix by tracking
per-block contributions (e.g., create a local block_items Vec or a boolean flag
inside the for loop), push SetFg/SetBg/Text into that per-block container, and
only when that per-block container is non-empty append ResetColors and Text(" ")
to the main items and/or extend items with the block_items; this ensures
ResetColors and spacing are only added for blocks that actually produced
content.

In `@rust/src/events.rs`:
- Line 582: The condition comparing e.atom to the WM_NAME atom uses a redundant
nested conversion u32::from(u32::from(AtomEnum::WM_NAME)); update the comparison
in events.rs (the if that checks e.atom == ...) to use a single conversion
u32::from(AtomEnum::WM_NAME) (as done earlier around the other WM_NAME check) so
the outer u32::from is removed and the comparison remains e.atom ==
u32::from(AtomEnum::WM_NAME) || e.atom == net_wm_name.

In `@rust/src/events.rs.orig`:
- Line 582: The comparison uses a redundant cast: replace the expression
u32::from(AtomEnum::WM_NAME as u32) with u32::from(AtomEnum::WM_NAME) so the
AtomEnum::WM_NAME value is converted directly; update the condition that
compares e.atom (and net_wm_name) to use u32::from(AtomEnum::WM_NAME) to match
the style used elsewhere.

In `@rust/src/events.rs.rej`:
- Around line 1-11: Remove the rejected patch file rust/src/events.rs.rej from
the commit (it’s a temporary patch rejection artifact); ensure the real source
file rust/src/events.rs contains the intended code in the property_notify
function (check the AtomEnum usage around AtomEnum::WM_NAME and netatom.wm_name
and the call to update_title_x11(&mut ctx.core, &ctx.x11, ctx.x11_runtime, win))
and commit only the corrected rust/src/events.rs rather than the .rej file.

In `@rust/src/ipc.rs`:
- Line 1: The file imports draw_bar but doesn't use it; remove the unused import
or replace it with the correct symbol. Locate the use of draw_bars_x11 in this
module (call site referenced as draw_bars_x11) and either delete the unused use
crate::bar::draw_bar; import or change it to use crate::bar::draw_bars_x11 so
the imported symbol matches the actual function used (draw_bars_x11) and
eliminate the unused-import warning.

---

Nitpick comments:
In `@rust/Cargo.toml`:
- Line 49: The Cargo.toml currently pins serde_json to an exact patch "1.0.149";
change the version spec for the serde_json dependency to a semver-compatible
string like "1.0" (in Cargo.toml, the serde_json dependency entry) so Cargo can
pull compatible patch/minor updates while preserving the 1.0 API compatibility
unless strict pinning is required.

In `@rust/src/bar/status.rs`:
- Line 63: The current use of the try-operator on block.as_object() (let obj =
block.as_object()?) causes the whole parse to fail when a single block is not an
object; change the logic in the function handling blocks to treat non-object
blocks as non-fatal by replacing the try with an explicit check (e.g., if let
Some(obj) = block.as_object() { ... } else { continue; }) so
malformed/non-object blocks are skipped and fallback parsing or subsequent
blocks can proceed; update any downstream uses of obj inside that scope
accordingly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 72a08541-0555-4860-a2aa-046ad05012de

📥 Commits

Reviewing files that changed from the base of the PR and between 343e40b and 06455f8.

⛔ Files ignored due to path filters (1)
  • rust/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (10)
  • patch.diff
  • rust/Cargo.toml
  • rust/src/bar/status.rs
  • rust/src/bin/instantwmctl.rs
  • rust/src/events.rs
  • rust/src/events.rs.orig
  • rust/src/events.rs.patch
  • rust/src/events.rs.rej
  • rust/src/ipc.rs
  • rust/src/ipc_types.rs

Comment thread rust/src/bar/status.rs
Comment on lines +55 to +83
fn parse_i3bar_json(bytes: &[u8]) -> Option<Vec<StatusItem>> {
let mut json_str = std::str::from_utf8(bytes).ok()?;
if json_str.starts_with(',') {
json_str = &json_str[1..];
}
let array: Vec<Value> = serde_json::from_str(json_str).ok()?;
let mut items = Vec::new();
for block in array {
let obj = block.as_object()?;
if let Some(color) = obj.get("color").and_then(Value::as_str) {
if color.starts_with('#') {
items.push(StatusItem::SetFg(color.to_string()));
}
}
if let Some(bg) = obj.get("background").and_then(Value::as_str) {
if bg.starts_with('#') {
items.push(StatusItem::SetBg(bg.to_string()));
}
}
if let Some(full_text) = obj.get("full_text").and_then(Value::as_str) {
items.push(StatusItem::Text(full_text.to_string()));
}
if !items.is_empty() {
items.push(StatusItem::ResetColors);
items.push(StatusItem::Text(" ".to_string()));
}
}
Some(items)
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Logic bug: ResetColors and spacing appended incorrectly for empty blocks.

The check at lines 77-80 tests if items is globally non-empty, not whether the current block contributed any items. After processing the first block with content, subsequent blocks that have no color, background, or full_text will still append ResetColors and a space because items is already non-empty from previous iterations.

🐛 Proposed fix: track per-block item count
 fn parse_i3bar_json(bytes: &[u8]) -> Option<Vec<StatusItem>> {
     let mut json_str = std::str::from_utf8(bytes).ok()?;
     if json_str.starts_with(',') {
         json_str = &json_str[1..];
     }
     let array: Vec<Value> = serde_json::from_str(json_str).ok()?;
     let mut items = Vec::new();
     for block in array {
         let obj = block.as_object()?;
+        let items_before = items.len();
         if let Some(color) = obj.get("color").and_then(Value::as_str) {
             if color.starts_with('#') {
                 items.push(StatusItem::SetFg(color.to_string()));
             }
         }
         if let Some(bg) = obj.get("background").and_then(Value::as_str) {
             if bg.starts_with('#') {
                 items.push(StatusItem::SetBg(bg.to_string()));
             }
         }
         if let Some(full_text) = obj.get("full_text").and_then(Value::as_str) {
             items.push(StatusItem::Text(full_text.to_string()));
         }
-        if !items.is_empty() {
+        if items.len() > items_before {
             items.push(StatusItem::ResetColors);
             items.push(StatusItem::Text(" ".to_string()));
         }
     }
     Some(items)
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@rust/src/bar/status.rs` around lines 55 - 83, In parse_i3bar_json, the code
appends StatusItem::ResetColors and a space whenever the cumulative items Vec is
non-empty, causing empty blocks to spuriously add separators; fix by tracking
per-block contributions (e.g., create a local block_items Vec or a boolean flag
inside the for loop), push SetFg/SetBg/Text into that per-block container, and
only when that per-block container is non-empty append ResetColors and Text(" ")
to the main items and/or extend items with the block_items; this ensures
ResetColors and spacing are only added for blocks that actually produced
content.

Comment thread rust/src/events.rs

let net_wm_name = ctx.x11_runtime.netatom.wm_name;
if e.atom == AtomEnum::WM_NAME.into() || e.atom == net_wm_name {
if e.atom == u32::from(u32::from(AtomEnum::WM_NAME)) || e.atom == net_wm_name {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

x11rb AtomEnum Into u32 conversion

💡 Result:

In x11rb, Atom is just a u32 type alias. (docs.rs)

So you can convert an AtomEnum to u32 with From/Into:

use x11rb::protocol::xproto::AtomEnum;

let id1: u32 = AtomEnum::WM_NAME.into();
let id2: u32 = u32::from(AtomEnum::WM_NAME);

AtomEnum explicitly implements From<AtomEnum> for u32, so both forms work (the .into() form just needs the target type known). (docs.rs)

Citations:


🏁 Script executed:

fd "events.rs" --type f rust/

Repository: instantOS/instantWM

Length of output: 184


🏁 Script executed:

sed -n '550,555p' rust/src/events.rs

Repository: instantOS/instantWM

Length of output: 260


🏁 Script executed:

sed -n '580,585p' rust/src/events.rs

Repository: instantOS/instantWM

Length of output: 304


Remove redundant outer u32::from() call.

Line 582 has a redundant nested u32::from(u32::from(AtomEnum::WM_NAME)) conversion. The inner u32::from() already produces a u32, making the outer call unnecessary. Line 552 uses the correct single conversion for the same WM_NAME comparison.

🐛 Proposed fix
-        if e.atom == u32::from(u32::from(AtomEnum::WM_NAME)) || e.atom == net_wm_name {
+        if e.atom == u32::from(AtomEnum::WM_NAME) || e.atom == net_wm_name {
             update_title_x11(&mut ctx.core, &ctx.x11, ctx.x11_runtime, win);
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if e.atom == u32::from(u32::from(AtomEnum::WM_NAME)) || e.atom == net_wm_name {
if e.atom == u32::from(AtomEnum::WM_NAME) || e.atom == net_wm_name {
update_title_x11(&mut ctx.core, &ctx.x11, ctx.x11_runtime, win);
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@rust/src/events.rs` at line 582, The condition comparing e.atom to the
WM_NAME atom uses a redundant nested conversion
u32::from(u32::from(AtomEnum::WM_NAME)); update the comparison in events.rs (the
if that checks e.atom == ...) to use a single conversion
u32::from(AtomEnum::WM_NAME) (as done earlier around the other WM_NAME check) so
the outer u32::from is removed and the comparison remains e.atom ==
u32::from(AtomEnum::WM_NAME) || e.atom == net_wm_name.

Comment thread rust/src/events.rs.orig
}

let net_wm_name = ctx.x11_runtime.netatom.wm_name;
if e.atom == u32::from(AtomEnum::WM_NAME as u32) || e.atom == net_wm_name {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Redundant as u32 cast inside u32::from().

Line 582 uses u32::from(AtomEnum::WM_NAME as u32) which is redundant—casting to u32 then calling u32::from() on a u32 is a no-op. This is inconsistent with line 552 which correctly uses u32::from(AtomEnum::WM_NAME).

🧹 Proposed fix
-        if e.atom == u32::from(AtomEnum::WM_NAME as u32) || e.atom == net_wm_name {
+        if e.atom == u32::from(AtomEnum::WM_NAME) || e.atom == net_wm_name {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if e.atom == u32::from(AtomEnum::WM_NAME as u32) || e.atom == net_wm_name {
if e.atom == u32::from(AtomEnum::WM_NAME) || e.atom == net_wm_name {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@rust/src/events.rs.orig` at line 582, The comparison uses a redundant cast:
replace the expression u32::from(AtomEnum::WM_NAME as u32) with
u32::from(AtomEnum::WM_NAME) so the AtomEnum::WM_NAME value is converted
directly; update the condition that compares e.atom (and net_wm_name) to use
u32::from(AtomEnum::WM_NAME) to match the style used elsewhere.

Comment thread rust/src/events.rs.rej
Comment on lines +1 to +11
--- rust/src/events.rs
+++ rust/src/events.rs
@@ -581,7 +581,7 @@ pub fn property_notify(ctx: &mut WmCtxX11<'_>, e: &PropertyNotifyEvent) {
}

let net_wm_name = ctx.x11_runtime.netatom.wm_name;
- if e.atom == u32::from(AtomEnum::WM_NAME) || e.atom == net_wm_name {
+ if e.atom == u32::from(AtomEnum::WM_NAME as u32) || e.atom == net_wm_name {
update_title_x11(&mut ctx.core, &ctx.x11, ctx.x11_runtime, win);
}
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Remove .rej file from the commit.

This is a patch rejection file (events.rs.rej) generated during a failed patch application. It should not be committed to the repository—these are temporary artifacts that indicate merge/patch conflicts.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@rust/src/events.rs.rej` around lines 1 - 11, Remove the rejected patch file
rust/src/events.rs.rej from the commit (it’s a temporary patch rejection
artifact); ensure the real source file rust/src/events.rs contains the intended
code in the property_notify function (check the AtomEnum usage around
AtomEnum::WM_NAME and netatom.wm_name and the call to update_title_x11(&mut
ctx.core, &ctx.x11, ctx.x11_runtime, win)) and commit only the corrected
rust/src/events.rs rather than the .rej file.

Comment thread rust/src/ipc.rs
@@ -1,3 +1,4 @@
use crate::bar::draw_bar;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Unused import: draw_bar.

This import is not used in the file. Line 292 calls draw_bars_x11, not draw_bar. The import should be removed or replaced.

🧹 Proposed fix
-use crate::bar::draw_bar;
+use crate::bar::draw_bars_x11;

Then update line 292 to use the imported name directly if preferred, or remove the import entirely since draw_bars_x11 is already called with its full path at crate::bar::draw_bars_x11.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@rust/src/ipc.rs` at line 1, The file imports draw_bar but doesn't use it;
remove the unused import or replace it with the correct symbol. Locate the use
of draw_bars_x11 in this module (call site referenced as draw_bars_x11) and
either delete the unused use crate::bar::draw_bar; import or change it to use
crate::bar::draw_bars_x11 so the imported symbol matches the actual function
used (draw_bars_x11) and eliminate the unused-import warning.

@paperbenni paperbenni merged commit 9160dc9 into main Mar 8, 2026
3 checks passed
@github-actions github-actions Bot mentioned this pull request Apr 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant