[GH-314] Persist and reapply users settings in mac app #331

Johennes · 2021-04-25T18:27:30Z

Summary

Since the native apps choose a random port on each launch, data stored into local storage becomes inaccessible after restarting. This PR ensures persistence of common user settings across restarts of the native mac app.

The program flow is as follows:

Exporting the settings

When closing a window, inject a JS snippet into the web view to read out the current settings
Store the settings into the native user defaults

Importing the settings

When opening a window, read the last stored settings from the native user defaults
Install a user script that re-imports the settings into local storage

A few notes on the implementation:

The initial idea of hooking into applicationWillTerminate turned out to be insufficient because macOS allows closing all windows of an app without terminating it. As a result, when applicationWillTerminate is called, there may be no windows / web views left to perform the export. As a workaround, I settled for doing the export whenever a window is closed.
When the app is quit with more than one window open, both of them will export their respective user settings. There is no race condition because the export runs synchronously on the main queue. However, the window to last export its settings will overwrite the settings of any previous exports. I believe this should be acceptable because different user settings per window should be a rather rare use case.
I ran into issues with escaped quotes and backslashes in the interplay between Swift and JavaScript. To avoid them, I chose to base64-encode the settings on export and base64-decode them on import, both on the JS side. This way the settings loaded from the native user defaults can safely be injected into a Swift string representing the JS snippet without worrying about escaping or special characters.
Since the import function is defined in the Focalboard JS code, it is only accessible once that code has loaded. The user script for importing the settings, therefore, had to be inserted at the document end. That in turn means a page reload is necessary to actually apply the settings. This incurs a small delay when starting the app and sometimes you can see the UI rendering with the old settings for the blink of a second. We could also insert the script at the document start but then the import function needs to be moved to the JS snippet on the native side.
In order to only import the settings once, I chose to insert a timestamp when exporting the user settings.
The WebKit messaging is only needed for logging. Unfortunately, it bloats the code a little so we could also remove it maybe. Here's an example of log messages on start-up / shutdown:

2021-04-25 20:33:14.440396+0200 Focalboard[28071:1457603] Imported user settings: {"language":"de","theme":"{\"mainBg\":\"255, 255, 255\",\"mainFg\":\"55, 53, 47\",\"buttonBg\":\"22, 109, 224\",\"buttonFg\":\"255, 255, 255\",\"sidebarBg\":\"20, 93, 191\",\"sidebarFg\":\"255, 255, 255\",\"sidebarWhiteLogo\":\"true\",\"link\":\"#0000ee\",\"linkVisited\":\"#551a8b\",\"propDefault\":\"#fff\",\"propGray\":\"#EDEDED\",\"propBrown\":\"#F7DDC3\",\"propOrange\":\"#ffd3c1\",\"propYellow\":\"#f7f0b6\",\"propGreen\":\"#c7eac3\",\"propBlue\":\"#B1D1F6\",\"propPurple\":\"#e6d0ff\",\"propPink\":\"#ffd6e9\",\"propRed\":\"#ffa9a9\"}","lastBoardId":"577613a1-6fb5-4f6d-884c-0001bc021ac3","lastViewId":"","emoji-mart.last":"null","emoji-mart.frequently":"null","timestamp":"1619375587862"}

2021-04-25 20:33:15.708352+0200 Focalboard[28071:1457603] Persisted user settings: {"language":"de","theme":"{\"mainBg\":\"255, 255, 255\",\"mainFg\":\"55, 53, 47\",\"buttonBg\":\"22, 109, 224\",\"buttonFg\":\"255, 255, 255\",\"sidebarBg\":\"20, 93, 191\",\"sidebarFg\":\"255, 255, 255\",\"sidebarWhiteLogo\":\"true\",\"link\":\"#0000ee\",\"linkVisited\":\"#551a8b\",\"propDefault\":\"#fff\",\"propGray\":\"#EDEDED\",\"propBrown\":\"#F7DDC3\",\"propOrange\":\"#ffd3c1\",\"propYellow\":\"#f7f0b6\",\"propGreen\":\"#c7eac3\",\"propBlue\":\"#B1D1F6\",\"propPurple\":\"#e6d0ff\",\"propPink\":\"#ffd6e9\",\"propRed\":\"#ffa9a9\"}","lastBoardId":"577613a1-6fb5-4f6d-884c-0001bc021ac3","lastViewId":"","emoji-mart.last":"null","emoji-mart.frequently":"null","timestamp":"1619375595704"}

Ticket Link

#314

mattermod · 2021-04-25T18:27:33Z

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.

Relates to: mattermost#314

Johennes · 2021-04-25T18:36:58Z

mac/Focalboard/AppDelegate.swift

 let windowController = mainStoryBoard.instantiateController(withIdentifier: "WindowController") as! NSWindowController
 windowController.showWindow(self)
- windowController.contentViewController = tabViewController


Setting the content controller isn't necessary here because it is already configured in the storyboard and will be correctly installed when loading WindowController. I had to remove it because setting the new content controller caused the one that was already installed to disappear which then in turn triggered it to export the user settings via the code in viewWillDisappear.

chenilim · 2021-04-25T19:17:04Z

mac/Focalboard/ViewController.swift

+ }
+ }
+ """,
+ injectionTime: .atDocumentEnd,


Thanks @Johennes for this PR! I smoke tested it and it appears to work seamlessly (didn't notice a refresh). Some quick thoughts on how we might avoid this reload:

What if the Swift code injected the base-64 settings string in a placeholder property (e.g. NativeApp.settingsBlob), atDocumentStart.

Then add code to the initialization routine (somewhere in app.tsx maybe, as long as it's before rendering)

It would check to see if Native.settingsBlob is set, if so, it would call importUserSettings, then delete the blob

Have to think a bit about security (I don't think that's an issue), but the extra benefit is the handling of the blob would be all in shared TS code.

What do you think?

This is a great idea! I've been able to drastically reduce the amount of JS code in the native app this way and there's no more page reload necessary. 🎉

Security-wise, I'm not an expert but I believe this at least doesn't make things worse. The injected blob is accessible by any other script running on the page but it's essentially the same data that's already accessible globally via the exportUserSettingsBlob function.

mac/Focalboard/ViewController.swift

webapp/webpack.common.js

…S, use proper quotes

Johennes · 2021-04-26T14:56:53Z

webapp/src/userSettings.ts

-export {UserSettings}
+const keys = ['language', 'theme', 'lastBoardId', 'lastViewId', 'emoji-mart.last', 'emoji-mart.frequently']
+
+export function exportUserSettingsBlob(): string {


Maybe a bit of an esoteric question but now that #310 has landed, this is mixing static class methods and standalone methods. Is there any preference for one or the other?

Willyfrog

some comments on this. but looking good :)

Willyfrog · 2021-04-27T09:40:08Z

mac/Focalboard/ViewController.swift

+ func userContentController(_ userContentController: WKUserContentController, didReceive message: WKScriptMessage) {
+ guard let body = message.body as? [String: String], let type = body["type"], let blob = body["settingsBlob"] else {
+ NSLog("Received unexpected script message \(message.body)")
+ return
+ }
+ NSLog("Received script message \(type): \(Data(base64Encoded: blob).flatMap { String(data: $0, encoding: .utf8) } ?? blob)")
+ }


not really sure what does this function does other than receiving a message and printing it. Am I missign something?

No, that's precisely it. 😅

I had mentioned this above in the PR description:

The WebKit messaging is only needed for logging. Unfortunately, it bloats the code a little so we could also remove it maybe. Here's an example of log messages on start-up / shutdown:

I found this helpful during development but it does add some complexity so we can take it out if there are concerns. Alternatively, we could also wrap it in something like #if DEBUG ... #endif so that we still have it available for future development but it doesn't land in the release build.

Willyfrog · 2021-04-27T09:43:37Z

mac/Focalboard/ViewController.swift

 let appDelegate = NSApplication.shared.delegate as! AppDelegate
- let script = WKUserScript(
+ let sessionTokenScript = WKUserScript(


if we are populating localstorage from the user preferences, should we first clear it so there are no old elements getting in the way (like an upgrade/downgrade?)

Hm, good question. I wonder if that's not a general problem that the web app needs to handle? When you use Focalboard in the browser and switch to a new version, it could happen as well that localStorage contains data from previous versions. If the semantics of the user settings change between versions, some migration logic will be needed but I'm not sure if we need to do anything special for this from the native app side. If we insert the last known settings into localStorage on launch, it's essentially the same problem as on the web version from that point on.

Willyfrog · 2021-04-27T09:57:54Z

webapp/src/userSettings.ts

+ const lastTimestamp = localStorage.getItem('timestamp')
+ if (!timestamp || (lastTimestamp && Number(timestamp) <= Number(lastTimestamp))) {
+ return false
+ }


if you save whenever you change one of the settings and clear the localstorage before loading any data, you shouldn't need to check for timestamps.

Yeah, I think that would work. The need for the timestamp stems from the fact that this PR only saves the user settings on window close and so I wanted to ensure that the settings are not imported more than once per web view. If we later switch to export the settings dynamically as they change, we can drop the time stamp. We just have to remember to also reinstall the user script when the settings change.

[mattermostGH-314] Persist and reapply users settings in mac app

6859cd2

Relates to: mattermost#314

Johennes force-pushed the feature/mac-settings branch from acef353 to 6859cd2 Compare April 25, 2021 18:31

Johennes commented Apr 25, 2021

View reviewed changes

Johennes mentioned this pull request Apr 25, 2021

Bug: Can't save language preference #314

Open

chenilim reviewed Apr 25, 2021

View reviewed changes

mac/Focalboard/ViewController.swift Outdated Show resolved Hide resolved

chenilim reviewed Apr 25, 2021

View reviewed changes

webapp/webpack.common.js Outdated Show resolved Hide resolved

Johennes added 4 commits April 26, 2021 16:41

Inject settings blob at document start, push base64 conversion into T…

d309e9e

…S, use proper quotes

Merge branch 'main' into feature/mac-settings

dcc5bd0

Remove whitespace

f25329d

Rename base64 to blob for consistency

b804f5f

Johennes commented Apr 26, 2021

View reviewed changes

Johennes mentioned this pull request Apr 26, 2021

save language prefs #333

Closed

Willyfrog reviewed Apr 27, 2021

View reviewed changes

chenilim merged commit 3fb078d into mattermost:main Apr 27, 2021

Johennes mentioned this pull request May 3, 2021

Bug: randomIcons user setting not stored in native app #375

Closed

baong12 mentioned this pull request Oct 28, 2023

Update win-wpf app to persist user settings (issue #314) #4915

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GH-314] Persist and reapply users settings in mac app #331

[GH-314] Persist and reapply users settings in mac app #331

Johennes commented Apr 25, 2021 •

edited

mattermod commented Apr 25, 2021

Johennes Apr 25, 2021

chenilim Apr 25, 2021

Johennes Apr 26, 2021

Johennes Apr 26, 2021

Willyfrog left a comment

Willyfrog Apr 27, 2021

Johennes Apr 27, 2021

Willyfrog Apr 27, 2021

Johennes Apr 27, 2021

Willyfrog Apr 27, 2021

Johennes Apr 27, 2021

[GH-314] Persist and reapply users settings in mac app #331

[GH-314] Persist and reapply users settings in mac app #331

Conversation

Johennes commented Apr 25, 2021 • edited

Summary

Ticket Link

mattermod commented Apr 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Willyfrog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Johennes commented Apr 25, 2021 •

edited