Fixes DragTarget crash if Draggable.data is null#133136
Fixes DragTarget crash if Draggable.data is null#133136auto-submit[bot] merged 16 commits intoflutter:masterfrom
DragTarget crash if Draggable.data is null#133136Conversation
|
@Piinks @chinmoy12c I created the PR to continue the discussion from #84816 I made |
Piinks
left a comment
There was a problem hiding this comment.
This looks like a pretty big breaking change. Can we address the crash in DragTarget without changing data? It looks like it crashes because of a bad assumption it will not be null, can we check for that and change the code path where the crash occurs?
|
@Piinks, thank you for the review. I'm okay to change the code to not make For example flutter/packages/flutter/lib/src/widgets/drag_target.dart Lines 742 to 751 in cd3aeef would become something like widget.onAccept?.call(avatar.data as T?);
widget.onAcceptWithDetails?.call(DragTargetDetails<T?>(data: avatar.data as T?, offset: avatar._lastOffset!)); which will force me to change the signature of Also, I might need to change Are you fine with it or do you see a better approach? |
Piinks
left a comment
There was a problem hiding this comment.
I don't think that we'd be able to land a change that makes it nullable, since all customers will need to change their function signature and then add handling for when it is null.
I am not sure I understand the issue this is trying to resolve, can you speak more to that? Maybe from there we can find a way to make a non breaking-change.
|
The issue is that flutter/packages/flutter/lib/src/widgets/drag_target.dart Lines 169 to 193 in c67e6df So But flutter/packages/flutter/lib/src/widgets/drag_target.dart Lines 742 to 752 in c67e6df which throws a An alternative that could dodge breaking changes would be to do something like: if (avatar.data != null) {
widget.onAccept?.call(avatar.data! as T);
widget.onAcceptWithDetails?.call(DragTargetDetails<T>(data: avatar.data! as T, offset: avatar._lastOffset!));
}It would not require us to change any function signature. What do you think? |
That sounds reasonable to me. :) Thanks! |
b09f983 to
753aeed
Compare
|
@Piinks What do you think of fix: Don't call .data! when it is null ? (Sorry I forced pushed, it was simpler than reverting my changes that were conflicting with |
|
@Piinks is there something else I should do on this PR or does it look good to you ? |
This is still marked as a draft, can you mark it ready for review? :) |
…null-value # Conflicts: # packages/flutter/test/widgets/draggable_test.dart
8ad5dc8 to
2dc04d8
Compare
|
@Piinks Do you have any more feedback on this PR? |
Makes the
dataparameter ofDraggablenon-nullable.Fixes #84816
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.