Skip to content

Conversation

@nate-thegrate
Copy link
Contributor

@nate-thegrate nate-thegrate commented Jul 15, 2024

This is a small change, but it still might be difficult to review.

Related:


Motivation

Dependency conflict

Currently, package:flutter/painting.dart depends on package:flutter/gestures.dart, so anything within the gestures library cannot use something declared in painting (since that would create a dependency loop).

This resulted in a less-than-ideal situation: the DragGestureRecognizer needs to use an Axis value for its primaryDragAxis field and its getSumDelta() method, but it isn't allowed to import the Axis enum.

So instead, the enum was copied into the file and given a private name. All methods that use this enum have to be private as well: if someone needs a version of PanGestureRecognizer with a slight change to the distance required to accept the gesture, they need to copy the entire DragGestureRecognizer interface and re-implement each abstract method before tweaking to the desired behavior.

Enum enhancements

Axis and AxisDirection were given a few helper methods before enhanced enum features were added to Dart. This provides an opportunity for a satisfying refactor:

// before
if (axisDirectionToAxis(intent.direction) == axisDirectionToAxis(state.direction)) {
  // ...
}


// after
if (intent.direction.axis == state.direction.axis) {
  // ...
}
// before
Flex(
  direction: flipAxis(axisDirectionToAxis(direction)),
  // ...
);


// after
Flex(
  direction: direction.axis.flipped,
  // ...
);

It's much easier for me to understand the process of "convert the AxisDirection to an Axis, and then flip it" when it isn't a nested function call that's read from right-to-left.


Summary of changes

Migration

Since painting/basic_types.dart is already exporting a type from the foundation package, the migration was more-or-less trivial: enums in that file were moved into foundation/basic_types.dart.

Deprecation

Axis and AxisDirection were given a .flipped getter that corresponds to flipAxis() and flipAxisDirection() respectively.

AxisDirection also has .axis and .isReversed that corresponds to axisDirectionToAxis() & axisDirectionIsReversed().


I also added @Deprecated() to the functions being replaced; unfortunately at this point we can't implement a data-driven fix for a "function → getter" migration. Perhaps instead of deprecating now, we could do something similar to WidgetStateProperty.all:

// TODO(darrenaustin): Deprecate this when we have the ability to create
// a dart fix that will replace this with WidgetStatePropertyAll:
// https://github.com/dart-lang/sdk/issues/49056.
static WidgetStateProperty<T> all<T>(T value) => WidgetStatePropertyAll<T>(value);

@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: scrolling Viewports, list views, slivers, etc. labels Jul 15, 2024
@nate-thegrate nate-thegrate marked this pull request as ready for review July 15, 2024 17:33
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Holding off on a review here until we can determine is it is necessary in #151627. It does not look like all of the methods being exposed are necessary (based on the test), so we might not need to make this change.

@nate-thegrate nate-thegrate marked this pull request as draft July 15, 2024 21:55
@flutter-dashboard
Copy link

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

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

@nate-thegrate
Copy link
Contributor Author

Closing, since this likely won't end up being necessary for #151627.

(I'd love to have these enum enhancements at some point, but as of right now, it seems like the hassle outweighs the benefit.)

@nate-thegrate nate-thegrate deleted the axis-foundation branch July 17, 2024 18:18
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 f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants