Refactor reorderable list semantics #123263

chunhtai · 2023-03-22T18:54:11Z

fixes #71396

and move semantics to sliverReoderableList

all i18n related change is in first commit.

reorderable list related change is in second commit

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

chunhtai · 2023-03-23T21:04:26Z

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

I make this required because this has to be the same as the scrollview, so It feels weird to me that we let people omit this property.

It's a bit surprising that you have to tell the sliver in which direction it scrolls (and keep that in sync manually with e.g. the ScrollView). I don't think we do this for any other sliver. Can this sliver not find out the scroll direction by e.g. looking up Scrollable.of?

cc @Piinks

This should not be necessary, the scroll direction is provided to the sliver during layout in the SliverConstraints.

Actually, it looks like the scroll direction is already known, there is a _scrollDirection available in the widgets/SliverReorderableListState

Rendering layer may be too late to apply the semantics actions to children.

Right right, SliverReorderableList does not actually have its own rendering object anyways, my mistake. Does the _scrollDirection that is already available from the state work?

@Piinks Do you think using Scrollable.maybeOf would be a good solution?

oh yeah, I missed it, it looks like it was using scrollable.of. I will just use that.

chunhtai · 2023-03-23T22:29:47Z

I have separated out i18n and reorderablelist changes into separate commit, hopefully this makes it easier to review

chunhtai · 2023-03-27T22:55:03Z

a friendly bump

goderbauer

The first commit is super-hard to review. Maybe separate hand-crafted changes to the tool and automatically generated changes into two separate commits.

Also, do the changes to the tool have to happen in the same PR that modifies the semantics of the reorderable list? Should/could those land separately?

Last, but not least, please get someone more familiar with dev/tools/localization and flutter_localizations to review those bits.

goderbauer · 2023-03-27T23:39:36Z

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

It's a bit surprising that you have to tell the sliver in which direction it scrolls (and keep that in sync manually with e.g. the ScrollView). I don't think we do this for any other sliver. Can this sliver not find out the scroll direction by e.g. looking up Scrollable.of?

cc @Piinks

goderbauer · 2023-03-27T23:41:06Z

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

Is this method a straight copy from reorderable_list.dart?

goderbauer · 2023-03-27T23:45:37Z

dev/tools/localization/bin/gen_localizations.dart

Should this be widgetsLocaleToResources? Maybe add a test that would have failed for this case

goderbauer · 2023-03-27T23:46:20Z

dev/tools/localization/bin/gen_localizations.dart

wondering: why are they not all const?

this bool decide whether to add const keyword when calling factory. Since material/cupertino pass in parameter to when calling factory, they can't use the const. Will update the name to be more precise

Piinks · 2023-03-28T16:53:07Z

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

What if this conflicts with the scroll direction provided by the constraints during layout?

Piinks · 2023-03-28T17:11:58Z

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

Suggested change

