-
Notifications
You must be signed in to change notification settings - Fork 2k
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
save language prefs #333
Conversation
@Willyfrog great work! I think we might want to synchronize this with #331 though. I believe passing in the language via the query in 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? |
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. |
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 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] |
There was a problem hiding this comment.
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.
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:
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? |
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.
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) |
Merged #331, closing this, and continuing the conversation in #314. Thanks @Willyfrog! |
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