Skip to content

Conversation

@chinmoy12c
Copy link
Member

This PR deprecates onWillAccept and onAccept over onWillAcceptWithDetails and onAcceptWithDetails.

Fixes: #133347

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. c: tech-debt Technical debt, code quality, testing, etc. labels Aug 30, 2023
@chinmoy12c chinmoy12c force-pushed the deprecate/on_will_accept_and_on_accept branch from 5ab998e to a01c49b Compare August 30, 2023 19:56
Comment on lines +15 to +16
Copy link
Member Author

@chinmoy12c chinmoy12c Aug 30, 2023

Choose a reason for hiding this comment

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

Hey @Piinks, even though this resolves the simple renaming issue, I wonder what might happen when the code is a bit more complex. Let's say that data used to be an 'int' in the previous 'onAccept' and the code was something like:

onAccept: (data) => {
    myVar += data; 
}

Ideally, after switching to onAcceptWithDetails this should be changed to:

onAcceptWithDetails: (details) => {
    myVar += details.data;
}

since details is not an int now but a DragTargetDetails<int>. Can this be also handled by a dart fix? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

No, this is something the user would have to update. Thankfully though they would only need to add the details. on the front.

@chinmoy12c chinmoy12c force-pushed the deprecate/on_will_accept_and_on_accept branch 3 times, most recently from 9e0817f to d371697 Compare September 1, 2023 07:09
@github-actions github-actions bot added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Sep 1, 2023
@chinmoy12c chinmoy12c force-pushed the deprecate/on_will_accept_and_on_accept branch from d371697 to e9358f7 Compare September 1, 2023 07:15
Copy link
Contributor

Choose a reason for hiding this comment

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

Since cupertino exports the widgets library, we'll want to include it too.

These uris are used to determine if a fix might be needed in a given file based on the imports, that is why we have tests for each uri. So for this one, the tests will also need a material and cupertino test set.

Comment on lines +15 to +16
Copy link
Contributor

Choose a reason for hiding this comment

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

No, this is something the user would have to update. Thankfully though they would only need to add the details. on the front.

@chinmoy12c chinmoy12c force-pushed the deprecate/on_will_accept_and_on_accept branch from e9358f7 to 73764b7 Compare September 15, 2023 12:00
@github-actions github-actions bot added f: material design flutter/packages/flutter/material repository. f: cupertino flutter/packages/flutter/cupertino repository labels Sep 15, 2023
@chinmoy12c chinmoy12c force-pushed the deprecate/on_will_accept_and_on_accept branch from 73764b7 to 2f08c71 Compare September 18, 2023 06:29
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.

Thanks very much for your patience! I've been in and out of office a lot lately. 🙏
LGTM! Thanks for following up on this!

@Piinks
Copy link
Contributor

Piinks commented Sep 22, 2023

See https://discord.com/channels/608014603317936148/611619879153565715/1154915603577049162 for contributor access invite. I really appreciate your contributions over the years, thanks for continuing to make Flutter better.

@chinmoy12c chinmoy12c force-pushed the deprecate/on_will_accept_and_on_accept branch from 2f08c71 to 3c0417f Compare September 25, 2023 06:10
@Piinks Piinks force-pushed the deprecate/on_will_accept_and_on_accept branch from 3c0417f to e516541 Compare September 25, 2023 16:40
@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 25, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 25, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Sep 25, 2023

auto label is removed for flutter/flutter/133691, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@Piinks Piinks added autosubmit Merge PR when tree becomes green via auto submit App and removed autosubmit Merge PR when tree becomes green via auto submit App labels Sep 25, 2023
@Piinks
Copy link
Contributor

Piinks commented Sep 25, 2023

I've removed the autosubmit because I am curious for your thoughts on #133136. I noticed there is a bit of a conundrum for folks that may have apps that allow the data to be nullable. The new methods that provide the details do not support data being nullable.

WDYT for folks that are migrating this change and have a case where data is null?

@chinmoy12c
Copy link
Member Author

I've removed the autosubmit because I am curious for your thoughts on #133136. I noticed there is a bit of a conundrum for folks that may have apps that allow the data to be nullable. The new methods that provide the details do not support data being nullable.

WDYT for folks that are migrating this change and have a case where data is null?

Hmm yes, that would indeed break their code after the migration. IMO it has become quite difficult to decide on a way to handle this without breaking things. I feel if the newer callbacks had supported nullable data and offset this could have been avoided in the newer callbacks 🤔. Should we make the decision now to change the newer callbacks making them accept nullable fields (Although the newer callbacks are out in the Flutter 3.15 beta)?

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.

Thanks for your patience while I was out! Yeah, I have long felt like there's this sticky conundrum here where we allow for null in some parts of the API and don't in others. Since we'll be moving folks over to a new API, let's go ahead and see if we can fix this the right way while we have the opportunity.

Should we make the decision now to change the newer callbacks making them accept nullable fields?

Can you go ahead and stage this and I can run extra testing to find out just how breaking it will be (hopefully not too breaking since these callbacks are new)? Then based on the results we can communicate the change before landing it.

Thanks so much for embracing the yak shave on this one!

auto-submit bot pushed a commit to flutter/packages that referenced this pull request Nov 14, 2023
Roll Flutter from f662150 to e8c2bb1 (66 revisions)

flutter/flutter@f662150...e8c2bb1

