Fix i3status-rs powerline gaps and default status flickering#226
Merged
paperbenni merged 1 commit intomainfrom Mar 13, 2026
Merged
Fix i3status-rs powerline gaps and default status flickering#226paperbenni merged 1 commit intomainfrom
paperbenni merged 1 commit intomainfrom
Conversation
- Fix powerline gap rendering by conditionally removing `TEXT_PADDING` when a block specifies no separator (`separator: false`) and a `separator_block_width` of 0. This allows powerline triangles to sit flush against adjacent blocks. - Fix UI flickering back to "instantwm-VERSION" default text by introducing a global atomic flag (`CUSTOM_STATUS_RECEIVED`) that halts the default status thread once a custom update is received via IPC. 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 i3status-rs block padding logic to remove unintended gaps for powerline-style blocks without separators, and introduces an atomic flag to permanently stop the default status-emitting thread once a custom status is received via IPC, preventing flickering/overwrites. Sequence diagram for default status thread halting on custom IPC statussequenceDiagram
actor User
participant ExternalStatusProducer
participant IpcClient
participant WindowManager as WindowManager_update_status
participant StatusModule as BarStatusModule
participant DefaultThread as DefaultStatusThread
User->>ExternalStatusProducer: Configure custom status command
ExternalStatusProducer->>IpcClient: Emit custom status text
IpcClient->>WindowManager: update_status(text)
alt text does not start with instantwm-
WindowManager->>BarStatusModule: set CUSTOM_STATUS_RECEIVED = true
else default instantwm status
WindowManager->>BarStatusModule: do not modify CUSTOM_STATUS_RECEIVED
end
WindowManager->>WindowManager: g.status_text = text
par default status loop
loop every 30 seconds
DefaultThread->>BarStatusModule: load CUSTOM_STATUS_RECEIVED
alt CUSTOM_STATUS_RECEIVED is true
DefaultThread-->>DefaultThread: break loop and stop thread
else CUSTOM_STATUS_RECEIVED is false
DefaultThread->>WindowManager: send_status_ipc(default_status_text())
end
end
end
Class diagram for updated status and IPC handling modulesclassDiagram
class BarStatusModule {
+CUSTOM_STATUS_RECEIVED : AtomicBool
+spawn_default_status()
+measure_i3_block_width(block, painter)
+draw_i3_block(block, painter, x, y)
}
class IpcGeneralModule {
+update_status(wm, text)
}
class WindowManager {
+g_status_text : String
+backend : Backend
}
class Backend {
}
class X11Backend {
}
Backend <|-- X11Backend
IpcGeneralModule --> WindowManager : modifies_g_status_text
IpcGeneralModule --> BarStatusModule : sets_CUSTOM_STATUS_RECEIVED
BarStatusModule --> WindowManager : send_status_ipc(default_status_text)
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 (2)
📝 WalkthroughWalkthroughThe changes introduce a new atomic flag ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
✨ 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 3 issues, and left some high level feedback:
- The
CUSTOM_STATUS_RECEIVEDAtomicBoolis declared as a public static; consider making it module-private (or exposing a small API) to avoid leaking this synchronization detail outsidebar::status. - The padding computation
if !block.separator && block.separator_block_width == 0 { 0 } else { TEXT_PADDING }is duplicated in bothmeasure_i3_block_widthanddraw_i3_block; factoring this into a small helper will reduce the risk of these diverging in future changes. - The
"instantwm-"string used to detect custom status inupdate_statusis a magic prefix; consider extracting it into a named constant so future changes to the default status format remain centralized.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `CUSTOM_STATUS_RECEIVED` `AtomicBool` is declared as a public static; consider making it module-private (or exposing a small API) to avoid leaking this synchronization detail outside `bar::status`.
- The padding computation `if !block.separator && block.separator_block_width == 0 { 0 } else { TEXT_PADDING }` is duplicated in both `measure_i3_block_width` and `draw_i3_block`; factoring this into a small helper will reduce the risk of these diverging in future changes.
- The `"instantwm-"` string used to detect custom status in `update_status` is a magic prefix; consider extracting it into a named constant so future changes to the default status format remain centralized.
## Individual Comments
### Comment 1
<location path="src/bar/status.rs" line_range="14" />
<code_context>
pub(crate) const TEXT_PADDING: i32 = 6;
const DEFAULT_SEPARATOR_BLOCK_WIDTH: i32 = 9;
+pub static CUSTOM_STATUS_RECEIVED: std::sync::atomic::AtomicBool = std::sync::atomic::AtomicBool::new(false);
+
#[derive(Debug, Clone)]
</code_context>
<issue_to_address>
**suggestion:** Consider avoiding a public global mutable state for the status flag
Public mutable `pub static` state makes the code harder to reason about as usage grows. Consider keeping this `AtomicBool` private to the module and exposing functions like `set_custom_status_received()` / `custom_status_received()` so the synchronization details remain encapsulated and can be changed later without impacting callers.
Suggested implementation:
```rust
pub(crate) const TEXT_PADDING: i32 = 6;
const DEFAULT_SEPARATOR_BLOCK_WIDTH: i32 = 9;
use std::sync::atomic::{AtomicBool, Ordering};
static CUSTOM_STATUS_RECEIVED: AtomicBool = AtomicBool::new(false);
pub(crate) fn set_custom_status_received() {
CUSTOM_STATUS_RECEIVED.store(true, Ordering::Relaxed);
}
pub(crate) fn clear_custom_status_received() {
CUSTOM_STATUS_RECEIVED.store(false, Ordering::Relaxed);
}
pub(crate) fn custom_status_received() -> bool {
CUSTOM_STATUS_RECEIVED.load(Ordering::Relaxed)
}
#[derive(Debug, Clone)]
pub(crate) enum StatusItem {
```
1. Replace any external uses of `CUSTOM_STATUS_RECEIVED` (e.g. reads with `.load(...)`) with calls to `custom_status_received()`.
2. Replace any external writes to `CUSTOM_STATUS_RECEIVED` (e.g. `.store(true/false, ...)` or `.swap(...)`) with the appropriate helper: `set_custom_status_received()` or `clear_custom_status_received()`. If there are more complex update patterns, add additional helper functions in this module instead of exposing the static.
3. Once all external uses are replaced, ensure `CUSTOM_STATUS_RECEIVED` is not re-exported elsewhere (e.g. via `pub use`), to keep the synchronization details encapsulated in this module.
</issue_to_address>
### Comment 2
<location path="src/bar/status.rs" line_range="878-882" />
<code_context>
thread::sleep(Duration::from_millis(500));
loop {
+ if CUSTOM_STATUS_RECEIVED.load(Ordering::Relaxed) {
+ break;
+ }
send_status_ipc(&default_status_text());
thread::sleep(Duration::from_secs(30));
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** There may be an extra default status update after a custom status arrives
Because the flag is only checked at the top of the loop, a custom status arriving after the `load` but before `send_status_ipc` will still result in one extra default update. If you want to stop sending immediately once the custom status is set, either re-check the flag right before `send_status_ipc`, or move the send to the end of the loop and guard it with a flag check before each send.
</issue_to_address>
### Comment 3
<location path="src/bar/status.rs" line_range="587-595" />
<code_context>
None => 0,
};
+ let padding = if !block.separator && block.separator_block_width == 0 {
+ 0
+ } else {
+ TEXT_PADDING
+ };
+
let content_width = text_width.max(min_width).max(0);
let border_width = block.border_left + block.border_right;
- let block_width = border_width + TEXT_PADDING * 2 + content_width;
+ let block_width = border_width + padding * 2 + content_width;
let separator_width = if block.separator {
</code_context>
<issue_to_address>
**suggestion:** The padding calculation is duplicated and could be centralized
The `padding` condition (`if !block.separator && block.separator_block_width == 0 { 0 } else { TEXT_PADDING }`) is repeated in `measure_i3_block_width` and `draw_i3_block`. Please extract it into a helper (e.g., `fn block_padding(block: &I3Block) -> i32`) to keep the behavior consistent and avoid future divergence.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| pub(crate) const TEXT_PADDING: i32 = 6; | ||
| const DEFAULT_SEPARATOR_BLOCK_WIDTH: i32 = 9; | ||
|
|
||
| pub static CUSTOM_STATUS_RECEIVED: std::sync::atomic::AtomicBool = std::sync::atomic::AtomicBool::new(false); |
There was a problem hiding this comment.
suggestion: Consider avoiding a public global mutable state for the status flag
Public mutable pub static state makes the code harder to reason about as usage grows. Consider keeping this AtomicBool private to the module and exposing functions like set_custom_status_received() / custom_status_received() so the synchronization details remain encapsulated and can be changed later without impacting callers.
Suggested implementation:
pub(crate) const TEXT_PADDING: i32 = 6;
const DEFAULT_SEPARATOR_BLOCK_WIDTH: i32 = 9;
use std::sync::atomic::{AtomicBool, Ordering};
static CUSTOM_STATUS_RECEIVED: AtomicBool = AtomicBool::new(false);
pub(crate) fn set_custom_status_received() {
CUSTOM_STATUS_RECEIVED.store(true, Ordering::Relaxed);
}
pub(crate) fn clear_custom_status_received() {
CUSTOM_STATUS_RECEIVED.store(false, Ordering::Relaxed);
}
pub(crate) fn custom_status_received() -> bool {
CUSTOM_STATUS_RECEIVED.load(Ordering::Relaxed)
}
#[derive(Debug, Clone)]
pub(crate) enum StatusItem {- Replace any external uses of
CUSTOM_STATUS_RECEIVED(e.g. reads with.load(...)) with calls tocustom_status_received(). - Replace any external writes to
CUSTOM_STATUS_RECEIVED(e.g..store(true/false, ...)or.swap(...)) with the appropriate helper:set_custom_status_received()orclear_custom_status_received(). If there are more complex update patterns, add additional helper functions in this module instead of exposing the static. - Once all external uses are replaced, ensure
CUSTOM_STATUS_RECEIVEDis not re-exported elsewhere (e.g. viapub use), to keep the synchronization details encapsulated in this module.
Comment on lines
878
to
882
| loop { | ||
| if CUSTOM_STATUS_RECEIVED.load(Ordering::Relaxed) { | ||
| break; | ||
| } | ||
| send_status_ipc(&default_status_text()); |
There was a problem hiding this comment.
suggestion (bug_risk): There may be an extra default status update after a custom status arrives
Because the flag is only checked at the top of the loop, a custom status arriving after the load but before send_status_ipc will still result in one extra default update. If you want to stop sending immediately once the custom status is set, either re-check the flag right before send_status_ipc, or move the send to the end of the loop and guard it with a flag check before each send.
Comment on lines
+587
to
+595
| let padding = if !block.separator && block.separator_block_width == 0 { | ||
| 0 | ||
| } else { | ||
| TEXT_PADDING | ||
| }; | ||
|
|
||
| let content_width = text_width.max(min_width).max(0); | ||
| let border_width = block.border_left + block.border_right; | ||
| let block_width = border_width + TEXT_PADDING * 2 + content_width; | ||
| let block_width = border_width + padding * 2 + content_width; |
There was a problem hiding this comment.
suggestion: The padding calculation is duplicated and could be centralized
The padding condition (if !block.separator && block.separator_block_width == 0 { 0 } else { TEXT_PADDING }) is repeated in measure_i3_block_width and draw_i3_block. Please extract it into a helper (e.g., fn block_padding(block: &I3Block) -> i32) to keep the behavior consistent and avoid future divergence.
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.
Fixes two separate issues regarding status bar rendering:
i3status-rsoutput blocks configured without separators were still having a hardcoded 6-pixel left and right padding added to their internal text width calculation, creating unintended background-colored gaps. Padding is now dynamically omitted ifseparator: falseandseparator_block_width: 0.status_commandwasn't explicitly managed through the WM config (e.g. running via autostart scripts). The thread now permanently halts if a custom non-default status is received via IPC.PR created automatically by Jules for task 889909543099433205 started by @paperbenni
Summary by Sourcery
Fix status bar rendering for i3status-rs blocks and prevent the default status updater from overwriting custom IPC status text.
Bug Fixes:
Summary by CodeRabbit
New Features
Bug Fixes