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 Hixie on the #hackers channel in Chat.

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

@HansMuller and/or @goderbauer PTAL (I can't seem to add reviewers). If the design is acceptable, I will add tests.

Also, I see the demonstration in #58982 seems to have a one-frame delay between the dragged square and child square. Does this implementation have the same issue?

Also, I see the demonstration in #58982 seems to have a one-frame delay between the dragged square and child square. Does this implementation have the same issue?

It does not. The commit implementation does a rebuild on move (for immediate updating of children) and also the onMove callback (for logic outside of the build tree)

Having those two different builder on the widget is a very strange API that I would like to avoid. It seems unnecessary confusing to the user. Hence my suggestion if you can do what you want to do by just adding the onMove callback.

Having those two different builder on the widget is a very strange API that I would like to avoid. It seems unnecessary confusing to the user. Hence my suggestion if you can do what you want to do by just adding the onMove callback.

I agree it's an unusual API. However, just using onMove() is not as ergonomic as something purely within a widget hierarchy. If I want to draw within the DragTarget while the Draggable moves, here's examples with the new callback, and with onMove:

Example of new detailedMovementBuilder, which has a more ergonomic tree of widgets, and could be used in both StatefulWidgets and StatelessWidgets

Widget build(_) =>
  ...
  child: DragTarget<int>(
    detailedMovementBuilder: (context, candidateDetails, rejectedDetails) => Stack(children:[
      if (candidateDetails.isNotEmpty)
        Positioned(...) // position based on candidateDetails
      )]);

Example with onMove, which now requires an additional piece of state and use of setState(), and thus only usable in StatefulWidgets

Offset _coords;

Widget build(_) =>
  ...
  child: DragTarget<int>(
    builder: (context, candidateData, rejectedData) =>Stack(children:[
      if (_coords != null)
        Positioned(...) // Layout based on value of _coords
      ]),
    onMove: (details) => setState(_coords = details.offset));

Quick discussion of an alternative, consisting of all the following

1. A breaking change to [builder] which provides DragTargetDetails instead of 'T data'. The signature would effectively be the same signature as the new proposed detailedMovementBuilder:

typedef DragTargetDetailedMovementBuilder<T> =
    Widget Function(BuildContext context, List<DragTargetDetails<T>> candidateDetails, List<DragTargetDetails<dynamic>> rejectedDetails);

. However, it would continue to only fire child rebuilds on Draggable enter/leave/drop/reject.
2) A bool flag (e.g. 'rebuildOnMove') which changes the behavior of the builder from the default, to rebuilding on each move.
3) Still have an onMove() callback for logic outside of the local widget build phase
This gets all the needed functionality, plus ergonomic stateless code, without a second builder method.

Thanks for taking a look.

I still think that just providing onMove is the best option here. It gives us a clean framework API without any user confucion and you can still do all the thinks you want to do.

I've removed the additional builder function, and only added the onMove() callback. PTAL.

Not sure why the doc shard is failing. Maybe try rebasing this onto the latest master and try again?

Rebased on latest and more stuff is green. I don't know how to remove or resolve the requested review from @dkwingsmt, though.

You're good to go with goderbauer's approval! :)

This pull request is not suitable for automatic merging in its current state.

• This pull request has changes requested by @dkwingsmt. Please resolve those before re-applying the label.

This pull request is not suitable for automatic merging in its current state.

• This pull request has changes requested by @dkwingsmt. Please resolve those before re-applying the label.