fix(ui): force full repaint when re-showing settings from tray - #3
Conversation
The settings window backed by the Slint software renderer can paint a stale framebuffer after a long hide-to-tray, leaving partially-rendered regions until the user moves the mouse over the window. request_redraw alone does not suffice — only a property-change on a sentinel marks the window dirty. Bump the redraw_tick sentinel (same mechanism the 2s gui refresh timer uses) immediately after w.show(), and schedule a second bump 50ms later in case the first redraw was processed before winit had finished realizing the window.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a fix for stale framebuffers when restoring a Slint window from the system tray by forcing a repaint and scheduling a delayed secondary redraw. The reviewer identified that the redraw_tick property used for opacity calculations could exceed valid ranges over time, and suggested using a modulo operation to keep the value within a safe range, ensuring long-term stability and consistency across all redraw triggers.
| // can otherwise paint a stale framebuffer until the user | ||
| // moves the mouse over it — only the property change marks | ||
| // the window dirty; request_redraw alone is not enough. | ||
| w.set_redraw_tick(w.get_redraw_tick().wrapping_add(1)); |
There was a problem hiding this comment.
The redraw_tick sentinel is used in ui/main.slint to calculate an opacity value (root.redraw-tick * 0.0001 + 0.0001). Incrementing it indefinitely will eventually cause the opacity to exceed 1.0 (after ~10,000 increments, or roughly 5.5 hours of uptime at the 2s refresh rate). If the Slint property system or renderer clamps this value, the property evaluation result may become constant, preventing change detection from triggering further repaints. Using a modulo keeps the value in a safe range while still ensuring every call marks the property as dirty. Consider applying this same change to the periodic refresh timer at line 251 for consistency.
| w.set_redraw_tick(w.get_redraw_tick().wrapping_add(1)); | |
| w.set_redraw_tick((w.get_redraw_tick() + 1) % 100); |
| std::time::Duration::from_millis(50), | ||
| move || { | ||
| if let Some(w) = weak.upgrade() { | ||
| w.set_redraw_tick(w.get_redraw_tick().wrapping_add(1)); |
There was a problem hiding this comment.
Applying the same modulo logic here for consistency and to ensure the sentinel remains effective over long periods of application uptime.
| w.set_redraw_tick(w.get_redraw_tick().wrapping_add(1)); | |
| w.set_redraw_tick((w.get_redraw_tick() + 1) % 100); |
|
@codex review Generated by Claude Code |
There was a problem hiding this comment.
Code Review
This pull request adds logic to force a full repaint when the application window is shown from the tray, addressing an issue where the software renderer might display a stale framebuffer. This is done by incrementing a 'redraw_tick' property and requesting a redraw immediately and after a 50ms delay. The review feedback suggests using a modulo operation for the tick increment to ensure it remains effective after the value exceeds the threshold where Slint's opacity clamping would prevent further UI updates.
| // can otherwise paint a stale framebuffer until the user | ||
| // moves the mouse over it — only the property change marks | ||
| // the window dirty; request_redraw alone is not enough. | ||
| w.set_redraw_tick(w.get_redraw_tick().wrapping_add(1)); |
There was a problem hiding this comment.
The redraw_tick sentinel is used in ui/main.slint to calculate opacity via root.redraw-tick * 0.0001 + 0.0001. Since Slint clamps opacity to the [0, 1] range, this hack will stop forcing repaints once the tick exceeds 10,000 (as the resulting opacity will remain 1.0). For a background app like Fade, this limit is reached in about 5.5 hours due to the 2-second refresh timer. Using a modulo ensures the property change always results in a unique visual value within the valid range.
| w.set_redraw_tick(w.get_redraw_tick().wrapping_add(1)); | |
| w.set_redraw_tick((w.get_redraw_tick() + 1) % 1000); |
| std::time::Duration::from_millis(50), | ||
| move || { | ||
| if let Some(w) = weak.upgrade() { | ||
| w.set_redraw_tick(w.get_redraw_tick().wrapping_add(1)); |
There was a problem hiding this comment.
As noted above, using a modulo here ensures the redraw_tick remains effective for forcing repaints over long periods of time.
| w.set_redraw_tick(w.get_redraw_tick().wrapping_add(1)); | |
| w.set_redraw_tick((w.get_redraw_tick() + 1) % 1000); |
- rustfmt collapsed the single_shot closure onto one line.
- The redraw_tick property feeds an opacity expression in main.slint
('tick * 0.0001 + 0.0001'). Past ~10k bumps the resulting opacity
saturates at 1.0 and Slint may stop treating it as a real change,
defeating the sentinel. Modulo 1000 keeps the value bounded well
below saturation while every bump still produces a unique input value.
Addresses gemini-code-assist review on PR #3.
|
Addressed the modulo feedback in 013f912. Could you take another look? @codex review Generated by Claude Code |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Code Review
This pull request implements a mechanism to force a full repaint of the Slint window when it is restored from the system tray, addressing an issue where the software renderer might display a stale framebuffer. The implementation involves incrementing a bounded redraw_tick property and requesting a redraw both immediately and after a 50ms delay. Feedback from the review suggests refactoring this logic to avoid code duplication and replacing magic numbers with named constants to improve maintainability.
| w.set_redraw_tick((w.get_redraw_tick() + 1) % 1000); | ||
| w.window().request_redraw(); | ||
| // Schedule a second redraw shortly after the OS has | ||
| // actually realized/sized the window, in case the first | ||
| // one happened before winit finished its show sequence. | ||
| let weak = w.as_weak(); | ||
| slint::Timer::single_shot(std::time::Duration::from_millis(50), move || { | ||
| if let Some(w) = weak.upgrade() { | ||
| w.set_redraw_tick((w.get_redraw_tick() + 1) % 1000); | ||
| w.window().request_redraw(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
There's some repeated logic and magic numbers here. To improve maintainability and readability, you could:
- Extract the repaint logic into a local closure to avoid duplication.
- Define constants for the magic numbers
1000and50.
This repaint logic is also used in the gui_refresh_timer. A single helper function defined at a higher scope would be an even better solution to avoid duplication across the file.
const REDRAW_TICK_MODULO: i32 = 1000;
const REPAINT_DELAY_MS: u64 = 50;
let force_repaint = |w: &SettingsWindow| {
w.set_redraw_tick((w.get_redraw_tick() + 1) % REDRAW_TICK_MODULO);
w.window().request_redraw();
};
force_repaint(&w);
// Schedule a second redraw shortly after the OS has
// actually realized/sized the window, in case the first
// one happened before winit finished its show sequence.
let weak = w.as_weak();
slint::Timer::single_shot(std::time::Duration::from_millis(REPAINT_DELAY_MS), move || {
if let Some(w) = weak.upgrade() {
force_repaint(&w);
}
});Per gemini-code-assist review on PR #3: - promote the redraw_tick bump + request_redraw pair into force_repaint(). - name the magic numbers (REDRAW_TICK_MODULO = 1000, POST_SHOW_REPAINT_DELAY_MS = 50) with one-line rationales for why each value is what it is. No behavior change — three call sites now share the same code path.
|
Refactor applied in 0217070 (force_repaint helper + named constants). One more look? @codex review Generated by Claude Code |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to address stale framebuffer issues in the Slint software renderer by implementing a force_repaint function. This function increments a redraw_tick property and requests a window redraw, ensuring the UI is marked dirty. The logic is applied immediately when showing the window and again after a 50ms delay. Feedback was provided to use wrapping_add when incrementing the tick to prevent potential integer overflow panics in debug builds.
| /// property change is what marks the window dirty for the Slint software | ||
| /// renderer — request_redraw on its own is not enough after a long hide. | ||
| fn force_repaint(w: &SettingsWindow) { | ||
| w.set_redraw_tick((w.get_redraw_tick() + 1) % REDRAW_TICK_MODULO); |
There was a problem hiding this comment.
To prevent a potential panic in debug builds due to integer overflow if get_redraw_tick() were to reach i32::MAX, it's safer to use wrapping_add. This makes the wrapping behavior explicit and consistent with release builds, where integer overflow wraps by default.
| w.set_redraw_tick((w.get_redraw_tick() + 1) % REDRAW_TICK_MODULO); | |
| w.set_redraw_tick(w.get_redraw_tick().wrapping_add(1) % REDRAW_TICK_MODULO); |
The modulo invariant already keeps the tick in [0, 999], so + 1 cannot overflow. wrapping_add costs nothing and makes the helper safe even if someone later sets redraw_tick out of band. Addresses gemini-code-assist review on PR #3.
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
request_redraw()alone wasn't enough; only a property-change on theredraw_ticksentinel marks the window dirty for that renderer. Bump it immediately afterw.show(), then bump it again 50ms later in case winit hadn't finished realizing the window on the first attempt.Test plan
Note: I couldn't reproduce live (Linux dev environment, no Windows GUI). If artifacts still appear after this lands, the next tries are a longer follow-up delay (250ms) or a brief hide/show kick.
Generated by Claude Code