feat: remember app window size and position on restart - #584
Conversation
Add window state persistence (width, height, x, y, maximized) to the config so the app restores its last-known size and position on next launch. The window position and size are captured on CloseRequested and written through save_config. Closes TabularisDB#534
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The previous SUGGESTION (Wayland detection test assumed Files Reviewed (1 file)
Previous Review Summaries (2 snapshots, latest commit 2c38c54)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 2c38c54)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous review (commit e65192a)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Reviewed by glm-5.2 · Input: 22.9K · Output: 2.1K · Cached: 143.1K |
Co-authored-by: kilo-code-bot[bot] <240665456+kilo-code-bot[bot]@users.noreply.github.com>
Co-authored-by: kilo-code-bot[bot] <240665456+kilo-code-bot[bot]@users.noreply.github.com>
debba
left a comment
There was a problem hiding this comment.
Hey @fuleinist, thanks for the PR! I tested this locally on my machine (Arch, GNOME on Wayland, 2560x1440) and found a couple of issues, one of which is pretty subtle.
The window grows on every restart under Wayland. I planted 900x600 in the config and instrumented the app to report its real geometry. The restore itself actually works (the webview content ends up exactly 900x600), but inner_size(), which is what gets saved on close, reports 952x699 because on Wayland GTK includes the client side decoration margins in that value. So every close saves content + ~52x99 px, and every launch applies that as the new content size. The window inflates by roughly 50x100 px per session, silently, no warning in the logs. Running the same build with GDK_BACKEND=x11 round trips exactly (900x600 in, 900x600 out), so this is Wayland only. It's the same tao/GTK quirk that's been reported upstream against tauri-plugin-window-state.
Closing while maximized poisons the restore size. This one is cross platform. On close the handler saves inner_size() unconditionally, so if you close a maximized window it stores the full screen geometry as the "normal" size. After restart, unmaximizing gives you a window the size of the whole screen instead of the previous size, which is exactly what test plan item 3 says should work. The usual fix is to only update width/height/x/y when the window is not maximized, and always update just the maximized flag.
Also worth noting: on Wayland the position gets saved as 0,0 (GTK can't query it there, and set_position is a no-op since the compositor decides placement). Not something this PR can fix, but if the same config is later loaded in an X11 session the window jumps to the top left corner. A sanity clamp against the current monitor would also help for the unplugged monitor case on X11/Windows.
On the plus side: the new config tests all pass (20/20 in config::tests), save on close fires reliably, the merge logic in save_config correctly keeps frontend saves from wiping the window fields, and first launch with no saved state falls back to the default size as expected.
So, requesting changes for now. I think the minimum here is: persist geometry only when not maximized, and on Wayland skip position persistence and either compensate for the decoration delta or skip size persistence too (checking WAYLAND_DISPLAY is enough to detect it). Happy to re-test once that's in, I have the setup ready.
…ndow_state - Only persist window size/position when not maximized (closing maximized window no longer poisons restore dimensions with full-screen geometry) - Skip position and size on Wayland (GTK includes CSD margins in inner_size, causing window to inflate ~50×100px per session; compositor controls position) - Add is_wayland() helper using WAYLAND_DISPLAY env var - Add test for is_wayland() behavior
|
Fixes applied:
Built successfully (0 errors, 1 warning - pre-existing unused import). Test added for . Ready for re-test on your Wayland setup. |
| #[test] | ||
| fn is_wayland_returns_false_without_wayland_display() { | ||
| // On most test runners WAYLAND_DISPLAY is not set, so this should be false. | ||
| assert!(!is_wayland()); |
There was a problem hiding this comment.
SUGGESTION: Test assumes WAYLAND_DISPLAY is unset rather than controlling it
is_wayland() reads the process environment, but this test never removes WAYLAND_DISPLAY — it only assumes it isn't set. On a Wayland Linux dev machine (or any CI runner where WAYLAND_DISPLAY is present) this assertion fails, despite the test name promising "without wayland_display". Consider removing the var for the test's duration (or refactoring is_wayland to take an injectable env source) so the test is isolated from the host session rather than dependent on where it runs.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Addressed in ffc52f6 — refactored is_wayland() to delegate to is_wayland_with_env(), which takes an injectable env lookup. The test now exercises both the present and absent cases via closures, so it is deterministic regardless of whether WAYLAND_DISPLAY is set on the host running it.
…ookup The is_wayland test asserted on the live process environment, assuming WAYLAND_DISPLAY was unset. On a Wayland dev machine (or a CI runner with WAYLAND_DISPLAY set) it would fail despite its name promising otherwise. Refactor per review feedback: is_wayland() now delegates to is_wayland_with_env() which takes an injectable env lookup. The test exercises both the present and absent cases deterministically, with no dependency on the host session. Addresses review comment on config.rs (2026-08-12).
|
@debba All review feedback is now addressed:
Happy to have this re-tested with your setup when convenient. |
Summary
Test Plan
Closes #534