Save and restore main Talk window position and size - #1857
Conversation
80cd542 to
50b54aa
Compare
Persist the normal BrowserWindow bounds in app config and restore them on reopen when still visible on a connected display. Fixes nextcloud#15 Signed-off-by: xhon-pelushi <xhon@pelushi.com>
50b54aa to
23bb6bc
Compare
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
ShGKme
left a comment
ShGKme
left a comment
There was a problem hiding this comment.
Hi, thank you for your contribution. This works and something we definitely want to have. But the current implementation misses some state properties and may miss the correct display on multiple displays setup. Could you adjust the solution?
| const { BUILD_CONFIG } = require('../shared/build.config.ts') | ||
| const { getBrowserWindowIcon } = require('../shared/icons.utils.js') | ||
|
|
||
| const SAVE_WINDOW_BOUNDS_DELAY = 500 |
There was a problem hiding this comment.
Thanks for adding the debounce!
Let's make it even slower to reduce the number of disk writes.
For example, 15_000 or even more.
They delay here is the time between the last window move and the app being closed. A short delay only helps when a user moves the window, then within half a second closes the app, and reopens it, expecting the position from the right before quit.
| /** | ||
| * Get saved Talk window bounds if they are still usable. | ||
| * | ||
| * @param {{ minWidth: number, minHeight: number }} minimumSize - Minimum Talk window size | ||
| * @return {import('electron').Rectangle | undefined} Restorable bounds, if available | ||
| */ | ||
| function getSavedTalkWindowBounds(minimumSize) { | ||
| const bounds = getAppConfig('talkWindowBounds') | ||
| if (!isValidWindowBounds(bounds)) { | ||
| return undefined | ||
| } | ||
|
|
||
| const savedBounds = { | ||
| ...bounds, | ||
| width: Math.max(bounds.width, minimumSize.minWidth), | ||
| height: Math.max(bounds.height, minimumSize.minHeight), | ||
| } | ||
|
|
||
| return isVisibleOnAnyDisplay(savedBounds) ? savedBounds : undefined | ||
| } |
There was a problem hiding this comment.
It searches for the first window it can fit, and if nothing is found, it fallbacks to the default size.
This means, it may always choose the wrong display just because the wrong one was first and it fits.
Let's adjust the algorithm here a bit:
- Check the exact display that was used the last time (it should be saved as well)
- If there is no saved display (or it is not connected), use the primary display
- If it doesn't fit anymore (for example, the user changed the resolution or scaling) - try to fit instead of choosing another display. For example, if the window was 2560 px wide and now the display is just 1920px, instead of using another display, we can decrease the width to 1920px.
If isMaximized or isFullscreen was set, the size doesn't matter.
| type WindowBounds = { | ||
| x: number | ||
| y: number | ||
| width: number | ||
| height: number | ||
| } |
There was a problem hiding this comment.
Let's add even more properties here to restore the state.
isMaximized- to restore the window maximized if it was maximizedisFullScreen- for macOS as another "maximized" statedisplayId- to save the exact display the Talk window was open on
|
I have just found out, that 10 days ago Electron released v44 with
https://releases.electronjs.org/release/v44.0.0 So, Electron has it as a built-in feature now. We don't use Electron 44 due to a bug on Linux. But it seems it was also fixed 2 weeks ago in 43.5.1 and the fix is included into Electron 44. So, we can upgrade now, and implement it via the built-in feature. |
|
We have upgraded Electron. Now built-in |
3 participants
Summary
Fixes #15
Test plan