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

save language prefs #333

Closed
wants to merge 1 commit into from

Conversation

Willyfrog
Copy link
Contributor

@Willyfrog Willyfrog commented Apr 25, 2021

Summary

add a mechanism to pass info between macOS and webview
use that for storing the language in the system

Ticket Link

this partially addresses #314 but it needs a windows and linux version of it as well

@Willyfrog Willyfrog added the 2: Dev Review Requires review by a core committer label Apr 25, 2021
@Johennes
Copy link
Contributor

@Willyfrog great work! I think we might want to synchronize this with #331 though.

I believe passing in the language via the query in loadHomepage should not be necessary anymore with #331 because the local storage is synced into the web view on launch and that (among others) restores the previously selected language.

What's nice though is that you store the language into the native user defaults right when it changes instead of on window close. Maybe instead of just storing the language we could perform a full export of the user settings, send the base64 blob to native as a script message and then reuse the code from #331 that persists the blob into the user defaults?

@Willyfrog
Copy link
Contributor Author

oh, definitely we can do a sync between PRs, I was mostly exploring how to do the conversation between both and I'm totally open to sync both PRs for a better outcome.

Regarding storing a full blob, I'd advise against that and rather have each element stored separatedly so the app can choose to modify elements of it individually as well as having an easier upgrade/downgrade of desktop versions. It might also allow for having settings specific for the desktop separated from the rest.

regarding the query, I felt it was not ideal, but since I only wanted to setup the language on initialization that seemed the best way to differentiate. But synching the localstorage is good too.

@Johennes
Copy link
Contributor

Yeah, sorry, I didn't realize you were working in the same area when I opened my PR. 🙈

Storing the individual settings instead of a combined blob is a good point actually. One use case for that which pops directly into my head is forwarding the system language setting into the web view on first launch after installing. Say you set up macOS to use Spanish, the native code could prefill "language": "es" into the user settings on first launch and inject them into the web view.

I think it wouldn't be too complicated to adapt the code in #331 to export / import the settings by localStorage key. We could also do that in a follow-up PR to be able to release user settings persistence earlier. However, the downside of that is that we'd need to add code to migrate the blob into the per-key settings for users who have used the blob-persisting release. That shouldn't be too hard but adds additional complexity so maybe better to do the per-key persistence from the start?

Looping in @chenilim who helped brainstorm and discuss the approach in #331 previously.

let lang = initialLang
if (!lang) {
lang = localStorage.getItem('language')
}
if (!lang) {
lang = navigator.language.split(/[-_]/)[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if navigator.language is not one of the supported languages? Does it just default to English? I think it does, but should smoke test this.

@chenilim
Copy link
Contributor

Thanks @Willyfrog! I may have missed it, but didn't realize you were looking at this too.

I re-read both changes, and I think the combined principles are:

  1. The native apps should do as little as necessary (i.e. be "dumb" pipes controlled by the shared TS code)
    a. This will make cross-platform work much easier (think mobile in the future)
    b. Also, let's try to keep the Native-TS interface as simple as possible (can refactor later though)
  2. I agree we could improve on storing the opaque blob, but I'm not sure the native apps should be manipulating it directly (per principle 1)
    a. Maybe just store a "schema version" alongside it?
  3. There's additional logic for specific settings (e.g. using the OS language as a default)
    a. I see you grab that from navagator.language, so that's in TS and cross-platform
    b. If there's additional logic needed in the native app that can't be done in TS, that could be another interface (e.g. NativeApp.defaultLanguage)

Anything else worth calling out?

If not, I propose we merge #331 first, then refactor based on that. For this PR, the main addition would be the navigator.language default, but we can continue to refactor this. Sounds good?

@Willyfrog
Copy link
Contributor Author

Willyfrog commented Apr 27, 2021

It was actually my bad, I wasn't really looking into solving the problem but rather have some place to start looking at the project and ended up coding the PR 😅. Also this is currently just fixing the problem on macos, no windows/linux solution in this case.

regarding 1. I'd recommend having some sort of API defined in how the webaapp should communicate with the host (be it a desktop app or a mobile one), it might not be needed right now, but having that defined will probably ease future development as there is a stablished way to do it.

  1. sure, you can version it, but you'll need migrations for upgrades/downgrades. in this case is mostly about where stuff should be handled and by whom, so 👍 .

  2. the navigator.language was already there, I just kept using it to avoid unexpected changes in behaviour.

And yes, we can close this one and merge #331, from there we can try and save on change of the preferences instead of only when closing (if there is a crash, you loose your settings)

@chenilim
Copy link
Contributor

Merged #331, closing this, and continuing the conversation in #314. Thanks @Willyfrog!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core committer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants