add a new PopScope.onPopWithResultInvoke widget to replace PopScope.onPopInvoke #147016

chunhtai · 2024-04-18T22:57:14Z

same as #139164, but make it a soft transition

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.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

navaronbracke

Just some minor docs suggestions while I was reading through it.

navaronbracke · 2024-04-19T06:35:58Z

examples/api/lib/widgets/pop_scope/pop_scope.1.dart

Suggested change

// This sample demonstrates showing how to use PopScopeWithResult to wrap widget that

// This sample demonstrates how to use PopScopeWithResult to wrap a widget that

// may pop the page with a result.

navaronbracke · 2024-04-19T06:38:11Z

examples/api/lib/widgets/pop_scope/pop_scope.1.dart

Suggested change

// The result contains pop result in `_PageTwo`.

// The result argument contains the pop result that is defined in `_PageTwo`.

navaronbracke · 2024-04-19T06:38:48Z

examples/api/lib/widgets/pop_scope/pop_scope.1.dart

Suggested change

// This is a PopScopeWithResult wrapper over _PageTwoBody

// This is a PopScopeWithResult wrapper over _PageTwoBody.

navaronbracke · 2024-04-19T06:40:33Z

packages/flutter/lib/src/material/about.dart

Having a void variable feels a bit weird when it happens due to a void API return.

Suggested change

