From b5eda10951f846c702347d109c079b3e2a553790 Mon Sep 17 00:00:00 2001 From: Kevin Reid Date: Thu, 9 Jan 2025 08:35:33 -0800 Subject: [PATCH] ui: Add inheritance between `Settings`es. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will help us not persist changes that shouldn’t be, and later I intend to add per-setting inheritance that will actually be useful to the user. --- all-is-cubes-ui/src/apps/session.rs | 6 +- all-is-cubes-ui/src/apps/settings.rs | 140 ++++++++++++++++++++++++--- 2 files changed, 132 insertions(+), 14 deletions(-) diff --git a/all-is-cubes-ui/src/apps/session.rs b/all-is-cubes-ui/src/apps/session.rs index 587fd6825..036b21d68 100644 --- a/all-is-cubes-ui/src/apps/session.rs +++ b/all-is-cubes-ui/src/apps/session.rs @@ -882,10 +882,12 @@ impl SessionBuilder { /// Enable reading and writing user settings. /// + /// The session will get a new [`Settings`] object which inherits the given settings. + /// /// If this is not called, then the session will have all default settings, /// and they will not be persisted. - pub fn settings(mut self, settings: Settings) -> Self { - self.settings = Some(settings); + pub fn settings_from(mut self, settings: Settings) -> Self { + self.settings = Some(Settings::inherit(settings)); self } diff --git a/all-is-cubes-ui/src/apps/settings.rs b/all-is-cubes-ui/src/apps/settings.rs index f48b63ebb..b4d458d63 100644 --- a/all-is-cubes-ui/src/apps/settings.rs +++ b/all-is-cubes-ui/src/apps/settings.rs @@ -1,7 +1,7 @@ use alloc::sync::Arc; use core::fmt; -use all_is_cubes::listen; +use all_is_cubes::listen::{self, Source as _}; use all_is_cubes_render::camera::GraphicsOptions; /// Currently, the settings data is *only* graphics options. @@ -16,8 +16,7 @@ type Data = Arc; /// /// This is separate from [`Session`](super::Session) so that it can be shared among /// multiple sessions without conflict. -/// All such sessions will edit the same settings. -// TODO: Add settings inheritance for session-specific settings. +/// All such sessions can edit the same settings. /// /// Having `&` access to a [`Settings`] grants permission to read the settings, follow /// changes to the settings, and write the settings. @@ -27,14 +26,23 @@ type Data = Arc; pub struct Settings(Arc); struct Inner { - data: listen::Cell, + /// If None, then use the hardcoded default. + inherit: Option, + + /// If `None`, then inherited or default. + /// TODO: should have individual settings be individual cells with individual inheritance. + state: listen::Cell>, + + /// Source which returns the [`Data`] that is stored *or* inherited. + final_data_source: listen::DynSource, + persister: Arc, } impl Settings { /// Creates a new [`Settings`] with the given initial state, and no persistence. pub fn new(initial_state: Data) -> Self { - Self::with_persistence(initial_state, Arc::new(|_| {})) + Self::new_general(None, Some(initial_state), Arc::new(|_| {})) } /// Creates a new [`Settings`] with the given initial state, @@ -46,8 +54,41 @@ impl Settings { initial_state: Data, persister: Arc, ) -> Self { + Self::new_general(None, Some(initial_state), persister) + } + + /// Creates a new [`Settings`] which reads and writes the given [`Settings`], + /// until such time as it is explicitly detached. + pub fn inherit(other: Settings) -> Self { + Self::new_general(Some(other), None, Arc::new(|_| {})) + } + + fn new_general( + inherit: Option, + initial_state: Option, + persister: Arc, + ) -> Self { + let state = listen::Cell::new(initial_state); Self(Arc::new(Inner { - data: listen::Cell::new(initial_state), + final_data_source: Arc::new( + state + .as_source() + .map({ + let inherit = inherit.clone(); + move |data: Option| -> listen::DynSource { + match (data, &inherit) { + (Some(data), _) => listen::constant(data), + (None, Some(settings)) => settings.as_source(), + (None, None) => { + listen::constant(Arc::new(GraphicsOptions::default())) + } + } + } + }) + .flatten(), + ), + inherit, + state, persister, })) } @@ -55,12 +96,12 @@ impl Settings { /// Returns a [`listen::Source`] of the settings. /// This may be used to follow changes to the settings. pub fn as_source(&self) -> listen::DynSource { - self.0.data.as_source() + self.0.final_data_source.clone() } /// Returns the current graphics options. pub fn get_graphics_options(&self) -> Arc { - self.0.data.get() + self.as_source().get() } /// Overwrites the graphics options. @@ -75,21 +116,43 @@ impl Settings { // TODO: Fix that (will require support from ListenableCell...compare-and-swap?) #[doc(hidden)] pub fn mutate_graphics_options(&self, f: impl FnOnce(&mut GraphicsOptions)) { - let mut options: Arc = self.0.data.get(); + let mut options: Arc = self.get_graphics_options(); f(Arc::make_mut(&mut options)); self.set_state(options); } + /// If this `Settings` was constructed to share another’s state using + /// [`inherit()`](Self::inherit), make it stop. + /// This means future changes will not be persisted. + pub fn disinherit(&self) { + // TODO: this should be an atomic transaction but is not + self.0 + .state + .set(Some(self.0.state.get().unwrap_or_default())); + } + fn set_state(&self, state: Data) { - (self.0.persister)(&state); - self.0.data.set(state); + // TODO: kludge: this condition is only vaguely reasonable because state never transitions + // from Some to None. In reality, we should be using our own mutex instead of depending on + // listen::Cell. + if let Some(parent) = self + .0 + .inherit + .as_ref() + .filter(|_| self.0.state.get().is_none()) + { + parent.set_state(state); + } else { + (self.0.persister)(&state); + self.0.state.set(Some(state)); + } } } impl fmt::Debug for Settings { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("Settings") - .field("data", &self.0.data) + .field("state", &self.0.state) // can't print persister .finish_non_exhaustive() } @@ -100,3 +163,56 @@ impl Default for Settings { Self::new(Default::default()) } } + +#[cfg(test)] +mod tests { + use super::*; + use all_is_cubes::math::ps64; + + #[test] + fn disinherit() { + let parent = Settings::new(Arc::new({ + let mut g = GraphicsOptions::default(); + g.fov_y = ps64(20.0); + g + })); + let child = Settings::inherit(parent.clone()); + + // Inherited values before child + assert_eq!( + ( + parent.get_graphics_options().fov_y, + child.get_graphics_options().fov_y + ), + (ps64(20.0), ps64(20.0)) + ); + assert_eq!( + child.get_graphics_options().show_ui, + true, + "original value for show_ui" + ); + + // Writing upward and reading downward + child.mutate_graphics_options(|g| g.show_ui = false); + assert_eq!( + ( + parent.get_graphics_options().show_ui, + child.get_graphics_options().show_ui + ), + (false, false), + "parent and child mutated" + ); + + // Then it doesn't any more + child.disinherit(); + child.mutate_graphics_options(|g| g.fov_y = ps64(10.0)); + assert_eq!( + ( + parent.get_graphics_options().fov_y, + child.get_graphics_options().fov_y + ), + (ps64(20.0), ps64(10.0)), + "after disinherit" + ); + } +}