[GH-314] Export native app user settings on change #380

Johennes · 2021-05-03T19:10:36Z

Summary

This switches from exporting the native app user settings on window close to exporting on settings change. This way the settings export remains independent of native application life-cycle events.

This is a stop-gap towards enabling settings export on the native Linux app. The latter does not have an easy way to catch window close events.

As a positive side-effect this also makes the settings export on the native macOS app more reliable and prevents settings loss when the app crashes or is force-killed. Additionally, the required code on the native app side is reduced.

On the negative side, the settings are still exported as blob which now causes more processing since the export happens more often. I already exposed the changed settings key in the script message so it should be relatively easy to switch to storing the settings individually in a follow-up change.

Ticket Link

Relates to #314

This switches from exporting the native app user settings on window close to exporting on settings change. This way the settings export remains independent of native application life-cycle events. This is a stop-gap towards enabling settings export on the native Linux app. The latter does not have an easy way to catch window close events. Relates to: mattermost-community#314

mattermod · 2021-05-03T19:10:38Z

Hello @Johennes,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

Johennes · 2021-05-03T19:11:08Z

mac/Focalboard/ViewController.swift

-		let timeout = DispatchTime.now() + .seconds(3)
-		var result: DispatchTimeoutResult?
-
-		// Busy wait because evaluateJavaScript can only be called from *and* signals on the main thread


Busy wait be gone! 🎉

Johennes · 2021-05-03T19:12:10Z

webapp/src/main.tsx


 import './styles/variables.scss'
 import './styles/main.scss'
 import './styles/labels.scss'

+store.setHandlers({getter: UserSettings.getEmojiMartSetting, setter: UserSettings.setEmojiMartSetting})
+importNativeAppSettings()


I moved this here because the initThemes call below already reads from localStorage.

Johennes · 2021-05-03T19:15:26Z

webapp/src/userSettings.ts


