Refactors page API #137792

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

auto-submit merged 4 commits into flutter:master from chunhtai:issues/137458

May 13, 2024

Contributor

chunhtai commented Nov 2, 2023 •

edited

Loading

Chagnes:

Navigator.pop will always pop page based route
add a onDidRemovePage callback to replace onPopPage
Page.canPop and Page.onPopInvoked mirrors the PopScope, but in Page class.

migration guide flutter/website#10523

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.

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

flutter-dashboard bot commented Nov 2, 2023

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 or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

github-actions bot added framework f: material design f: cupertino d: api docs d: examples f: routes labels

chunhtai requested review from goderbauer and justinmc

November 2, 2023 21:24

Contributor Author

chunhtai commented Nov 2, 2023

I will add test later if this approach looks good

justinmc reviewed

View reviewed changes

Contributor

justinmc left a comment •

edited

Loading

I've been away from predictive back for awhile, but this approach looks good to me. It seems a lot simpler with no onPopPage and the same canPop and onPopInvoked API everywhere. Especially because it works with system backs out of the box as users will expect.

Thank you for refactoring this!

packages/flutter/lib/src/widgets/navigator.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/widgets/navigator.dart Outdated

Contributor

justinmc Nov 3, 2023

Good call pulling this out into a function.

Member

goderbauer Nov 28, 2023

Nice reafactoring, indeed!

packages/flutter/lib/src/widgets/navigator.dart Outdated

Contributor

justinmc Nov 3, 2023

The name sounds good to me.

packages/flutter/lib/src/widgets/navigator.dart Outdated

Contributor

justinmc Nov 3, 2023

Nit: Is there anything that could be deduplicated with a macro in these docs? Here and for canPop.

Contributor Author

chunhtai Nov 29, 2023

the content is slightly different. I can't find a good way to dedup these doc

Contributor Author

chunhtai commented Nov 3, 2023

@justinmc There is one thing I am not too sure about the popscope. Is there a way for PopScope to know what result the original pop used? For example if the pop is generated by Navigator.maybePop('some text'), if you want to launch a confirmation dialog in popScope, there isn't seem to be a way to get the string as pop result if user click confirm

chunhtai force-pushed the issues/137458 branch from dc9e288 to db0ea04 Compare

November 7, 2023 17:12

chunhtai requested a review from justinmc

November 10, 2023 21:21

chunhtai force-pushed the issues/137458 branch from db0ea04 to 26af71d Compare

November 28, 2023 19:11

goderbauer reviewed

View reviewed changes

packages/flutter/lib/src/widgets/navigator.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/widgets/navigator.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/widgets/navigator.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/widgets/navigator.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/widgets/navigator.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/widgets/navigator.dart Outdated

Member

goderbauer Nov 28, 2023

Nice reafactoring, indeed!

packages/flutter/lib/src/widgets/navigator.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/widgets/navigator.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/widgets/navigator.dart Outdated Show resolved Hide resolved

packages/flutter/test/widgets/navigator_test.dart Outdated Show resolved Hide resolved

chunhtai mentioned this pull request

[DO NOT MERGE] Adds page apis migration guide flutter/website#9873

Closed

3 tasks

chunhtai force-pushed the issues/137458 branch from 26af71d to 0fbd1eb Compare

November 29, 2023 22:47

chunhtai requested a review from goderbauer

November 29, 2023 22:49

chunhtai mentioned this pull request

[go_router] PopScope seems to be incompatible with GoRouter #138737

Open

2 tasks

chunhtai force-pushed the issues/137458 branch from 0fbd1eb to 595de72 Compare

December 11, 2023 23:43

chunhtai force-pushed the issues/137458 branch from 595de72 to 95405e7 Compare

December 21, 2023 19:07

btrautmann commented Jan 4, 2024

Hi 👋 Any idea when this might land? 🙏

Contributor

justinmc commented Jan 4, 2024

@justinmc There is one thing I am not too sure about the popscope. Is there a way for PopScope to know what result the original pop used? For example if the pop is generated by Navigator.maybePop('some text'), if you want to launch a confirmation dialog in popScope, there isn't seem to be a way to get the string as pop result if user click confirm

I pasted an example below that hopefully covers what you mean.

I don't think you can get the result of the pop that triggered onPopInvoked from inside of onPopInvoked. I'm not sure we want to enable that. I think that's the same as it was with WillPopScope. See the example, where I show the dialog from inside of onPopInvoked and receive the result there.

Example

import 'package:flutter/material.dart';

void main() => runApp(const NavigatorPopHandlerApp());

class NavigatorPopHandlerApp extends StatelessWidget {
  const NavigatorPopHandlerApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      initialRoute: '/home',
      routes: <String, WidgetBuilder>{
        '/home': (BuildContext context) => const _HomePage(),
        '/two': (BuildContext context) => const _PageTwo(),
      },
    );
  }
}

class _HomePage extends StatefulWidget {
  const _HomePage();

  @override
  State<_HomePage> createState() => _HomePageState();
}

class _HomePageState extends State<_HomePage> {

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            const Text('Page One'),
            TextButton(
              onPressed: () {
                Navigator.of(context).pushNamed('/two');
              },
              child: const Text('Next page'),
            ),
          ],
        ),
      ),
    );
  }
}

class _PageTwo extends StatefulWidget {
  const _PageTwo();

  @override
  State<_PageTwo> createState() => _PageTwoState();
}

class _PageTwoState extends State<_PageTwo> {
  Future<bool?> _showBackDialog() {
    return showDialog<bool>(
      context: context,
      builder: (BuildContext context) {
        return AlertDialog(
          title: const Text('Are you sure?'),
          content: const Text(
            'Are you sure you want to leave this page?',
          ),
          actions: <Widget>[
            TextButton(
              style: TextButton.styleFrom(
                textStyle: Theme.of(context).textTheme.labelLarge,
              ),
              child: const Text('Nevermind'),
              onPressed: () {
                Navigator.pop(context, false);
              },
            ),
            TextButton(
              style: TextButton.styleFrom(
                textStyle: Theme.of(context).textTheme.labelLarge,
              ),
              child: const Text('Leave'),
              onPressed: () {
                Navigator.pop(context, true);
              },
            ),
          ],
        );
      },
    );
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            const Text('Page Two'),
            PopScope(
              canPop: false,
              onPopInvoked: (bool didPop) async {
                if (didPop) {
                  return;
                }
                final NavigatorState navigator = Navigator.of(context);
                final bool? shouldPop = await _showBackDialog();
                if (shouldPop ?? false) {
                  navigator.pop();
                }
              },
              child: TextButton(
                onPressed: () {
                  _showBackDialog();
                },
                child: const Text('Go back'),
              ),
            ),
          ],
        ),
      ),
    );
  }
}

Contributor

justinmc commented Jan 4, 2024

@btrautmann The author is on leave until April, but maybe I can take this over if people need it.

lucavenir mentioned this pull request

[go_router] WillPopScope/PopScope doesn't trigger with back button navigation on root screens #140869

Closed

Contributor Author

chunhtai commented Jan 16, 2024 •

edited

Loading

I pasted an example below that hopefully covers what you mean.

I don't think you can get the result of the pop that triggered onPopInvoked from inside of onPopInvoked. I'm not sure we want to enable that. I think that's the same as it was with WillPopScope. See the example, where I show the dialog from inside of onPopInvoked and receive the result there.

Example

See this example.

void main() {
  runApp(MaterialApp(
   home: Home(),
  ));
}

class Home extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: TextButton(
                onPressed: () async {
                  final myMessage = await Navigator.of(context).push(MaterialPageRoute(builder: (_) => MyWidget()));
                  print(myMessage);
                },
                child: const Text('push'),
              ),
      ),
    );
  }
}


class MyWidget extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            const Text('Page Two'),
            PopScope(
              canPop: false,
              onPopInvoked: (bool didPop) async {
                if (didPop) {
                  return;
                }
                final NavigatorState navigator = Navigator.of(context);
                final bool? shouldPop = await _showBackDialog();
                if (shouldPop ?? false) {
                  // I want to be able to pass original result 'my result' to the pending future in Home!!
                }
              },
              child: TextButton(
                onPressed: () {
                  Navigator.pop(context, 'my result');
                },
                child: const Text('Go back'),
              ),
            ),
          ],
        ),
      ),
    );
  }
}

I have a pr up for adding this feature #139164
I will reopen it once i come back.

Contributor

justinmc commented Jan 18, 2024

@chunhtai Thanks, I see what you mean. Indeed I don't think it's possible to do that right now without your PR. I think we should put both PRs on hold for now until you get back.

This was referenced Jan 24, 2024

PopScope does not invoke onPopInvoked for iOS back gesture when canPop set to false #138624

Open

Preserve WillPopScope as an alternate for PopScope #138614

Open

chunhtai added 2 commits

May 7, 2024 09:37


          Refactors page API

b49509f


          fix conflict

bfe4743

chunhtai force-pushed the issues/137458 branch from 32fc63f to bfe4743 Compare

May 7, 2024 16:38

chunhtai mentioned this pull request

Add migration guide for page apis flutter/website#10523

Merged

3 tasks

