Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: update settings in all open windows #1376

Merged
merged 5 commits into from
Jun 30, 2023

Conversation

erikian
Copy link
Member

@erikian erikian commented Jun 21, 2023

Here's my stab at #1352. I mostly followed the approach suggested in that issue, except that I'm listening for updates in the settings using the storage event instead of using IPC, so we can get the same result for free without touching the main process.

Just a consideration: any open windows (except the one in which the setting was changed) updates its AppState object in response to a setting being changed. However, this PR doesn't (directly) change anything related to the UI state. As you can see in the video below, if I have two windows with the Settings modal opened and I make changes in one of them, checkboxes are checked/unchecked in both windows, while text inputs don't change in the other window (unless I close and reopen the Settings modal manually). This is purely a side effect of how the respective components handle their state; if "simultaneous edition" is desired, it will require some changes (maybe just triggering a re-render of the Settings modal on focus, or even changing how the components respond to updates in the global state) that might or might not be part of the scope here, so I decided to check before going any further.

2023-06-21_00-06-40.mp4

Fixes #1352.

@erikian erikian requested review from a team and codebytere as code owners June 21, 2023 03:53
@dsanders11 dsanders11 self-requested a review June 21, 2023 10:55
@dsanders11
Copy link
Member

Will give this a more thorough review later.

I mostly followed the approach suggested in that issue, except that I'm listening for updates in the settings using the storage event instead of using IPC, so we can get the same result for free without touching the main process.

Nice! Wasn't aware of that event. That makes sense and simplifies things.

However, this PR doesn't (directly) change anything related to the UI state. As you can see in the video below, if I have two windows with the Settings modal opened and I make changes in one of them, checkboxes are checked/unchecked in both windows, while text inputs don't change in the other window (unless I close and reopen the Settings modal manually). This is purely a side effect of how the respective components handle their state; if "simultaneous edition" is desired, it will require some changes (maybe just triggering a re-render of the Settings modal on focus, or even changing how the components respond to updates in the global state) that might or might not be part of the scope here, so I decided to check before going any further.

This seems fine for now. 👍

Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to play with this some more when I get the time next week, but dropped some initial review comments.

src/renderer/state.ts Outdated Show resolved Hide resolved
src/renderer/state.ts Show resolved Hide resolved
src/renderer/state.ts Outdated Show resolved Hide resolved
@dsanders11
Copy link
Member

Nice work on this @erikian, and thanks for grabbing the issue!

@dsanders11 dsanders11 merged commit 4eee716 into electron:main Jun 30, 2023
@erikian erikian deleted the fix/sync-localstorage-settings branch July 2, 2023 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Settings Changes Not Reflected in Other Windows
2 participants