if (widget.scrollDirection == Axis.horizontal) {

if (_scrollDirection == Axis.horizontal) {

Maybe?

chunhtai · 2023-03-28T17:35:24Z

turning this into draft until https://github.com/flutter/flutter/pull/123620/files is merged

Piinks · 2023-04-06T16:56:33Z

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


-  /// The semantics label used for [ReorderableListView] to reorder an item in the
-  /// list to the start of the list.
-  String get reorderItemToStart;


Will removing these public getters be a breaking change?

If someone is using it, then yes. It is hard to imagine how people will reuse this though.

I am not sure whether people would extend this class. @thkim1011, do you know if there is use case for that?

I'm not familiar with any use cases regarding extending the class, but I assume anyone can get the localizations and use those strings in their project? I'm not too sure how we've handled deprecating strings in the past.

This is technically not a breaking change given all tests pass. The breaking change guide states it would be optional whether we announce this as a courtesy. I personally think it is very unlikely this would be a breaking change. WDYT?

If no tests broke that sounds fine to me. I would probably do a global test run in Google before this lands though to make sure.

Piinks

goderbauer

LGTM!

This reverts commit 94327e7.

borjandev · 2023-04-07T00:32:04Z

@Piinks @chunhtai This breaks https://pub.dev/packages/reorderables

Could not build the precompiled application for the device.
Error (Xcode): ../../.pub-cache/hosted/pub.dev/reorderables-0.6.0/lib/src/widgets/reorderable_wrap.dart:737:34: Error: The getter 'reorderItemToStart' isn't defined for the class 'MaterialLocalizations'.

However, maybe the resolution is for the package maintainer, I believe @hanshengchiu to update the package instead

chunhtai · 2023-04-07T16:30:16Z

I will prepare a migration guide and dart fix, it looks like it is more widely used than I think

Piinks · 2023-04-07T16:42:46Z

Thanks @chunhtai !

@borjandev unfortunately any change could potentially break a package on pub.dev
We cannot account for all of them, but we do have several signals we use to indicate breaking changes to us.

Any contributor can add tests to flutter/tests, and if a change breaks them, we won't make the change without working with the contributor to ensure the package is not broken. Maybe https://pub.dev/packages/reorderables would like to contribute tests.

We also look at how our own tests are affected by a change, and how customer tests inside of Google are affected.

In this case, our signals worked and the change was reverted after we found Google tests were broken by the change.

To learn more about our breaking change policy and how we determine breaking changes, you can read the breaking change section in the wiki.

Refactor reorderable list semantics

flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Mar 22, 2023

chunhtai force-pushed the issues/71396 branch from 66ca1f9 to fb53ef4 Compare March 23, 2023 20:09

flutter-dashboard bot added a: internationalization Supporting other languages or locales. (aka i18n) c: contributor-productivity Team-specific productivity, code health, technical debt. labels Mar 23, 2023

chunhtai marked this pull request as ready for review March 23, 2023 20:17

chunhtai force-pushed the issues/71396 branch 2 times, most recently from 7cd4d89 to a18805d Compare March 23, 2023 20:55

chunhtai commented Mar 23, 2023

View reviewed changes

chunhtai force-pushed the issues/71396 branch from a18805d to 481772c Compare March 23, 2023 21:12

chunhtai requested review from goderbauer and thkim1011 March 23, 2023 21:18

goderbauer reviewed Mar 27, 2023

View reviewed changes

Piinks reviewed Mar 28, 2023

View reviewed changes

chunhtai mentioned this pull request Mar 28, 2023

Adds i18n to widget layer #123620

Merged

8 tasks

Piinks reviewed Mar 28, 2023

View reviewed changes

chunhtai marked this pull request as draft March 28, 2023 17:35

chunhtai force-pushed the issues/71396 branch 2 times, most recently from 0ecd0a7 to eab3c1a Compare April 5, 2023 20:11

remove reorder-related localization string from material

70de0ad

chunhtai force-pushed the issues/71396 branch from eab3c1a to 0113f78 Compare April 5, 2023 20:12

chunhtai marked this pull request as ready for review April 5, 2023 20:12

Fixes reoderable semantics and move to widget layer

d7f783d

chunhtai force-pushed the issues/71396 branch from 0113f78 to d7f783d Compare April 5, 2023 20:43

Piinks reviewed Apr 6, 2023

View reviewed changes

chunhtai requested a review from goderbauer April 6, 2023 20:26

chunhtai requested a review from Piinks April 6, 2023 20:26

Piinks approved these changes Apr 6, 2023

View reviewed changes

goderbauer approved these changes Apr 6, 2023

View reviewed changes

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 6, 2023

auto-submit bot merged commit 94327e7 into flutter:master Apr 6, 2023

XilaiZhang added a commit that referenced this pull request Apr 6, 2023

Revert "Refactor reorderable list semantics (#123263)"

f4681f8

This reverts commit 94327e7.

XilaiZhang mentioned this pull request Apr 7, 2023

Revert "Refactor reorderable list semantics" #124368

Merged

engine-flutter-autoroll mentioned this pull request Apr 7, 2023

Roll Flutter from a086c640dcc4 to aa6a0a9dea63 (25 revisions) flutter/packages#3661

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 7, 2023

94327e7 Refactor reorderable list semantics (flutter/flutter#123263)

d57e865

chunhtai added revert Autorevert PR (with "Reason for revert:" comment) and removed revert Autorevert PR (with "Reason for revert:" comment) labels Apr 7, 2023

HansMuller mentioned this pull request Apr 7, 2023

Flutter Framework Team: Notable Commits #121415

Closed

exaby73 pushed a commit to NevercodeHQ/flutter that referenced this pull request Apr 17, 2023

Refactor reorderable list semantics (flutter#123263)

8f45d98

Refactor reorderable list semantics

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023

94327e7 Refactor reorderable list semantics (flutter/flutter#123263)

bdc8b2d

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023

94327e7 Refactor reorderable list semantics (flutter/flutter#123263)

13c7c36

	if (widget.scrollDirection == Axis.horizontal) {
	if (_scrollDirection == Axis.horizontal) {

Refactor reorderable list semantics #123263

Refactor reorderable list semantics #123263

Uh oh!

Conversation

chunhtai commented Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

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!

chunhtai commented Mar 23, 2023

Uh oh!

chunhtai commented Mar 27, 2023

Uh oh!

goderbauer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

chunhtai commented Mar 28, 2023

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!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

borjandev commented Apr 7, 2023

Uh oh!

chunhtai commented Apr 7, 2023

Uh oh!

Piinks commented Apr 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chunhtai commented Mar 22, 2023 •

edited

Loading

goderbauer left a comment •

edited

Loading