Implementing control flow collections #146601

nate-thegrate · 2024-04-10T20:27:33Z

This pull request aims for improved readability, based on issue #146600.

// before
Set<Color> _distinctVisibleColors() {
  final Set<Color> distinctVisibleColors = <Color>{};
  if (top.style != BorderStyle.none) {
    distinctVisibleColors.add(top.color);
  }
  if (right.style != BorderStyle.none) {
    distinctVisibleColors.add(right.color);
  }
  if (bottom.style != BorderStyle.none) {
    distinctVisibleColors.add(bottom.color);
  }
  if (left.style != BorderStyle.none) {
    distinctVisibleColors.add(left.color);
  }
  return distinctVisibleColors;
}


// after
Set<Color> _distinctVisibleColors() {
  return <Color>{
    if (top.style != BorderStyle.none) top.color,
    if (right.style != BorderStyle.none) right.color,
    if (bottom.style != BorderStyle.none) bottom.color,
    if (left.style != BorderStyle.none) left.color,
  };
}

Most of the repo should be covered in this PR (aside from flutter_tools/, since there was a lot going on in there).

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.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

goderbauer

LGTM

Thank you!

Piinks

auto-submit · 2024-04-12T23:10:20Z

auto label is removed for flutter/flutter/146601, due to Pull request flutter/flutter/146601 is not in a mergeable state.

justinmc

LGTM 👍 Thanks for making the codebase more readable as always.

justinmc · 2024-04-12T23:13:08Z

dev/bots/test.dart

  final List<String> allExpectedFiles = binariesWithEntitlements(flutterRoot) + binariesWithoutEntitlements(flutterRoot);
+  final Set<String> foundFiles = <String>{
+    for (final String binaryPath in binaryPaths)
+      if (allExpectedFiles.contains(binaryPath)) binaryPath


I had to double check the styleguide but looks like this is right. It should be on one line like this (as long as it fits in the line length). https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#prefer-single-line-for-short-collection-if-and-collection-for

justinmc · 2024-04-12T23:13:56Z

dev/devicelab/lib/tasks/microbenchmarks.dart

            device.deviceId,
+            benchmarkPath,
          ];
-          options.add(benchmarkPath);


That was a weird one...

justinmc · 2024-04-12T23:15:08Z

dev/integration_tests/android_semantics_testing/lib/src/common.dart