onPopInvoked: (bool didPop, void result) {

onPopInvoked: (bool didPop, _) {

That's true but we also have avoid anonymous parameter names in the styleguide. I agree that void result is weird, though.

navaronbracke · 2024-04-19T06:42:30Z

packages/flutter/test/widgets/navigator_test.dart

Suggested change

onPopInvoked: (bool didPop, void result) {

onPopInvoked: (bool didPop, _) {

As Justin said we can't do the above, maybe we can get away with Object? result for the void result variables? It's a bit less weird and should work too?

navaronbracke · 2024-04-19T06:43:57Z

packages/flutter/lib/src/widgets/pop_scope.dart

Suggested change

/// Deprecated, use [PopInvokedWithResultCallback] instead

/// Deprecated, use [PopInvokedWithResultCallback] instead.

navaronbracke · 2024-04-19T06:44:37Z

packages/flutter/lib/src/widgets/pop_scope.dart

Missing "a" and a slight rewording:

Suggested change

/// The generic type should match or be supertype of the generic type of the

/// The generic type should match or be a supertype of the generic type of the

/// enclosing [Route]. For example, if the enclosing Route is a `MaterialPageRoute<int>`,

/// you can define [PopScopeWithResult] with `int` or any supertype of `int`.

navaronbracke · 2024-04-19T06:49:03Z

packages/flutter/lib/src/widgets/pop_scope.dart

Suggested change

/// This sample demonstrates showing how to use PopScope to wrap widget that

/// This sample demonstrates how to use PopScope to wrap a widget that

navaronbracke · 2024-04-19T06:50:58Z

packages/flutter/lib/src/widgets/routes.dart

Suggested change

// Holding as Object? instead of T so that PopScopeWithResult in this route can be

// Use Object? instead of T for the generic type of the PopEntry so that PopScopeWithResult in this route can be

navaronbracke · 2024-04-19T06:53:48Z

packages/flutter/lib/src/widgets/routes.dart

Suggested change

// This can't be a function getter since dart vm doesn't allow upcasting

// This can't be a function getter since the Dart VM doesn't allow upcasting

// the generic type of a function getter. This prevents users from declaring

// PopScopeWithResult with any generic type that is subtype of ModalRoute._popEntries.

justinmc

Overall this looks good, but it's is going to be a contentious change. I'm not sure if there's a way to make that any better, but we should discuss it.

People are already upset about PopScope (see #138614) and this is going to make them even more unhappy, at least at first. Imagine receiving the deprecation warning for WillPopScope, struggling to migrate to PopScope, and then quickly thereafter getting a new deprecation warning for PopScope.

So are there any other options? We could just make a direct breaking change to PopScope without a deprecation by adding the type parameter, as in #139164. But that was reverted (#147015). Can you point to the conversation that lead to the revert?

Anyway, I guess seeing a hard breakage in PopScope might be even worse than a new deprecation warning, so I guess this is what we have to do. Are there any other options we're not thinking of?

And once again I apologize for not including this type parameter in the original PopScope PR 😭

justinmc · 2024-04-19T16:45:32Z

examples/api/lib/widgets/pop_scope/pop_scope.1.dart

This should be indented.

justinmc · 2024-04-19T16:46:57Z

examples/api/lib/widgets/pop_scope/pop_scope.1.dart

There should be an empty line above this line.

justinmc · 2024-04-19T17:11:15Z

packages/flutter/lib/src/widgets/pop_scope.dart

Nit: I worry about removing all of the docs on these things and replacing them with just the sentence about deprecation. Can you just leave the docs duplicated?

justinmc · 2024-04-19T17:14:30Z

packages/flutter/lib/src/widgets/pop_scope.dart

Smart to extend PopScopeWithResult here.

justinmc · 2024-04-19T17:15:25Z

packages/flutter/lib/src/widgets/pop_scope.dart

Have you thought about changing these file names to pop_scope_with_result.dart (also the examples)? I guess it's probably overkill, but just wanted to bring it up.

justinmc · 2024-04-19T17:19:39Z

examples/api/lib/widgets/pop_scope/pop_scope.1.dart

This should be child.

justinmc · 2024-04-19T17:23:43Z

packages/flutter/lib/src/material/about.dart

That's true but we also have avoid anonymous parameter names in the styleguide. I agree that void result is weird, though.

justinmc · 2024-04-19T17:26:12Z

packages/flutter/test/widgets/pop_scope_test.dart

Nit: Maybe say "PopScopeWithResult" instead of "pop scope", here and below.

justinmc · 2024-04-19T17:33:23Z

packages/flutter/test/widgets/pop_scope_test.dart

Is this necessary? Since the code being tested doesn't have anything platform specific, and in the course of Flutter's CI process this will end up getting tested on devices of all platforms anyway.

Same for below.

chunhtai · 2024-04-19T18:16:13Z

Someone reminded me it is against style guide for breaking change. I found this in style guide

If you decide your change is valuable enough to deploy, adjust your PR so that it introduces the new functionality, API, behavior change, etc, in an opt-in fashion, thus avoiding the immediate breakage.

For example, rather than replacing a widget with another, introduce the new widget and discourage use of the old one. Rather than changing the order in which a certain argument is processed, provide a flag that selects which order the arguments will be processed in.

When changing the semantics of an API with a temporary opt-in, a three-phase change is needed (adding the new API and opt-in, then removing the old API, then removing the opt-in.)

I am not sure how strict this should be followed. I am also not a fan of having yet another new widget. The alternative is to not have generic type and then have it always typed as Object?, and then reimplement the original onPopInvoked. but I am not sure if losing type safe is worth it.

The other alternative is still have the generic type, but deprecate the onPopInvoke and add the onPopInvokeWithResult. note that missing generic type is just a lint error, not a hard breaking change.

WDYT?

chunhtai · 2024-04-19T18:34:57Z

I guess I will just do the latter, it seems to make the most sense

examples/api/lib/widgets/pop_scope/pop_scope.1.dart

justinmc

LGTM with nits 👍

I agree that the current approach seems to be the best option for minimizing pains related to breaking changes. I can't think of anything better.

justinmc · 2024-04-22T19:56:17Z

packages/flutter/lib/src/widgets/pop_scope.dart

Nit: Maybe add a message here, in case someone didn't see the deprecation error? "onPopInvoked is deprecated, use onPopWithResultInvoked"

justinmc · 2024-04-22T19:57:47Z

dev/integration_tests/flutter_gallery/lib/demo/shrine/expanding_bottom_sheet.dart

Nit: Maybe onPopInvokedWithResult is a better name? Up to you.

justinmc · 2024-04-22T20:10:37Z

packages/flutter/lib/src/widgets/routes.dart

It looks like this doesn't have any other implications in this PR, does it? Like I seem to see PopEntry is still being used in the same way.

)" (flutter#147015)" This reverts commit bb4c9b3.

Co-authored-by: Navaron Bracke <[email protected]>

…pScope.onPopInvoke (flutter/flutter#147016)

flutter/flutter@5d3bca4...2e80670 2024-04-26 [email protected] Roll Flutter Engine from d79458007712 to c410180e5bba (1 revision) (flutter/flutter#147407) 2024-04-26 [email protected] Roll Flutter Engine from 11a857e1599f to d79458007712 (1 revision) (flutter/flutter#147401) 2024-04-26 [email protected] Roll Flutter Engine from e99fc6ef91ef to 11a857e1599f (4 revisions) (flutter/flutter#147399) 2024-04-26 [email protected] Fix `FloatingActionButton` docs for `background` and `foreground` properties (flutter/flutter#147372) 2024-04-26 49699333+dependabot[bot]@users.noreply.github.com Bump peter-evans/create-pull-request from 6.0.4 to 6.0.5 (flutter/flutter#147388) 2024-04-26 [email protected] Fix `DateRangePickerDialog` does not use `rangePickerHeaderBackgroundColor` from `DatePickerTheme` in M2 (flutter/flutter#147370) 2024-04-26 [email protected] Roll Flutter Engine from a09295fe03c0 to e99fc6ef91ef (4 revisions) (flutter/flutter#147391) 2024-04-26 [email protected] Clean up leaks in a test. (flutter/flutter#147312) 2024-04-26 [email protected] add a new PopScope.onPopWithResultInvoke widget to replace PopScope.onPopInvoke (flutter/flutter#147016) 2024-04-26 [email protected] zero-sized RenderConstraintsTransformBox respects clipBehavior (flutter/flutter#147349) 2024-04-25 [email protected] Roll Flutter Engine from 3768ca0c02da to a09295fe03c0 (1 revision) (flutter/flutter#147386) 2024-04-25 [email protected] [devicelab] explicitly enable vulkan validation in test. (flutter/flutter#147382) 2024-04-25 [email protected] Roll Flutter Engine from 10a3504a9261 to 3768ca0c02da (1 revision) (flutter/flutter#147381) 2024-04-25 [email protected] Roll Flutter Engine from 694756b875bf to 10a3504a9261 (3 revisions) (flutter/flutter#147380) 2024-04-25 [email protected] Roll Flutter Engine from 19c182f47bde to 694756b875bf (1 revision) (flutter/flutter#147379) 2024-04-25 [email protected] Roll Flutter Engine from 163c18e870a9 to 19c182f47bde (1 revision) (flutter/flutter#147378) 2024-04-25 [email protected] Makes badge to auto size with content (flutter/flutter#146853) 2024-04-25 [email protected] Fix memory leaks in `Hero` widget (flutter/flutter#147303) 2024-04-25 [email protected] Roll Packages from cf6d280 to fde908d (11 revisions) (flutter/flutter#147375) 2024-04-25 [email protected] Roll Flutter Engine from 674890ce7141 to 163c18e870a9 (1 revision) (flutter/flutter#147373) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

auto-submit · 2024-04-30T20:07:00Z

Time to revert pull request flutter/flutter/147016 has elapsed.
You need to open the revert manually and process as a regular pull request.

chunhtai · 2024-04-30T20:07:04Z

https://github.com/ethanblake4/flutter_eval needs to be migrated in order to roll this change into internal code base

…pScope.onPopInvoke (flutter#147016)" This reverts commit 8031a3e.

flutter/flutter@5d3bca4...2e80670 2024-04-26 [email protected] Roll Flutter Engine from d79458007712 to c410180e5bba (1 revision) (flutter/flutter#147407) 2024-04-26 [email protected] Roll Flutter Engine from 11a857e1599f to d79458007712 (1 revision) (flutter/flutter#147401) 2024-04-26 [email protected] Roll Flutter Engine from e99fc6ef91ef to 11a857e1599f (4 revisions) (flutter/flutter#147399) 2024-04-26 [email protected] Fix `FloatingActionButton` docs for `background` and `foreground` properties (flutter/flutter#147372) 2024-04-26 49699333+dependabot[bot]@users.noreply.github.com Bump peter-evans/create-pull-request from 6.0.4 to 6.0.5 (flutter/flutter#147388) 2024-04-26 [email protected] Fix `DateRangePickerDialog` does not use `rangePickerHeaderBackgroundColor` from `DatePickerTheme` in M2 (flutter/flutter#147370) 2024-04-26 [email protected] Roll Flutter Engine from a09295fe03c0 to e99fc6ef91ef (4 revisions) (flutter/flutter#147391) 2024-04-26 [email protected] Clean up leaks in a test. (flutter/flutter#147312) 2024-04-26 [email protected] add a new PopScope.onPopWithResultInvoke widget to replace PopScope.onPopInvoke (flutter/flutter#147016) 2024-04-26 [email protected] zero-sized RenderConstraintsTransformBox respects clipBehavior (flutter/flutter#147349) 2024-04-25 [email protected] Roll Flutter Engine from 3768ca0c02da to a09295fe03c0 (1 revision) (flutter/flutter#147386) 2024-04-25 [email protected] [devicelab] explicitly enable vulkan validation in test. (flutter/flutter#147382) 2024-04-25 [email protected] Roll Flutter Engine from 10a3504a9261 to 3768ca0c02da (1 revision) (flutter/flutter#147381) 2024-04-25 [email protected] Roll Flutter Engine from 694756b875bf to 10a3504a9261 (3 revisions) (flutter/flutter#147380) 2024-04-25 [email protected] Roll Flutter Engine from 19c182f47bde to 694756b875bf (1 revision) (flutter/flutter#147379) 2024-04-25 [email protected] Roll Flutter Engine from 163c18e870a9 to 19c182f47bde (1 revision) (flutter/flutter#147378) 2024-04-25 [email protected] Makes badge to auto size with content (flutter/flutter#146853) 2024-04-25 [email protected] Fix memory leaks in `Hero` widget (flutter/flutter#147303) 2024-04-25 [email protected] Roll Packages from cf6d280 to fde908d (11 revisions) (flutter/flutter#147375) 2024-04-25 [email protected] Roll Flutter Engine from 674890ce7141 to 163c18e870a9 (1 revision) (flutter/flutter#147373) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…pScope.onPopInvoke (flutter/flutter#147016)

IanVS · 2024-09-03T15:28:15Z

I'm not sure if this is the best place to ask this, but maybe the docs for migrating a back confirmation dialog could be updated to avoid the use of the now-deprecated onPopInvoked? (https://docs.flutter.dev/release/breaking-changes/android-predictive-back#migrating-a-back-confirmation-dialog)

chunhtai requested a review from justinmc April 18, 2024 22:57

navaronbracke reviewed Apr 19, 2024

View reviewed changes

justinmc reviewed Apr 19, 2024

View reviewed changes

chunhtai force-pushed the popscope-reland branch from 5f47550 to 5b5b156 Compare April 19, 2024 19:05

chunhtai requested review from justinmc and navaronbracke April 19, 2024 19:11

navaronbracke reviewed Apr 19, 2024

View reviewed changes

examples/api/lib/widgets/pop_scope/pop_scope.1.dart Outdated Show resolved Hide resolved

chunhtai changed the title ~~add a new popscopewithresult widget to replace popscope~~ add a new PopScope.onPopWithResultInvoke widget to replace PopScope.onPopInvoke Apr 22, 2024

chunhtai force-pushed the popscope-reland branch from d0ddfd1 to 404a50a Compare April 22, 2024 16:54

chunhtai requested a review from navaronbracke April 22, 2024 19:28

justinmc approved these changes Apr 22, 2024

View reviewed changes

chunhtai force-pushed the popscope-reland branch from 404a50a to 9af5a27 Compare April 23, 2024 18:06

chunhtai and others added 7 commits April 24, 2024 11:14

Reland "Reverts "Add generic type for result in PopScope (flutter#139164

05e9637

)" (flutter#147015)" This reverts commit bb4c9b3.

update

5668d9c

update

71c50e0

Update examples/api/lib/widgets/pop_scope/pop_scope.1.dart

2bb23be

Co-authored-by: Navaron Bracke <[email protected]>

fix ci

18111c2

update

39b6cd1

form

3a1c032

chunhtai force-pushed the popscope-reland branch from 9af5a27 to 3a1c032 Compare April 24, 2024 18:14

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 25, 2024

auto-submit bot merged commit 8031a3e into flutter:master Apr 26, 2024

engine-flutter-autoroll mentioned this pull request Apr 26, 2024

Roll Flutter from 5d3bca4500b1 to 2e806700b928 (20 revisions) flutter/packages#6616

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 26, 2024

8031a3e add a new PopScope.onPopWithResultInvoke widget to replace Po…

29deb12

…pScope.onPopInvoke (flutter/flutter#147016)

chunhtai added the revert Autorevert PR (with "Reason for revert:" comment) label Apr 30, 2024

auto-submit bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Apr 30, 2024

chunhtai added a commit to chunhtai/flutter that referenced this pull request Apr 30, 2024

Revert "add a new PopScope.onPopWithResultInvoke widget to replace Po…

9d06911

…pScope.onPopInvoke (flutter#147016)" This reverts commit 8031a3e.

chunhtai mentioned this pull request Apr 30, 2024

Revert "add a new PopScope.onPopWithResultInvoke widget to replace Po… #147597

Merged

9 tasks

chingjun mentioned this pull request Apr 30, 2024

Revert "add a new PopScope.onPopWithResultInvoke widget to replace Po… #147598

Merged

18 tasks

justinmc mentioned this pull request May 1, 2024

Reapply new PopScope API #147607

Merged

9 tasks

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024

8031a3e add a new PopScope.onPopWithResultInvoke widget to replace Po…

95b8dd3

…pScope.onPopInvoke (flutter/flutter#147016)

	// This sample demonstrates showing how to use PopScopeWithResult to wrap widget that
	// This sample demonstrates how to use PopScopeWithResult to wrap a widget that
	// may pop the page with a result.

	// The result contains pop result in `_PageTwo`.
	// The result argument contains the pop result that is defined in `_PageTwo`.

	// This is a PopScopeWithResult wrapper over _PageTwoBody
	// This is a PopScopeWithResult wrapper over _PageTwoBody.

	onPopInvoked: (bool didPop, void result) {
	onPopInvoked: (bool didPop, _) {

	/// Deprecated, use [PopInvokedWithResultCallback] instead
	/// Deprecated, use [PopInvokedWithResultCallback] instead.

-/// The generic type should match or be supertype of the generic type of the
+/// The generic type should match or be a supertype of the generic type of the
+/// enclosing [Route]. For example, if the enclosing Route is a `MaterialPageRoute<int>`,
+/// you can define [PopScopeWithResult] with `int` or any supertype of `int`.

	/// This sample demonstrates showing how to use PopScope to wrap widget that
	/// This sample demonstrates how to use PopScope to wrap a widget that

	// Holding as Object? instead of T so that PopScopeWithResult in this route can be
	// Use Object? instead of T for the generic type of the PopEntry so that PopScopeWithResult in this route can be

-  // This can't be a function getter since dart vm doesn't allow upcasting
+// This can't be a function getter since the Dart VM doesn't allow upcasting
+// the generic type of a function getter. This prevents users from declaring
+// PopScopeWithResult with any generic type that is subtype of ModalRoute._popEntries.

add a new PopScope.onPopWithResultInvoke widget to replace PopScope.onPopInvoke #147016

add a new PopScope.onPopWithResultInvoke widget to replace PopScope.onPopInvoke #147016

Uh oh!

Conversation

chunhtai commented Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

navaronbracke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navaronbracke Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Apr 19, 2024 • edited by justinmc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chunhtai commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Apr 30, 2024

Uh oh!

chunhtai commented Apr 30, 2024

Uh oh!

IanVS commented Sep 3, 2024

Uh oh!

chunhtai commented Apr 18, 2024 •

edited

Loading

navaronbracke Apr 19, 2024 •

edited

Loading

chunhtai commented Apr 19, 2024 •

edited by justinmc

Loading

chunhtai commented Apr 19, 2024 •

edited

Loading