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

Restart the app when language changed #1168

Closed
emlys opened this issue Feb 6, 2023 · 3 comments · Fixed by #1330
Closed

Restart the app when language changed #1168

emlys opened this issue Feb 6, 2023 · 3 comments · Fixed by #1330
Assignees
Labels
enhancement New feature or request in progress This issue is actively being worked on workbench For issues relating to the workbench front-end of invest
Milestone

Comments

@emlys
Copy link
Member

emlys commented Feb 6, 2023

Tell users they must restart the app (or automate restarting it) when the language setting is changed.

  • This would eliminate some complexity where the renderer process has to tell the main process to change its language. Instead, the language setting would be constant throughout the life of each process.
  • It would fix an (unimportant) loophole where changing the language setting does not affect the "About"/"Report a problem" windows if they are already open. Because they are parallel renderer processes, it would be complicated to communicate the change of language and reload those windows.

The downside is that changing the language setting is a larger interruption to using the app, but

  • we already reload the renderer process, which closes all your tabs
  • requiring restart for settings to take effect is a common pattern
  • we don't expect users to change this setting very often

current process

(on startup)

  1. main process starts
  2. renderer process starts
  3. renderer process reads language setting from storage
  4. renderer sets its i18n instance to that language
  5. via IPC, renderer tells main to change its language
  6. main sets its i18n instance to that language

(on change of the language setting)

  1. renderer process updates the language setting in storage
  2. renderer process restarts (window.reload)
  3. repeat steps 3 - 6 above

alternative

(on startup)

  1. main process starts
  2. main process reads language setting from storage
  3. main process sets its i18n instance to that language
  4. renderer process starts
  5. renderer process sets its i18n instance to that language (could be read from a variable exposed in preload)

(on change of the language setting)

  1. renderer process (tells main process to) updates the language setting in storage
  2. user quits and restarts the app
  3. repeat steps 1 - 5 above
@emlys emlys added the enhancement New feature or request label Feb 6, 2023
@davemfish
Copy link
Contributor

I think this alternative plan is a really good one. It requires figuring out a good way to manage persistent data available to the main process. Right now we have that sort of data (i.e. "invest settings") stored in the browser using localForage. But for this language setting, and probably others too, it might make a lot more sense to manage them all via main and just make ipc calls from the renderer when the user changes them.

This looks like a simple solution: https://github.com/sindresorhus/electron-store

@davemfish davemfish added the workbench For issues relating to the workbench front-end of invest label Feb 23, 2023
@emlys
Copy link
Member Author

emlys commented Feb 23, 2023

As part of this work, also should revisit the mysterious warning that appears in the renderer tests (but not when running the app for real):
react-i18next:: You will need to pass in an i18next instance by using initReactI18next

@davemfish
Copy link
Contributor

It looks like this is a common pattern and we can use app.relaunch ourselves to take the burden off the user. https://www.electronjs.org/docs/latest/api/app#apprelaunchoptions

With some extra consideration we can probably also allow the python process to persist, saving the bulk of the shutdown & startup time costs.

@emlys emlys self-assigned this Jun 14, 2023
@emlys emlys added the in progress This issue is actively being worked on label Jun 14, 2023
emlys added a commit to emlys/invest that referenced this issue Jun 14, 2023
@davemfish davemfish added this to the 3.13.1 milestone Jun 22, 2023
emlys added a commit to emlys/invest that referenced this issue Jul 20, 2023
emlys added a commit to emlys/invest that referenced this issue Jul 20, 2023
emlys added a commit to emlys/invest that referenced this issue Jul 20, 2023
emlys added a commit to emlys/invest that referenced this issue Jul 25, 2023
emlys added a commit to emlys/invest that referenced this issue Jul 25, 2023
emlys added a commit to emlys/invest that referenced this issue Jul 25, 2023
emlys added a commit to emlys/invest that referenced this issue Jul 26, 2023
emlys added a commit to emlys/invest that referenced this issue Jul 26, 2023
emlys added a commit to emlys/invest that referenced this issue Jul 26, 2023
emlys added a commit to emlys/invest that referenced this issue Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in progress This issue is actively being worked on workbench For issues relating to the workbench front-end of invest
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants