feat(settings): add the ability to read and update settings without tauri store plugin #5774
Conversation
…auri store plugin
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
in theory this should be backwards compatible with the existing setup, and we could just switch on the javascript end |
|
@Byron so i am toying with an idea here of replacing the tauri-store plugin. Can it be as simple as this? |
crates/gitbutler-settings/src/lib.rs
Outdated
| use serde::{Deserialize, Serialize}; | ||
| use std::{fs, path::PathBuf}; | ||
|
|
||
| const SETTINGS_FILE: &str = "projects.json"; |
| @@ -9,3 +15,42 @@ pub struct AppSettings { | |||
| /// Client ID for the GitHub OAuth application | |||
| pub github_oauth_client_id: Option<String>, | |||
| } | |||
|
|
|||
| #[derive(Debug, Deserialize, Serialize, Clone, Default, PartialEq, Hash)] | |||
| pub struct AppSettingsUpdateRequest { | |||
There was a problem hiding this comment.
Is it possible to apply #[serde(default, skip_serializing_if = "Option::is_none")] to the struct itself? *edit: so we don't end up with null for anything not set.
There was a problem hiding this comment.
I am not sure I am following here
There was a problem hiding this comment.
I have never seen it on the struct itself, but if there was a field: AppSettingsUpdateRequest it would certainly be possible to implement something like it.
Right now I see it used only on the top-level of tauri function calls.
Probably I am missing something though.
There was a problem hiding this comment.
I don't understand the issue tbh - having the update request contain Option fields allows us to do partial updating without having to pass the entire settings struct over, which I think is desirable. @mtsgrd can you clarify what you meant?
There was a problem hiding this comment.
What I mean is, if you have a struct with a bunch of optional values then on serializing that it will write out all keys, and null values for them. Instead, it would be great if it omits unset fields.
There was a problem hiding this comment.
Lol, sorry, I could be more clear from start. I just don't want the settings.json file to contain every possible key unless set. Wdyt?
There was a problem hiding this comment.
Oh yeah, that makes total sense. We anyways need to make sure that the settings is backwards / forwards compatible.
But - I sense the reason you are saying this is for user edit-ability. But this setup is inherently not appropriate for that. I wanna make this friendly for a user to modify and screw up, but not sure yet how to build it in a nice way. This was actually the root discussion point i had here #5774 (comment)
There was a problem hiding this comment.
Editability yes, but also readability and to make settings easier to share. Most of the time I edit vscode settings in the UI, but every once in a while I copy paste something in that I found online or got in a chat.
While we're on this topic. Could there be any settings we want to share at the repo level? Similar to how .vscode is used.
There was a problem hiding this comment.
I guess what I am saying is - I would like to make a separation between the settings and the file that a user reads and updates. Have them be two separate things, such that it's both safer to update and as you said easier to read.
I am not sure yet what is the best way to implement that, but this PR was not trying to implement this, but instead just move us away from that tauri store plugin.
Regarding project settings - yes it will be great to move away from projects.json but let's nail app settings first, then it will be easy to extend from there
There was a problem hiding this comment.
Just to clarify and summarize - the settings.json in it's current form is not appropriate for user editing.
The reason for that is not just the readability and serializing the None values, but also the fact that it is fragile - if this file doesn't parse, the app will not start.
I would love to move to a settings.json VSCode style, and not serializing unset values is necessary but not sufficient to getting there. I was initially thinking of this PR as simple change to get off the tauri store plugin, but since you fixed the state issue in the frontend, perhaps the scope of this can be expanded to land us in the endgame setup
krlvi
force-pushed
the
add-serde-support-for-settings
branch
from
8999bfe to
46a461f
Compare
krlvi
force-pushed
the
add-serde-support-for-settings
branch
from
46a461f to
377e8f0
Compare
There was a problem hiding this comment.
@Byron so i am toying with an idea here of replacing the tauri-store plugin. Can it be as simple as this?
I vaguely remember the tauri settings plugin implementation and can say that it also unsafely wrote the file contents (i..e without temporary file).
But its main selling point to me was that it would let subscribers be informed when updates happened automatically. Lastly, it could also debounce and throttle these updates.
Without subscription, UIs will be forced to poll to assure updates are picked up, and I think once there are multiple windows, keeping settings in sync gets much harder.
Personally, I still find that using a settings file for storage is a bit error prone, but with built-in rate limiting it probably is OK to write consistently even on Windows. If there was partial-write protection I think I'd be happy with the tauri plugin as something that can scale.
| @@ -9,3 +15,42 @@ pub struct AppSettings { | |||
| /// Client ID for the GitHub OAuth application | |||
| pub github_oauth_client_id: Option<String>, | |||
| } | |||
|
|
|||
| #[derive(Debug, Deserialize, Serialize, Clone, Default, PartialEq, Hash)] | |||
| pub struct AppSettingsUpdateRequest { | |||
There was a problem hiding this comment.
I have never seen it on the struct itself, but if there was a field: AppSettingsUpdateRequest it would certainly be possible to implement something like it.
Right now I see it used only on the top-level of tauri function calls.
Probably I am missing something though.
Actually yes, needed for multi-window setup. |
|
It's worth noting, the one feature we need that isn't provided by the plugin is an ability to store nested json. While e.g. vscode supports both nested objects, as well as a flatter dot.notation, forcing a flat key/value structure is too restrictive. |
|
The The thinking here is using a good old boring RPC in the fronted - read the config when a settings page is loaded and persist the config when a "save" button is pressed. On the storage end and also the long term idea for this - I would love to have a settings.json file similar to apps like Zed or VSCode that are user-friendly and editable. Right now, if the user messes up the name of a key or something, the config will not load at all. What would be a good way of achieving a nice user-editable setup? In my head I was thinking of having some sort of schema with defaults, and then, when parsing the |
Imagine you turn off telemetry in window A, you would reasonably expect all windows to immediately stop sharing such data right? It would be great if we had our own custom config, and some minimal thing we can subscribe to so the front end knows when to reload. |
Yeah we can totally add an event "settings" updated that all windows would get. I guess reliability is my primary motivation in wanting to keep stuff simple as much as possible, but this seems needed |
|
What really helped me in this conversation is the goal of eventually having a user-editable file of sorts, similar to Zed or VSCode. Last time I checked the plugin I also noticed that it was able to force pending writes during shutdown as well, so overall the lifecycle seemed well thought out. Personally, I also wouldn't be surprised if ultimately we would end up with a 'lite' implementation of the plugin, with all the tweaks and customisations that were needed to get there. |
I started implementing something new so I will probably abandon this pr |
|
closing in favor of #5815 |
No description provided.