-    return convertedActions;
+    return <AndroidSemanticsAction>[
+      for (final int id in actions)
+        if (AndroidSemanticsAction.deserialize(id) case final AndroidSemanticsAction action)


I didn't know about this case syntax.

rakudrama · 2024-04-13T14:42:00Z

packages/flutter/lib/src/foundation/timeline.dart

+    return <String?>[
+      for (final List<String?> slice in _chain) ...slice,
+      ..._slice.sublist(0, _pointer),
+    ].cast<String>();


Avoid .cast.
The result checks the value on every read and write, rather than once in constructing the list.
This kind of inefficiency should be avoided in the foundation.

Instead, nest a for-in in the control flow collection to make it equivalent to the original code.

I had no idea that .cast worked that way, thank you!

rakudrama · 2024-04-13T14:48:25Z

packages/flutter/lib/src/foundation/timeline.dart

-    return result;
+    return <String?>[
+      for (final List<String?> slice in _chain) ...slice,
+      ..._slice.sublist(0, _pointer),


..._slice.sublist(...) is creating an extra temporary list.
Presumably the original code avoided result.addAll(_slice.sublist(0, _pointer)) for some reason.

I would make this more like the original, i.e.
for (int i = 0; i < _pointer; i++) _slice[i]!,

nate-thegrate · 2024-04-15T15:56:50Z

Thank you all very much!

It looks like a merge conflict popped up and autosubmit was removed, so if a kind soul is able to re-add it, that would be much appreciated.

Manual roll requested by [email protected] flutter/flutter@53cba24...2e748e8 2024-04-15 [email protected] Implementing control flow collections (flutter/flutter#146601) 2024-04-15 [email protected] Roll Packages from 78f684c to 6698b2d (3 revisions) (flutter/flutter#146761) 2024-04-15 [email protected] test: Fix memory leak in transitions test (flutter/flutter#146747) 2024-04-15 [email protected] Fix filled text field active indicator overflows container bounds (flutter/flutter#146637) 2024-04-14 [email protected] - Fixes _DropdownMenuState leaking text controller (flutter/flutter#146571) 2024-04-13 [email protected] Fix memory leaks in `FloatingActionButton` (flutter/flutter#146711) 2024-04-12 [email protected] Roll Flutter Engine from 0e56e3dffe43 to 1a13c7d1f40e (2 revisions) (flutter/flutter#146703) 2024-04-12 [email protected] Roll pub packages (flutter/flutter#146704) 2024-04-12 49699333+dependabot[bot]@users.noreply.github.com Bump peter-evans/create-pull-request from 6.0.2 to 6.0.3 (flutter/flutter#146702) 2024-04-12 [email protected] Avoid forwarding the data after socket is disconnected. (flutter/flutter#146665) 2024-04-12 [email protected] [flutter_tools] Fix conductor for package args roll (flutter/flutter#146646) 2024-04-12 [email protected] Roll Flutter Engine from 6b37b170998e to 0e56e3dffe43 (4 revisions) (flutter/flutter#146698) 2024-04-12 [email protected] Light sliver clean up before SliverTree (flutter/flutter#146696) 2024-04-12 [email protected] Fix label text color is wrong for a focused and hovered TextField (flutter/flutter#146572) 2024-04-12 [email protected] Fix `getOffsetForCaret` crash (flutter/flutter#146669) 2024-04-12 [email protected] Update gen_keycodes templates (flutter/flutter#146481) 2024-04-12 [email protected] Support `flutter run --wasm` and `flutter drive --wasm`. (flutter/flutter#146231) 2024-04-12 [email protected] Fix curved animation memory leak for scrollbar (flutter/flutter#146670) 2024-04-12 [email protected] Roll Packages from e98839a to 78f684c (6 revisions) (flutter/flutter#146691) 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 [email protected],[email protected],[email protected] 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

This pull request aims for improved readability, based on issue flutter#146600. ```dart // before Set<Color> _distinctVisibleColors() { final Set<Color> distinctVisibleColors = <Color>{}; if (top.style != BorderStyle.none) { distinctVisibleColors.add(top.color); } if (right.style != BorderStyle.none) { distinctVisibleColors.add(right.color); } if (bottom.style != BorderStyle.none) { distinctVisibleColors.add(bottom.color); } if (left.style != BorderStyle.none) { distinctVisibleColors.add(left.color); } return distinctVisibleColors; } // after Set<Color> _distinctVisibleColors() { return <Color>{ if (top.style != BorderStyle.none) top.color, if (right.style != BorderStyle.none) right.color, if (bottom.style != BorderStyle.none) bottom.color, if (left.style != BorderStyle.none) left.color, }; } ``` Most of the repo should be covered in this PR (aside from `flutter_tools/`, since there was a lot going on in there).

) Manual roll requested by [email protected] flutter/flutter@53cba24...2e748e8 2024-04-15 [email protected] Implementing control flow collections (flutter/flutter#146601) 2024-04-15 [email protected] Roll Packages from 78f684c to 6698b2d (3 revisions) (flutter/flutter#146761) 2024-04-15 [email protected] test: Fix memory leak in transitions test (flutter/flutter#146747) 2024-04-15 [email protected] Fix filled text field active indicator overflows container bounds (flutter/flutter#146637) 2024-04-14 [email protected] - Fixes _DropdownMenuState leaking text controller (flutter/flutter#146571) 2024-04-13 [email protected] Fix memory leaks in `FloatingActionButton` (flutter/flutter#146711) 2024-04-12 [email protected] Roll Flutter Engine from 0e56e3dffe43 to 1a13c7d1f40e (2 revisions) (flutter/flutter#146703) 2024-04-12 [email protected] Roll pub packages (flutter/flutter#146704) 2024-04-12 49699333+dependabot[bot]@users.noreply.github.com Bump peter-evans/create-pull-request from 6.0.2 to 6.0.3 (flutter/flutter#146702) 2024-04-12 [email protected] Avoid forwarding the data after socket is disconnected. (flutter/flutter#146665) 2024-04-12 [email protected] [flutter_tools] Fix conductor for package args roll (flutter/flutter#146646) 2024-04-12 [email protected] Roll Flutter Engine from 6b37b170998e to 0e56e3dffe43 (4 revisions) (flutter/flutter#146698) 2024-04-12 [email protected] Light sliver clean up before SliverTree (flutter/flutter#146696) 2024-04-12 [email protected] Fix label text color is wrong for a focused and hovered TextField (flutter/flutter#146572) 2024-04-12 [email protected] Fix `getOffsetForCaret` crash (flutter/flutter#146669) 2024-04-12 [email protected] Update gen_keycodes templates (flutter/flutter#146481) 2024-04-12 [email protected] Support `flutter run --wasm` and `flutter drive --wasm`. (flutter/flutter#146231) 2024-04-12 [email protected] Fix curved animation memory leak for scrollbar (flutter/flutter#146670) 2024-04-12 [email protected] Roll Packages from e98839a to 78f684c (6 revisions) (flutter/flutter#146691) 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 [email protected],[email protected],[email protected] 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

nate-thegrate added 3 commits April 10, 2024 11:38

clean up a bunch of lists

1740827

revert flutter_tools/ changes

23024de

misc improvements

a0afa71

nate-thegrate added 2 commits April 10, 2024 15:17

minor cleanup

6bfdbcd

revert list_section.dart

6b5ed10

nate-thegrate changed the title ~~implementing Control Flow Collections~~ Implementing control flow collections Apr 10, 2024

nate-thegrate marked this pull request as ready for review April 10, 2024 22:13

goderbauer approved these changes Apr 12, 2024

View reviewed changes

Piinks approved these changes Apr 12, 2024

View reviewed changes

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 12, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 12, 2024

Merge branch 'master' of github.com:flutter/flutter into clean-up-lists

52eabf3

justinmc approved these changes Apr 12, 2024

View reviewed changes

rakudrama reviewed Apr 13, 2024

View reviewed changes

nate-thegrate added 2 commits April 13, 2024 16:59

implement suggestions from @rakudrama

a9350cc

Merge branch 'master' into clean-up-lists

f45a215

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 15, 2024

auto-submit bot merged commit 2e748e8 into flutter:master Apr 15, 2024

nate-thegrate deleted the clean-up-lists branch April 15, 2024 16:07

engine-flutter-autoroll mentioned this pull request Apr 15, 2024

Roll Flutter from 53cba24de3a0 to 2e748e8598b3 (19 revisions) flutter/packages#6539

Closed

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

2e748e8 Implementing control flow collections (flutter/flutter#146601)

b8bb391

engine-flutter-autoroll mentioned this pull request Apr 15, 2024

Manual roll Flutter from 53cba24de3a0 to 2e748e8598b3 (19 revisions) flutter/packages#6541

Merged

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

2e748e8 Implementing control flow collections (flutter/flutter#146601)

ef6c588

engine-flutter-autoroll mentioned this pull request Apr 15, 2024

Roll Flutter from 53cba24de3a0 to 882bcbb9aa44 (20 revisions) flutter/packages#6542

Closed

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

2e748e8 Implementing control flow collections (flutter/flutter#146601)

c2238bd

HansMuller mentioned this pull request Apr 26, 2024

Flutter Framework Team: Notable Commits #121415

Closed

nate-thegrate added the refactor Improving readability/efficiency without behavioral changes label Jul 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implementing control flow collections #146601

Implementing control flow collections #146601

Uh oh!

nate-thegrate commented Apr 10, 2024 •

edited

Loading

Uh oh!

goderbauer left a comment

Uh oh!

Piinks left a comment

Uh oh!

auto-submit bot commented Apr 12, 2024

Uh oh!

justinmc left a comment

Uh oh!

justinmc Apr 12, 2024

Uh oh!

justinmc Apr 12, 2024

Uh oh!

justinmc Apr 12, 2024

Uh oh!

rakudrama Apr 13, 2024

Uh oh!

nate-thegrate Apr 13, 2024

Uh oh!

rakudrama Apr 13, 2024

Uh oh!

nate-thegrate commented Apr 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Implementing control flow collections #146601

Implementing control flow collections #146601

Uh oh!

Conversation

nate-thegrate commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Apr 12, 2024

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc Apr 12, 2024

Choose a reason for hiding this comment

Uh oh!

justinmc Apr 12, 2024

Choose a reason for hiding this comment

Uh oh!

justinmc Apr 12, 2024

Choose a reason for hiding this comment

Uh oh!

rakudrama Apr 13, 2024

Choose a reason for hiding this comment

Uh oh!

nate-thegrate Apr 13, 2024

Choose a reason for hiding this comment

Uh oh!

rakudrama Apr 13, 2024

Choose a reason for hiding this comment

Uh oh!

nate-thegrate commented Apr 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nate-thegrate commented Apr 10, 2024 •

edited

Loading