2023-11-14 tessertaha@gmail.com Reland "Update `framework_test.dart` to remove `ButtonBar` usage and remove references from other clases (#137550) (flutter/flutter#137753)
2023-11-14 jacksongardner@google.com Consume flutter.js from the engine artifacts. (flutter/flutter#137113)
2023-11-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 995ebcefb0d4 to 1b3fd80812c3 (1 revision) (flutter/flutter#138415)
2023-11-14 engine-flutter-autoroll@skia.org Roll Packages from 17bd92e to 428ba3e (2 revisions) (flutter/flutter#138412)
2023-11-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 712094481217 to 995ebcefb0d4 (1 revision) (flutter/flutter#138409)
2023-11-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2c54b5f70c94 to 712094481217 (1 revision) (flutter/flutter#138401)
2023-11-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from d100e8912eb0 to 2c54b5f70c94 (4 revisions) (flutter/flutter#138399)
2023-11-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 74a9de45f128 to d100e8912eb0 (3 revisions) (flutter/flutter#138383)
2023-11-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 77b952f3add4 to 74a9de45f128 (3 revisions) (flutter/flutter#138382)
2023-11-14 79099771+peterabrahamdev@users.noreply.github.com Fixing typo (flutter/flutter#138253)
2023-11-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 046ec85dffc6 to 77b952f3add4 (1 revision) (flutter/flutter#138377)
2023-11-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from db6da000a17e to 046ec85dffc6 (5 revisions) (flutter/flutter#138375)
2023-11-13 goderbauer@google.com Finally remove analysis_options_user.yaml (flutter/flutter#138261)
2023-11-13 43759233+kenzieschmoll@users.noreply.github.com Add a DevTools section to CONTRIBUTING.md (flutter/flutter#137193)
2023-11-13 104349824+huycozy@users.noreply.github.com Update DraggableScrollableSheet docs to reflect API change (flutter/flutter#136471)
2023-11-13 gspencergoog@users.noreply.github.com Clean up synonyms, key code generation. (flutter/flutter#138192)
2023-11-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from 74ba6c17a488 to db6da000a17e (2 revisions) (flutter/flutter#138364)
2023-11-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5d62f1a2392a to 74ba6c17a488 (1 revision) (flutter/flutter#138362)
2023-11-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7de793d2bb68 to 5d62f1a2392a (1 revision) (flutter/flutter#138358)
2023-11-13 polinach@google.com Upgrade leak tracker. (flutter/flutter#138283)
2023-11-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from fe11f3a46bac to 7de793d2bb68 (2 revisions) (flutter/flutter#138353)
2023-11-13 engine-flutter-autoroll@skia.org Roll Packages from a682189 to 17bd92e (2 revisions) (flutter/flutter#138347)
2023-11-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from a18ee3c7f57a to fe11f3a46bac (2 revisions) (flutter/flutter#138344)
2023-11-12 engine-flutter-autoroll@skia.org Roll Flutter Engine from 828e4dbf6693 to a18ee3c7f57a (7 revisions) (flutter/flutter#138332)
2023-11-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from e2e07eab35ec to 828e4dbf6693 (1 revision) (flutter/flutter#138282)
2023-11-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1c29ce15c528 to e2e07eab35ec (1 revision) (flutter/flutter#138280)
2023-11-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from aa6753fdbb51 to 1c29ce15c528 (1 revision) (flutter/flutter#138277)
2023-11-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from 00db306f6f7b to aa6753fdbb51 (1 revision) (flutter/flutter#138269)
2023-11-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9d8a1125640d to 00db306f6f7b (7 revisions) (flutter/flutter#138266)
2023-11-10 15619084+vashworth@users.noreply.github.com Only run tests on macOS 12 (flutter/flutter#138260)
2023-11-10 kristijan.zic@gmail.com Fixes vscode path installed via snap (flutter/flutter#136997)
2023-11-10 rossllewallyn@proton.me Docs typo: comprised -> composed (flutter/flutter#137896)
2023-11-10 chinmoy12c@gmail.com Deprecates onWillAccept and onAccept callbacks in DragTarget. (flutter/flutter#133691)
2023-11-10 chris@bracken.jp [macOS] Suppress Xcode 15 createItemModels warning (flutter/flutter#138243)
2023-11-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 275ddb296ec9 to 9d8a1125640d (1 revision) (flutter/flutter#138252)
2023-11-10 sokolovskyi.konstantin@gmail.com GestureRecognizer should dispatch creation and disposal events. (flutter/flutter#138223)
2023-11-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5c2e16c5a95a to 275ddb296ec9 (1 revision) (flutter/flutter#138249)
2023-11-10 engine-flutter-autoroll@skia.org Roll Packages from b69f54e to a682189 (3 revisions) (flutter/flutter#138239)
2023-11-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from e5b75177ac8e to 5c2e16c5a95a (1 revision) (flutter/flutter#138247)
2023-11-10 42216813+eliasyishak@users.noreply.github.com `CommandResultEvent` migrated (flutter/flutter#138165)
2023-11-10 john@johnmccutchan.com Fix #128925 by properly setting the Android Event Source (flutter/flutter#138241)
2023-11-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 77349dc8e27b to e5b75177ac8e (2 revisions) (flutter/flutter#138244)
2023-11-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from a5eab0d281fe to 77349dc8e27b (4 revisions) (flutter/flutter#138237)
2023-11-10 42216813+eliasyishak@users.noreply.github.com Update analytics constructor to include `FLUTTER_HOST` (flutter/flutter#138107)
2023-11-10 srawlins@google.com Prepare the analyze_once test for removal of analysis_options_user support (flutter/flutter#138229)
2023-11-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from b020893cba29 to a5eab0d281fe (5 revisions) (flutter/flutter#138231)
...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App c: tech-debt Technical debt, code quality, testing, etc. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deprecate old accept callbacks in DragTarget

2 participants