Repository navigation
fix(desktop): restore edge-snapped window bounds - #57088
LeON-Nie-code wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
|
The following comment was made by an LLM, it may be inaccurate: |
There was a problem hiding this comment.
The edge tolerance does what it says. I checked it with the real readWindowState → resolveWindowState path in both checkouts: a snapped window saved at x: -7 falls back to the default 1280×800 on dev and keeps its saved bounds with this change. I couldn't test on Windows itself, and you note you couldn't reproduce #56970 there either, so it's still open whether this fixes that report. Tests in src/main/windows and the desktop typecheck pass. Building the app shows the schema adds no real size to the first-window startup chunk.
Public API: unchanged. The window-state file format stays the same; only reading it is stricter.
One concern, inline: the schema makes the whole saved file all-or-nothing. The tolerance alone fixes the reported path, so the schema part could be dropped (or made lenient) to keep this change small and the current fallback behaviour intact.
Related: #56970
| height: Schema.Int, | ||
| }) | ||
|
|
||
| const SavedWindowState = Schema.Struct({ |
There was a problem hiding this comment.
This changes the fallback from per-field to all-or-nothing. Before, a file with a maximized/fullscreen flag but unusable bounds (width: 0, fractional or null x/y) still restored the window maximized. Now any invalid field makes readWindowState return undefined, so the flag is lost too. I checked {isMaximized:true,width:0,height:0}, {x:10.5,…,isMaximized:true} and {x:null,y:null,…,isMaximized:true}: all keep isMaximized on dev and drop it here. The removed {width:0,height:-1} / "junk" assertions show the same shift. The edge tolerance alone fixes the snapped case. I'd drop the schema change from this PR, or decode the bounds and the flags separately, so bad bounds still fall back the way hasBounds did.
There was a problem hiding this comment.
Updated in 1456e7d. readWindowState now falls back to decoding just isMaximized/isFullScreen when the full state is invalid, so unusable bounds no longer drop the flags. Added cases for width: 0, fractional x, and null positions; the window still restores maximized/fullscreen and falls back to the default size. The suite is now 15 pass in src/main/windows.
There was a problem hiding this comment.
#60482 landed the schema/fallback work upstream, so this PR now drops that part and only keeps the 16px tolerance for normal snapped windows.
1456e7d to
844d23a
Compare
|
Hey, checking in on this one. The fallback now keeps the maximized/fullscreen flags even when the bounds are invalid; if you'd rather I drop the schema and keep the diff smaller, I can do that. Anything else you need? |
|
Related case: #58247 saves valid 1284×783 bounds at (2, 34) on a 1388×781 display. They extend 36 px below the display, so the 16 px tolerance here still takes the off-display fallback and loses I have a separate two-line fix preserving both mode flags in that fallback. Five new regression cases fail on dev; all 17 window tests pass over 10 randomized repetitions on Windows and native Linux with Bun 1.4.4. This includes real-file persistence across three launches, using a window stub rather than an Electron GUI. To avoid duplicating your schema work: could a maintainer confirm whether that minimal fix should follow this PR? The whole changed-file lint requirement also flags the existing parser diagnostics that your PR addresses. I will preserve your ownership of those changes. AI-assisted investigation; reviewed and tested locally. |
844d23a to
5073edb
Compare
There was a problem hiding this comment.
The earlier point is fixed: when the bounds are unusable, readWindowState now falls back to decoding only the maximized/fullscreen flags. I re-ran the three cases from before (width: 0, fractional x, null x/y). Each restores as maximized at the default size, the same as dev. The edge-snapped x: -7 window still keeps its saved bounds. Tests in src/main/windows pass, and so does tsc -b in packages/desktop. I couldn't test on Windows itself.
Public API: unchanged.
No new concerns.
5073edb to
9ed0e49
Compare
|
Upstream #60482 now covers the schema and maximized/fullscreen fallback parts this branch had, so I narrowed this PR to the remaining 16px snap tolerance and rebased it on latest dev. The window-state tests (9 pass), desktop typecheck, and lint are clean. |
9ed0e49 to
63d76da
Compare
63d76da to
3c879c1
Compare
Issue for this PR
Related to #56970
Type of change
What does this PR do?
resolveWindowStatedrops saved bounds unless the whole window fits inside a display. Windows can report a snapped window a few pixels outside the display because of its invisible resize borders, and the old check discards the saved position even though the window is still visible.This adds a 16px tolerance to that bounds check. A window saved at
x: -7keeps its saved bounds; one that is genuinely far off-screen still falls back to the default size.Upstream #60482 now covers the maximized/fullscreen and display-fallback paths, so this PR is narrowed to the remaining normal-window edge case and leaves the new
readWindowState/resolveWindowStatebehavior alone.I couldn't reproduce the reporter's exact case on Windows 11 with 2.0.32: snapping, free-moving, and maximizing all restored correctly, and the saved bounds were normalized (
x=1/x=307), not negative. So I'm not sure this is the full fix for #56970 — it's the defensive fix for the negative-bounds path. Happy to test on Windows 10 if someone has it.How did you verify your code works?
bun test src/main/windows/window-state.test.tsinpackages/desktop: 9 passbun run typecheck:tscinpackages/desktop: passbun run lint:changed: 0 problemsScreenshots / recordings
Not a UI change.
Checklist