Rethink Forms. #6569

mpcomplete · 2016-10-27T21:41:40Z

FormField is now a widget that can contain any type of field. Input no
longer has special code to handle form fields. Instead, there is a
helper widget InputFormField for using an Input inside a FormField.

Fixes #6097 and based on
feedback from the same.

mpcomplete · 2016-10-27T21:44:57Z

One more attempt at making sense of Forms. This is based on customer feedback from #6097 .

There's a number of advantages to this approach, IMO:

It works with any type of widget.
It's easy to make helper functions/widgets a la InputFormField to simplify creating FormFields that contain a specific widget.
FormFields work outside of a Form, if you just want to use it for managing the state of a widget.
Forms save and validate as a unit, making it easy to reset the form state (see the ExpansionPanelDemo's cancel).
Input no longer has a bunch of special case code for working inside a Form.

Hixie · 2016-10-27T21:47:05Z

examples/flutter_gallery/lib/demo/expansion_panels_demo.dart

@@ -185,27 +189,30 @@ class _ExpansionPanelsDemoState extends State<ExpasionPanelsDemo> {
        builder: (DemoItem<String> item) { // ignore: argument_type_not_assignable, https://github.com/flutter/flutter/issues/5771
          void close() {
            setState(() {
+              // Clear the input state.
+              // TODO(mpcomplete): is there a way to have a Form.reset that purges its children?


(please make sure you answer this question before check-in)

shouldn't this be straight-forward? I don't really understand the problem here.

Yes it should! I don't know how, though. :)

Tips?

Hixie · 2016-10-27T21:48:43Z

examples/flutter_gallery/lib/demo/expansion_panels_demo.dart

+                  );
+                },
+              ),
+              onSave: () { form3Key.currentState.save(); close(); },


it's sad we have to get the state via a key here... not sure what to suggest instead though

Maybe onSave should give you a BuildContext you can use to find the Form.

That could work here where we have a custom widget that implements the onSave function. But sadly there doesn't seem to be a general solution to this issue (see text_field_demo, where we want to call save when a generic RaisedButton is pressed).

I've actually found myself wanting something like this multiple times in this patch - a way for a child widget to refer to its ancestor. The whole reason FormField takes a builder function instead of a child widget is that I want to use FormField's state in the child's builder. A builder function is the only way for child widgets to depend on their ancestor's state in some way.

Hixie · 2016-10-27T21:49:21Z

packages/flutter/lib/src/material/input.dart

-  }
+/// A [FormField] that contains an [Input].
+class InputFormField extends FormField<InputValue> {
+  // TODO(mpcomplete): is there some way to keep args these in sync with the Input constructor?


Hixie · 2016-10-27T21:51:22Z

packages/flutter/lib/src/widgets/form.dart

+
+  @override
+  void deactivate() {
+    super.deactivate();


nit: call super.deactivate last

BTW, is there a better way to do this registration? I noticed you did something fancy with inheritFromWidgetOfExactType, but I didn't quite understand it and wasn't sure if it applied here.

In my patch the Form didn't need to do anything except cause the registered widgets to rebuild, so it could be vastly simpler. Here you're actually doing things with the widgets, so it's much more complicated.

Hixie · 2016-10-27T21:52:44Z

packages/flutter/lib/src/widgets/form.dart

+    super.deactivate();
+    FormScope formScope = FormScope.of(context);
+    if (formScope != null)
+      formScope._formState._unregister(this);


rather than these three lines, we should just make the API be Form.register(context, this);

I guess it doesn't matter since this is all private stuff.

This won't actually work. Consider what happens if you're moved around in the tree.

I think the right way to do this is to use the pattern I had in the other patch, where Form itself is Inherited and so on, and then for your functions that call things on all the dependents, just add a visitDependents method to InheritedElement that lets you walk all the _dependents, similar to visitChildren, and then call them that way, similar to how I called dispatchDependenciesChanged in my fieldChanged function in my patch.

This won't actually work. Consider what happens if you're moved around in the tree.

I thought deactivate was called when this object was moved around in the tree. That's why I'm unregistering here. My understanding was that it would re-register at its new location in the build function. Is this wrong?

You're right, my bad.

Make sure you have a test for this particular case. I'm not confident we get the order of operations right here. It's possible we detach you from the tree before calling deactivate.

Hixie · 2016-10-27T21:59:25Z

packages/flutter/lib/src/widgets/form.dart

  int generation = 0;
+  Set<FormFieldState> _fields = new Set<FormFieldState>();

  void _onFieldChanged() {


Our convention is:

onFoo for variables that take callbacks

handleFoo for functions that are assigned to onFoo variables

FooCallback for the types of those variables

didFoo for methods that you call to notify the object

So I think here you want fieldDidChange or some such.

Hixie · 2016-10-27T22:04:15Z

I like this general approach.

Hixie · 2016-10-27T22:04:35Z

My main concern is over the need to use GlobalKeys so much to make use of this.

Hixie · 2016-10-27T22:10:52Z

examples/flutter_gallery/lib/demo/expansion_panels_demo.dart

-            onCancel: close
+              onSave: () { form1Key.currentState.save(); close(); },
+              onCancel: close
+            )


btw, minor side-note, please move the onSave and onCancel arguments to above the child argument, so that it's clearer which widget it belongs to. Both Adam and I independently misread this the first time (I thought they were on Form, he thought they were on InputFormField).

Hixie · 2016-10-27T22:16:29Z

This seems like a net improvement on what we have now, so, modulo the comments above, travis being angry, and the need for tests, LGTM.

We should loop back with customer:leafy once this is in and see how they like it.

FormField is now a widget that can contain any type of field. Input no longer has special code to handle form fields. Instead, there is a helper widget InputFormField for using an Input inside a FormField. Fixes #6097 and based on feedback from the same.

mpcomplete · 2016-11-01T22:20:21Z

OK, added a test for the FormState.deactivate case. I think this is ready for commit (pending travis approval).

Hixie · 2016-11-02T17:16:41Z

examples/flutter_gallery/lib/demo/expansion_panels_demo.dart

+            key: form1Key,
+            child: new CollapsibleBody(
+              margin: const EdgeInsets.symmetric(horizontal: 16.0),
+              onSave: () { form1Key.currentState.save(); close(); },


you're in the form here, so you could avoid using a Key if you instead put a Builder below the form and used Form.of

Hixie · 2016-11-02T17:18:20Z

examples/flutter_gallery/lib/demo/expansion_panels_demo.dart

+            child: new CollapsibleBody(
+              onSave: () { form2Key.currentState.save(); close(); },
+              onCancel: () { form2Key.currentState.reset(); close(); },
+              child: new FormField<_Location>(


are you imagining that eventually we'll have a RadioButtonGroupField or something which generates all the radio buttons and so forth, similar to how we do InputFormField for an Input?

Yeah, I imagine we'll have helper Field widgets for the common cases. But it's hopefully easy enough to use FormField directly for the cases we don't cover.

Hixie · 2016-11-02T17:18:41Z

examples/flutter_gallery/lib/demo/text_field_demo.dart

-        _validatePhoneNumber(person.phoneNumber) != null ||
-        _validatePassword(person.password) != null) {
+    FormState form = _formKey.currentState;
+    if (form.hasErrors) {


Hixie · 2016-11-02T17:19:43Z

examples/flutter_gallery/lib/demo/text_field_demo.dart

-                setter:  (String val) { person.name = val; },
-                validator: _validateName
-              )
+            new FormField<InputValue>(


why not an InputFormField here?

I added a comment - I want to demonstrate the equivalence of the 2 ways of doing it.

Hixie · 2016-11-02T18:00:16Z

LGTM modulo minor comments

mpcomplete assigned Hixie Oct 27, 2016

Hixie reviewed Oct 27, 2016

View reviewed changes

This was referenced Nov 1, 2016

Remove Forms and simplify the Input API. #6533

Closed

Allow null fields in InputValue, to represent 'unchanged'. #6460

Closed

Rethink Forms.

0d05896

FormField is now a widget that can contain any type of field. Input no longer has special code to handle form fields. Instead, there is a helper widget InputFormField for using an Input inside a FormField. Fixes #6097 and based on feedback from the same.

Hixie reviewed Nov 2, 2016

View reviewed changes

comment

e644e77

form.of

f00cd53

mpcomplete merged commit 113991d into flutter:master Nov 2, 2016

mpcomplete deleted the forms branch November 2, 2016 18:27

nelsonkam mentioned this pull request Nov 9, 2016

Update documentation on text input. #6779

Closed

This was referenced Dec 1, 2016

Forms would like a cheaper GlobalKey for accessing widget State #7124

Closed

Expand support for Form widget beyond Inputs #3250

Closed

kezhuw mentioned this pull request Nov 23, 2017

InheritedElement.dispatchDidChangeDependencies behaves different to this document #13176

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2021

Rethink Forms. #6569

Rethink Forms. #6569

Uh oh!

Conversation

mpcomplete commented Oct 27, 2016

Uh oh!

mpcomplete commented Oct 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Oct 27, 2016

Uh oh!

Hixie commented Oct 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Oct 27, 2016

Uh oh!

mpcomplete commented Nov 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Nov 2, 2016

Uh oh!

Uh oh!