Desktop: Fixes #14861: Restore window position and size after relaunch on Windows#15003
Desktop: Fixes #14861: Restore window position and size after relaunch on Windows#15003gagansokhal-coder wants to merge 3 commits intolaurent22:devfrom
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
📝 WalkthroughWalkthroughInserted inline comments in Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
| this.win_.on('close', () => { | ||
| // Cancel any pending debounced save and do a final immediate save | ||
| if (snapSaveTimer) { | ||
| clearTimeout(snapSaveTimer); | ||
| snapSaveTimer = null; | ||
| } | ||
| try { | ||
| if (this.win_ && !this.win_.isDestroyed()) { | ||
| windowState.saveState(this.win_); | ||
| } | ||
| } catch (_e) { | ||
| // Window may have been destroyed | ||
| } | ||
| }); |
There was a problem hiding this comment.
This ensures that the final window position and size are saved when the app is closed.
Without this, when the window is snapped (e.g using Win + Arrow keys) the last updated bounds may not be persisted correctly because the debounced save might not execute before the app closes.
By handling the 'close' event, we cancel any pending debounced save and immediately persist the latest window state, ensuring the correct snapped position is restored on next launch.
There was a problem hiding this comment.
Without this, when the window is snapped (e.g using Win + Arrow keys) the last updated bounds may not be persisted correctly because the debounced save might not execute before the app closes.
"may not be" - under what condition that can happen?
There was a problem hiding this comment.
This can happen when the window state is updated shortly before the app is closed, while the debounced save is still pending.
For example, when the user snaps the window (Win + Arrow keys), a resize/move event triggers the debounced save. If the user immediately closes the app before the debounce delay completes, the pending save may not run, so the latest bounds are not persisted.
In that case, the previously saved window state (before snapping) would be restored on the next launch instead of the snapped position.
There was a problem hiding this comment.
You mean if the user resize then close the window within 1 second? is it worth the extra code?
There was a problem hiding this comment.
You're right that this scenario happens only when the window is resized or snapped and then closed very quickly.
I added this to ensure consistency, especially for snap behavior, since users might expect the last visible state to always be restored.
However, I agree that if this is considered too edge-case or adds unnecessary complexity, we can simplify the approach or rely on the existing debounced save.
Happy to adjust or remove this part based on your preference.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app-desktop/ElectronAppWrapper.ts`:
- Around line 607-612: The indentation in the conditional block inside the
ElectronAppWrapper class uses spaces instead of tabs; update the whitespace in
the block containing the lines "if (syncPending)", "dispatch({ type:
'QUIT_SYNC_DIALOG_OPEN' });" and "this.quit();" to use tabs for indentation to
match project TypeScript coding guidelines and ensure consistent formatting
across the method.
- Around line 552-559: The comment inside the ElectronAppWrapper class
referencing a "longer debounced save" for Windows Snap is stale because the
supplemental implementation was removed; either remove the comment entirely or
update it to state the current limitation (that electron-window-state's 100ms
debounce can miss snapped positions and no extra debounced save is implemented).
Locate the Windows Snap comment block in ElectronAppWrapper (near where
electron-window-state and the window close/closed handling are discussed), and
change the wording to accurately reflect the absence of a supplemental save, or
restore the previously-removed longer-debounced save logic and wire it into the
window close/closed handlers so snapped bounds are persisted. Ensure the updated
comment references electron-window-state and the window close/closed behavior so
future readers know the constraint or the fix applied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a6619ae1-f763-4098-ba79-280e7dcfb30c
📒 Files selected for processing (1)
packages/app-desktop/ElectronAppWrapper.ts
| // Windows Snap fix: electron-window-state uses a 100ms debounce for | ||
| // resize/move events, which can miss the final bounds after a Windows | ||
| // Snap gesture. Additionally, Joplin's close handler calls | ||
| // event.preventDefault() (for the appClose round-trip or tray hide), | ||
| // which prevents the library's 'closed' handler from firing — so the | ||
| // state may never be written to disk. We supplement with a longer | ||
| // debounced save that ensures the snapped position is always persisted. | ||
|
|
There was a problem hiding this comment.
Comment references a non-existent implementation.
The comment states "We supplement with a longer debounced save that ensures the snapped position is always persisted," but no code implementing this supplement is present. Based on the past review discussion, the code that handled this was removed. The comment should either be updated to describe the actual current state (i.e., the limitation exists without a fix) or removed entirely if it no longer applies.
Suggested fix
If the issue remains unaddressed, consider revising the comment to describe the limitation without claiming a fix:
-// Windows Snap fix: electron-window-state uses a 100ms debounce for
-// resize/move events, which can miss the final bounds after a Windows
-// Snap gesture. Additionally, Joplin's close handler calls
-// event.preventDefault() (for the appClose round-trip or tray hide),
-// which prevents the library's 'closed' handler from firing — so the
-// state may never be written to disk. We supplement with a longer
-// debounced save that ensures the snapped position is always persisted.
+// Windows Snap limitation: electron-window-state uses a 100ms debounce for
+// resize/move events, which can miss the final bounds after a Windows
+// Snap gesture if the user closes the app within the debounce window.
+// Additionally, Joplin's close handler calls event.preventDefault() (for
+// the appClose round-trip or tray hide), which prevents the library's
+// 'closed' handler from firing in some cases.Alternatively, if this comment serves no purpose without the fix, consider removing it.
📝 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.
| // Windows Snap fix: electron-window-state uses a 100ms debounce for | |
| // resize/move events, which can miss the final bounds after a Windows | |
| // Snap gesture. Additionally, Joplin's close handler calls | |
| // event.preventDefault() (for the appClose round-trip or tray hide), | |
| // which prevents the library's 'closed' handler from firing — so the | |
| // state may never be written to disk. We supplement with a longer | |
| // debounced save that ensures the snapped position is always persisted. | |
| // Windows Snap limitation: electron-window-state uses a 100ms debounce for | |
| // resize/move events, which can miss the final bounds after a Windows | |
| // Snap gesture if the user closes the app within the debounce window. | |
| // Additionally, Joplin's close handler calls event.preventDefault() (for | |
| // the appClose round-trip or tray hide), which prevents the library's | |
| // 'closed' handler from firing in some cases. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/app-desktop/ElectronAppWrapper.ts` around lines 552 - 559, The
comment inside the ElectronAppWrapper class referencing a "longer debounced
save" for Windows Snap is stale because the supplemental implementation was
removed; either remove the comment entirely or update it to state the current
limitation (that electron-window-state's 100ms debounce can miss snapped
positions and no extra debounced save is implemented). Locate the Windows Snap
comment block in ElectronAppWrapper (near where electron-window-state and the
window close/closed handling are discussed), and change the wording to
accurately reflect the absence of a supplemental save, or restore the
previously-removed longer-debounced save logic and wire it into the window
close/closed handlers so snapped bounds are persisted. Ensure the updated
comment references electron-window-state and the window close/closed behavior so
future readers know the constraint or the fix applied.
| if (syncPending) { | ||
| dispatch({ type: 'QUIT_SYNC_DIALOG_OPEN' }); | ||
| } else { | ||
| this.quit(); | ||
| } | ||
| dispatch({ type: 'QUIT_SYNC_DIALOG_OPEN' }); | ||
| } else { | ||
| this.quit(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Use tabs for indentation.
The indentation in this method uses spaces instead of tabs, violating the coding guidelines. As per coding guidelines, tabs should be used for indentation in TypeScript files.
🔧 Proposed fix to use tabs
public quitWithSyncCheck(
dispatch: (action: { type: string; [key: string]: unknown })=> void,
syncPending: boolean,
) {
if (syncPending) {
- dispatch({ type: 'QUIT_SYNC_DIALOG_OPEN' });
- } else {
- this.quit();
- }
+ dispatch({ type: 'QUIT_SYNC_DIALOG_OPEN' });
+ } else {
+ this.quit();
+ }
}📝 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.
| if (syncPending) { | |
| dispatch({ type: 'QUIT_SYNC_DIALOG_OPEN' }); | |
| } else { | |
| this.quit(); | |
| } | |
| dispatch({ type: 'QUIT_SYNC_DIALOG_OPEN' }); | |
| } else { | |
| this.quit(); | |
| } | |
| } | |
| public quitWithSyncCheck( | |
| dispatch: (action: { type: string; [key: string]: unknown })=> void, | |
| syncPending: boolean, | |
| ) { | |
| if (syncPending) { | |
| dispatch({ type: 'QUIT_SYNC_DIALOG_OPEN' }); | |
| } else { | |
| this.quit(); | |
| } | |
| } |
🧰 Tools
🪛 ESLint
[error] 608-608: Mixed spaces and tabs.
(no-mixed-spaces-and-tabs)
[error] 610-610: Mixed spaces and tabs.
(no-mixed-spaces-and-tabs)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/app-desktop/ElectronAppWrapper.ts` around lines 607 - 612, The
indentation in the conditional block inside the ElectronAppWrapper class uses
spaces instead of tabs; update the whitespace in the block containing the lines
"if (syncPending)", "dispatch({ type: 'QUIT_SYNC_DIALOG_OPEN' });" and
"this.quit();" to use tabs for indentation to match project TypeScript coding
guidelines and ensure consistent formatting across the method.
|
Thanks for the submission. While this PR targets an existing issue, the proposed solution is not suitable for the project in its current form. Bringing it up to an acceptable standard would require significant redesign and guidance. For future contributions, please ensure changes are well-aligned with the existing architecture and sufficiently tested. Please make sure you read our guidelines: https://github.com/joplin/gsoc And our AI policy: https://github.com/joplin/gsoc/blob/master/ai_policy.md |
|
Thank you for the feedback. I understand that the approach doesn't align with the current architecture. I’ll review the guidelines and codebase more carefully before attempting another fix. |
2 participants
Fixes #14861
Problem
On Windows, when the Joplin window is snapped (docked) to the left or right, closing and reopening the app resets the window position and size.
Solution
Ensured that the correct window bounds are saved after snap by handling resize and move events and persisting final bounds.
Testing
Tested the following scenarios:
All cases correctly restore position and size after relaunch.
Notes
This change focuses on Windows behavior and does not affect macOS/Linux.