Skip to content

Conversation

@bleroux
Copy link
Contributor

@bleroux bleroux commented Mar 4, 2024

Description

This PR makes CupertinoTextFormFieldRow restorable.
The implementation is based on #78835 which made FormField and TextFormField restorable.

Related Issue

Fixes #144504.

Tests

Adds 4 tests.

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository labels Mar 4, 2024
@bleroux bleroux force-pushed the restorable_CupertinoTextFormFieldRow branch from 91e8897 to b68797d Compare March 4, 2024 13:51
@bleroux bleroux requested a review from justinmc March 4, 2024 14:34
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nits 👍 . Thanks for adding restoration support here. We must have missed it earlier when state restoration was added to the framework.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call making this non-nullable now!

Comment on lines 294 to 295
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

// This makes sure to...

I think this is more clear that the comment is just an explanation. When I first read it, I thought it might be a command to callers to make sure that they also update the value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think it's kind of hard to read like this, because TestWidget seems so generic here. Maybe just rename it to "RestorableCupertinoTextFormFieldRow" and "useExternal" to "useExternalController"? And/or move the widgets other than the CupertinoTextFormFieldRow out?

I don't know, maybe that makes it more confusing haha. Up to you.

Copy link
Contributor Author

@bleroux bleroux Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copy/pasted those names from other form field restoration tests. I was not pleased with the TestWidget but it is used on several existing tests and I did not have a good name to replace it.
I updated the PR with useExternalController (great suggestion) and renamed TestWidget to RestorableTestWidget. I tried RestorableCupertinoTextFormFieldRow but it is not very readable and it might be confusing because one can think it is an existing widget. Keeping 'Test' in the widget class name makes it clear it is test only.

@bleroux bleroux force-pushed the restorable_CupertinoTextFormFieldRow branch from b68797d to fe048ef Compare March 5, 2024 08:03
@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 5, 2024
@auto-submit auto-submit bot merged commit 67e6cad into flutter:master Mar 5, 2024
@bleroux bleroux deleted the restorable_CupertinoTextFormFieldRow branch March 5, 2024 10:37
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 5, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Mar 5, 2024
flutter/flutter@65cd84b...3b5a2ec

