I think we don't need the defaultValue. The field will be ignored when the default value is applied in the reading which might cause extra drama. What do you think?

For now, DragDownDetails().toString() prints: DragDownDetails#63b8c(localPosition: Offset(0.0, 0.0)) which is a bit abnormal.

Where is the design document motivating this new API?

Based on a Discord conversation, the ideal "design doc" for this change might just be a 1-pager that we discuss on the GitHub issue rather than in a Google Doc.

• Generalize gesture details that concerns cartesian positioning in a common interface #101530

Based on a Discord conversation, the ideal "design doc" for this change might just be a 1-pager that we discuss on the GitHub issue rather than in a Google Doc.

• Generalize gesture details that concerns cartesian positioning in a common interface #101530

This doc isn't very informative as it basically just blows the name of the class up into a sentence, see also https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#avoid-useless-documentation.

See also https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#writing-prompts-for-good-documentation for some prompts on writing better documentation.

Updated.

So, localPosition defaulting to globalPosition was done for backwards-compatibility when we introduced localPosition. This is not a great choice for new GestureDetails and we shouldn't elevate this to be the default.

No new gesture details were added in this PR IMO, only some information was extracted as an abstraction.

Sure, but encoding this in a base class implicitly says all future gesture detail subclasses should follow this model.

There are still some gestures that do not provide position info, they have another mechanism (focal points) which I was planning to abstract in the future. So maybe we will have multiple gesture details base classes like GestureDetailsWithPositions, GestureDetailsWithPoints, etc.

I noticed that a lot of the gesture callback typedefs use details as the parameter name; I'm wondering if it'd make sense for the superclass name to also end with "details" for consistency.

As far as fallback values for compatibility, maybe there could be multiple constructors:

abstract class PositionedGestureDetails with Diagnosticable {
  const PositionedGestureDetails({
    required this.globalPosition,
    required this.localPosition,
  });

  const PositionedGestureDetails.withDefaults({
    this.globalPosition = Offset.zero,
    Offset? localPosition,
  }) : localPosition = localPosition ?? globalPosition;

  // ...
}

I'm not a native English speaker so I asked the agent for the comparison and it seems the PositionedGestureDetails is preferred, so I guess we go for it and see if we may have more feedback.

I've also updated the base class always to require both arguments.

I believe as of now, localPosition is still set as an optional parameter.

abstract class PositionedGestureDetails with Diagnosticable {
  const PositionedGestureDetails({required this.globalPosition, Offset? localPosition})
    : localPosition = localPosition ?? globalPosition;

  // ...
}

If we tweaked the base class as described above, it'd be possible to require both parameters in the unnamed constructor without breaking changes.

class DragDownDetails extends PositionedGestureDetails {
  /// Super-parameters are required unless `super.withDefaults` is specified.
  const DragDownDetails({super.globalPosition, super.localPosition}) : super.withDefaults();
}

If we tweaked the base class as described above, it'd be possible to require both parameters in the unnamed constructor without breaking changes.

I believe we can write this:

class DragDownDetails extends PositionedGestureDetails {
  /// Creates details for a [GestureDragDownCallback].
  const DragDownDetails({super.globalPosition = Offset.zero, Offset? localPosition})
    : super(localPosition: localPosition ?? globalPosition);
}

That's true! If we don't mind a bit of extra boilerplate, a second constructor isn't necessary :)

Updated. ☕

This is hard to comprehend. I had to do a double take to understand what star-start details are.

Also, its odd that the base class is describing semantics differences of subclasses. Might be a sign that this is not a great abstraction, see https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#refactor-the-code-when-the-documentation-would-be-incomprehensible

Also, its odd that the base class is describing semantics differences of subclasses.

It depends on how much info we want from this abstraction. All subclasses were used to have the same comment: "The global position at which the pointer interacts with the screen.", so we may revert to this version instead?

Same

What does DragDownDetails.toString() give you now with this removed?

It was taken by the diagnostic builder, so the result of
DragDownDetails(globalPosition: Offset(1.0, 1.0), localPosition: Offset(2.0, 2.0)).toString()
will be
DragDownDetails#63b8c(globalPosition: Offset(1.0, 1.0), localPosition: Offset(2.0, 2.0)).

This is exactly the code we don't want people to write, see also https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#summary:

Avoid is [...].

This is part of my reservations against this pattern. IMO, the better way to handle this is to factor out the common business logic in a method and define separate handler methods that then call the common code where needed.

This is exactly the code we don't want people to write, see also https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#summary:

Avoid is [...].

Well, this is the style guide for the Flutter repo, but the example is for all users regardless Flutter repo themselves. Also, the style guide does not clarify why the pattern should be avoided. Unfortunately, our repo also contains is in multiple places, those are also reasonably written.

This is part of my reservations against this pattern. IMO, the better way to handle this is to factor out the common business logic in a method and define separate handler methods that then call the common code where needed.

My understanding is people write code as they wish, the quality could be defeated by unsound declarations.

If I'm writing a gesture helper package to handle these gestures, I would definitely go for some solid type definition such as:

Offset handleYourPositionedGestures(PositionedGestureDetails details) {
  final Offset transformedBySomeMathematics = calculate(details.globalPosition, details.localPosition);
  return transformedBySomeMathematics;
}

Rather than:

Offset handleYourPositionedGestures(Offset globalPosition, Offset localPosition) {
  final Offset transformedBySomeMathematics = calculate(globalPosition, localPosition);
  return transformedBySomeMathematics;
}

The latter lost consistency and users could pass whatever positions even when the parameter suggested specified, while the previous one declared that only positions in a single gesture could be handled.

Of course, there would a multiple use cases that argue how a common business logic should be written, but providing a good common interface would also be helpful in some way IMO (as @nate-thegrate also said in #160714 (comment)).

P.S. The example here could be updated to a proper one to avoid is, totally agree. Maybe the one I just wrote stands.

The example has been updated.