Use ArcStr in LocalizedString and LabelText

This lets us share our text between druid and the piet layout object, and starts the process of moving us away from using String where possible. This was motivated by explorations around making Label optionally work as a `Widget<ArcStr>`, and using lens-like adapters to generate that text from some other data types as needed; this would be a small improvement of the label API.
linebender · Sep 18, 2020 · 7530005 · 7530005
1 parent 23cc3a4
commit 7530005
Show file tree

Hide file tree

Showing 6 changed files with 63 additions and 38 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -34,7 +34,7 @@ You can find its changes [documented below](#060---2020-06-01).
 - `TextLayout` type simplifies drawing text ([#1182] by [@cmyr])
 - Implementation of `Data` trait for `i128` and `u128` primitive data types. ([#1214] by [@koutoftimer])
 - `LineBreaking` enum allows configuration of label line-breaking ([#1195] by [@cmyr])
-- `TextAlignment` support in `TextLayout` and `Label` ([#1210] by [@cmyr])`
+- `TextAlignment` support in `TextLayout` and `Label` ([#1210] by [@cmyr])
 - `UpdateCtx` gets `env_changed` and `env_key_changed` methods ([#1207] by [@cmyr])
 - `Button::from_label` to construct a `Button` with a provided `Label`. ([#1226] by [@ForLoveOfCats])
 - Lens: Added Unit lens for type erased / display only widgets that do not need data. ([#1232] by [@rjwittams])
@@ -58,6 +58,7 @@ You can find its changes [documented below](#060---2020-06-01).
 - `Movement::RightOfLine` to `Movement::NextLineBreak`, and `Movement::LeftOfLine` to `Movement::PrecedingLineBreak`. ([#1092] by [@sysint64])
 - `AnimFrame` was moved from `lifecycle` to `event` ([#1155] by [@jneem])
 - Contexts' `text()` methods return `&mut PietText` instead of cloning ([#1205] by [@cmyr])
+- `LocalizedString` and `LabelText` use `ArcStr` instead of String ([#1245] by [@cmyr])
 
 ### Deprecated
 
@@ -459,6 +460,7 @@ Last release without a changelog :(
 [#1231]: https://github.com/linebender/druid/pull/1231
 [#1220]: https://github.com/linebender/druid/pull/1220
 [#1238]: https://github.com/linebender/druid/pull/1238
+[#1245]: https://github.com/linebender/druid/pull/1245
 
 [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/src/app.rs b/druid/src/app.rs
@@ -283,7 +283,7 @@ impl<T: Data> WindowDesc<T> {
 
         builder.set_window_state(self.state);
 
-        builder.set_title(self.title.display_text());
+        builder.set_title(self.title.display_text().to_string());
         if let Some(menu) = platform_menu {
             builder.set_menu(menu);
         }

diff --git a/druid/src/localization.rs b/druid/src/localization.rs
@@ -40,8 +40,7 @@ use std::{fs, io};
 
 use log::{debug, error, warn};
 
-use crate::env::Env;
-use crate::shell::Application;
+use crate::{Application, ArcStr, Env};
 
 use fluent_bundle::{
     FluentArgs, FluentBundle, FluentError, FluentMessage, FluentResource, FluentValue,
@@ -91,9 +90,9 @@ struct ArgSource<T>(ArgClosure<T>);
 #[derive(Debug, Clone)]
 pub struct LocalizedString<T> {
     pub(crate) key: &'static str,
-    placeholder: Option<String>,
+    placeholder: Option<ArcStr>,
     args: Option<Vec<(&'static str, ArgSource<T>)>>,
-    resolved: Option<String>,
+    resolved: Option<ArcStr>,
     resolved_lang: Option<LanguageIdentifier>,
 }
 
@@ -253,7 +252,7 @@ impl L10nManager {
         &'args self,
         key: &str,
         args: impl Into<Option<&'args FluentArgs<'args>>>,
-    ) -> Option<String> {
+    ) -> Option<ArcStr> {
         let args = args.into();
         let value = match self
             .current_bundle
@@ -281,10 +280,11 @@ impl L10nManager {
                 result
                     .chars()
                     .filter(|c| c != &START_ISOLATE && c != &END_ISOLATE)
-                    .collect(),
+                    .collect::<String>()
+                    .into(),
             )
         } else {
-            Some(result)
+            Some(result.into())
         }
     }
     //TODO: handle locale change
@@ -305,18 +305,18 @@ impl<T> LocalizedString<T> {
     /// Add a placeholder value. This will be used if localization fails.
     ///
     /// This is intended for use during prototyping.
-    pub fn with_placeholder(mut self, placeholder: impl Into<String>) -> Self {
+    pub fn with_placeholder(mut self, placeholder: impl Into<ArcStr>) -> Self {
         self.placeholder = Some(placeholder.into());
         self
     }
 
     /// Return the localized value for this string, or the placeholder, if
     /// the localization is missing, or the key if there is no placeholder.
-    pub fn localized_str(&self) -> &str {
+    pub fn localized_str(&self) -> ArcStr {
         self.resolved
-            .as_deref()
-            .or_else(|| self.placeholder.as_deref())
-            .unwrap_or(self.key)
+            .clone()
+            .or_else(|| self.placeholder.clone())
+            .unwrap_or_else(|| self.key.into())
     }
 
     /// Add a named argument and a corresponding closure. This closure

diff --git a/druid/src/menu.rs b/druid/src/menu.rs
@@ -347,7 +347,7 @@ impl<T: Data> MenuDesc<T> {
                     item.platform_id = MenuItemId::next();
                     menu.add_item(
                         item.platform_id.as_u32(),
-                        item.title.localized_str(),
+                        &item.title.localized_str(),
                         item.hotkey.as_ref(),
                         item.enabled,
                         item.selected,

diff --git a/druid/src/widget/label.rs b/druid/src/widget/label.rs
@@ -17,8 +17,8 @@
 use crate::piet::{Color, PietText};
 use crate::widget::prelude::*;
 use crate::{
-    BoxConstraints, Data, FontDescriptor, KeyOrValue, LocalizedString, Point, Size, TextAlignment,
-    TextLayout,
+    ArcStr, BoxConstraints, Data, FontDescriptor, KeyOrValue, LocalizedString, Point, Size,
+    TextAlignment, TextLayout,
 };
 
 // added padding between the edges of the widget and the text.
@@ -34,8 +34,17 @@ const LABEL_X_PADDING: f64 = 2.0;
 pub enum LabelText<T> {
     /// Localized string that will be resolved through `Env`.
     Localized(LocalizedString<T>),
-    /// Specific text
-    Specific(String),
+    /// Specific text.
+    Specific {
+        /// The text.
+        string: ArcStr,
+        /// Whether or not the `resolved` method has been called yet.
+        ///
+        /// We want to return `true` from that method when it is first called,
+        /// so that callers will know to retrieve the text. This matches
+        /// the behaviour of the other variants.
+        resolved: bool,
+    },
     /// The provided closure is called on update, and its return
     /// value is used as the text for the label.
     Dynamic(Dynamic<T>),
@@ -45,7 +54,7 @@ pub enum LabelText<T> {
 #[doc(hidden)]
 pub struct Dynamic<T> {
     f: Box<dyn Fn(&T, &Env) -> String>,
-    resolved: String,
+    resolved: ArcStr,
 }
 
 /// A label that draws some text.
@@ -218,11 +227,6 @@ impl<T: Data> Label<T> {
         self.needs_rebuild = true;
     }
 
-    /// Returns this label's current text.
-    pub fn text(&self) -> &str {
-        self.text.display_text()
-    }
-
     /// Set the text color.
     ///
     /// The argument can be either a `Color` or a [`Key<Color>`].
@@ -309,8 +313,8 @@ impl<T: Data> Label<T> {
 impl<T> Dynamic<T> {
     fn resolve(&mut self, data: &T, env: &Env) -> bool {
         let new = (self.f)(data, env);
-        let changed = new != self.resolved;
-        self.resolved = new;
+        let changed = new.as_str() != &*self.resolved;
+        self.resolved = new.into();
         changed
     }
 }
@@ -319,18 +323,18 @@ impl<T: Data> LabelText<T> {
     /// Call callback with the text that should be displayed.
     pub fn with_display_text<V>(&self, mut cb: impl FnMut(&str) -> V) -> V {
         match self {
-            LabelText::Specific(s) => cb(s.as_str()),
-            LabelText::Localized(s) => cb(s.localized_str()),
-            LabelText::Dynamic(s) => cb(s.resolved.as_str()),
+            LabelText::Specific { string, .. } => cb(&string),
+            LabelText::Localized(s) => cb(&s.localized_str()),
+            LabelText::Dynamic(s) => cb(&s.resolved),
         }
     }
 
     /// Return the current resolved text.
-    pub fn display_text(&self) -> &str {
+    pub fn display_text(&self) -> ArcStr {
         match self {
-            LabelText::Specific(s) => s.as_str(),
+            LabelText::Specific { string, .. } => string.clone(),
             LabelText::Localized(s) => s.localized_str(),
-            LabelText::Dynamic(s) => s.resolved.as_str(),
+            LabelText::Dynamic(s) => s.resolved.clone(),
         }
     }
 
@@ -340,7 +344,11 @@ impl<T: Data> LabelText<T> {
     /// Returns `true` if the string has changed.
     pub fn resolve(&mut self, data: &T, env: &Env) -> bool {
         match self {
-            LabelText::Specific(_) => false,
+            LabelText::Specific { resolved, .. } => {
+                let is_first_call = !*resolved;
+                *resolved = true;
+                is_first_call
+            }
             LabelText::Localized(s) => s.resolve(data, env),
             LabelText::Dynamic(s) => s.resolve(data, env),
         }
@@ -388,13 +396,28 @@ impl<T: Data> Widget<T> for Label<T> {
 
 impl<T> From<String> for LabelText<T> {
     fn from(src: String) -> LabelText<T> {
-        LabelText::Specific(src)
+        LabelText::Specific {
+            string: src.into(),
+            resolved: false,
+        }
     }
 }
 
 impl<T> From<&str> for LabelText<T> {
     fn from(src: &str) -> LabelText<T> {
-        LabelText::Specific(src.to_string())
+        LabelText::Specific {
+            string: src.into(),
+            resolved: false,
+        }
+    }
+}
+
+impl<T> From<ArcStr> for LabelText<T> {
+    fn from(string: ArcStr) -> LabelText<T> {
+        LabelText::Specific {
+            string,
+            resolved: false,
+        }
     }
 }
 
@@ -409,7 +432,7 @@ impl<T, F: Fn(&T, &Env) -> String + 'static> From<F> for LabelText<T> {
         let f = Box::new(src);
         LabelText::Dynamic(Dynamic {
             f,
-            resolved: String::default(),
+            resolved: ArcStr::from(""),
         })
     }
 }
diff --git a/druid/src/window.rs b/druid/src/window.rs
@@ -422,7 +422,7 @@ impl<T: Data> Window<T> {
 
     pub(crate) fn update_title(&mut self, data: &T, env: &Env) {
         if self.title.resolve(data, env) {
-            self.handle.set_title(self.title.display_text());
+            self.handle.set_title(&self.title.display_text());
         }
     }