2024-03-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 17a4b66e0729 to 8916bb32b7b8 (1 revision) (flutter/flutter#144615)
2024-03-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from d514a302cdff to 17a4b66e0729 (8 revisions) (flutter/flutter#144607)
2024-03-05 matej.knopp@gmail.com Do not shorten native assets framework names (flutter/flutter#144568)
2024-03-05 leroux_bruno@yahoo.fr Restorable CupertinoTextFormFieldRow (flutter/flutter#144541)
2024-03-05 hans.muller@gmail.com Updated the TextButton image button example artwork (flutter/flutter#144583)
2024-03-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 728aa4ef05d4 to d514a302cdff (2 revisions) (flutter/flutter#144581)
2024-03-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 62cf93be60b2 to 728aa4ef05d4 (3 revisions) (flutter/flutter#144578)
2024-03-05 jonahwilliams@google.com Disable super flakey impeller test. (flutter/flutter#144573)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 40018811ec0e to 62cf93be60b2 (1 revision) (flutter/flutter#144572)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0d8588b1de7a to 40018811ec0e (6 revisions) (flutter/flutter#144570)
2024-03-04 magder@google.com Print warning and exit when iOS device is unpaired (flutter/flutter#144551)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from a31209171949 to 0d8588b1de7a (1 revision) (flutter/flutter#144558)
2024-03-04 31859944+LongCatIsLooong@users.noreply.github.com Remove unnecessary (and the only) `RenderObject.markParentNeedsLayout` override (flutter/flutter#144466)
2024-03-04 leroux_bruno@yahoo.fr Fix text color for default CupertinoContextMenuAction (flutter/flutter#144542)
2024-03-04 nate.w5687@gmail.com Add missing parameter to `TableBorder.symmetric`, and improve class constructors (flutter/flutter#144279)
2024-03-04 matej.knopp@gmail.com Fix build mode not propagated in android native asset build (flutter/flutter#144550)
2024-03-04 engine-flutter-autoroll@skia.org Roll Packages from a9c68b8 to 0625827 (3 revisions) (flutter/flutter#144556)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from afd2d9f9421d to a31209171949 (1 revision) (flutter/flutter#144554)
2024-03-04 36861262+QuncCccccc@users.noreply.github.com Doc fix for `DropdownButtonFormField.value` (flutter/flutter#144427)
2024-03-04 pateltirth454@gmail.com Fix Small Typo in Skia_Client Doc Comment (flutter/flutter#144490)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2338a557e018 to afd2d9f9421d (1 revision) (flutter/flutter#144549)
2024-03-04 737941+loic-sharma@users.noreply.github.com [Windows] Update keyboard modifiers link (flutter/flutter#144426)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9cbca80e2196 to 2338a557e018 (2 revisions) (flutter/flutter#144545)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from c52b3033eca6 to 9cbca80e2196 (1 revision) (flutter/flutter#144523)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 024ea680ceed to c52b3033eca6 (1 revision) (flutter/flutter#144522)
2024-03-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2706c732b22a to 024ea680ceed (1 revision) (flutter/flutter#144519)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
LouiseHsu pushed a commit to LouiseHsu/packages that referenced this pull request Mar 7, 2024
flutter/flutter@65cd84b...3b5a2ec

2024-03-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 17a4b66e0729 to 8916bb32b7b8 (1 revision) (flutter/flutter#144615)
2024-03-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from d514a302cdff to 17a4b66e0729 (8 revisions) (flutter/flutter#144607)
2024-03-05 matej.knopp@gmail.com Do not shorten native assets framework names (flutter/flutter#144568)
2024-03-05 leroux_bruno@yahoo.fr Restorable CupertinoTextFormFieldRow (flutter/flutter#144541)
2024-03-05 hans.muller@gmail.com Updated the TextButton image button example artwork (flutter/flutter#144583)
2024-03-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 728aa4ef05d4 to d514a302cdff (2 revisions) (flutter/flutter#144581)
2024-03-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 62cf93be60b2 to 728aa4ef05d4 (3 revisions) (flutter/flutter#144578)
2024-03-05 jonahwilliams@google.com Disable super flakey impeller test. (flutter/flutter#144573)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 40018811ec0e to 62cf93be60b2 (1 revision) (flutter/flutter#144572)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0d8588b1de7a to 40018811ec0e (6 revisions) (flutter/flutter#144570)
2024-03-04 magder@google.com Print warning and exit when iOS device is unpaired (flutter/flutter#144551)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from a31209171949 to 0d8588b1de7a (1 revision) (flutter/flutter#144558)
2024-03-04 31859944+LongCatIsLooong@users.noreply.github.com Remove unnecessary (and the only) `RenderObject.markParentNeedsLayout` override (flutter/flutter#144466)
2024-03-04 leroux_bruno@yahoo.fr Fix text color for default CupertinoContextMenuAction (flutter/flutter#144542)
2024-03-04 nate.w5687@gmail.com Add missing parameter to `TableBorder.symmetric`, and improve class constructors (flutter/flutter#144279)
2024-03-04 matej.knopp@gmail.com Fix build mode not propagated in android native asset build (flutter/flutter#144550)
2024-03-04 engine-flutter-autoroll@skia.org Roll Packages from a9c68b8 to 0625827 (3 revisions) (flutter/flutter#144556)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from afd2d9f9421d to a31209171949 (1 revision) (flutter/flutter#144554)
2024-03-04 36861262+QuncCccccc@users.noreply.github.com Doc fix for `DropdownButtonFormField.value` (flutter/flutter#144427)
2024-03-04 pateltirth454@gmail.com Fix Small Typo in Skia_Client Doc Comment (flutter/flutter#144490)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2338a557e018 to afd2d9f9421d (1 revision) (flutter/flutter#144549)
2024-03-04 737941+loic-sharma@users.noreply.github.com [Windows] Update keyboard modifiers link (flutter/flutter#144426)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9cbca80e2196 to 2338a557e018 (2 revisions) (flutter/flutter#144545)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from c52b3033eca6 to 9cbca80e2196 (1 revision) (flutter/flutter#144523)
2024-03-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 024ea680ceed to c52b3033eca6 (1 revision) (flutter/flutter#144522)
2024-03-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2706c732b22a to 024ea680ceed (1 revision) (flutter/flutter#144519)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CupertinoTextFormFieldRow is missing restorationId parameter

2 participants