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

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

Merged
merged 5 commits into from
Apr 27, 2021

Conversation

Johennes
Copy link
Contributor

@Johennes Johennes commented Apr 25, 2021

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
Copy link
Contributor

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.

let windowController = mainStoryBoard.instantiateController(withIdentifier: "WindowController") as! NSWindowController
windowController.showWindow(self)
windowController.contentViewController = tabViewController
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
}
""",
injectionTime: .atDocumentEnd,
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

webapp/webpack.common.js Outdated Show resolved Hide resolved
export {UserSettings}
const keys = ['language', 'theme', 'lastBoardId', 'lastViewId', 'emoji-mart.last', 'emoji-mart.frequently']

export function exportUserSettingsBlob(): string {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

@Johennes Johennes mentioned this pull request Apr 26, 2021
Copy link
Contributor

@Willyfrog Willyfrog left a comment

Choose a reason for hiding this comment

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

some comments on this. but looking good :)

Comment on lines +265 to +271
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)")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

let appDelegate = NSApplication.shared.delegate as! AppDelegate
let script = WKUserScript(
let sessionTokenScript = WKUserScript(
Copy link
Contributor

Choose a reason for hiding this comment

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

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment on lines +33 to +36
const lastTimestamp = localStorage.getItem('timestamp')
if (!timestamp || (lastTimestamp && Number(timestamp) <= Number(lastTimestamp))) {
return false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants