From 685b345600f322d6dadc4086f95916d8cd435d5f Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Wed, 2 Sep 2020 15:19:14 -0400 Subject: [PATCH] Remove fancy lifetime stuff from Env Previously, we were doing a bunch of gymnastics in order to allow theEnv to return either borrowed or owned data, depending on the type; the borrowed case was only used for Strings. This code was brittle and hard to understand, and ultimately I think it was solving the wrong problem: instead of trying to avoid cloning expensive data when we get things out of the map, I would prefer to just not have expensive-to-clone things in there in the first place. This introduces the ArcStr type alias for Arc, which is my preferred type for simple strings in druid. Realistically we should have a conversation soon about string types, since we're almost certainly going to be bringing in the xi Rope structure to represent editable text, and we may just want to use that for everything; in that case it will be easy enough to just replace ArcStr with Rope next week. This opens up a bunch of possible future work. The problem I'm most focused on right now is how to synthesize values out of the env; this is sort of like 'env lensing', and it wasn't possible because of the lifetimes. I also think that we should consider removing the `Data` impl from String, but I want to wait until we have a clearer sense of what our text types end up being. --- CHANGELOG.md | 2 + druid/examples/styled_text.rs | 8 +-- druid/src/data.rs | 7 +++ druid/src/env.rs | 104 ++++++++++++---------------------- druid/src/lib.rs | 2 +- druid/src/theme.rs | 4 +- druid/src/widget/label.rs | 10 ++-- druid/src/widget/switch.rs | 2 +- druid/src/widget/textbox.rs | 2 +- 9 files changed, 58 insertions(+), 83 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 343f2bd229..c00a250559 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ You can find its changes [documented below](#060---2020-06-01). - Use new Piet text api ([#1143] by [@cmyr]) - `Env::try_get` (and related methods) return a `Result` instead of an `Option`. ([#1172] by [@cmyr]) - `lens!` macro to use move semantics for the index. ([#1171] by [@finnerale]) +- `Env` stores `Arc` instead of `String` ([#1173] by [@cmyr]) ### Deprecated @@ -408,6 +409,7 @@ Last release without a changelog :( [#1157]: https://github.com/linebender/druid/pull/1157 [#1171]: https://github.com/linebender/druid/pull/1171 [#1172]: https://github.com/linebender/druid/pull/1172 +[#1173]: https://github.com/linebender/druid/pull/1173 [Unreleased]: https://github.com/linebender/druid/compare/v0.6.0...master [0.6.0]: https://github.com/linebender/druid/compare/v0.5.0...v0.6.0 diff --git a/druid/examples/styled_text.rs b/druid/examples/styled_text.rs index 32151c2abf..73b5103973 100644 --- a/druid/examples/styled_text.rs +++ b/druid/examples/styled_text.rs @@ -16,14 +16,14 @@ use druid::widget::{Checkbox, Flex, Label, MainAxisAlignment, Painter, Parse, Stepper, TextBox}; use druid::{ - theme, AppLauncher, Color, Data, Key, Lens, LensExt, LensWrap, LocalizedString, PlatformError, - RenderContext, Widget, WidgetExt, WindowDesc, + theme, AppLauncher, ArcStr, Color, Data, Key, Lens, LensExt, LensWrap, LocalizedString, + PlatformError, RenderContext, Widget, WidgetExt, WindowDesc, }; use std::fmt::Display; // This is a custom key we'll use with Env to set and get our text size. const MY_CUSTOM_TEXT_SIZE: Key = Key::new("styled_text.custom_text_size"); -const MY_CUSTOM_FONT: Key<&str> = Key::new("styled_text.custom_font"); +const MY_CUSTOM_FONT: Key = Key::new("styled_text.custom_font"); #[derive(Clone, Lens, Data)] struct AppData { @@ -99,7 +99,7 @@ fn ui_builder() -> impl Widget { if data.mono { env.set(MY_CUSTOM_FONT, "monospace"); } else { - env.set(MY_CUSTOM_FONT, env.get(theme::FONT_NAME).to_string()); + env.set(MY_CUSTOM_FONT, env.get(theme::FONT_NAME)); } }); diff --git a/druid/src/data.rs b/druid/src/data.rs index 14eef4ae23..ca4643d09b 100644 --- a/druid/src/data.rs +++ b/druid/src/data.rs @@ -119,6 +119,12 @@ pub trait Data: Clone + 'static { //// ANCHOR_END: same_fn } +/// A reference counted string slice. +/// +/// This is a data-friendly way to represent strings in druid. Unlike `String` +/// it cannot be mutated, but unlike `String` it can be cheaply cloned. +pub type ArcStr = Arc; + /// An impl of `Data` suitable for simple types. /// /// The `same` method is implemented with equality, so the type should @@ -145,6 +151,7 @@ impl_data_simple!(u64); impl_data_simple!(usize); impl_data_simple!(char); impl_data_simple!(bool); +//TODO: remove me!? impl_data_simple!(String); impl Data for f32 { diff --git a/druid/src/env.rs b/druid/src/env.rs index e3d13693b9..fa07b95d78 100644 --- a/druid/src/env.rs +++ b/druid/src/env.rs @@ -23,7 +23,7 @@ use std::ops::Deref; use std::sync::Arc; use crate::localization::L10nManager; -use crate::{Color, Data, Point, Rect, Size}; +use crate::{ArcStr, Color, Data, Point, Rect, Size}; /// An environment passed down through all widget traversals. /// @@ -52,7 +52,7 @@ pub struct Env(Arc); #[derive(Clone)] struct EnvImpl { - map: HashMap, + map: HashMap, debug_colors: Vec, l10n: Arc, } @@ -107,7 +107,7 @@ pub enum Value { Float(f64), Bool(bool), UnsignedInt(u64), - String(String), + String(ArcStr), } // ANCHOR_END: value_type @@ -132,17 +132,9 @@ pub enum KeyOrValue { } /// Values which can be stored in an environment. -/// -/// Note that for "expensive" types this is the reference. For example, -/// for strings, this trait is implemented on `&'a str`. The trait is -/// parametrized on a lifetime so that it can be used for references in -/// this way. -pub trait ValueType<'a>: Sized { - /// The corresponding owned type. - type Owned: Into; - +pub trait ValueType: Sized + Into { /// Attempt to convert the generic `Value` into this type. - fn try_from_value(v: &'a Value) -> Result; + fn try_from_value(v: &Value) -> Result; } /// The error type for environment access. @@ -214,7 +206,7 @@ impl Env { /// # Panics /// /// Panics if the key is not found, or if it is present with the wrong type. - pub fn get<'a, V: ValueType<'a>>(&'a self, key: impl Borrow>) -> V { + pub fn get(&self, key: impl Borrow>) -> V { match self.try_get(key) { Ok(value) => value, Err(err) => panic!("{}", err), @@ -228,10 +220,7 @@ impl Env { /// # Panics /// /// Panics if the value for the key is found, but has the wrong type. - pub fn try_get<'a, V: ValueType<'a>>( - &'a self, - key: impl Borrow>, - ) -> Result { + pub fn try_get(&self, key: impl Borrow>) -> Result { self.0 .map .get(key.borrow().key) @@ -277,12 +266,12 @@ impl Env { /// /// *WARNING:* This is not intended for general use, but only for inspecting an `Env` e.g. /// for debugging, theme editing, and theme loading. - pub fn get_all(&self) -> impl ExactSizeIterator { + pub fn get_all(&self) -> impl ExactSizeIterator { self.0.map.iter() } /// Adds a key/value, acting like a builder. - pub fn adding<'a, V: ValueType<'a>>(mut self, key: Key, value: impl Into) -> Env { + pub fn adding(mut self, key: Key, value: impl Into) -> Env { let env = Arc::make_mut(&mut self.0); env.map.insert(key.into(), value.into().into()); self @@ -294,7 +283,7 @@ impl Env { /// /// Panics if the environment already has a value for the key, but it is /// of a different type. - pub fn set<'a, V: ValueType<'a>>(&'a mut self, key: Key, value: impl Into) { + pub fn set(&mut self, key: Key, value: impl Into) { let env = Arc::make_mut(&mut self.0); let value = value.into().into(); let key = key.into(); @@ -369,7 +358,7 @@ impl Value { /// # Panics /// /// Panics when the value variant doesn't match the provided type. - pub fn to_inner_unchecked<'a, V: ValueType<'a>>(&'a self) -> V { + pub fn to_inner_unchecked(&self) -> V { match ValueType::try_from_value(self) { Ok(v) => v, Err(s) => panic!("{}", s), @@ -420,7 +409,7 @@ impl Data for Value { (Float(f1), Float(f2)) => f1.same(&f2), (Bool(b1), Bool(b2)) => b1 == b2, (UnsignedInt(f1), UnsignedInt(f2)) => f1.same(&f2), - (String(s1), String(s2)) => s1 == s2, + (String(s1), String(s2)) => s1.same(s2), _ => false, } } @@ -482,9 +471,9 @@ impl Default for Env { } } -impl From> for String { - fn from(src: Key) -> String { - String::from(src.key) +impl From> for ArcStr { + fn from(src: Key) -> ArcStr { + ArcStr::from(src.key) } } @@ -520,10 +509,9 @@ impl std::error::Error for ValueTypeError {} impl std::error::Error for MissingKeyError {} /// Use this macro for types which are cheap to clone (ie all `Copy` types). -macro_rules! impl_value_type_owned { +macro_rules! impl_value_type { ($ty:ty, $var:ident) => { - impl<'a> ValueType<'a> for $ty { - type Owned = $ty; + impl ValueType for $ty { fn try_from_value(value: &Value) -> Result { match value { Value::$var(f) => Ok(f.to_owned()), @@ -540,43 +528,21 @@ macro_rules! impl_value_type_owned { }; } -/// Use this macro for types which require allocation but are not too -/// expensive to clone. -macro_rules! impl_value_type_borrowed { - ($ty:ty, $owned:ty, $var:ident) => { - impl<'a> ValueType<'a> for &'a $ty { - type Owned = $owned; - fn try_from_value(value: &'a Value) -> Result { - match value { - Value::$var(f) => Ok(f), - other => Err(ValueTypeError::new(any::type_name::<$ty>(), other.clone())), - } - } - } - - impl Into for $owned { - fn into(self) -> Value { - Value::$var(self) - } - } - }; -} - -impl_value_type_owned!(f64, Float); -impl_value_type_owned!(bool, Bool); -impl_value_type_owned!(u64, UnsignedInt); -impl_value_type_owned!(Color, Color); -impl_value_type_owned!(Rect, Rect); -impl_value_type_owned!(Point, Point); -impl_value_type_owned!(Size, Size); -impl_value_type_borrowed!(str, String, String); +impl_value_type!(f64, Float); +impl_value_type!(bool, Bool); +impl_value_type!(u64, UnsignedInt); +impl_value_type!(Color, Color); +impl_value_type!(Rect, Rect); +impl_value_type!(Point, Point); +impl_value_type!(Size, Size); +impl_value_type!(ArcStr, String); -impl<'a, T: ValueType<'a>> KeyOrValue { +impl KeyOrValue { /// Resolve the concrete type `T` from this `KeyOrValue`, using the provided /// [`Env`] if required. /// /// [`Env`]: struct.Env.html - pub fn resolve(&'a self, env: &'a Env) -> T { + pub fn resolve(&self, env: &Env) -> T { match self { KeyOrValue::Concrete(value) => value.to_inner_unchecked(), KeyOrValue::Key(key) => env.get(key), @@ -584,13 +550,13 @@ impl<'a, T: ValueType<'a>> KeyOrValue { } } -impl<'a, V: Into, T: ValueType<'a, Owned = V>> From for KeyOrValue { - fn from(value: V) -> KeyOrValue { +impl> From for KeyOrValue { + fn from(value: T) -> KeyOrValue { KeyOrValue::Concrete(value.into()) } } -impl<'a, T: ValueType<'a>> From> for KeyOrValue { +impl From> for KeyOrValue { fn from(key: Key) -> KeyOrValue { KeyOrValue::Key(key) } @@ -602,12 +568,12 @@ mod tests { #[test] fn string_key_or_value() { - const MY_KEY: Key<&str> = Key::new("test.my-string-key"); - let env = Env::default().adding(MY_KEY, "Owned".to_string()); - assert_eq!(env.get(MY_KEY), "Owned"); + const MY_KEY: Key = Key::new("test.my-string-key"); + let env = Env::default().adding(MY_KEY, "Owned"); + assert_eq!(env.get(MY_KEY).as_ref(), "Owned"); - let key: KeyOrValue<&str> = MY_KEY.into(); - let value: KeyOrValue<&str> = "Owned".to_string().into(); + let key: KeyOrValue = MY_KEY.into(); + let value: KeyOrValue = ArcStr::from("Owned").into(); assert_eq!(key.resolve(&env), value.resolve(&env)); } diff --git a/druid/src/lib.rs b/druid/src/lib.rs index fead9a50e9..42e9ac03d5 100644 --- a/druid/src/lib.rs +++ b/druid/src/lib.rs @@ -185,7 +185,7 @@ pub use app_delegate::{AppDelegate, DelegateCtx}; pub use box_constraints::BoxConstraints; pub use command::{sys as commands, Command, Selector, SingleUse, Target}; pub use contexts::{EventCtx, LayoutCtx, LifeCycleCtx, PaintCtx, UpdateCtx}; -pub use data::Data; +pub use data::{ArcStr, Data}; pub use env::{Env, Key, KeyOrValue, Value, ValueType}; pub use event::{Event, InternalEvent, InternalLifeCycle, LifeCycle}; pub use ext_event::{ExtEventError, ExtEventSink}; diff --git a/druid/src/theme.rs b/druid/src/theme.rs index d2be75d70e..8012eeaaa3 100644 --- a/druid/src/theme.rs +++ b/druid/src/theme.rs @@ -17,7 +17,7 @@ #![allow(missing_docs)] use crate::piet::Color; -use crate::{Env, Key}; +use crate::{ArcStr, Env, Key}; pub const WINDOW_BACKGROUND_COLOR: Key = Key::new("window_background_color"); @@ -41,7 +41,7 @@ pub const SELECTION_COLOR: Key = Key::new("selection_color"); pub const SELECTION_TEXT_COLOR: Key = Key::new("selection_text_color"); pub const CURSOR_COLOR: Key = Key::new("cursor_color"); -pub const FONT_NAME: Key<&str> = Key::new("font_name"); +pub const FONT_NAME: Key = Key::new("font_name"); pub const TEXT_SIZE_NORMAL: Key = Key::new("text_size_normal"); pub const TEXT_SIZE_LARGE: Key = Key::new("text_size_large"); pub const BASIC_WIDGET_HEIGHT: Key = Key::new("basic_widget_height"); diff --git a/druid/src/widget/label.rs b/druid/src/widget/label.rs index 910ece6806..688d663b35 100644 --- a/druid/src/widget/label.rs +++ b/druid/src/widget/label.rs @@ -19,7 +19,7 @@ use crate::piet::{ TextLayoutBuilder, UnitPoint, }; use crate::{ - theme, BoxConstraints, Data, Env, Event, EventCtx, KeyOrValue, LayoutCtx, LifeCycle, + theme, ArcStr, BoxConstraints, Data, Env, Event, EventCtx, KeyOrValue, LayoutCtx, LifeCycle, LifeCycleCtx, LocalizedString, PaintCtx, Point, Size, UpdateCtx, Widget, }; @@ -55,7 +55,7 @@ pub struct Label { text: LabelText, color: KeyOrValue, size: KeyOrValue, - font: KeyOrValue<&'static str>, + font: KeyOrValue, } impl Label { @@ -141,7 +141,7 @@ impl Label { /// The argument can be a `&str`, `String`, or [`Key<&str>`]. /// /// [`Key<&str>`]: ../struct.Key.html - pub fn with_font(mut self, font: impl Into>) -> Self { + pub fn with_font(mut self, font: impl Into>) -> Self { self.font = font.into(); self } @@ -187,7 +187,7 @@ impl Label { /// The argument can be a `&str`, `String`, or [`Key<&str>`]. /// /// [`Key<&str>`]: ../struct.Key.html - pub fn set_font(&mut self, font: impl Into>) { + pub fn set_font(&mut self, font: impl Into>) { self.font = font.into(); } @@ -198,7 +198,7 @@ impl Label { // TODO: caching of both the format and the layout self.text.with_display_text(|text| { - let font = t.font_family(font_name).unwrap_or(FontFamily::SYSTEM_UI); + let font = t.font_family(&font_name).unwrap_or(FontFamily::SYSTEM_UI); t.new_text_layout(&text) .font(font, font_size) .text_color(color.clone()) diff --git a/druid/src/widget/switch.rs b/druid/src/widget/switch.rs index 7ea7a0bea7..f09cbab3d3 100644 --- a/druid/src/widget/switch.rs +++ b/druid/src/widget/switch.rs @@ -59,7 +59,7 @@ impl Switch { let font = ctx .text() - .font_family(font_name) + .font_family(&font_name) .unwrap_or(FontFamily::SYSTEM_UI); // off/on labels diff --git a/druid/src/widget/textbox.rs b/druid/src/widget/textbox.rs index 6d4a059994..a2b7084874 100644 --- a/druid/src/widget/textbox.rs +++ b/druid/src/widget/textbox.rs @@ -100,7 +100,7 @@ impl TextBox { // TODO: caching of both the format and the layout let font = piet_text - .font_family(font_name) + .font_family(&font_name) .unwrap_or(FontFamily::SYSTEM_UI); piet_text