various minor adjustments

- add docs according to what I think it does
- make settings access blocking to avoid spurious but avoidable failures.
- rename `SettingsHandle` to `AppSettingsWithDiskSync`, and additional renames to make clear what's happening.
- make interacting with the app-settings more pleasant.

crates/gitbutler-settings/src/api.rs

@@ -1,18 +1,7 @@
-use crate::{AppSettings, SettingsHandle};
+use crate::AppSettingsWithDiskSync;
 use anyhow::Result;
 use serde::{Deserialize, Serialize};
 
-pub fn get_app_settings(handle: &SettingsHandle) -> Result<AppSettings> {
-    let settings = handle.read()?;
-    Ok(settings.clone())
-}
-
-pub fn update_onboarding_complete(handle: &SettingsHandle, update: bool) -> Result<()> {
-    let mut settings = handle.write()?;
-    settings.onboarding_complete = update;
-    settings.save(handle.config_path())
-}
-
 #[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, Eq)]
 #[serde(rename_all = "camelCase")]
 /// Update request for [`crate::app_settings::TelemetrySettings`].
@@ -22,16 +11,25 @@ pub struct TelemetryUpdate {
     pub app_non_anon_metrics_enabled: Option<bool>,
 }
 
-pub fn update_telemetry(handle: &SettingsHandle, update: TelemetryUpdate) -> Result<()> {
-    let mut settings = handle.write()?;
-    if let Some(app_metrics_enabled) = update.app_metrics_enabled {
-        settings.telemetry.app_metrics_enabled = app_metrics_enabled;
-    }
-    if let Some(app_error_reporting_enabled) = update.app_error_reporting_enabled {
-        settings.telemetry.app_error_reporting_enabled = app_error_reporting_enabled;
+/// Mutation, immediately followed by writing everything to disk.
+impl AppSettingsWithDiskSync {
+    pub fn update_onboarding_complete(&self, update: bool) -> Result<()> {
+        let mut settings = self.get_mut_enforce_save()?;
+        settings.onboarding_complete = update;
+        settings.save()
     }
-    if let Some(app_non_anon_metrics_enabled) = update.app_non_anon_metrics_enabled {
-        settings.telemetry.app_non_anon_metrics_enabled = app_non_anon_metrics_enabled;
+
+    pub fn update_telemetry(&self, update: TelemetryUpdate) -> Result<()> {
+        let mut settings = self.get_mut_enforce_save()?;
+        if let Some(app_metrics_enabled) = update.app_metrics_enabled {
+            settings.telemetry.app_metrics_enabled = app_metrics_enabled;
+        }
+        if let Some(app_error_reporting_enabled) = update.app_error_reporting_enabled {
+            settings.telemetry.app_error_reporting_enabled = app_error_reporting_enabled;
+        }
+        if let Some(app_non_anon_metrics_enabled) = update.app_non_anon_metrics_enabled {
+            settings.telemetry.app_non_anon_metrics_enabled = app_non_anon_metrics_enabled;
+        }
+        settings.save()
     }
-    settings.save(handle.config_path())
 }

crates/gitbutler-settings/src/app_settings.rs

@@ -1,16 +1,5 @@
 use serde::{Deserialize, Serialize};
 
-#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)]
-#[serde(rename_all = "camelCase")]
-pub struct AppSettings {
-    /// Whether the user has passed the onboarding flow.
-    pub onboarding_complete: bool,
-    /// Telemetry settings
-    pub telemetry: TelemetrySettings,
-    /// Client ID for the GitHub OAuth application
-    pub github_oauth_app: GitHubOAuthAppSettings,
-}
-
 #[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, Eq)]
 #[serde(rename_all = "camelCase")]
 pub struct TelemetrySettings {

crates/gitbutler-settings/src/lib.rs

@@ -1,11 +1,24 @@
 #![allow(deprecated)]
+use serde::{Deserialize, Serialize};
+
 mod legacy;
 pub use legacy::LegacySettings;
 
-mod app_settings;
+#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)]
+#[serde(rename_all = "camelCase")]
+pub struct AppSettings {
+    /// Whether the user has passed the onboarding flow.
+    pub onboarding_complete: bool,
+    /// Telemetry settings
+    pub telemetry: app_settings::TelemetrySettings,
+    /// Client ID for the GitHub OAuth application.
+    pub github_oauth_app: app_settings::GitHubOAuthAppSettings,
+}
+
+pub mod app_settings;
 mod json;
 mod persistence;
 mod watch;
-pub use app_settings::AppSettings;
-pub use watch::SettingsHandle;
+pub use watch::AppSettingsWithDiskSync;
+
 pub mod api;

crates/gitbutler-settings/src/persistence.rs

@@ -1,34 +1,33 @@
-use std::path::PathBuf;
+use std::path::Path;
 
-use crate::app_settings::AppSettings;
 use crate::json::{json_difference, merge_non_null_json_value};
+use crate::AppSettings;
 use anyhow::Result;
 use serde_json::json;
 
 static DEFAULTS: &str = include_str!("../assets/defaults.jsonc");
 
 impl AppSettings {
-    /// Load the settings from the configuration directory. If a config file name is not provided, the default `gitbutler_settings.json` one is used.
-    pub fn load(config_path: PathBuf) -> Result<Self> {
-        // Load the defaults
-        let mut settings: serde_json::Value = serde_json_lenient::from_str(DEFAULTS)?;
-
+    /// Load the settings from the configuration directory, or initialize the file with an empty JSON object at `config_path`.
+    /// Finally, merge all customizations from `config_path` into the default settings.
+    pub fn load(config_path: &Path) -> Result<Self> {
         // If the file on config_path does not exist, create it empty
         if !config_path.exists() {
-            gitbutler_fs::write(config_path.clone(), "{}\n")?;
+            gitbutler_fs::write(config_path, "{}\n")?;
         }
-        // Load customizations
+
+        // merge customizations from disk into the defaults to get a complete set of settings.
         let customizations = serde_json_lenient::from_str(&std::fs::read_to_string(config_path)?)?;
+        let mut settings: serde_json::Value = serde_json_lenient::from_str(DEFAULTS)?;
 
-        // Merge the customizations into the settings
         merge_non_null_json_value(customizations, &mut settings);
         Ok(serde_json::from_value(settings)?)
     }
 
-    /// Save the updated fields of the AppSettings in the custom configuration file.
-    pub fn save(&self, config_path: PathBuf) -> Result<()> {
+    /// Save all value in this instance to the custom configuration file *if they differ* from the defaults.
+    pub fn save(&self, config_path: &Path) -> Result<()> {
         // Load the current settings
-        let current = serde_json::to_value(AppSettings::load(config_path.clone())?)?;
+        let current = serde_json::to_value(AppSettings::load(config_path)?)?;
 
         // Derive changed values only compared to the current settings
         let update = serde_json::to_value(self)?;
@@ -41,7 +40,7 @@ impl AppSettings {
 
         // Load the existing customizations only
         let mut customizations =
-            serde_json_lenient::from_str(&std::fs::read_to_string(config_path.clone())?)?;
+            serde_json_lenient::from_str(&std::fs::read_to_string(config_path)?)?;
 
         // Merge the new customizations into the existing ones
         // TODO: This will nuke any comments in the file

crates/gitbutler-settings/src/watch.rs

@@ -1,76 +1,138 @@
+use crate::AppSettings;
+use anyhow::Result;
+use notify::{event::ModifyKind, Config, Event, RecommendedWatcher, RecursiveMode, Watcher};
+use std::ops::{Deref, DerefMut};
+use std::path::Path;
 use std::{
     path::PathBuf,
     sync::{mpsc, Arc, RwLock, RwLockReadGuard, RwLockWriteGuard},
     time::Duration,
 };
 
-use crate::{app_settings, AppSettings};
-use anyhow::Result;
-use notify::{event::ModifyKind, Config, Event, RecommendedWatcher, RecursiveMode, Watcher};
-
-pub struct SettingsHandle {
+/// A monitor for [`AppSettings`] on disk which will keep its internal state in sync with
+/// what's on disk.
+///
+/// It will also distribute the latest version of the application settings.
+pub struct AppSettingsWithDiskSync {
     config_path: PathBuf,
-    app_settings: Arc<RwLock<AppSettings>>,
+    /// The source of truth for the application settings, as previously read from disk.
+    snapshot: Arc<RwLock<AppSettings>>,
+    /// A function to receive the most recent app settings as read from disk.
     #[allow(clippy::type_complexity)]
-    send_event: Arc<dyn Fn(AppSettings) -> Result<()> + Send + Sync + 'static>,
+    subscriber: Option<Box<dyn Fn(AppSettings) -> Result<()> + Send + Sync + 'static>>,
+}
+
+/// Allow changes to the most recent [`AppSettings`] and force them to be saved.
+pub(crate) struct AppSettingsEnforceSaveToDisk<'a> {
+    config_path: &'a Path,
+    snapshot: RwLockWriteGuard<'a, AppSettings>,
+    saved: bool,
+}
+
+impl AppSettingsEnforceSaveToDisk<'_> {
+    pub fn save(&mut self) -> Result<()> {
+        // Mark as completed first so failure to save will not make us complain about not saving.
+        self.saved = true;
+        self.snapshot.save(self.config_path)?;
+        Ok(())
+    }
+}
+
+impl Deref for AppSettingsEnforceSaveToDisk<'_> {
+    type Target = AppSettings;
+
+    fn deref(&self) -> &Self::Target {
+        &self.snapshot
+    }
+}
+
+impl DerefMut for AppSettingsEnforceSaveToDisk<'_> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.snapshot
+    }
+}
+
+impl Drop for AppSettingsEnforceSaveToDisk<'_> {
+    fn drop(&mut self) {
+        assert!(
+            self.saved,
+            "BUG: every change must immediately be saved to disk."
+        );
+    }
 }
 
 const SETTINGS_FILE: &str = "settings.json";
 
-impl SettingsHandle {
-    pub fn create(
-        config_dir: impl Into<PathBuf>,
-        send_event: impl Fn(AppSettings) -> Result<()> + Send + Sync + 'static,
+impl AppSettingsWithDiskSync {
+    /// Create a new instance without actually starting to [watch in the background](Self::watch_in_background()).
+    ///
+    /// * `config_dir` contains the application settings file.
+    /// * `subscriber` receives any change to it.
+    pub fn new(
+        config_dir: impl AsRef<Path>,
+        subscriber: impl Fn(AppSettings) -> Result<()> + Send + Sync + 'static,
     ) -> Result<Self> {
-        let config_path = config_dir.into().join(SETTINGS_FILE);
-        let app_settings = app_settings::AppSettings::load(config_path.clone())?;
-
+        let config_path = config_dir.as_ref().join(SETTINGS_FILE);
+        let app_settings = AppSettings::load(&config_path)?;
         let app_settings = Arc::new(RwLock::new(app_settings));
+
         Ok(Self {
             config_path,
-            app_settings,
-            send_event: Arc::new(send_event),
+            snapshot: app_settings,
+            subscriber: Some(Box::new(subscriber)),
         })
     }
 
-    pub fn read(&self) -> Result<RwLockReadGuard<'_, AppSettings>> {
-        self.app_settings
-            .try_read()
+    /// Return a reference to the most recently loaded [`AppSettings`].
+    pub fn get(&self) -> Result<RwLockReadGuard<'_, AppSettings>> {
+        self.snapshot
+            .read()
             .map_err(|e| anyhow::anyhow!("Could not read settings: {:?}", e))
     }
 
-    pub fn write(&self) -> Result<RwLockWriteGuard<'_, AppSettings>> {
-        self.app_settings
-            .try_write()
+    /// Allow changes only from within this crate to implement all possible settings updates [here](crate::api).
+    pub(crate) fn get_mut_enforce_save(&self) -> Result<AppSettingsEnforceSaveToDisk<'_>> {
+        self.snapshot
+            .write()
+            .map(|snapshot| AppSettingsEnforceSaveToDisk {
+                snapshot,
+                config_path: &self.config_path,
+                saved: false,
+            })
             .map_err(|e| anyhow::anyhow!("Could not write settings: {:?}", e))
     }
 
-    pub fn config_path(&self) -> PathBuf {
-        self.config_path.clone()
+    /// The path from which application settings will be read from disk.
+    pub fn config_path(&self) -> &Path {
+        &self.config_path
     }
 
-    pub fn watch_in_background(&self) -> Result<()> {
+    /// Start watching [`Self::config_path()`] for changes and inform
+    pub fn watch_in_background(&mut self) -> Result<()> {
         let (tx, rx) = mpsc::channel();
-        let settings = self.app_settings.clone();
-        let config_path = self.config_path.clone();
-        let send_event = self.send_event.clone();
+        let snapshot = self.snapshot.clone();
+        let config_path = self.config_path.to_owned();
+        let send_event = self
+            .subscriber
+            .take()
+            .expect("BUG: must not call this more than once");
+        let watcher_config = Config::default()
+            .with_compare_contents(true)
+            .with_poll_interval(Duration::from_secs(2));
+        let mut watcher: RecommendedWatcher = Watcher::new(tx, watcher_config)?;
+        watcher.watch(&config_path, RecursiveMode::NonRecursive)?;
 
         tokio::task::spawn_blocking(move || -> Result<()> {
-            let watcher_config = Config::default()
-                .with_compare_contents(true)
-                .with_poll_interval(Duration::from_secs(2));
-            let mut watcher: RecommendedWatcher = Watcher::new(tx, watcher_config)?;
-            watcher.watch(config_path.as_path(), RecursiveMode::NonRecursive)?;
             loop {
                 match rx.recv() {
                     Ok(Ok(Event {
                         kind: notify::event::EventKind::Modify(ModifyKind::Data(_)),
                         ..
                     })) => {
-                        let Ok(mut last_seen_settings) = settings.try_write() else {
+                        let Ok(mut last_seen_settings) = snapshot.write() else {
                             continue;
                         };
-                        if let Ok(update) = app_settings::AppSettings::load(config_path.clone()) {
+                        if let Ok(update) = AppSettings::load(&config_path) {
                             if *last_seen_settings != update {
                                 tracing::info!("settings.json modified; refreshing settings");
                                 *last_seen_settings = update.clone();

crates/gitbutler-settings/tests/mod.rs

@@ -4,7 +4,7 @@ use gitbutler_settings::AppSettings;
 #[allow(clippy::bool_assert_comparison)]
 fn test_load_settings() {
     let settings =
-        AppSettings::load("tests/fixtures/modify_default_true_to_false.json".into()).unwrap();
+        AppSettings::load("tests/fixtures/modify_default_true_to_false.json".as_ref()).unwrap();
     assert_eq!(settings.telemetry.app_metrics_enabled, false); // modified
     assert_eq!(settings.telemetry.app_error_reporting_enabled, true); // default
     assert_eq!(settings.telemetry.app_non_anon_metrics_enabled, false); // default

crates/gitbutler-tauri/src/github.rs

@@ -1,5 +1,5 @@
 pub mod commands {
-    use gitbutler_settings::SettingsHandle;
+    use gitbutler_settings::AppSettingsWithDiskSync;
     use std::collections::HashMap;
     use tauri::State;
 
@@ -18,10 +18,10 @@ pub mod commands {
     #[tauri::command(async)]
     #[instrument(skip(settings), err(Debug))]
     pub async fn init_device_oauth(
-        settings: State<'_, SettingsHandle>,
+        settings: State<'_, AppSettingsWithDiskSync>,
     ) -> Result<Verification, Error> {
         let mut req_body = HashMap::new();
-        let client_id = settings.read()?.github_oauth_app.oauth_client_id.clone();
+        let client_id = settings.get()?.github_oauth_app.oauth_client_id.clone();
         req_body.insert("client_id", client_id.as_str());
         req_body.insert("scope", "repo");
 
@@ -50,7 +50,7 @@ pub mod commands {
     #[tauri::command(async)]
     #[instrument(skip(settings), err(Debug))]
     pub async fn check_auth_status(
-        settings: State<'_, SettingsHandle>,
+        settings: State<'_, AppSettingsWithDiskSync>,
         device_code: &str,
     ) -> Result<String, Error> {
         #[derive(Debug, Deserialize, Serialize, Clone, Default)]
@@ -59,7 +59,7 @@ pub mod commands {
         }
 
         let mut req_body = HashMap::new();
-        let client_id = settings.read()?.github_oauth_app.oauth_client_id.clone();
+        let client_id = settings.get()?.github_oauth_app.oauth_client_id.clone();
         req_body.insert("client_id", client_id.as_str());
         req_body.insert("device_code", device_code);
         req_body.insert("grant_type", "urn:ietf:params:oauth:grant-type:device_code");

crates/gitbutler-tauri/src/main.rs

@@ -11,7 +11,7 @@
     clippy::too_many_lines
 )]
 
-use gitbutler_settings::SettingsHandle;
+use gitbutler_settings::AppSettingsWithDiskSync;
 use gitbutler_tauri::settings::SettingsStore;
 use gitbutler_tauri::{
     askpass, commands, config, forge, github, logs, menu, modes, open, projects, remotes, repo,
@@ -105,7 +105,7 @@ fn main() {
 
                     app_handle.manage(WindowState::new(app_handle.clone()));
 
-                    let app_settings = SettingsHandle::create(config_dir, {
+                    let mut app_settings = AppSettingsWithDiskSync::new(config_dir, {
                         let app_handle = app_handle.clone();
                         move |app_settings| {
                             gitbutler_tauri::ChangeForFrontend::from(app_settings).send(&app_handle)