`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 433d360eee11 to 44278941443e (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 [email protected] Use `ErrorHandlingFileSystem.deleteIfExists` when deleting .plugin_symlinks (flutter/flutter#151073) 2024-07-02 [email protected] ScrollEndNotification example: auto-scroll based on RenderSliver constraints and geometry (flutter/flutter#143538) 2024-07-02 [email protected] Roll Packages from 412ec46 to d2705fb (13 revisions) (flutter/flutter#151169) 2024-07-02 [email protected] docimports for painting (flutter/flutter#151143) 2024-07-02 [email protected] docimports for scheduler (flutter/flutter#151126) 2024-07-02 [email protected] `dismissible.dart` code cleanup (flutter/flutter#150276) 2024-07-02 [email protected] docimports for physics (flutter/flutter#151125) 2024-07-02 [email protected] docimports for services (flutter/flutter#151134) 2024-07-02 [email protected] docimports for cupertino (flutter/flutter#151149) 2024-07-02 [email protected] docimports for gestures (flutter/flutter#151123) 2024-07-02 [email protected] Docimports for foundation (flutter/flutter#151119) 2024-07-02 [email protected] docimports for semantics (flutter/flutter#151132) 2024-07-02 [email protected] [flutter_driver] add allocator mtl to memory event allowlist. (flutter/flutter#151153) 2024-07-02 [email protected] Roll Flutter Engine from 40c087b31515 to 433d360eee11 (7 revisions) (flutter/flutter#151165) 2024-07-02 [email protected] Refactor BuildInfo to always require packageConfigPath (flutter/flutter#150559) 2024-07-02 [email protected] Roll Flutter Engine from d3c5bd66a78f to 40c087b31515 (1 revision) (flutter/flutter#151156) 2024-07-02 [email protected] Roll Flutter Engine from fc5bc14e6091 to d3c5bd66a78f (1 revision) (flutter/flutter#151155) 2024-07-02 [email protected] Fix: `CupertinoActionSheet` should take up max height when actions section is short (flutter/flutter#150708) 2024-07-02 [email protected] Roll Flutter Engine from 3456fee1a6b9 to fc5bc14e6091 (8 revisions) (flutter/flutter#151150) 2024-07-02 [email protected] [tool] remove some temporary printTrace calls (flutter/flutter#151074) 2024-07-01 [email protected] 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 [email protected] Upgrade template Gradle, App AGP, Module AGP, and Kotlin versions, and tests (flutter/flutter#150969) 2024-07-01 [email protected] Roll Flutter Engine from b57a044ed10f to 3456fee1a6b9 (5 revisions) (flutter/flutter#151127) 2024-07-01 [email protected] Read AndroidManifest.xml and emit manifest-aar-impeller-(enabled|disabled) analytics (flutter/flutter#150970) 2024-07-01 [email protected] More docimports for animation library (flutter/flutter#151011) 2024-07-01 [email protected] Bump dartdoc to 8.0.10 (flutter/flutter#151107) 2024-07-01 [email protected] Fix missing `[` in docs (flutter/flutter#151091) 2024-07-01 [email protected] Roll pub packages (flutter/flutter#151028) 2024-07-01 [email protected] 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 [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

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
72 checks passed

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!

Uh oh!

Uh oh!

		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!

Uh oh!

Uh oh!

`dismissible.dart` code cleanup #150276

`dismissible.dart` code cleanup #150276

nate-thegrate commented Jun 14, 2024 •

edited

Loading