-const keys = ['language', 'theme', 'lastBoardId', 'lastViewId', 'emoji-mart.last', 'emoji-mart.frequently']
+    static getEmojiMartSetting(key: string): any {


This method and the one below are a bit hairy. There's some documentation about the emoji-mart settings keys and values in the upstream README (see the "Storage" section).

Yeah, thanks for parsing the Emojimart settings! We expect to refactor the emoji picker in the future, so this will likely change (not for a while though).

Johennes · 2021-05-03T19:17:58Z

For some reason ESLint keeps rejecting the enum as a shadowing declaration.

 /home/runner/work/focalboard/focalboard/webapp/src/userSettings.ts
  7:6  error  'UserSettingKey' is already declared in the upper scope on line 7 column 6  no-shadow

I suspect this is a false positive as I can't see how this would shadow an existing declaration?

EDIT: I added an inline comment to disable the rule on this line.

Conflicts: webapp/src/userSettings.ts

mattermod · 2021-05-21T01:00:18Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

chenilim · 2021-05-21T16:12:54Z

Thanks so much @Johennes! I'm so sorry for the delay. Totally my fault for missing this on my todo list! Reviewing now.

chenilim

Thanks @Johennes, and sorry again for the delay. This looks great, and the only change I think we should make now is to check the key membership for UserSettings.

Adding @sbishel as an FYI. We need to later expand the UserSettings persistence to Windows and Linux, and can continue to refine it.

chenilim · 2021-05-21T16:14:13Z

webapp/src/nativeApp.ts

+export function notifySettingsChanged(key: string) {
+    postWebKitMessage({type: 'didChangeUserSettings', key, settingsBlob: exportUserSettingsBlob()})
+}
+
 function postWebKitMessage(message: any) {


Love this construct - for extending webapp-to-native messaging in the future. We'll add a mental todo to also adapt this for Windows / Linux in the future.

chenilim · 2021-05-21T16:15:15Z

webapp/src/userSettings.ts


-const keys = ['language', 'theme', 'lastBoardId', 'lastViewId', 'emoji-mart.last', 'emoji-mart.frequently']
+    static getEmojiMartSetting(key: string): any {


Yeah, thanks for parsing the Emojimart settings! We expect to refactor the emoji picker in the future, so this will likely change (not for a while though).

webapp/src/userSettings.ts

Johennes · 2021-05-21T17:22:01Z

Thanks so much @Johennes! I'm so sorry for the delay. Totally my fault for missing this on my todo list! Reviewing now.

Ah, no worries at all @chenilim. I actually forgot that I did this myself until I received the notification about the "stale" label being added last night. 😅

I made the change to check the local storage keys.

chenilim

Thanks for making the changes @Johennes! LGTM!

I thought about it a bit more, and it's probably less about being a security issue (if someone can inject JS, there's not much we can do), but this verification would help catch any code or typo errors in the future.

hanzei · 2021-06-01T09:57:58Z

@chenilim Should this PR be reviewed by someone else?

Johennes · 2021-06-04T17:29:45Z

I rebased this on main and resolved the conflict but I think it needs another look now. When I start and stop the app multiple times in a row, I sometimes see the previous settings not being retained on relaunch. They're imported at first but then something seems to overwrite them again. I will have to investigate that. 😞

…ettings

Johennes · 2021-06-05T18:56:28Z

So this was one of these cases where a simple typo makes you go on a two-hour goose chase and lets you question God and the world until you finally realize that you fat-fingeredly used Object.values(UserSettings) instead of Object.values(UserSettingKey). 🤦

This is fixed now and things appear to work correctly again. I left some of the extended logging that I used to analyze the issue around as well. The interaction between the web view and the app is quite tedious to debug so this might come in handy in future. 🙂

jasonblais · 2021-06-07T12:58:44Z

cc @jespino on above

mattermod · 2021-07-05T01:00:15Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

Conflicts: webapp/src/app.tsx webapp/src/i18n.tsx webapp/src/main.tsx webapp/src/pages/boardPage.tsx

Johennes · 2021-08-12T13:51:15Z

@jespino / @chenilim many apologies for the huge delay. I finally found the time to merge the main branch in again. I briefly re-tested the change again and it's now ready for code review (and hopefully merging). 🙂

jespino · 2021-08-13T12:41:42Z

I'll review it ASAP. Thanks for all the effort and the patience @Johennes

jespino

LGTM

Johennes commented May 3, 2021

View reviewed changes

Johennes mentioned this pull request May 3, 2021

Bug: Can't save language preference #314

Open

Disable no-shadow rule to prevent false-positive

d843b2f

jespino requested a review from chenilim May 10, 2021 15:20

Merge branch 'main' into feature/export-settings-on-change

48f27c4

Conflicts: webapp/src/userSettings.ts

mattermod added the Lifecycle/1:stale label May 21, 2021

chenilim suggested changes May 21, 2021

View reviewed changes

Johennes added 2 commits May 21, 2021 19:00

Merge branch 'main' into feature/export-settings-on-change

80f2d20

Verify allowed localStorage keys

c237179

chenilim approved these changes May 21, 2021

View reviewed changes

Merge branch 'main' into feature/export-settings-on-change

453a66c

hanzei added the 2: Dev Review Requires review by a core committer label Jun 1, 2021

Merge branch 'main' into feature/export-settings-on-change

f9ae2c9

Johennes added 4 commits June 4, 2021 19:30

Fix import order/spacing

894d03f

Treat JSON parsing errors as failed import

5006bd4

Read known keys from the correct type 🤦

485ee55

Extend logging with imported keys and always include _current_ user s…

b9a400c

…ettings

jespino self-requested a review June 24, 2021 22:28

jespino removed the Lifecycle/1:stale label Jun 24, 2021

mattermod added the Lifecycle/1:stale label Jul 5, 2021

Merge branch 'main' into feature/export-settings-on-change

77b0e27

Conflicts: webapp/src/app.tsx webapp/src/i18n.tsx webapp/src/main.tsx webapp/src/pages/boardPage.tsx

Johennes requested a review from a team as a code owner August 12, 2021 13:37

Merge branch 'main' into feature/export-settings-on-change

34f7a8e

Merge branch 'main' into feature/export-settings-on-change

b0b6ec1

hahmadia removed the request for review from a team August 13, 2021 15:11

Merge branch 'main' into feature/export-settings-on-change

4ea405e

jespino removed the Lifecycle/1:stale label Aug 15, 2021

jespino approved these changes Aug 15, 2021

View reviewed changes

Merge branch 'main' into feature/export-settings-on-change

ddae786

jespino added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Aug 15, 2021

Fixing eslint

5c4ac0b

jespino merged commit b6d32da into mattermost-community:main Aug 15, 2021

snyk-bot mentioned this pull request Jan 17, 2022

[Snyk] Fix for 2 vulnerabilities dev-paradigm/render.deploy.focal#9

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GH-314] Export native app user settings on change #380

[GH-314] Export native app user settings on change #380

Johennes commented May 3, 2021

mattermod commented May 3, 2021

Johennes May 3, 2021

Johennes May 3, 2021

Johennes May 3, 2021

chenilim May 21, 2021

Johennes commented May 3, 2021 •

edited

Loading

mattermod commented May 21, 2021

chenilim commented May 21, 2021

chenilim left a comment

chenilim May 21, 2021

chenilim May 21, 2021

Johennes commented May 21, 2021 •

edited

Loading

chenilim left a comment

hanzei commented Jun 1, 2021

Johennes commented Jun 4, 2021

Johennes commented Jun 5, 2021

jasonblais commented Jun 7, 2021

mattermod commented Jul 5, 2021

Johennes commented Aug 12, 2021 •

edited

Loading

jespino commented Aug 13, 2021

jespino left a comment


		const keys = ['language', 'theme', 'lastBoardId', 'lastViewId', 'emoji-mart.last', 'emoji-mart.frequently']
		static getEmojiMartSetting(key: string): any {

[GH-314] Export native app user settings on change #380

[GH-314] Export native app user settings on change #380

Conversation

Johennes commented May 3, 2021

Summary

Ticket Link

mattermod commented May 3, 2021

Johennes May 3, 2021

Choose a reason for hiding this comment

Johennes May 3, 2021

Choose a reason for hiding this comment

Johennes May 3, 2021

Choose a reason for hiding this comment

chenilim May 21, 2021

Choose a reason for hiding this comment

Johennes commented May 3, 2021 • edited Loading

mattermod commented May 21, 2021

chenilim commented May 21, 2021

chenilim left a comment

Choose a reason for hiding this comment

chenilim May 21, 2021

Choose a reason for hiding this comment

chenilim May 21, 2021

Choose a reason for hiding this comment

Johennes commented May 21, 2021 • edited Loading

chenilim left a comment

Choose a reason for hiding this comment

hanzei commented Jun 1, 2021

Johennes commented Jun 4, 2021

Johennes commented Jun 5, 2021

jasonblais commented Jun 7, 2021

mattermod commented Jul 5, 2021

Johennes commented Aug 12, 2021 • edited Loading

jespino commented Aug 13, 2021

jespino left a comment

Choose a reason for hiding this comment

Johennes commented May 3, 2021 •

edited

Loading

Johennes commented May 21, 2021 •

edited

Loading

Johennes commented Aug 12, 2021 •

edited

Loading