sfshaza2 pushed a commit to flutter/website that referenced this pull request


          Add migration guide for page apis (#10523)

dba6a15

migration guide for flutter/flutter#137792

## Presubmit checklist

- [ ] This PR doesn’t contain automatically generated corrections
(Grammarly or similar).
- [ ] This PR follows the [Google Developer Documentation Style
Guidelines](https://developers.google.com/style) — for example, it
doesn’t use _i.e._ or _e.g._, and it avoids _I_ and _we_ (first person).
- [ ] This PR uses [semantic line
breaks](https://github.com/dart-lang/site-shared/blob/main/doc/writing-for-dart-and-flutter-websites.md#semantic-line-breaks)
of 80 characters or fewer.

chunhtai mentioned this pull request

[GoRouter] onExit does not work with ShellRoute #137829

Open

2 tasks


          Merge branch 'master' into issues/137458

e76543b

chunhtai added the autosubmit label

Contributor

auto-submit bot commented May 13, 2024

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

auto-submit bot removed the autosubmit label


          Merge branch 'master' into issues/137458

4e5ebc9

chunhtai added the autosubmit label

auto-submit bot removed the autosubmit label

Contributor

auto-submit bot commented May 13, 2024

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

chunhtai added the autosubmit label

auto-submit bot merged commit a36ff80 into flutter:master

engine-flutter-autoroll mentioned this pull request

Roll Flutter from 125543505d26 to cb656c21f082 (23 revisions) flutter/packages#6726

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request


          a36ff80 Refactors page API (flutter/flutter#137792)

1def2db

engine-flutter-autoroll mentioned this pull request

Roll Flutter from 125543505d26 to a39555d4fe89 (24 revisions) flutter/packages#6728

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request


          a36ff80 Refactors page API (flutter/flutter#137792)

a4f164e

engine-flutter-autoroll mentioned this pull request

Roll Flutter from 125543505d26 to d2da1b280b51 (26 revisions) flutter/packages#6729

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request


          a36ff80 Refactors page API (flutter/flutter#137792)

1e0eeb1

auto-submit bot pushed a commit to flutter/packages that referenced this pull request


          Roll Flutter from 1255435 to d2da1b2 (26 revisions) (#6729)

2cc307e

flutter/flutter@1255435...d2da1b2

2024-05-14 [email protected] Roll Flutter Engine from 491f460a25fb to 7bf865774d06 (1 revision) (flutter/flutter#148325)
2024-05-14 [email protected] Roll Packages from 1412041 to fd714bd (1 revision) (flutter/flutter#148324)
2024-05-14 [email protected] Roll Flutter Engine from c381b852605f to 491f460a25fb (1 revision) (flutter/flutter#148319)
2024-05-14 [email protected] Roll Flutter Engine from b4b798d2e706 to c381b852605f (1 revision) (flutter/flutter#148303)
2024-05-14 [email protected] Roll Flutter Engine from 0de6701b537a to b4b798d2e706 (2 revisions) (flutter/flutter#148299)
2024-05-14 [email protected] Roll Flutter Engine from 764c33c3c773 to 0de6701b537a (2 revisions) (flutter/flutter#148297)
2024-05-14 [email protected] Roll Flutter Engine from 797eab742925 to 764c33c3c773 (1 revision) (flutter/flutter#148290)
2024-05-14 [email protected] Roll Flutter Engine from 84687fe0f199 to 797eab742925 (1 revision) (flutter/flutter#148288)
2024-05-14 [email protected] Roll Flutter Engine from bee398d95abe to 84687fe0f199 (3 revisions) (flutter/flutter#148282)
2024-05-14 [email protected] Roll Flutter Engine from a1cdcb6a6687 to bee398d95abe (2 revisions) (flutter/flutter#148280)
2024-05-14 [email protected] Roll Flutter Engine from 13a561cb6d5a to a1cdcb6a6687 (1 revision) (flutter/flutter#148276)
2024-05-14 [email protected] Roll Flutter Engine from 7dcbd93e5c1a to 13a561cb6d5a (6 revisions) (flutter/flutter#148274)
2024-05-13 [email protected] Roll Flutter Engine from aeff9b174c84 to 7dcbd93e5c1a (1 revision) (flutter/flutter#148266)
2024-05-13 [email protected] Mark platform_views_scroll_perf_ad_banners__timeline_summary not flaky (flutter/flutter#148263)
2024-05-13 [email protected] add more print traces in hot runner workflow (flutter/flutter#148258)
2024-05-13 [email protected] Refactors page API (flutter/flutter#137792)
2024-05-13 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.3 to 3.25.5 (flutter/flutter#148262)
2024-05-13 [email protected] Roll Flutter Engine from 11502404a52a to aeff9b174c84 (2 revisions) (flutter/flutter#148260)
2024-05-13 [email protected] Use super.key instead of manually passing the Key parameter to the parent class (flutter/flutter#147621)
2024-05-13 [email protected] Try fix module test (flutter/flutter#147934)
2024-05-13 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reland Native ios context menu (#143002) (#148238)" (flutter/flutter#148254)
2024-05-13 [email protected] Reland Native ios context menu (#143002) (flutter/flutter#148238)
2024-05-13 [email protected] Roll Flutter Engine from 636374fd00ee to 11502404a52a (1 revision) (flutter/flutter#148242)
2024-05-13 [email protected] Roll Packages from 6c4482a to 1412041 (16 revisions) (flutter/flutter#148239)
2024-05-13 [email protected] Roll Flutter Engine from 0050bf9a8094 to 636374fd00ee (1 revision) (flutter/flutter#148216)
2024-05-13 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Native ios context menu (#143002)" (flutter/flutter#148237)

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] 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

Piinks mentioned this pull request

Flutter Framework Team: Notable Commits #121415

Closed

TecHaxter pushed a commit to TecHaxter/flutter_packages that referenced this pull request


          Roll Flutter from 1255435 to d2da1b2 (26 revisions) (flutter#6729)

08f6120

flutter/flutter@1255435...d2da1b2

2024-05-14 [email protected] Roll Flutter Engine from 491f460a25fb to 7bf865774d06 (1 revision) (flutter/flutter#148325)
2024-05-14 [email protected] Roll Packages from 1412041 to fd714bd (1 revision) (flutter/flutter#148324)
2024-05-14 [email protected] Roll Flutter Engine from c381b852605f to 491f460a25fb (1 revision) (flutter/flutter#148319)
2024-05-14 [email protected] Roll Flutter Engine from b4b798d2e706 to c381b852605f (1 revision) (flutter/flutter#148303)
2024-05-14 [email protected] Roll Flutter Engine from 0de6701b537a to b4b798d2e706 (2 revisions) (flutter/flutter#148299)
2024-05-14 [email protected] Roll Flutter Engine from 764c33c3c773 to 0de6701b537a (2 revisions) (flutter/flutter#148297)
2024-05-14 [email protected] Roll Flutter Engine from 797eab742925 to 764c33c3c773 (1 revision) (flutter/flutter#148290)
2024-05-14 [email protected] Roll Flutter Engine from 84687fe0f199 to 797eab742925 (1 revision) (flutter/flutter#148288)
2024-05-14 [email protected] Roll Flutter Engine from bee398d95abe to 84687fe0f199 (3 revisions) (flutter/flutter#148282)
2024-05-14 [email protected] Roll Flutter Engine from a1cdcb6a6687 to bee398d95abe (2 revisions) (flutter/flutter#148280)
2024-05-14 [email protected] Roll Flutter Engine from 13a561cb6d5a to a1cdcb6a6687 (1 revision) (flutter/flutter#148276)
2024-05-14 [email protected] Roll Flutter Engine from 7dcbd93e5c1a to 13a561cb6d5a (6 revisions) (flutter/flutter#148274)
2024-05-13 [email protected] Roll Flutter Engine from aeff9b174c84 to 7dcbd93e5c1a (1 revision) (flutter/flutter#148266)
2024-05-13 [email protected] Mark platform_views_scroll_perf_ad_banners__timeline_summary not flaky (flutter/flutter#148263)
2024-05-13 [email protected] add more print traces in hot runner workflow (flutter/flutter#148258)
2024-05-13 [email protected] Refactors page API (flutter/flutter#137792)
2024-05-13 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.3 to 3.25.5 (flutter/flutter#148262)
2024-05-13 [email protected] Roll Flutter Engine from 11502404a52a to aeff9b174c84 (2 revisions) (flutter/flutter#148260)
2024-05-13 [email protected] Use super.key instead of manually passing the Key parameter to the parent class (flutter/flutter#147621)
2024-05-13 [email protected] Try fix module test (flutter/flutter#147934)
2024-05-13 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reland Native ios context menu (#143002) (#148238)" (flutter/flutter#148254)
2024-05-13 [email protected] Reland Native ios context menu (#143002) (flutter/flutter#148238)
2024-05-13 [email protected] Roll Flutter Engine from 636374fd00ee to 11502404a52a (1 revision) (flutter/flutter#148242)
2024-05-13 [email protected] Roll Packages from 6c4482a to 1412041 (16 revisions) (flutter/flutter#148239)
2024-05-13 [email protected] Roll Flutter Engine from 0050bf9a8094 to 636374fd00ee (1 revision) (flutter/flutter#148216)
2024-05-13 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Native ios context menu (#143002)" (flutter/flutter#148237)

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] 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

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request


          a36ff80 Refactors page API (flutter/flutter#137792)

b77edbb

SchabanBo mentioned this pull request

Deprecated "onPopPage" SchabanBo/qlevar_router#158

Open

jsroest mentioned this pull request

onDidRemovePage is called when deleting a page below. #160463

Open

maRci002 mentioned this pull request

Listen to results with navigator 2.0 #74195

Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit d: api docs d: examples f: cupertino f: material design f: routes framework