`dismissible.dart` code cleanup #150276

nate-thegrate · 2024-06-14T18:59:20Z

While I was working on #149966, I noticed a couple of ways that _DismissibleState could be tweaked to be easier to follow.

Changes are described in comments below.

flutter-dashboard · 2024-06-14T18:59:23Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

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

nate-thegrate · 2024-06-14T19:13:38Z

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

-    if (!_isActive || _moveController.isAnimating) {
+    if (!_dragUnderway || _moveController.isAnimating) {


The _isActive getter is only used in the context of (!_isActive || _moveController.isAnimating), which expands to:

(!(_dragUnderway || _moveController.isAnimating) || _moveController.isAnimating)

I made a truth table to try to understand what was going on. Hopefully now it won't be necessary 🙂

nate-thegrate · 2024-06-14T19:15:01Z

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

-  bool get _isActive {
-    return _dragUnderway || _moveController.isAnimating;
-  }
+  double get _dismissThreshold => widget.dismissThresholds[_dismissDirection] ?? _kDismissThreshold;


I added a _dismissThreshold getter, since widget.dismissThresholds[_dismissDirection] ?? _kDismissThreshold was used in 4 different spots.

gnprice

LGTM. Thanks for the simplifications!

I was reading this same code last week, as it happens, and also found myself having to puzzle out what that !_isActive || _moveController!.isAnimating condition ends up boiling down to.

gnprice · 2024-06-26T18:45:17Z

I was curious why _isActive is written that way, when it seems redundant given how it's used. From git log -p -G isActive --follow packages/flutter/lib/src/widgets/dismissible.dart, it looks like the story is that it used to have a more complicated definition, and also to be used in more contexts; then commit a78d2c9 (from pre-GitHub, in 2016) made some simplifications which opened the way for this one.

nate-thegrate · 2024-06-26T18:56:44Z

I was curious about that as well—good to know!

(I should probably brush up on the various git command options 🙂)

gnprice · 2024-06-26T19:00:04Z

Indeed, there's a lot of useful stuff there. Here's my own short writeup of tips for git log and reading Git history:
https://github.com/zulip/zulip-mobile/blob/e352f563e/docs/howto/git.md#reading-history

stuartmorgan-g · 2024-07-02T12:59:39Z

test-exempt: code refactor with no semantic change

* master: (88 commits) Fix scheduler event loop being stuck due to task with Priority.idle (flutter#151168) Fix result propagation in RenderSliverEdgeInsetsPadding.hitTestChildren (flutter#149825) docImports for flutter_test (flutter#151189) Interactable ScrollView content when settling a scroll activity (flutter#145848) [flutter_tools] Update the mapping for the Dart SDK internal URI (flutter#151170) Roll pub packages (flutter#151129) Fix typo (flutter#151192) [tool] Fix `stdin.flush` calls on processes started by `FakeProcessManager` (flutter#151183) Roll Flutter Engine from 433d360 to 4427894 (4 revisions) (flutter#151186) Use `ErrorHandlingFileSystem.deleteIfExists` when deleting .plugin_symlinks (flutter#151073) ScrollEndNotification example: auto-scroll based on RenderSliver constraints and geometry (flutter#143538) Roll Packages from 412ec46 to d2705fb (13 revisions) (flutter#151169) docimports for painting (flutter#151143) docimports for scheduler (flutter#151126) `dismissible.dart` code cleanup (flutter#150276) docimports for physics (flutter#151125) docimports for services (flutter#151134) docimports for cupertino (flutter#151149) docimports for gestures (flutter#151123) Docimports for foundation (flutter#151119) ...

flutter/flutter@99bb2ff...af913a7 2024-07-02 andrewrkolos@gmail.com Use `ErrorHandlingFileSystem.deleteIfExists` when deleting .plugin_symlinks (flutter/flutter#151073) 2024-07-02 hans.muller@gmail.com ScrollEndNotification example: auto-scroll based on RenderSliver constraints and geometry (flutter/flutter#143538) 2024-07-02 engine-flutter-autoroll@skia.org Roll Packages from 412ec46 to d2705fb (13 revisions) (flutter/flutter#151169) 2024-07-02 goderbauer@google.com docimports for painting (flutter/flutter#151143) 2024-07-02 goderbauer@google.com docimports for scheduler (flutter/flutter#151126) 2024-07-02 nate.w5687@gmail.com `dismissible.dart` code cleanup (flutter/flutter#150276) 2024-07-02 goderbauer@google.com docimports for physics (flutter/flutter#151125) 2024-07-02 goderbauer@google.com docimports for services (flutter/flutter#151134) 2024-07-02 goderbauer@google.com docimports for cupertino (flutter/flutter#151149) 2024-07-02 goderbauer@google.com docimports for gestures (flutter/flutter#151123) 2024-07-02 goderbauer@google.com Docimports for foundation (flutter/flutter#151119) 2024-07-02 goderbauer@google.com docimports for semantics (flutter/flutter#151132) 2024-07-02 jonahwilliams@google.com [flutter_driver] add allocator mtl to memory event allowlist. (flutter/flutter#151153) 2024-07-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from 40c087b31515 to 433d360eee11 (7 revisions) (flutter/flutter#151165) 2024-07-02 sigurdm@google.com Refactor BuildInfo to always require packageConfigPath (flutter/flutter#150559) 2024-07-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from d3c5bd66a78f to 40c087b31515 (1 revision) (flutter/flutter#151156) 2024-07-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from fc5bc14e6091 to d3c5bd66a78f (1 revision) (flutter/flutter#151155) 2024-07-02 dkwingsmt@users.noreply.github.com Fix: `CupertinoActionSheet` should take up max height when actions section is short (flutter/flutter#150708) 2024-07-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3456fee1a6b9 to fc5bc14e6091 (8 revisions) (flutter/flutter#151150) 2024-07-02 andrewrkolos@gmail.com [tool] remove some temporary printTrace calls (flutter/flutter#151074) 2024-07-01 nate.w5687@gmail.com Implementing a few switch statements (flutter/flutter#150946) 2024-07-01 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Upgrade template Gradle, App AGP, Module AGP, and Kotlin versions, and tests (#150969)" (flutter/flutter#151147) 2024-07-01 34871572+gmackall@users.noreply.github.com Upgrade template Gradle, App AGP, Module AGP, and Kotlin versions, and tests (flutter/flutter#150969) 2024-07-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from b57a044ed10f to 3456fee1a6b9 (5 revisions) (flutter/flutter#151127) 2024-07-01 codefu@google.com Read AndroidManifest.xml and emit manifest-aar-impeller-(enabled|disabled) analytics (flutter/flutter#150970) 2024-07-01 goderbauer@google.com More docimports for animation library (flutter/flutter#151011) 2024-07-01 goderbauer@google.com Bump dartdoc to 8.0.10 (flutter/flutter#151107) 2024-07-01 matthiasngeozhongsim@yahoo.com Fix missing `[` in docs (flutter/flutter#151091) 2024-07-01 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#151028) 2024-07-01 jason-simmons@users.noreply.github.com Fix teardown of a FocusScopeNode in material/app_test (flutter/flutter#151115) 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 camillesimon@google.com,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

While I was working on flutter#149966, I noticed a couple of ways that `_DismissibleState` could be tweaked to be easier to follow. Changes are described in comments below, and we're currently [awaiting a test exemption](https://discord.com/channels/608014603317936148/608018585025118217/1251255349277626490).

github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Jun 14, 2024

nate-thegrate commented Jun 14, 2024

View reviewed changes

nate-thegrate added 2 commits June 17, 2024 10:03

non-nullable _moveController

11c3fbc

swap _isActive for a more useful getter

eb6937e

nate-thegrate force-pushed the dismissible-refactor branch from 99fad3d to eb6937e Compare June 17, 2024 16:04

gnprice approved these changes Jun 26, 2024

View reviewed changes

gnprice added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 2, 2024

auto-submit bot merged commit c9ec61e into flutter:master Jul 2, 2024

engine-flutter-autoroll mentioned this pull request Jul 2, 2024

Roll Flutter from 99bb2ff6a614 to 12e3635e8e27 (27 revisions) flutter/packages#7045

Closed

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

c9ec61e dismissible.dart code cleanup (flutter/flutter#150276)

b5c68c9

nate-thegrate deleted the dismissible-refactor branch July 2, 2024 17:50

engine-flutter-autoroll mentioned this pull request Jul 2, 2024

Roll Flutter from 99bb2ff6a614 to 9527259c705c (28 revisions) flutter/packages#7046

Closed

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

c9ec61e dismissible.dart code cleanup (flutter/flutter#150276)

5acc0aa

engine-flutter-autoroll mentioned this pull request Jul 2, 2024

Roll Flutter from 99bb2ff6a614 to af913a75b5d2 (30 revisions) flutter/packages#7047

Merged

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

c9ec61e dismissible.dart code cleanup (flutter/flutter#150276)

75150e9

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

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

c9ec61e dismissible.dart code cleanup (flutter/flutter#150276)

b5e52f9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

`dismissible.dart` code cleanup #150276

`dismissible.dart` code cleanup #150276

Uh oh!

nate-thegrate commented Jun 14, 2024 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Jun 14, 2024

Uh oh!

Uh oh!

nate-thegrate Jun 14, 2024

Uh oh!

nate-thegrate Jun 14, 2024

Uh oh!

gnprice left a comment

Uh oh!

gnprice commented Jun 26, 2024

Uh oh!

nate-thegrate commented Jun 26, 2024

Uh oh!

gnprice commented Jun 26, 2024

Uh oh!

stuartmorgan-g commented Jul 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		if (!_isActive \|\| _moveController.isAnimating) {
		if (!_dragUnderway \|\| _moveController.isAnimating) {

dismissible.dart code cleanup #150276

dismissible.dart code cleanup #150276

Uh oh!

Conversation

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

Uh oh!

Uh oh!

flutter-dashboard bot commented Jun 14, 2024

Uh oh!

Uh oh!

nate-thegrate Jun 14, 2024

Choose a reason for hiding this comment

Uh oh!

nate-thegrate Jun 14, 2024

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice commented Jun 26, 2024

Uh oh!

nate-thegrate commented Jun 26, 2024

Uh oh!

gnprice commented Jun 26, 2024

Uh oh!

stuartmorgan-g commented Jul 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

`dismissible.dart` code cleanup #150276

`dismissible.dart` code cleanup #150276

nate-thegrate commented Jun 14, 2024 •

edited

Loading