Cleanup formatter work

cmyr · cmyr · commit feb602c9da81 · 2020-11-25T14:58:03.000-05:00
This resolves a bunch of fixmes and loose ends
diff --git a/druid/examples/flex.rs b/druid/examples/flex.rs
@@ -14,14 +14,13 @@
 
 //! Demonstrates alignment of children in the flex container.
 
+use druid::text::format::ParseFormatter;
 use druid::widget::prelude::*;
 use druid::widget::{
     Button, Checkbox, CrossAxisAlignment, Flex, Label, MainAxisAlignment, ProgressBar, RadioGroup,
     SizedBox, Slider, Stepper, Switch, TextBox, WidgetExt,
 };
-use druid::{
-    AppLauncher, Color, Data, Lens, LensExt, LocalizedString, PlatformError, WidgetId, WindowDesc,
-};
+use druid::{AppLauncher, Color, Data, Lens, LocalizedString, PlatformError, WidgetId, WindowDesc};
 
 const DEFAULT_SPACER_SIZE: f64 = 8.;
 
@@ -210,11 +209,8 @@ fn make_spacer_select() -> impl Widget<Params> {
             Flex::row()
                 .with_child(
                     TextBox::new()
-                        .parse()
-                        .lens(
-                            Params::spacer_size
-                                .map(|x| Some(*x), |x, y| *x = y.unwrap_or(DEFAULT_SPACER_SIZE)),
-                        )
+                        .with_formatter(ParseFormatter)
+                        .lens(Params::spacer_size)
                         .fix_width(60.0),
                 )
                 .with_spacer(druid::theme::WIDGET_CONTROL_COMPONENT_PADDING)
diff --git a/druid/src/core.rs b/druid/src/core.rs
@@ -1094,6 +1094,7 @@ impl WidgetState {
 mod tests {
     use super::*;
     use crate::ext_event::ExtEventHost;
+    use crate::text::format::ParseFormatter;
     use crate::widget::{Flex, Scroll, Split, TextBox};
     use crate::{WidgetExt, WindowHandle, WindowId};
 
@@ -1103,13 +1104,13 @@ mod tests {
 
     #[test]
     fn register_children() {
-        fn make_widgets() -> impl Widget<Option<u32>> {
+        fn make_widgets() -> impl Widget<u32> {
             Split::columns(
-                Flex::<Option<u32>>::row()
-                    .with_child(TextBox::new().with_id(ID_1).parse())
-                    .with_child(TextBox::new().with_id(ID_2).parse())
-                    .with_child(TextBox::new().with_id(ID_3).parse()),
-                Scroll::new(TextBox::new().parse()),
+                Flex::<u32>::row()
+                    .with_child(TextBox::new().with_formatter(ParseFormatter).with_id(ID_1))
+                    .with_child(TextBox::new().with_formatter(ParseFormatter).with_id(ID_2))
+                    .with_child(TextBox::new().with_formatter(ParseFormatter).with_id(ID_3)),
+                Scroll::new(TextBox::new().with_formatter(ParseFormatter)),
             )
         }
 
@@ -1136,7 +1137,7 @@ mod tests {
 
         let env = Env::default();
 
-        widget.lifecycle(&mut ctx, &LifeCycle::WidgetAdded, &None, &env);
+        widget.lifecycle(&mut ctx, &LifeCycle::WidgetAdded, &1, &env);
         assert!(ctx.widget_state.children.may_contain(&ID_1));
         assert!(ctx.widget_state.children.may_contain(&ID_2));
         assert!(ctx.widget_state.children.may_contain(&ID_3));
diff --git a/druid/src/widget/textbox.rs b/druid/src/widget/textbox.rs
@@ -36,6 +36,13 @@ const COMPLETE_EDITING: Selector = Selector::new("druid.builtin.textbox-complete
 const CANCEL_EDITING: Selector = Selector::new("druid.builtin.textbox-cancel-editing");
 
 /// A widget that allows user text input.
+///
+/// # Editing values
+///
+/// If the text you are editing represents a value of some other type, such
+/// as a number, you should use a [`ValueTextBox`] and an appropriate
+/// [`Formatter`]. You can create a [`ValueTextBox`] by passing the appropriate
+/// [`Formatter`] to [`TextBox::with_formatter`].
 #[derive(Debug, Clone)]
 pub struct TextBox<T> {
     placeholder: TextLayout<String>,
@@ -275,14 +282,6 @@ impl<T> TextBox<T> {
     pub fn editor(&self) -> &Editor<T> {
         &self.editor
     }
-
-    /// Return a mutable reference to the [`Editor`] used by this `TextBox`.
-    ///
-    /// [`Editor`]: crate::text::Editor
-    //TODO: document the ways you should and shouldn't use this
-    pub fn editor_mut(&mut self) -> &mut Editor<T> {
-        &mut self.editor
-    }
 }
 
 impl TextBox<String> {
@@ -300,8 +299,16 @@ impl TextBox<String> {
 }
 
 impl<T: TextStorage + EditableText> TextBox<T> {
+    /// Set the textbox's selection.
+    pub fn set_selection(&mut self, selection: Selection) {
+        self.editor.set_selection(selection);
+    }
+
     /// Set the text and force the editor to update.
-    //FIXME: do we need this? can we not just rely on `update`?
+    ///
+    /// This should be rarely needed; the main use-case would be if you need
+    /// to manually set the text and then immediately do hit-testing or other
+    /// tasks that rely on having an up-to-date text layout.
     pub fn force_rebuild(&mut self, text: T, factory: &mut PietText, env: &Env) {
         self.editor.set_text(text);
         self.editor.rebuild_if_needed(factory, env);
@@ -634,14 +641,13 @@ impl<T: Data> ValueTextBox<T> {
         self
     }
 
-    fn complete(&mut self, ctx: &mut EventCtx, data: &mut T, env: &Env) {
+    fn complete(&mut self, ctx: &mut EventCtx, data: &mut T) {
         match self.formatter.value(&self.buffer) {
             Ok(new_data) => {
                 *data = new_data;
-                self.inner
-                    .force_rebuild(self.formatter.format(data), ctx.text(), env);
+                self.buffer = self.formatter.format(data);
                 self.is_editing = false;
-                ctx.request_layout();
+                ctx.request_update();
                 if ctx.has_focus() {
                     ctx.resign_focus();
                 }
@@ -665,24 +671,19 @@ impl<T: Data> ValueTextBox<T> {
         }
     }
 
-    fn cancel(&mut self, ctx: &mut EventCtx, data: &T, env: &Env) {
+    fn cancel(&mut self, ctx: &mut EventCtx, data: &T) {
         self.is_editing = false;
         self.buffer = self.formatter.format(data);
-        ctx.request_layout();
+        ctx.request_update();
         ctx.resign_focus();
-        self.inner
-            .force_rebuild(self.buffer.clone(), ctx.text(), env);
         self.send_event(ctx, TextBoxEvent::Cancel);
     }
 
-    fn begin(&mut self, ctx: &mut EventCtx, data: &T, env: &Env) {
+    fn begin(&mut self, ctx: &mut EventCtx, data: &T) {
         self.is_editing = true;
         self.buffer = self.formatter.format_for_editing(data);
         self.last_known_data = Some(data.clone());
-        self.inner
-            .force_rebuild(self.buffer.clone(), ctx.text(), env);
-        self.old_buffer = self.buffer.clone();
-        ctx.request_layout();
+        ctx.request_update();
         self.send_event(ctx, TextBoxEvent::Began);
     }
 
@@ -696,27 +697,23 @@ impl<T: Data> ValueTextBox<T> {
 impl<T: Data> Widget<T> for ValueTextBox<T> {
     fn event(&mut self, ctx: &mut EventCtx, event: &Event, data: &mut T, env: &Env) {
         if matches!(event, Event::Command(cmd) if cmd.is(BEGIN_EDITING)) {
-            return self.begin(ctx, data, env);
+            return self.begin(ctx, data);
         }
 
         if self.is_editing {
             // if we reject an edit we want to reset the selection
             let pre_sel = *self.inner.editor().selection();
             match event {
-                Event::Command(cmd) if cmd.is(COMPLETE_EDITING) => {
-                    return self.complete(ctx, data, env)
-                }
-                Event::Command(cmd) if cmd.is(CANCEL_EDITING) => {
-                    return self.cancel(ctx, data, env)
-                }
+                Event::Command(cmd) if cmd.is(COMPLETE_EDITING) => return self.complete(ctx, data),
+                Event::Command(cmd) if cmd.is(CANCEL_EDITING) => return self.cancel(ctx, data),
                 Event::KeyDown(k_e) if HotKey::new(None, KbKey::Enter).matches(k_e) => {
                     ctx.set_handled();
-                    self.complete(ctx, data, env);
+                    self.complete(ctx, data);
                     return;
                 }
                 Event::KeyDown(k_e) if HotKey::new(None, KbKey::Escape).matches(k_e) => {
                     ctx.set_handled();
-                    self.cancel(ctx, data, env);
+                    self.cancel(ctx, data);
                     return;
                 }
                 event => {
@@ -759,10 +756,8 @@ impl<T: Data> Widget<T> for ValueTextBox<T> {
                     };
 
                     if let Some(new_buf) = new_buf {
-                        self.buffer = new_buf.clone();
-                        self.inner.editor_mut().set_text(new_buf);
+                        self.buffer = new_buf;
                     }
-                    //FIXME we stash this and set it in update; can we do the same with `new_buf`?
                     self.force_selection = new_sel;
 
                     if self.update_data_while_editing && !validation.is_err() {
@@ -782,7 +777,11 @@ impl<T: Data> Widget<T> for ValueTextBox<T> {
             }
             ctx.request_update();
         } else if let Event::MouseDown(_) = event {
-            self.begin(ctx, data, env);
+            self.begin(ctx, data);
+            // we need to rebuild immediately here in order for the click
+            // to be handled with the most recent text.
+            self.inner
+                .force_rebuild(self.buffer.clone(), ctx.text(), env);
             self.inner.event(ctx, event, &mut self.buffer, env);
         }
     }
@@ -820,17 +819,23 @@ impl<T: Data> Widget<T> for ValueTextBox<T> {
                     self.formatter.format(data)
                 );
             }
-        } else if !old_data.same(data) {
-            // we aren't editing and data changed
-            let new_text = self.formatter.format(data);
-            self.old_buffer = std::mem::replace(&mut self.buffer, new_text);
-            self.inner.update(ctx, &self.old_buffer, &self.buffer, env);
-        } else if ctx.env_changed() {
-            self.inner.update(ctx, &self.buffer, &self.buffer, env);
-            ctx.request_layout();
+        } else {
+            if !old_data.same(data) {
+                // we aren't editing and data changed
+                let new_text = self.formatter.format(data);
+                self.old_buffer = std::mem::replace(&mut self.buffer, new_text);
+            }
+
+            if !self.old_buffer.same(&self.buffer) {
+                // inner widget handles calling request_layout, as needed
+                self.inner.update(ctx, &self.old_buffer, &self.buffer, env);
+                self.old_buffer = self.buffer.clone();
+            } else if ctx.env_changed() {
+                self.inner.update(ctx, &self.buffer, &self.buffer, env);
+            }
         }
         if let Some(sel) = self.force_selection.take() {
-            self.inner.editor_mut().set_selection(sel);
+            self.inner.set_selection(sel);
         }
     }
 
diff --git a/druid/src/widget/widget_ext.rs b/druid/src/widget/widget_ext.rs
@@ -220,6 +220,7 @@ pub trait WidgetExt<T: Data>: Widget<T> + Sized + 'static {
     }
 
     /// Parse a `Widget<String>`'s contents
+    #[deprecated(since = "0.7.0", note = "Use TextBox::with_formatter instead")]
     fn parse(self) -> Parse<Self>
     where
         Self: Widget<String>,

Original file line number	Diff line number	Diff line change
`@@ -220,6 +220,7 @@ pub trait WidgetExt<T: Data>: Widget<T> + Sized + 'static {`
`220`	`220`	`}`
`221`	`221`
`222`	`222`	/// Parse a `Widget<String>`'s contents
	`223`	`+ #[deprecated(since = "0.7.0", note = "Use TextBox::with_formatter instead")]`
`223`	`224`	`fn parse(self) -> Parse<Self>`
`224`	`225`	`where`
`225`	`226`	`Self: Widget<String>,`