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<str>, 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.

Author: cmyr

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<str>` 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

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<f64> = Key::new("styled_text.custom_text_size");
-const MY_CUSTOM_FONT: Key<&str> = Key::new("styled_text.custom_font");
+const MY_CUSTOM_FONT: Key<ArcStr> = Key::new("styled_text.custom_font");
 
 #[derive(Clone, Lens, Data)]
 struct AppData {
@@ -99,7 +99,7 @@ fn ui_builder() -> impl Widget<AppData> {
             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));
             }
         });
 

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<str>;
+
 /// 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 {

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<EnvImpl>);
 
 #[derive(Clone)]
 struct EnvImpl {
-    map: HashMap<String, Value>,
+    map: HashMap<ArcStr, Value>,
     debug_colors: Vec<Color>,
     l10n: Arc<L10nManager>,
 }
@@ -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<T> {
 }
 
 /// 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<Value>;
-
+pub trait ValueType: Sized + Into<Value> {
     /// Attempt to convert the generic `Value` into this type.
-    fn try_from_value(v: &'a Value) -> Result<Self, ValueTypeError>;
+    fn try_from_value(v: &Value) -> Result<Self, ValueTypeError>;
 }
 
 /// 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<Key<V>>) -> V {
+    pub fn get<V: ValueType>(&self, key: impl Borrow<Key<V>>) -> 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<Key<V>>,
-    ) -> Result<V, MissingKeyError> {
+    pub fn try_get<V: ValueType>(&self, key: impl Borrow<Key<V>>) -> Result<V, MissingKeyError> {
         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<Item = (&String, &Value)> {
+    pub fn get_all(&self) -> impl ExactSizeIterator<Item = (&ArcStr, &Value)> {
         self.0.map.iter()
     }
 
     /// Adds a key/value, acting like a builder.
-    pub fn adding<'a, V: ValueType<'a>>(mut self, key: Key<V>, value: impl Into<V::Owned>) -> Env {
+    pub fn adding<V: ValueType>(mut self, key: Key<V>, value: impl Into<V>) -> 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<V>, value: impl Into<V::Owned>) {
+    pub fn set<V: ValueType>(&mut self, key: Key<V>, value: impl Into<V>) {
         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<V: ValueType>(&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<T> From<Key<T>> for String {
-    fn from(src: Key<T>) -> String {
-        String::from(src.key)
+impl<T> From<Key<T>> for ArcStr {
+    fn from(src: Key<T>) -> 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<Self, ValueTypeError> {
                 match value {
                     Value::$var(f) => Ok(f.to_owned()),
@@ -540,57 +528,35 @@ 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<Self, ValueTypeError> {
-                match value {
-                    Value::$var(f) => Ok(f),
-                    other => Err(ValueTypeError::new(any::type_name::<$ty>(), other.clone())),
-                }
-            }
-        }
-
-        impl Into<Value> 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<T> {
+impl<T: ValueType> KeyOrValue<T> {
     /// 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),
         }
     }
 }
 
-impl<'a, V: Into<Value>, T: ValueType<'a, Owned = V>> From<V> for KeyOrValue<T> {
-    fn from(value: V) -> KeyOrValue<T> {
+impl<T: Into<Value>> From<T> for KeyOrValue<T> {
+    fn from(value: T) -> KeyOrValue<T> {
         KeyOrValue::Concrete(value.into())
     }
 }
 
-impl<'a, T: ValueType<'a>> From<Key<T>> for KeyOrValue<T> {
+impl<T: ValueType> From<Key<T>> for KeyOrValue<T> {
     fn from(key: Key<T>) -> KeyOrValue<T> {
         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<ArcStr> = 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<ArcStr> = MY_KEY.into();
+        let value: KeyOrValue<ArcStr> = ArcStr::from("Owned").into();
 
         assert_eq!(key.resolve(&env), value.resolve(&env));
     }

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};

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<Color> = Key::new("window_background_color");
 
@@ -41,7 +41,7 @@ pub const SELECTION_COLOR: Key<Color> = Key::new("selection_color");
 pub const SELECTION_TEXT_COLOR: Key<Color> = Key::new("selection_text_color");
 pub const CURSOR_COLOR: Key<Color> = Key::new("cursor_color");
 
-pub const FONT_NAME: Key<&str> = Key::new("font_name");
+pub const FONT_NAME: Key<ArcStr> = Key::new("font_name");
 pub const TEXT_SIZE_NORMAL: Key<f64> = Key::new("text_size_normal");
 pub const TEXT_SIZE_LARGE: Key<f64> = Key::new("text_size_large");
 pub const BASIC_WIDGET_HEIGHT: Key<f64> = Key::new("basic_widget_height");

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<T> {
     text: LabelText<T>,
     color: KeyOrValue<Color>,
     size: KeyOrValue<f64>,
-    font: KeyOrValue<&'static str>,
+    font: KeyOrValue<ArcStr>,
 }
 
 impl<T: Data> Label<T> {
@@ -141,7 +141,7 @@ impl<T: Data> Label<T> {
     /// The argument can be a `&str`, `String`, or [`Key<&str>`].
     ///
     /// [`Key<&str>`]: ../struct.Key.html
-    pub fn with_font(mut self, font: impl Into<KeyOrValue<&'static str>>) -> Self {
+    pub fn with_font(mut self, font: impl Into<KeyOrValue<ArcStr>>) -> Self {
         self.font = font.into();
         self
     }
@@ -187,7 +187,7 @@ impl<T: Data> Label<T> {
     /// The argument can be a `&str`, `String`, or [`Key<&str>`].
     ///
     /// [`Key<&str>`]: ../struct.Key.html
-    pub fn set_font(&mut self, font: impl Into<KeyOrValue<&'static str>>) {
+    pub fn set_font(&mut self, font: impl Into<KeyOrValue<ArcStr>>) {
         self.font = font.into();
     }
 
@@ -198,7 +198,7 @@ impl<T: Data> Label<T> {
 
         // 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())

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

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