Catch electron-store errors #4552
Conversation
Signed-off-by: Miro Spönemann <[email protected]>
|
Does that mean that in the case where you would get an error, it wouldn't crash but it also wouldn't actually save the geometry? |
|
Correct, but it might save the geometry on the next call of that function. I have not been able to find out what is the root cause of the problem, and I doubt that would be feasible because it occurs only in a very specific environment, not on every Windows system. So you could argue that my change does not actually fix #4551, but at least it makes sure that the error is not propagated to the user and that the error becomes so rare that the geometry is stored "often enough". |
There was a problem hiding this comment.
Thanks for the fix! Generally looks good to me, so approving in the interest of time, but please have a look at my questions and nit-pickings below (feel free to ignore those that are not useful.)
I see that you've added a try { } catch (e) { console.error('...', e) } in the state-saving function. But you've also done some minor refactoring, and added debouncing on 'move' and 'resize' events. It would have been nice to mention the debouncing in the commit message as well.
| const saveWindowStateDelayed = () => { | ||
| if (delayedSaveTimeout) { | ||
| clearTimeout(delayedSaveTimeout); | ||
| } |
There was a problem hiding this comment.
Nit: You don't need to check for if (delayedSaveTimeout), because Passing an invalid ID to clearTimeout() silently does nothing; no exception is thrown.
Or, if you really want to implement an if, please fix the logic by setting delayedSaveTimeout = undefined after clearing the timeout.
There was a problem hiding this comment.
The link you provided points to the web API. Does it also hold for the Node.js API?
| } | ||
| }; | ||
| let delayedSaveTimeout; | ||
| const saveWindowStateDelayed = () => { |
There was a problem hiding this comment.
Nit: I think the correct term for "Delayed" is "Debounced". If you agree, you could replace all occurrences of "Delayed" with "Debounced".
There was a problem hiding this comment.
I think both terms are valid...
| try { | ||
| let bounds; | ||
| if (newWindow.isMaximized()) { | ||
| bounds = electronStore.get(WINDOW_STATE, {}); |
There was a problem hiding this comment.
What is the point of re-doing electronStore.get(WINDOW_STATE, {}) when the window is maximized? Also:
-
Couldn't you just re-use
windowStatefrom above? -
It seems your are calling
electronStore.set()with the same values you got fromelectronStore.get(). Maybe just skip saving window state when it's maximized?
There was a problem hiding this comment.
What is the point of re-doing
electronStore.get(WINDOW_STATE, {})when the window is maximized?
It makes sure that the bounds are kept, so after unmaximizing we get the same bounds we had before maximizing.
Couldn't you just re-use windowState from above?
No, the stored bounds might have been changed in the meantime. We could write the bounds into the local variable on every store operation if we want to avoid the additional get.
It seems your are calling electronStore.set() with the same values you got from electronStore.get().
No, the isMaximized property is changed.
|
sindresorhus/electron-store#31 (comment) Maybe just a case of adding a debouncer? edit: Ah @jankeromnes you beat me to it :) |
Which you did, didn't see the |
Fixes #4551.
electron-storewhile saving the window state.setTimeoutto decrease the number of save operations. This significantly reduces the frequency of thrown errors.