From 905c6187c58c7ab1099c3a6f24dd015d3558a861 Mon Sep 17 00:00:00 2001 From: Mike Matiunin Date: Thu, 1 Oct 2026 19:02:20 +0400 Subject: [PATCH 1/8] Release 0.1.1: fix the predictive back, the route keys and the observers The back button: on Android with the predictive back gesture a view tells the platform that the framework handles the press when its tabs can return to the previous tab or it has its own onBackButtonPressed, otherwise the application was closed without reaching didPopRoute. An inactive tab no longer reports the state of the back button for the visible one. The controller: observers of a change made during the build are notified at the end of the frame, a failed observer callback no longer swallows the remaining events, a guard returning an empty stack is reported instead of asserting and dropping the queue, pop and removeKey from a listener are postponed instead of being checked against the stack about to change, the result of a cancelled pop is dropped right away, a guard may dispose the controller. The routes: the default key escapes %, & and = in the arguments and writes a null argument without a value, so different arguments no longer collide. A page built from the context of the view is rebuilt when it changes. The CI publishes only from the tag of the version, the pub cache depends on the pubspecs and a .pubignore keeps the development files out of the package. The entries published with 0.1.0 are moved under 0.1.0. --- .github/actions/setup/action.yaml | 14 +-- .github/workflows/deploy.yml | 7 +- .pubignore | 67 +++++++++++++ CHANGELOG.md | 79 +++++++++++---- lib/src/controller.dart | 155 +++++++++++++++++++++++------- lib/src/guard.dart | 10 ++ lib/src/route.dart | 38 ++++++-- lib/src/tabs.dart | 13 +++ lib/src/view.dart | 82 ++++++++++++++-- pubspec.yaml | 2 +- test/context_test.dart | 50 ++++++++++ test/controller_test.dart | 126 ++++++++++++++++++++++++ test/observer_edge_test.dart | 14 +-- test/routes_edge_test.dart | 21 +++- test/tabs_test.dart | 73 ++++++++++++++ test/view_test.dart | 52 ++++++++++ 16 files changed, 706 insertions(+), 97 deletions(-) create mode 100644 .pubignore diff --git a/.github/actions/setup/action.yaml b/.github/actions/setup/action.yaml index f8d8a23..87b5607 100644 --- a/.github/actions/setup/action.yaml +++ b/.github/actions/setup/action.yaml @@ -17,18 +17,6 @@ runs: - name: 📦 Checkout the repo uses: actions/checkout@v4 - - name: 🔢 Set up version from tags - id: set-version - if: startsWith(github.ref, 'refs/tags') - shell: bash - run: | - BASE_VERSION="${GITHUB_REF#refs/tags/v}" - UNIXTIME=$(date +%s) - VERSION="${BASE_VERSION}+${UNIXTIME}" - echo "VERSION=$VERSION" >> $GITHUB_ENV - sed -i "s/^version: .*/version: ${VERSION}/" pubspec.yaml - echo "Version set to $VERSION" - - name: 🚂 Setup Flutter uses: subosito/flutter-action@v2 with: @@ -41,7 +29,7 @@ runs: with: path: | /home/runner/.pub-cache - key: ${{ runner.os }}-pub-${{ inputs.pub-cache }}-${{ hashFiles('pubspec.lock') }} + key: ${{ runner.os }}-pub-${{ inputs.pub-cache }}-${{ hashFiles('pubspec.yaml', 'example/pubspec.yaml') }} - name: 👷 Install Dependencies shell: bash diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 1110c84..5edf2ba 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -39,9 +39,14 @@ jobs: flutter pub get flutter pub get --directory example + # Also guards a manual run: it must be started from the tag of the + # version, never from a branch. - name: 🔢 Check that the tag matches the version - if: startsWith(github.ref, 'refs/tags/') run: | + if [ "${GITHUB_REF_TYPE}" != "tag" ]; then + echo "::error::Publish from a tag, not from ${GITHUB_REF}" + exit 1 + fi TAG="${GITHUB_REF_NAME}" VERSION="$(awk '/^version:/ {print $2; exit}' pubspec.yaml)" if [ "$TAG" != "$VERSION" ]; then diff --git a/.pubignore b/.pubignore new file mode 100644 index 0000000..fa6b6aa --- /dev/null +++ b/.pubignore @@ -0,0 +1,67 @@ +# pub ignores .gitignore when this file exists, so it repeats it. + +# Miscellaneous +*.class +*.log +*.pyc +*.swp +.DS_Store +.atom/ +.buildlog/ +.history +.svn/ +migrate_working_dir/ + +# IntelliJ related +*.iml +*.ipr +*.iws +.idea/ + +# The .vscode folder contains launch configuration and tasks you configure in +# VS Code which you may wish to be included in version control, so this line +# is commented out by default. +#.vscode/ + +# Flutter/Dart/Pub related +# Libraries should not include pubspec.lock, per https://dart.dev/guides/libraries/private-files#pubspeclock. +/pubspec.lock +.dart_tool +build/ +**/doc/api/ +**/ios/Flutter/.last_build_id +.flutter-plugins +.flutter-plugins-dependencies +.packages +.pub-cache/ + +# Pana +log.pana.json + +# Test +.coverage/ +coverage/ +doc/api/ +/test/**/*.json +/test/.test_coverage.dart +reports/ + +# Temp +/tmp +/temp + +# FVM +.fvm/flutter_sdk + +# Generated files +*.*.dart + +# LLMS +.claude/ +# Example +/example/pubspec.lock + +# Development only +Makefile +.vscode/ +.github/ diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a3f70a..8fe69e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,8 +2,65 @@ All notable changes to this project will be documented in this file. -## Unreleased +## 0.1.1 +- **Fixed**: on Android with the predictive back gesture, the back button + no longer closes the application when a `NavigationTabsController` can + return to the previous tab or a `NavigationView` has its own + `onBackButtonPressed`: the view tells the platform that the framework + handles the press, and an inactive tab no longer speaks for the visible + one. +- **Fixed**: a page built from the context of the view (the locale, the + theme, an inherited scope read in `NavigationRoute.page`) is built again + when that context changes instead of being taken from the cache. +- **Fixed**: the default `key` of a route escapes `%`, `&` and `=` in the + arguments and writes a `null` argument without a value, so different + arguments, e.g. `{'q': 'a&b=c'}` and `{'q': 'a', 'b': 'c'}`, or `null` and + `'null'`, no longer produce the same key. +- **Fixed**: observers are notified at the end of the frame, together with + the listeners, when a `ContextGuard` changes the stack during the build, + so an observer calling `setState` no longer throws. +- **Fixed**: an exception in one callback of an observer no longer swallows + the remaining events of the same transition for that observer. +- **Fixed**: a guard returning an empty stack is reported to `FlutterError` + instead of throwing an assertion, which dropped the changes queued after + it in a debug build. +- **Fixed**: `pop` and `removeKey` called from a guard, an observer or a + listener are postponed instead of being checked against the stack that is + about to change, e.g. `push(route)` followed by `remove(route)`. +- **Fixed**: the result of a `pop` cancelled by a guard is no longer passed + to a later removal of the same route. +- **Fixed**: a guard disposing the controller no longer leads to a + notification of a disposed controller. +- **Docs**: a guard keeping a route closed by the user brings back a new + instance of it, use a `PopScope` to prevent the closing. +- **Docs**: the name of a class route is minified in an obfuscated build, + routes compare by instance in a plain `List`, overridden `tags` must keep + `kModalTag`, `pushForResult` completes the previous waiter of the same + route with `null`, `controller.stack` while the guards run. +- **CI**: a manual publication must start from the tag of the version, the + pub cache depends on the pubspecs, a `.pubignore` keeps the development + files out of the package. + +## 0.1.0 + +- Initial release. +- `NavigationRoute` — a route is an immutable value: an `enum` value when it + has no parameters, a `sealed class` when it has. +- `NavigationController` — the owner of the stack, usable without any widget, + with guards, observers and revalidation built in. +- `NavigationView` — renders a controller with a `Navigator`, reports the + routes closed by the user and handles the system back button. +- `DialogRouteMixin` and `BottomSheetRouteMixin` — dialogs and bottom sheets + as ordinary entries of the navigation stack. +- `NavigationGuard` with the built-in `PriorityGuard`, `RootGuard`, + `LimitGuard` and `GateGuard`. +- `ContextGuard` — the rules that depend on the widget tree (the size of the + window, the theme, an inherited scope) re-run by themselves whenever what + they read changes. +- `NavigationObserver` and `NavigationLogger`. +- `NavigationTabsController` — several stacks side by side, cross tab + navigation and automatic routing of the back button. - **Docs**: "Helpers of your application" — an extension type over `BuildContext` with application specific shortcuts and `popModals`, which closes the declarative and the imperative popups at once. @@ -40,23 +97,3 @@ All notable changes to this project will be documented in this file. `opened()`, the same way a closed gate falls back to `closed()`. - **Fixed**: replacing the controller of a `NavigationView` runs its guards with the context of the view, so a `ContextGuard` sees it immediately. - -## 0.1.0 - -- Initial release. -- `NavigationRoute` — a route is an immutable value: an `enum` value when it - has no parameters, a `sealed class` when it has. -- `NavigationController` — the owner of the stack, usable without any widget, - with guards, observers and revalidation built in. -- `NavigationView` — renders a controller with a `Navigator`, reports the - routes closed by the user and handles the system back button. -- `DialogRouteMixin` and `BottomSheetRouteMixin` — dialogs and bottom sheets - as ordinary entries of the navigation stack. -- `NavigationGuard` with the built-in `PriorityGuard`, `RootGuard`, - `LimitGuard` and `GateGuard`. -- `ContextGuard` — the rules that depend on the widget tree (the size of the - window, the theme, an inherited scope) re-run by themselves whenever what - they read changes. -- `NavigationObserver` and `NavigationLogger`. -- `NavigationTabsController` — several stacks side by side, cross tab - navigation and automatic routing of the back button. diff --git a/lib/src/controller.dart b/lib/src/controller.dart index a7315e9..86e844c 100644 --- a/lib/src/controller.dart +++ b/lib/src/controller.dart @@ -30,6 +30,10 @@ abstract interface class NavigationAttachment { /// Navigator driven by the controller, `null` before the first build. NavigatorState? get navigator; + + /// Called when the group of the controller has changed, e.g. another tab + /// has been selected, so the widget may handle the back button differently. + void didChangeGroup(); } /// A group of controllers where only one of them is active at a time. @@ -45,6 +49,9 @@ abstract interface class NavigationGroup { /// Returns `true` when the group switched back to the previously /// selected member. bool popGroupMember(); + + /// Whether [popGroupMember] would switch to another member. + bool get canPopGroupMember; } /// {@template squid.controller} @@ -129,6 +136,10 @@ class NavigationController final Map _results = {}; /// Maximum number of changes requested from within a single transition. + /// + /// The changes over the limit are reported to `FlutterError` and dropped: + /// use [pushAll] or a single [change] instead of many separate calls from + /// a listener. static const int _maxQueuedChanges = 64; NavigationStack _stack; @@ -142,6 +153,11 @@ class NavigationController bool _deferNotifications = false; bool _notificationScheduled = false; + /// Transitions waiting to be reported to the [observers] at the end of the + /// frame, see [revalidateDuringBuild]. + final List<(NavigationStack, NavigationStack)> _deferredObserverEvents = + <(NavigationStack, NavigationStack)>[]; + /* #region State */ /// Current navigation stack, an unmodifiable list ordered from the root @@ -292,6 +308,10 @@ class NavigationController /// `null` when the route is closed in any other way: a swipe back, a tap on /// the barrier of a dialog, the system back button or a guard. /// + /// Only one caller can wait for a route: pushing a route with the same + /// [NavigationRoute.key] again completes the previous future with `null` + /// right away, even though the route stays on the screen. + /// /// ```dart /// final confirmed = /// await controller.pushForResult(const ConfirmDialog()); @@ -316,8 +336,12 @@ class NavigationController /// /// Does nothing and returns `false` when there is only one route left, /// because an empty stack is not allowed. + /// + /// When called from a guard, an observer or a listener, the pop is + /// postponed until the current change is committed and `true` only means + /// that it has been scheduled. bool pop([Object? result]) { - if (_stack.length < 2) return false; + if (!_processing && _stack.length < 2) return false; // The result is bound to the route that is actually removed, which is // not necessarily the current top when the call is postponed. change((stack) { @@ -370,8 +394,13 @@ class NavigationController /// Removes the route with the given [key] and completes its /// [pushForResult] future with [result]. + /// + /// Returns `false` when the stack has no such route. When called from a + /// guard, an observer or a listener, the removal is postponed until the + /// current change is committed and `true` only means that it has been + /// scheduled. bool removeKey(LocalKey key, [Object? result]) { - if (!containsKey(key)) return false; + if (!_processing && !containsKey(key)) return false; change((stack) { final length = stack.length; stack.removeWhere((route) => route.key == key); @@ -416,6 +445,19 @@ class NavigationController @internal bool get isActiveInGroup => _group?.isActiveMember(this) ?? true; + /// Whether the group of this controller handles a back button press that + /// the controller itself cannot handle. + @internal + bool get canPopGroupMember => _group?.canPopGroupMember ?? false; + + /// Tells the widgets rendering this controller that its group has changed. + @internal + void didChangeGroup() { + for (final attachment in List.of(_attachments)) { + attachment.didChangeGroup(); + } + } + /// Joins a group of controllers, e.g. a set of tabs. @internal // ignore: use_setters_to_change_properties @@ -429,30 +471,40 @@ class NavigationController /// Applies a single change: mutation, guards and commit. void _transition(NavigationChange fn) { - final previous = _stack; - NavigationStack next; try { - next = fn(List.of(previous)); - } on Object catch (error, stackTrace) { - _reportError(error, stackTrace, 'changing the navigation stack'); - return; - } - next = _dedupe(_runGuards(_dedupe(next))); - if (next.isEmpty) { - assert( - false, - 'The navigation stack of $this cannot be empty, ' - 'the change has been rejected', + final previous = _stack; + NavigationStack next; + try { + next = fn(List.of(previous)); + } on Object catch (error, stackTrace) { + _reportError(error, stackTrace, 'changing the navigation stack'); + return; + } + next = _dedupe(_runGuards(_dedupe(next))); + // A guard may dispose the controller, e.g. on sign out. + if (_disposed) return; + if (next.isEmpty) { + // Reported instead of asserted: an exception here would drop the + // changes queued after this one. + _reportError( + StateError('The navigation stack cannot be empty'), + StackTrace.current, + 'committing a change rejected by the guards', + ); + return; + } + if (_sameStack(previous, next)) return; + _stack = UnmodifiableListView( + List.of(next), ); - return; + _completeRemoved(previous, next); + _notifyListeners(); + _notifyObservers(previous, next); + } finally { + // A result belongs to the change that set it: the result of a pop + // cancelled by a guard must not leak into a later removal. + _results.clear(); } - if (_sameStack(previous, next)) return; - _stack = UnmodifiableListView( - List.of(next), - ); - _completeRemoved(previous, next); - _notifyListeners(); - _notifyObservers(previous, next); } /// Runs all the guards, skipping the failed ones. @@ -518,12 +570,26 @@ class NavigationController void _notifyListeners() { if (!_deferNotifications) return notifyListeners(); + _scheduleNotifications(); + } + + /// Notifies the listeners and the observers at the end of the frame. + void _scheduleNotifications() { if (_notificationScheduled) return; _notificationScheduled = true; SchedulerBinding.instance ..addPostFrameCallback((_) { _notificationScheduled = false; - if (!_disposed) notifyListeners(); + if (_disposed) return; + notifyListeners(); + final events = List<(NavigationStack, NavigationStack)>.of( + _deferredObserverEvents, + ); + _deferredObserverEvents.clear(); + for (final (previous, next) in events) { + if (_disposed) return; + _dispatchObservers(previous, next); + } }, debugLabel: 'NavigationController.notifyListeners') ..ensureVisualUpdate(); } @@ -549,25 +615,41 @@ class NavigationController void _notifyObservers(NavigationStack previous, NavigationStack next) { if (observers.isEmpty) return; + if (!_deferNotifications && _deferredObserverEvents.isEmpty) { + return _dispatchObservers(previous, next); + } + // An observer may mark widgets dirty, which is not allowed during the + // build: report the transition at the end of the frame, in order. + _deferredObserverEvents.add((previous, next)); + _scheduleNotifications(); + } + + void _dispatchObservers(NavigationStack previous, NavigationStack next) { final before = {for (final route in previous) route.key}; final after = {for (final route in next) route.key}; for (final observer in observers) { - try { - observer.onChange(this, previous, next); - for (final route in next) { - if (before.contains(route.key)) continue; - observer.onAdd(this, route); - } - for (final route in previous) { - if (after.contains(route.key)) continue; - observer.onRemove(this, route); - } - } on Object catch (error, stackTrace) { - _reportError(error, stackTrace, 'notifying the observer $observer'); + // Every callback is isolated, so a failed one does not swallow the + // remaining events of the same observer. + _notifyObserver(observer, () => observer.onChange(this, previous, next)); + for (final route in next) { + if (before.contains(route.key)) continue; + _notifyObserver(observer, () => observer.onAdd(this, route)); + } + for (final route in previous) { + if (after.contains(route.key)) continue; + _notifyObserver(observer, () => observer.onRemove(this, route)); } } } + void _notifyObserver(NavigationObserver observer, void Function() fn) { + try { + fn(); + } on Object catch (error, stackTrace) { + _reportError(error, stackTrace, 'notifying the observer $observer'); + } + } + void _reportError(Object error, StackTrace stackTrace, String context) => FlutterError.reportError( FlutterErrorDetails( @@ -591,6 +673,7 @@ class NavigationController } _completers.clear(); _results.clear(); + _deferredObserverEvents.clear(); super.dispose(); } diff --git a/lib/src/guard.dart b/lib/src/guard.dart index 5dd61e0..fb09e6b 100644 --- a/lib/src/guard.dart +++ b/lib/src/guard.dart @@ -38,6 +38,16 @@ typedef NavigationGuardCallback = /// * a different stack — the navigation is redirected; /// * `controller.stack` — the change is cancelled. /// +/// A route closed by the user — a swipe back, a tap on the barrier of a +/// dialog — has already left the navigator when the guards see its removal. +/// A guard keeping it brings back a new instance of the route, with a new +/// state. To prevent the user from closing a route, use a [PopScope] or the +/// `canPop` of its page instead. +/// +/// While a change is validated, `controller.stack` is the stack committed +/// before it. The only exception is the validation of the initial stack in +/// the constructor, where `controller.stack` is already the requested stack. +/// /// Guards are applied in the order they are declared, each one receiving the /// result of the previous one. An exception thrown by a guard is reported /// to `FlutterError` and the guard is skipped, so a broken rule cannot break diff --git a/lib/src/route.dart b/lib/src/route.dart index d344dd1..048bf56 100644 --- a/lib/src/route.dart +++ b/lib/src/route.dart @@ -6,6 +6,12 @@ import 'package:flutter/material.dart'; /// This is a plain [List], so any operation you already know works here: /// `[...stack, route]`, `stack.where(...)`, `stack.sublist(...)` and so on. /// +/// Routes do not override `==`, so `stack.contains(route)`, +/// `stack.indexOf(route)` and `stack.remove(route)` compare instances, while +/// the controller identifies a route by its [NavigationRoute.key]: prefer +/// `controller.contains(route)`, `stack.containsKey(route.key)` and +/// `stack.without(route)` for the routes that are not constants. +/// /// {@template squid.stack} /// The last element is the visible one, the first element is the root of /// the navigation. An empty stack is not allowed and will be rejected @@ -61,6 +67,10 @@ mixin NavigationRoute { /// Used for debugging, analytics and as a part of the default [key]. /// Defaults to the enum value name for enums and to the runtime type /// for classes. + /// + /// The runtime type is minified in an obfuscated build (`--obfuscate`, a + /// release build for the web), so a class used for analytics, for logs or + /// with `findName` must override this getter with a constant string. String get name => switch (this) { Enum e => e.name, _ => runtimeType.toString(), @@ -73,7 +83,9 @@ mixin NavigationRoute { /// Keys must be unique within a single stack. /// /// The default value is built from [name] and [arguments], which is enough - /// for enums and for parameterless routes. **Routes that carry their + /// for enums and for parameterless routes: `name?a=1&b=2`, in the order of + /// the entries of the map, with `%`, `&` and `=` escaped and a `null` + /// value written as a bare `name?a`. **Routes that carry their /// parameters as fields must override this getter**, otherwise all of them /// will collide: /// @@ -86,20 +98,29 @@ mixin NavigationRoute { if (args.isEmpty) return ValueKey(name); final buffer = StringBuffer(name)..write('?'); var first = true; - for (final entry in args.entries) { + for (final MapEntry(key: argument, :value) in args.entries) { if (first) { first = false; } else { buffer.write('&'); } - buffer - ..write(entry.key) - ..write('=') - ..write(entry.value); + buffer.write(_escape(argument)); + if (value != null) { + buffer + ..write('=') + ..write(_escape(value.toString())); + } } return ValueKey(buffer.toString()); } + /// Escapes the separators of the default [key], so that different + /// arguments never produce the same key. + static String _escape(String value) => value + .replaceAll('%', '%25') + .replaceAll('&', '%26') + .replaceAll('=', '%3D'); + /// Arbitrary marks used to address groups of routes declaratively. /// /// Tags are the cheapest way to write a guard or a bulk operation without @@ -110,6 +131,11 @@ mixin NavigationRoute { /// controller.change((stack) => /// stack.where((route) => !route.tags.contains('modal')).toList()); /// ``` + /// + /// [DialogRouteMixin] and [BottomSheetRouteMixin] mark their routes with + /// [kModalTag]: a route overriding the tags must keep it, + /// `{...super.tags, 'paywall'}`, otherwise `removeTag(kModalTag)` will not + /// close it. Set get tags => const {}; /// Ordering hint for the [PriorityGuard]. diff --git a/lib/src/tabs.dart b/lib/src/tabs.dart index 553d305..8be5787 100644 --- a/lib/src/tabs.dart +++ b/lib/src/tabs.dart @@ -120,6 +120,7 @@ class NavigationTabsController ..add(_active); _active = tab; notifyListeners(); + _didChangeGroup(); } /// Returns to the previously selected tab. @@ -129,9 +130,18 @@ class NavigationTabsController if (_history.isEmpty) return false; _active = _history.removeLast(); notifyListeners(); + _didChangeGroup(); return true; } + /// Lets the views of the tabs report to the platform whether the back + /// button is handled, e.g. by returning to the previous tab. + void _didChangeGroup() { + for (final controller in tabs.values) { + controller.didChangeGroup(); + } + } + /// Tries to close the visible route of the active tab, and returns to the /// previously selected tab when it cannot be closed. /// @@ -145,6 +155,9 @@ class NavigationTabsController @override bool popGroupMember() => back(); + @override + bool get canPopGroupMember => _history.isNotEmpty; + @override void dispose() { for (final controller in tabs.values) { diff --git a/lib/src/view.dart b/lib/src/view.dart index fc8a2ea..d13b76b 100644 --- a/lib/src/view.dart +++ b/lib/src/view.dart @@ -1,4 +1,5 @@ import 'package:flutter/foundation.dart'; +import 'package:flutter/scheduler.dart'; import 'package:flutter/widgets.dart'; import 'package:squid/src/controller.dart'; import 'package:squid/src/route.dart'; @@ -135,6 +136,8 @@ class _NavigationViewState extends State /// about a stack this view has already built. NavigationStack? _built; + bool _reportScheduled = false; + @override NavigatorState? get navigator => _spy.navigator; @@ -196,6 +199,11 @@ class _NavigationViewState extends State // // When no guard reads anything, this element has no dependencies at all // and the callback is invoked exactly once, right after the mount. + // + // `NavigationRoute.page` is built with this context too: a page reading + // the locale or the theme has to be built again, not taken from the + // cache. + _cache.clear(); widget.controller.revalidateDuringBuild(); } @@ -205,6 +213,12 @@ class _NavigationViewState extends State if (!identical(widget.observers, oldWidget.observers)) { _observers = [_spy, ...widget.observers]; } + if (!identical(widget.controller, oldWidget.controller) || + widget.interceptBackButton != oldWidget.interceptBackButton || + (widget.onBackButtonPressed == null) != + (oldWidget.onBackButtonPressed == null)) { + _reportBackButton(); + } if (identical(widget.controller, oldWidget.controller)) return; oldWidget.controller ..removeListener(_handleStackChanged) @@ -260,6 +274,53 @@ class _NavigationViewState extends State return widget.controller.maybePop(); } + /// Whether this view handles the back button even when its navigator + /// cannot pop: it has its own handler or its tabs return to the previous + /// one. + bool get _claimsBackButton => + widget.interceptBackButton && + widget.controller.isActiveInGroup && + (widget.onBackButtonPressed != null || + widget.controller.canPopGroupMember); + + /// Tells the platform whether the framework handles the back button. + /// + /// The navigator reports `canHandlePop: false` when it has a single route, + /// so on Android with the predictive back gesture the press would close + /// the application without ever reaching [didPopRoute]. + bool _handleNavigationNotification(NavigationNotification notification) { + // An inactive tab kept alive by an `IndexedStack` must not speak for + // the visible one. + if (widget.interceptBackButton && !widget.controller.isActiveInGroup) { + return true; + } + if (notification.canHandlePop || !_claimsBackButton) return false; + const NavigationNotification(canHandlePop: true).dispatch(context); + return true; + } + + /// Reports the current state of the back button, when it may have changed + /// without a change of the navigator, e.g. another tab has been selected. + void _reportBackButton() { + if (_reportScheduled) return; + _reportScheduled = true; + SchedulerBinding.instance.addPostFrameCallback((_) { + _reportScheduled = false; + if (!mounted || !_visible) return; + if (!widget.interceptBackButton || !widget.controller.isActiveInGroup) { + return; + } + final canPop = navigator?.canPop() ?? false; + NavigationNotification( + canHandlePop: canPop || _claimsBackButton, + ).dispatch(context); + }, debugLabel: 'NavigationView.reportBackButton'); + SchedulerBinding.instance.ensureVisualUpdate(); + } + + @override + void didChangeGroup() => _reportBackButton(); + /// Called by the [Navigator] when a route has been closed by the user /// or by the framework: a swipe back, a tap on the barrier of a dialog, /// the button of an [AppBar], a `Navigator.pop(context, result)`. @@ -307,15 +368,18 @@ class _NavigationViewState extends State onRoute: (route) => _route = route, child: NavigationScope( controller: widget.controller, - child: Navigator( - pages: _buildPages(context), - onDidRemovePage: _handleDidRemovePage, - transitionDelegate: widget.transitionDelegate, - observers: _observers, - restorationScopeId: widget.restorationScopeId, - requestFocus: widget.requestFocus, - clipBehavior: widget.clipBehavior, - reportsRouteUpdateToEngine: widget.reportsRouteUpdateToEngine, + child: NotificationListener( + onNotification: _handleNavigationNotification, + child: Navigator( + pages: _buildPages(context), + onDidRemovePage: _handleDidRemovePage, + transitionDelegate: widget.transitionDelegate, + observers: _observers, + restorationScopeId: widget.restorationScopeId, + requestFocus: widget.requestFocus, + clipBehavior: widget.clipBehavior, + reportsRouteUpdateToEngine: widget.reportsRouteUpdateToEngine, + ), ), ), ), diff --git a/pubspec.yaml b/pubspec.yaml index ad15d8f..48fd6cd 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -1,7 +1,7 @@ name: squid description: "A simple, reliable and extensible declarative navigator for Flutter. The whole navigation is a plain list of routes you can rewrite at any moment." -version: 0.1.0 +version: 0.1.1 homepage: https://github.com/PlugFox/squid diff --git a/test/context_test.dart b/test/context_test.dart index f320672..eded30c 100644 --- a/test/context_test.dart +++ b/test/context_test.dart @@ -230,6 +230,43 @@ void main() => group('context', () { ); } + testWidgets('an observer marking an ancestor dirty during a build time ' + 'change is notified at the end of the frame', (tester) async { + final current = ValueNotifier(''); + addTearDown(current.dispose); + final controller = NavigationController( + [Routes.home], + guards: [const _AdaptiveGuard()], + observers: [_TopObserver(current)], + ); + addTearDown(controller.dispose); + + Widget app(double width) => MaterialApp( + home: MediaQuery( + data: MediaQueryData(size: Size(width, 600)), + child: ValueListenableBuilder( + valueListenable: current, + builder: (context, top, _) => Column( + children: [ + Text('observed:$top'), + Expanded(child: NavigationView(controller: controller)), + ], + ), + ), + ), + ); + + await tester.pumpWidget(app(800)); + await tester.pumpAndSettle(); + expect(tester.takeException(), isNull); + expect(find.text('observed:_SidePaneRoute'), findsOneWidget); + + await tester.pumpWidget(app(300)); + await tester.pumpAndSettle(); + expect(tester.takeException(), isNull); + expect(find.text('observed:home'), findsOneWidget); + }); + testWidgets('a page reading the stack follows a build time change ' 'in the same frame', (tester) async { final controller = NavigationController( @@ -262,3 +299,16 @@ final class _StackLengthRoute with NavigationRoute { Widget build(BuildContext context) => Text('length:${NavigationScope.stackOf(context).length}'); } + +class _TopObserver with NavigationObserver { + _TopObserver(this.current); + + final ValueNotifier current; + + @override + void onChange( + NavigationController controller, + NavigationStack previous, + NavigationStack next, + ) => current.value = next.last.name; +} diff --git a/test/controller_test.dart b/test/controller_test.dart index 567e192..648527c 100644 --- a/test/controller_test.dart +++ b/test/controller_test.dart @@ -504,8 +504,134 @@ void main() => group('controller', () { expect(controller.isDisposed, isTrue); expect(controller.top, equals(Routes.catalog)); }); + + test('a guard returning an empty stack is reported, the queue goes on', () { + final errors = []; + final previous = FlutterError.onError; + FlutterError.onError = (details) => errors.add(details.exception); + addTearDown(() => FlutterError.onError = previous); + + final controller = NavigationController( + [Routes.home], + guards: [ + NavigationGuard( + (controller, stack) => stack.containsKey(Routes.signIn.key) + ? [] + : stack, + ), + ], + ); + addTearDown(controller.dispose); + + var queued = false; + controller.addListener(() { + if (queued) return; + queued = true; + controller + ..push(Routes.signIn) + ..push(Routes.settings); + }); + controller.push(Routes.catalog); + + expect(errors, hasLength(1)); + expect(errors.single, isStateError); + expect( + controller.stack, + equals([Routes.home, Routes.catalog, Routes.settings]), + ); + }); + + test('pop and remove from a listener see the pending changes', () { + final controller = NavigationController([Routes.home]); + addTearDown(controller.dispose); + + var queued = false; + controller.addListener(() { + if (queued) return; + queued = true; + controller.push(const ProductRoute(1)); + expect(controller.remove(const ProductRoute(1)), isTrue); + controller.push(Routes.settings); + }); + controller.push(Routes.catalog); + + expect( + controller.stack, + equals([Routes.home, Routes.catalog, Routes.settings]), + ); + }); + + test('a stale result of a cancelled pop is not used by a queued removal', () { + final locked = Flag(value: true); + final controller = NavigationController( + [Routes.home], + guards: [ + NavigationGuard( + (controller, stack) => + locked.value && + !stack.containsTag(kModalTag) && + controller.stack.containsTag(kModalTag) + ? controller.stack + : stack, + ), + ], + ); + addTearDown(controller.dispose); + final result = controller.pushForResult(const ConfirmDialogRoute()); + + // The pop is cancelled, then a removal without a result is queued. + controller.change((stack) { + controller + ..pop(true) + ..change((stack) { + locked.value = false; + return stack.withoutTag(kModalTag); + }); + return stack; + }); + + expect(controller.top, equals(Routes.home)); + expect(result, completion(isNull)); + }); + + test('a failed callback does not swallow the other events', () { + final errors = []; + final previous = FlutterError.onError; + FlutterError.onError = (details) => errors.add(details.exception); + addTearDown(() => FlutterError.onError = previous); + + final observer = _ThrowingOnAddObserver(); + final controller = NavigationController( + [Routes.home, Routes.catalog], + observers: [observer], + ); + addTearDown(controller.dispose); + + controller.change( + (stack) => [Routes.home, Routes.settings, Routes.signIn], + ); + + expect(errors, hasLength(2)); + expect(observer.added, equals(['settings', 'signIn'])); + expect(observer.removed, equals(['catalog'])); + }); }); +class _ThrowingOnAddObserver with NavigationObserver { + final List added = []; + final List removed = []; + + @override + void onAdd(NavigationController controller, NavigationRoute route) { + added.add(route.name); + throw StateError('onAdd'); + } + + @override + void onRemove(NavigationController controller, NavigationRoute route) => + removed.add(route.name); +} + class _RecordingObserver with NavigationObserver { final List added = []; final List removed = []; diff --git a/test/observer_edge_test.dart b/test/observer_edge_test.dart index fe6e73e..165b28c 100644 --- a/test/observer_edge_test.dart +++ b/test/observer_edge_test.dart @@ -179,8 +179,8 @@ void main() => group('observer edge cases', () { ); expect( first.events, - equals(['change']), - reason: 'its own onAdd is skipped after the throw', + equals(['change', 'add:catalog']), + reason: 'its own onAdd still runs after the throw', ); expect(second.events, equals(['change', 'add:catalog'])); expect(third.events, equals(['change', 'add:catalog'])); @@ -199,8 +199,7 @@ void main() => group('observer edge cases', () { expect(errors, hasLength(3), reason: 'one per throwing callback'); }); - test('a throw from onAdd skips the remaining routes of that observer ' - 'only', () { + test('a throw from onAdd does not skip the remaining routes', () { final broken = _ThrowingObserver(_Stage.add); final recording = _RecordingObserver(); final controller = NavigationController( @@ -210,12 +209,15 @@ void main() => group('observer edge cases', () { addTearDown(controller.dispose); controller.pushAll([Routes.catalog, Routes.settings]); - expect(broken.events, equals(['change', 'add:catalog'])); + expect( + broken.events, + equals(['change', 'add:catalog', 'add:settings']), + ); expect( recording.events, equals(['change', 'add:catalog', 'add:settings']), ); - expect(errors, hasLength(1)); + expect(errors, hasLength(2), reason: 'one per throwing callback'); expect(controller.length, equals(3)); }); }); diff --git a/test/routes_edge_test.dart b/test/routes_edge_test.dart index 79f3f76..747a2e9 100644 --- a/test/routes_edge_test.dart +++ b/test/routes_edge_test.dart @@ -77,6 +77,21 @@ void main() => group('routes edge cases', () { ); }); + test('the separators of the arguments are escaped', () { + const joined = _ArgumentsRoute({'q': 'a&b=c'}); + const split = _ArgumentsRoute({'q': 'a', 'b': 'c'}); + expect( + joined.key, + equals(const ValueKey('_ArgumentsRoute?q=a%26b%3Dc')), + ); + expect(joined.key, isNot(equals(split.key))); + const percent = _ArgumentsRoute({'q': '%26'}); + expect(percent.key, isNot(equals(joined.key))); + const nothing = _ArgumentsRoute({'id': null}); + const text = _ArgumentsRoute({'id': 'null'}); + expect(nothing.key, isNot(equals(text.key))); + }); + test('the arguments are written in their declaration order', () { const ab = _ArgumentsRoute({'a': 1, 'b': 2}); const ba = _ArgumentsRoute({'b': 2, 'a': 1}); @@ -85,7 +100,7 @@ void main() => group('routes edge cases', () { expect(ab.key, isNot(equals(ba.key))); }); - test('null, boolean and collection arguments are written as strings', () { + test('a null argument has no value, the others are written as strings', () { const route = _ArgumentsRoute({ 'id': null, 'flag': true, @@ -94,9 +109,7 @@ void main() => group('routes edge cases', () { expect( route.key, equals( - const ValueKey( - '_ArgumentsRoute?id=null&flag=true&ids=[1, 2]', - ), + const ValueKey('_ArgumentsRoute?id&flag=true&ids=[1, 2]'), ), ); }); diff --git a/test/tabs_test.dart b/test/tabs_test.dart index b0e7bd3..0b2bf12 100644 --- a/test/tabs_test.dart +++ b/test/tabs_test.dart @@ -155,6 +155,79 @@ void main() => group('tabs', () { expect(tabs.active, equals(_Tab.feed), reason: 'back to the previous tab'); }); + testWidgets('the platform is told when the tabs handle the back button', ( + tester, + ) async { + // On Android the predictive back gesture closes the application on its + // own unless the framework claims the press. + bool? handlesBack; + await tester.pumpWidget( + MaterialApp( + onNavigationNotification: (notification) { + handlesBack = notification.canHandlePop; + return true; + }, + home: ListenableBuilder( + listenable: tabs, + builder: (context, _) => IndexedStack( + index: tabs.activeIndex, + children: [ + for (final controller in tabs.tabs.values) + NavigationView(controller: controller), + ], + ), + ), + ), + ); + await tester.pumpAndSettle(); + expect(handlesBack, isFalse, reason: 'a single route, no previous tab'); + + tabs.select(_Tab.cart); + await tester.pumpAndSettle(); + expect(handlesBack, isTrue, reason: 'returns to the previous tab'); + + tabs[_Tab.cart].push(const ProductRoute(1)); + await tester.pumpAndSettle(); + expect(handlesBack, isTrue); + + tabs[_Tab.cart].pop(); + await tester.pumpAndSettle(); + expect(handlesBack, isTrue, reason: 'still returns to the feed'); + + expect(await tester.binding.handlePopRoute(), isTrue); + await tester.pumpAndSettle(); + expect(tabs.active, equals(_Tab.feed)); + expect(handlesBack, isFalse, reason: 'nothing left to go back to'); + }); + + testWidgets('a view with its own back handler claims the back button', ( + tester, + ) async { + final controller = NavigationController([Routes.home]); + addTearDown(controller.dispose); + bool? handlesBack; + var presses = 0; + await tester.pumpWidget( + MaterialApp( + onNavigationNotification: (notification) { + handlesBack = notification.canHandlePop; + return true; + }, + home: NavigationView( + controller: controller, + onBackButtonPressed: (controller) async { + presses++; + return true; + }, + ), + ), + ); + await tester.pumpAndSettle(); + expect(handlesBack, isTrue); + expect(await tester.binding.handlePopRoute(), isTrue); + expect(presses, equals(1)); + }); + testWidgets('switching tabs keeps the stack of every tab', (tester) async { await tester.pumpWidget( MaterialApp( diff --git a/test/view_test.dart b/test/view_test.dart index 0deb853..5bf843d 100644 --- a/test/view_test.dart +++ b/test/view_test.dart @@ -7,6 +7,29 @@ import 'package:squid/squid.dart'; import 'src/routes.dart'; void main() => group('view', () { + testWidgets('a page reading the context is built again when it changes', ( + tester, + ) async { + final controller = NavigationController([ + const _TitledRoute(), + ]); + addTearDown(controller.dispose); + + Widget app(String title) => MaterialApp( + home: _Title( + title: title, + child: NavigationView(controller: controller), + ), + ); + + await tester.pumpWidget(app('first')); + expect(find.text('title:first'), findsOneWidget); + + await tester.pumpWidget(app('second')); + await tester.pumpAndSettle(); + expect(find.text('title:second'), findsOneWidget); + }); + Future pumpView(WidgetTester tester, NavigationController controller) => tester.pumpWidget( MaterialApp(home: NavigationView(controller: controller)), @@ -614,3 +637,32 @@ class _WidthGuard extends ContextGuard { ? stack.withRoute(Routes.settings) : stack; } + +class _Title extends InheritedWidget { + const _Title({required this.title, required super.child}); + + final String title; + + @override + bool updateShouldNotify(_Title oldWidget) => oldWidget.title != title; +} + +final class _TitledRoute with NavigationRoute { + const _TitledRoute(); + + @override + String get name => 'titled'; + + @override + Page page(BuildContext context) { + final title = context.dependOnInheritedWidgetOfExactType<_Title>()!.title; + return MaterialPage( + key: key, + name: name, + child: Text('title:$title'), + ); + } + + @override + Widget build(BuildContext context) => const SizedBox.shrink(); +} From 6b7990eccde3a20efef2ad80f02b76921508211e Mon Sep 17 00:00:00 2001 From: Mike Matiunin Date: Thu, 1 Oct 2026 19:15:48 +0400 Subject: [PATCH 2/8] Escape the name in the route keys and fix the pub cache of the CI The default key now escapes the name as well as the arguments, and escapes "?" as %3F, so a separator inside the name can no longer produce the key of another route: "a?b" with {"c": 1} and "a" with {"b?c": 1} were both "a?b?c=1". The documentation of the key states what is guaranteed: values are compared by their toString() and the order of the arguments matters. The pub cache key of the CI includes the Flutter version and the week, restores a warm cache when a pubspec changes, and covers the dependencies of the example. A comment in the deploy workflow notes that a manual publish must be enabled on pub.dev. --- .github/actions/setup/action.yaml | 14 ++++- .github/workflows/deploy.yml | 3 + lib/src/route.dart | 28 +++++++--- test/routes_edge_test.dart | 92 ++++++++++++++++++++++++++++++- 4 files changed, 126 insertions(+), 11 deletions(-) diff --git a/.github/actions/setup/action.yaml b/.github/actions/setup/action.yaml index 87b5607..cc2abc8 100644 --- a/.github/actions/setup/action.yaml +++ b/.github/actions/setup/action.yaml @@ -23,13 +23,24 @@ runs: flutter-version: "${{ inputs.flutter-version }}" channel: "stable" + # Without a lockfile the pubspecs alone do not pin the resolved versions: + # the week in the key refreshes the cache regularly, and the restore keys + # start from the newest cache of the same SDK when a pubspec changes. + - name: 🗓️ Compute the cache week + id: cache-week + shell: bash + run: echo "week=$(date -u +%G-W%V)" >> "$GITHUB_OUTPUT" + - name: 📤 Restore Pub modules id: cache-pub-restore uses: actions/cache/restore@v4 with: path: | /home/runner/.pub-cache - key: ${{ runner.os }}-pub-${{ inputs.pub-cache }}-${{ hashFiles('pubspec.yaml', 'example/pubspec.yaml') }} + key: ${{ runner.os }}-pub-${{ inputs.pub-cache }}-flutter-${{ inputs.flutter-version }}-${{ steps.cache-week.outputs.week }}-${{ hashFiles('pubspec.yaml', 'example/pubspec.yaml') }} + restore-keys: | + ${{ runner.os }}-pub-${{ inputs.pub-cache }}-flutter-${{ inputs.flutter-version }}-${{ steps.cache-week.outputs.week }}- + ${{ runner.os }}-pub-${{ inputs.pub-cache }}-flutter-${{ inputs.flutter-version }}- - name: 👷 Install Dependencies shell: bash @@ -37,6 +48,7 @@ runs: echo /home/runner/.pub-cache/bin >> $GITHUB_PATH flutter config --no-cli-animations --no-analytics flutter pub get + flutter pub get --directory example #- name: ⏲️ Run build runner # shell: bash diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 5edf2ba..8b5b815 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -1,6 +1,9 @@ name: Deploy on: + # pub.dev rejects a publish started by workflow_dispatch unless + # "Enable publishing from workflow_dispatch events" is checked in the + # automated publishing settings of the package admin page on pub.dev. workflow_dispatch: push: tags: diff --git a/lib/src/route.dart b/lib/src/route.dart index 048bf56..3a5d2d0 100644 --- a/lib/src/route.dart +++ b/lib/src/route.dart @@ -83,9 +83,18 @@ mixin NavigationRoute { /// Keys must be unique within a single stack. /// /// The default value is built from [name] and [arguments], which is enough - /// for enums and for parameterless routes: `name?a=1&b=2`, in the order of - /// the entries of the map, with `%`, `&` and `=` escaped and a `null` - /// value written as a bare `name?a`. **Routes that carry their + /// for enums and for parameterless routes: `name` without arguments, + /// `name?a=1&b=2` with them, in the order of the entries of the map, and a + /// `null` value written as a bare `name?a`. The name, the argument names + /// and the values are escaped (`%` as `%25`, `&` as `%26`, `=` as `%3D`, + /// `?` as `%3F`), so a `?`, `&` or `=` inside any of them never collides + /// with the separators: `a?b` gives `a%3Fb`, unlike `a` with `{'b': null}`. + /// + /// Two default keys are equal exactly when the names are equal and the + /// arguments have the same names, in the same order, with the same `null` + /// values and the same `toString()` of the other values. So + /// `{'a': 1, 'b': 2}` and `{'b': 2, 'a': 1}` are different keys, while + /// `{'x': 1}` and `{'x': '1'}` are the same one. **Routes that carry their /// parameters as fields must override this getter**, otherwise all of them /// will collide: /// @@ -95,8 +104,9 @@ mixin NavigationRoute { /// ``` LocalKey get key { final args = arguments; - if (args.isEmpty) return ValueKey(name); - final buffer = StringBuffer(name)..write('?'); + final escapedName = _escape(name); + if (args.isEmpty) return ValueKey(escapedName); + final buffer = StringBuffer(escapedName)..write('?'); var first = true; for (final MapEntry(key: argument, :value) in args.entries) { if (first) { @@ -114,12 +124,14 @@ mixin NavigationRoute { return ValueKey(buffer.toString()); } - /// Escapes the separators of the default [key], so that different - /// arguments never produce the same key. + /// Escapes the separators of the default [key], so that a separator inside + /// the name, an argument name or a value never produces the key of another + /// route. `%` goes first, so an already escaped sequence stays distinct. static String _escape(String value) => value .replaceAll('%', '%25') .replaceAll('&', '%26') - .replaceAll('=', '%3D'); + .replaceAll('=', '%3D') + .replaceAll('?', '%3F'); /// Arbitrary marks used to address groups of routes declaratively. /// diff --git a/test/routes_edge_test.dart b/test/routes_edge_test.dart index 747a2e9..6fca3d4 100644 --- a/test/routes_edge_test.dart +++ b/test/routes_edge_test.dart @@ -38,6 +38,20 @@ final class _ArgumentsRoute with NavigationRoute { Widget build(BuildContext context) => const SizedBox.shrink(); } +/// A route with the name and the arguments given from the outside. +final class _NamedRoute with NavigationRoute { + const _NamedRoute(this.name, [this.arguments = const {}]); + + @override + final String name; + + @override + final Map arguments; + + @override + Widget build(BuildContext context) => const SizedBox.shrink(); +} + /// A generic route: its runtime type includes the type argument. final class _Generic with NavigationRoute { const _Generic(); @@ -85,13 +99,87 @@ void main() => group('routes edge cases', () { equals(const ValueKey('_ArgumentsRoute?q=a%26b%3Dc')), ); expect(joined.key, isNot(equals(split.key))); - const percent = _ArgumentsRoute({'q': '%26'}); - expect(percent.key, isNot(equals(joined.key))); + for (final (escaped, raw) in const <(String, String)>[ + ('%26', '&'), + ('%3D', '='), + ('%3F', '?'), + ('%25', '%'), + ]) { + final percent = _ArgumentsRoute({'q': escaped}); + final separator = _ArgumentsRoute({'q': raw}); + expect(percent.key, isNot(equals(separator.key)), reason: escaped); + final percentName = _ArgumentsRoute({escaped: 1}); + final separatorName = _ArgumentsRoute({raw: 1}); + expect( + percentName.key, + isNot(equals(separatorName.key)), + reason: escaped, + ); + } const nothing = _ArgumentsRoute({'id': null}); const text = _ArgumentsRoute({'id': 'null'}); expect(nothing.key, isNot(equals(text.key))); }); + test('a "?" in the name does not collide with the arguments', () { + expect( + const _NamedRoute('a?b', {'c': 1}).key, + isNot(equals(const _NamedRoute('a', {'b?c': 1}).key)), + ); + }); + + test('a name without arguments does not collide with the arguments', () { + expect( + const _NamedRoute('a?b=c').key, + isNot(equals(const _NamedRoute('a', {'b': 'c'}).key)), + ); + expect( + const _NamedRoute('a?b').key, + isNot(equals(const _NamedRoute('a', {'b': null}).key)), + ); + }); + + test('the separators in the name do not collide with the arguments', () { + expect( + const _NamedRoute('a?b&c').key, + isNot( + equals( + const _NamedRoute('a', {'b': null, 'c': null}).key, + ), + ), + ); + }); + + test('the default key escapes the name and the arguments', () { + expect( + const _NamedRoute('catalog', {'id': 1}).key, + equals(const ValueKey('catalog?id=1')), + ); + expect( + const _NamedRoute('a?b').key, + equals(const ValueKey('a%3Fb')), + ); + expect( + const _NamedRoute('a?b=c&d%', { + 'k?': 'v?', + 'e': null, + }).key, + equals(const ValueKey('a%3Fb%3Dc%26d%25?k%3F=v%3F&e')), + ); + expect( + const _NamedRoute('a%3Fb').key, + isNot(equals(const _NamedRoute('a?b').key)), + ); + }); + + test('the values are compared by their string representation', () { + // By design: the default key writes every value with toString(). + expect( + const _ArgumentsRoute({'x': 1}).key, + equals(const _ArgumentsRoute({'x': '1'}).key), + ); + }); + test('the arguments are written in their declaration order', () { const ab = _ArgumentsRoute({'a': 1, 'b': 2}); const ba = _ArgumentsRoute({'b': 2, 'a': 1}); From 680ef59bface5ea5d3042bb31ec2bc253f0a0df0 Mon Sep 17 00:00:00 2001 From: Mike Matiunin Date: Thu, 1 Oct 2026 19:17:44 +0400 Subject: [PATCH 3/8] Fix the order of the deferred observer events, maybePop from a listener and a dispose from a guard - Drain the deferred observer events in place and keep the notification flag set during the end of frame dispatch, so a change made by an observer is reported after the pending events and a change made by a listener does not schedule a spurious notification next frame. - maybePop called from a guard, an observer or a listener decides against the stack the pop applies to and falls back to the group. - Stop running the guards once one of them disposes the controller, and do not subscribe a controller disposed during its initial validation to the revalidation. - Do not notify the observers when a listener disposes the controller. - Document the queued changes from a guard, the deferred observer events and the pushForResult waiter replacement. --- lib/src/controller.dart | 101 +++++++++++--- lib/src/guard.dart | 6 + lib/src/observer.dart | 9 ++ test/context_test.dart | 61 +++++++++ test/controller_test.dart | 272 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 429 insertions(+), 20 deletions(-) diff --git a/lib/src/controller.dart b/lib/src/controller.dart index 86e844c..07041e5 100644 --- a/lib/src/controller.dart +++ b/lib/src/controller.dart @@ -106,6 +106,9 @@ class NavigationController // reading `controller.stack` sees the stack it is validating. _stack = UnmodifiableListView(candidate); final next = _dedupe(_runGuards(candidate)); + // A guard may dispose the controller, which must not subscribe it to + // the revalidation afterwards. + if (_disposed) return; if (next.isEmpty) { throw ArgumentError.value( initial, @@ -135,6 +138,10 @@ class NavigationController >{}; final Map _results = {}; + /// Results of the [maybePop] calls postponed until the current change is + /// committed, completed with `false` when the queue is dropped. + final List> _pendingPops = >[]; + /// Maximum number of changes requested from within a single transition. /// /// The changes over the limit are reported to `FlutterError` and dropped: @@ -153,6 +160,10 @@ class NavigationController bool _deferNotifications = false; bool _notificationScheduled = false; + /// Whether a change committed during the build waits for the listeners to + /// be notified at the end of the frame. + bool _listenersPending = false; + /// Transitions waiting to be reported to the [observers] at the end of the /// frame, see [revalidateDuringBuild]. final List<(NavigationStack, NavigationStack)> _deferredObserverEvents = @@ -264,6 +275,7 @@ class NavigationController _queue = null; _processing = false; _settleResults(); + _settlePendingPops(); } } @@ -308,9 +320,10 @@ class NavigationController /// `null` when the route is closed in any other way: a swipe back, a tap on /// the barrier of a dialog, the system back button or a guard. /// - /// Only one caller can wait for a route: pushing a route with the same - /// [NavigationRoute.key] again completes the previous future with `null` - /// right away, even though the route stays on the screen. + /// Only one caller can wait for a route: calling [pushForResult] again with + /// the same [NavigationRoute.key] completes the previous future with `null` + /// right away, even though the route stays on the screen. A plain [push] of + /// that route keeps the previous future waiting. /// /// ```dart /// final confirmed = @@ -339,7 +352,8 @@ class NavigationController /// /// When called from a guard, an observer or a listener, the pop is /// postponed until the current change is committed and `true` only means - /// that it has been scheduled. + /// that it has been scheduled: the stack it applies to is not known yet, + /// and the pop does nothing if that stack has a single route. bool pop([Object? result]) { if (!_processing && _stack.length < 2) return false; // The result is bound to the route that is actually removed, which is @@ -360,7 +374,9 @@ class NavigationController /// first. When the stack cannot be popped and the controller belongs to a /// group of tabs, the group switches back to the previously selected tab. /// - /// Returns `true` when the back navigation was handled. + /// Returns `true` when the back navigation was handled. When called from a + /// guard, an observer or a listener, the decision is made against the + /// stack the pop applies to, once the current change is committed. Future maybePop() async { if (_disposed) return false; final group = _group; @@ -368,12 +384,25 @@ class NavigationController final navigator = this.navigator; if (navigator != null) { if (await navigator.maybePop()) return true; - } else if (pop()) { + } else if (_processing ? await _popPending() : pop()) { return true; } return group?.popGroupMember() ?? false; } + /// Queues a pop after the current change and completes with whether it + /// has found a route to remove. + Future _popPending() { + final completer = Completer(); + _pendingPops.add(completer); + change((stack) { + final popped = stack.length > 1; + if (!completer.isCompleted) completer.complete(popped); + return popped ? (stack..removeLast()) : stack; + }); + return completer.future; + } + /// Removes the routes from the top of the stack until [predicate] returns /// `true` for the visible route. /// @@ -398,7 +427,8 @@ class NavigationController /// Returns `false` when the stack has no such route. When called from a /// guard, an observer or a listener, the removal is postponed until the /// current change is committed and `true` only means that it has been - /// scheduled. + /// scheduled: the removal does nothing if the stack it applies to has no + /// such route. bool removeKey(LocalKey key, [Object? result]) { if (!_processing && !containsKey(key)) return false; change((stack) { @@ -499,6 +529,8 @@ class NavigationController ); _completeRemoved(previous, next); _notifyListeners(); + // A listener may dispose the controller while it is being notified. + if (_disposed) return; _notifyObservers(previous, next); } finally { // A result belongs to the change that set it: the result of a pop @@ -511,6 +543,8 @@ class NavigationController NavigationStack _runGuards(NavigationStack candidate) { var next = candidate; for (final guard in guards) { + // A guard may dispose the controller, e.g. on sign out. + if (_disposed) break; try { next = guard(this, List.of(next)); } on Object catch (error, stackTrace) { @@ -570,6 +604,7 @@ class NavigationController void _notifyListeners() { if (!_deferNotifications) return notifyListeners(); + _listenersPending = true; _scheduleNotifications(); } @@ -579,16 +614,28 @@ class NavigationController _notificationScheduled = true; SchedulerBinding.instance ..addPostFrameCallback((_) { - _notificationScheduled = false; if (_disposed) return; - notifyListeners(); - final events = List<(NavigationStack, NavigationStack)>.of( - _deferredObserverEvents, - ); - _deferredObserverEvents.clear(); - for (final (previous, next) in events) { - if (_disposed) return; - _dispatchObservers(previous, next); + // The flag stays set while the listeners and the observers are + // notified: the observer event of a change they make is appended to + // the queue drained below instead of scheduling another callback, + // which would notify the listeners once more for nothing. + try { + _listenersPending = false; + notifyListeners(); + // Every event stays in the queue until it is dispatched, so the + // change made by an observer is reported after the pending ones + // instead of in the middle of them, see [_notifyObservers]. + while (!_disposed && _deferredObserverEvents.isNotEmpty) { + final (previous, next) = _deferredObserverEvents.first; + _dispatchObservers(previous, next); + if (_disposed) return; + _deferredObserverEvents.removeAt(0); + } + } finally { + _notificationScheduled = false; + // Only a change deferred by the dispatch itself needs another + // callback: everything else has been reported by this one. + if (!_disposed && _listenersPending) _scheduleNotifications(); } }, debugLabel: 'NavigationController.notifyListeners') ..ensureVisualUpdate(); @@ -596,10 +643,11 @@ class NavigationController /// Completes the futures of the routes that are not in the stack once all /// the queued changes have been committed, e.g. a [pushForResult] requested - /// from a listener and then rejected by a guard, and drops the results that - /// were not consumed by any removal. + /// from a listener and then rejected by a guard. + /// + /// The results that were not consumed by any removal are already dropped + /// by [_transition]. void _settleResults() { - _results.clear(); if (_completers.isEmpty) return; final keys = {for (final route in _stack) route.key}; final orphans = >[]; @@ -613,13 +661,26 @@ class NavigationController } } + /// Completes the [maybePop] calls whose pop has been dropped with the + /// queue, e.g. because the controller has been disposed. + void _settlePendingPops() { + if (_pendingPops.isEmpty) return; + final pending = List>.of(_pendingPops); + _pendingPops.clear(); + for (final completer in pending) { + if (!completer.isCompleted) completer.complete(false); + } + } + void _notifyObservers(NavigationStack previous, NavigationStack next) { if (observers.isEmpty) return; if (!_deferNotifications && _deferredObserverEvents.isEmpty) { return _dispatchObservers(previous, next); } // An observer may mark widgets dirty, which is not allowed during the - // build: report the transition at the end of the frame, in order. + // build: report the transition at the end of the frame, in order. While + // older transitions wait in the queue, this one waits behind them, even + // when it is not made during the build. _deferredObserverEvents.add((previous, next)); _scheduleNotifications(); } diff --git a/lib/src/guard.dart b/lib/src/guard.dart index fb09e6b..0f4046c 100644 --- a/lib/src/guard.dart +++ b/lib/src/guard.dart @@ -48,6 +48,12 @@ typedef NavigationGuardCallback = /// before it. The only exception is the validation of the initial stack in /// the constructor, where `controller.stack` is already the requested stack. /// +/// A guard should express a redirect through the stack it returns. A +/// `controller.pop`, `controller.removeKey` or `controller.change` called +/// from a guard does not touch the stack being validated: it is queued and +/// applied to the stack committed after the current change, and is validated +/// by the guards in turn. +/// /// Guards are applied in the order they are declared, each one receiving the /// result of the previous one. An exception thrown by a guard is reported /// to `FlutterError` and the guard is skipped, so a broken rule cannot break diff --git a/lib/src/observer.dart b/lib/src/observer.dart index ccd9af8..28cff41 100644 --- a/lib/src/observer.dart +++ b/lib/src/observer.dart @@ -9,6 +9,15 @@ import 'package:squid/src/route.dart'; /// new stack has been accepted and is meant for the side effects — analytics, /// logging, the title of the application window and so on. /// +/// The callbacks are usually invoked synchronously, right after the commit. +/// A change made during the build — e.g. by a `ContextGuard` reacting to a +/// new size of the window — is reported at the end of the frame instead, +/// because an observer may mark widgets dirty. In that case all the +/// listeners of the controller are notified before the observer events, +/// the events are delivered in the order of the commits, and +/// `controller.stack` may already be newer than the `next` stack of the +/// event. +/// /// ```dart /// class AnalyticsObserver with NavigationObserver { /// AnalyticsObserver(this._analytics); diff --git a/test/context_test.dart b/test/context_test.dart index eded30c..5840870 100644 --- a/test/context_test.dart +++ b/test/context_test.dart @@ -267,6 +267,43 @@ void main() => group('context', () { expect(find.text('observed:home'), findsOneWidget); }); + testWidgets('observers see the build time changes in order when one of ' + 'them changes the stack', (tester) async { + final first = _TranscriptObserver(pushOnFirst: Routes.settings); + final second = _TranscriptObserver(); + final controller = NavigationController( + [Routes.home], + guards: [const _AdaptiveGuard()], + observers: [first, second], + ); + addTearDown(controller.dispose); + + var notifications = 0; + controller.addListener(() => notifications++); + + await tester.pumpWidget( + MaterialApp( + home: MediaQuery( + data: const MediaQueryData(size: Size(800, 600)), + child: NavigationView(controller: controller), + ), + ), + ); + await tester.pumpAndSettle(); + expect(tester.takeException(), isNull); + + // The guard adds the side pane while the view is built, the observers + // hear about it at the end of the frame, and the first one reacts. + const transcript = [ + 'home -> home>_SidePaneRoute', + 'home>_SidePaneRoute -> home>_SidePaneRoute>settings', + ]; + expect(first.transcript, equals(transcript)); + expect(second.transcript, equals(transcript)); + expect(notifications, equals(2), reason: 'one per transition'); + expect(find.text('screen:settings'), findsOneWidget); + }); + testWidgets('a page reading the stack follows a build time change ' 'in the same frame', (tester) async { final controller = NavigationController( @@ -312,3 +349,27 @@ class _TopObserver with NavigationObserver { NavigationStack next, ) => current.value = next.last.name; } + +/// Records every transition as `previous -> next`, and pushes [pushOnFirst] +/// on the first one. +class _TranscriptObserver with NavigationObserver { + _TranscriptObserver({this.pushOnFirst}); + + final NavigationRoute? pushOnFirst; + + final List transcript = []; + + static String _names(NavigationStack stack) => + stack.map((route) => route.name).join('>'); + + @override + void onChange( + NavigationController controller, + NavigationStack previous, + NavigationStack next, + ) { + transcript.add('${_names(previous)} -> ${_names(next)}'); + final route = pushOnFirst; + if (route != null && transcript.length == 1) controller.push(route); + } +} diff --git a/test/controller_test.dart b/test/controller_test.dart index 648527c..8e60b1f 100644 --- a/test/controller_test.dart +++ b/test/controller_test.dart @@ -561,6 +561,238 @@ void main() => group('controller', () { ); }); + test('a guard can dispose the controller', () async { + final errors = []; + final previous = FlutterError.onError; + FlutterError.onError = (details) => errors.add(details.exception); + addTearDown(() => FlutterError.onError = previous); + + var later = 0; + final controller = NavigationController( + [Routes.home], + guards: [ + NavigationGuard((controller, stack) { + // E.g. a sign out tearing down the whole navigation. + if (stack.contains(Routes.settings)) controller.dispose(); + return stack; + }), + NavigationGuard((controller, stack) { + later++; + return stack; + }), + ], + ); + expect(later, equals(1), reason: 'the initial validation'); + + final result = controller.pushForResult(Routes.settings); + expect(controller.isDisposed, isTrue); + expect(later, equals(1), reason: 'no guard runs after the dispose'); + expect(await result, isNull); + expect(errors, isEmpty); + expect(controller.stack, equals([Routes.home])); + }); + + test('a guard disposing the controller during the initial validation ' + 'does not subscribe it to revalidate', () { + final flag = _CountingListenable(); + final controller = NavigationController( + [Routes.home], + revalidate: flag, + guards: [ + NavigationGuard((controller, stack) { + controller.dispose(); + return stack; + }), + ], + ); + expect(controller.isDisposed, isTrue); + expect(flag.listeners, isEmpty); + }); + + test('observers are not notified about a change after a listener has ' + 'disposed the controller', () { + final errors = []; + final previous = FlutterError.onError; + FlutterError.onError = (details) => errors.add(details.exception); + addTearDown(() => FlutterError.onError = previous); + + final observer = _RecordingObserver(); + late final NavigationController controller; + controller = NavigationController( + [Routes.home], + observers: [observer], + )..addListener(() => controller.dispose()); + + controller.push(Routes.catalog); + expect(controller.isDisposed, isTrue); + expect(observer.changes, isZero); + expect(observer.added, isEmpty); + }); + + group('maybePop from a listener', () { + test('returns false when the pending stack has a single route', () async { + final controller = NavigationController([ + Routes.home, + Routes.catalog, + ]); + addTearDown(controller.dispose); + + Future? popped; + controller.addListener(() => popped ??= controller.maybePop()); + expect(controller.pop(), isTrue); + + expect(controller.stack, equals([Routes.home])); + expect(await popped, isFalse); + expect(controller.stack, equals([Routes.home])); + }); + + test('pops the route left by the pending changes', () async { + final controller = NavigationController([Routes.home]); + addTearDown(controller.dispose); + + Future? popped; + controller.addListener(() => popped ??= controller.maybePop()); + controller.pushAll([Routes.catalog, Routes.settings]); + + expect(await popped, isTrue); + expect( + controller.stack, + equals([Routes.home, Routes.catalog]), + ); + }); + + test('completes with false when the queue is dropped', () async { + final errors = []; + final previous = FlutterError.onError; + FlutterError.onError = (details) => errors.add(details.exception); + addTearDown(() => FlutterError.onError = previous); + + late final NavigationController controller; + controller = NavigationController( + [Routes.home], + observers: [_DisposingObserver()], + ); + Future? popped; + controller + ..addListener(() => popped ??= controller.maybePop()) + ..push(Routes.catalog); + + expect(controller.isDisposed, isTrue); + expect(await popped, isFalse); + expect(errors, isEmpty); + }); + }); + + group('notifications deferred to the end of the frame', () { + testWidgets('are delivered in order when an observer changes the stack', ( + tester, + ) async { + var redirect = false; + final a = _ChainingObserver(); + final b = _TranscriptObserver(); + final controller = NavigationController( + [Routes.home], + guards: [ + NavigationGuard( + (controller, stack) => redirect && !stack.contains(Routes.catalog) + ? [...stack, Routes.catalog] + : stack, + ), + ], + observers: [a, b], + ); + addTearDown(controller.dispose); + + redirect = true; + controller.revalidateDuringBuild(); + expect(b.transcript, isEmpty, reason: 'deferred to the end of the frame'); + await tester.pump(); + + expect( + b.transcript, + equals([ + 'home -> home>catalog', + 'home>catalog -> home>catalog>settings', + ]), + ); + expect(a.transcript, equals(b.transcript)); + }); + + testWidgets('notify every listener once per frame', (tester) async { + var redirect = false; + final controller = NavigationController( + [Routes.home], + guards: [ + NavigationGuard( + (controller, stack) => redirect && !stack.contains(Routes.catalog) + ? [...stack, Routes.catalog] + : stack, + ), + ], + observers: [_TranscriptObserver()], + ); + addTearDown(controller.dispose); + + var notifications = 0; + controller.addListener(() { + // The first notification is the deferred one: the listener reacts + // with a change of its own, notified synchronously. + if (notifications++ == 0) controller.push(Routes.settings); + }); + + redirect = true; + controller.revalidateDuringBuild(); + expect(notifications, isZero); + await tester.pump(); + expect(notifications, equals(2)); + expect( + controller.stack, + equals([Routes.home, Routes.catalog, Routes.settings]), + ); + + // Nothing has changed since: no spurious notification next frame. + tester.binding.scheduleFrame(); + await tester.pump(); + await tester.pump(); + expect(notifications, equals(2)); + }); + + testWidgets('a change deferred by a listener is notified next frame', ( + tester, + ) async { + final redirects = {}; + final controller = NavigationController( + [Routes.home], + guards: [ + NavigationGuard( + (controller, stack) => [ + ...stack, + for (final route in redirects) + if (!stack.contains(route)) route, + ], + ), + ], + observers: [_TranscriptObserver()], + ); + addTearDown(controller.dispose); + + final seen = []; + controller.addListener(() { + seen.add(controller.top); + if (seen.length > 1) return; + redirects.add(Routes.settings); + controller.revalidateDuringBuild(); + }); + + redirects.add(Routes.catalog); + controller.revalidateDuringBuild(); + await tester.pump(); + expect(seen, equals([Routes.catalog])); + await tester.pump(); + expect(seen, equals([Routes.catalog, Routes.settings])); + }); + }); + test('a stale result of a cancelled pop is not used by a queued removal', () { final locked = Flag(value: true); final controller = NavigationController( @@ -661,3 +893,43 @@ class _DisposingObserver with NavigationObserver { NavigationStack next, ) => controller.dispose(); } + +/// Records every transition as `previous -> next`, and pushes the settings +/// on the first one. +class _ChainingObserver extends _TranscriptObserver { + @override + void onChange( + NavigationController controller, + NavigationStack previous, + NavigationStack next, + ) { + super.onChange(controller, previous, next); + if (transcript.length == 1) controller.push(Routes.settings); + } +} + +/// Records every transition as `previous -> next`. +class _TranscriptObserver with NavigationObserver { + final List transcript = []; + + static String _names(NavigationStack stack) => + stack.map((route) => route.name).join('>'); + + @override + void onChange( + NavigationController controller, + NavigationStack previous, + NavigationStack next, + ) => transcript.add('${_names(previous)} -> ${_names(next)}'); +} + +/// A listenable that exposes its listeners, to check for the leaks. +class _CountingListenable implements Listenable { + final List listeners = []; + + @override + void addListener(VoidCallback listener) => listeners.add(listener); + + @override + void removeListener(VoidCallback listener) => listeners.remove(listener); +} From 4d03f15f93767a36abb1d838db945da12748a2e0 Mon Sep 17 00:00:00 2001 From: Mike Matiunin Date: Thu, 1 Oct 2026 19:20:04 +0400 Subject: [PATCH 4/8] Report the back button of the whole application to the platform On Android the predictive back gesture closes the application unless a NavigationNotification with canHandlePop: true reaches it, so the view must report exactly what didPopRoute would do. - Every view swallows the notifications of its navigator, its routes and its nested views, and the top level views report one combined state: a visible nested view that can go back, a route to close, a PopScope blocking the visible route, a handler of the view or a previous tab. Two top level views (hand-made tabs) no longer overwrite each other. - A nested view reports again when its route becomes current again. - A disabled view (interceptBackButton: false) no longer claims the back button, and disabling it is reported. - The tabs controller reports when it is created around mounted views and when it is disposed, so no stale claim of a previous tab remains. - The pages are built with a context of their own, so a guard reading e.g. the size of the window no longer rebuilds every page. --- lib/src/tabs.dart | 9 +- lib/src/view.dart | 212 ++++++++++++++------ test/back_button_test.dart | 390 +++++++++++++++++++++++++++++++++++++ test/squid_test.dart | 2 + 4 files changed, 553 insertions(+), 60 deletions(-) create mode 100644 test/back_button_test.dart diff --git a/lib/src/tabs.dart b/lib/src/tabs.dart index 8be5787..2f26778 100644 --- a/lib/src/tabs.dart +++ b/lib/src/tabs.dart @@ -61,6 +61,9 @@ class NavigationTabsController ..joinGroup(this) ..addListener(notifyListeners); } + // The views of the controllers may already be mounted: an inactive tab + // must stop claiming the back button. + _didChangeGroup(); } /// The controllers of all the tabs, in their declaration order. @@ -160,13 +163,15 @@ class NavigationTabsController @override void dispose() { + _history.clear(); for (final controller in tabs.values) { + // A view outliving the tabs must stop returning to the previous tab. controller ..removeListener(notifyListeners) - ..leaveGroup(this); + ..leaveGroup(this) + ..didChangeGroup(); if (disposeTabs) controller.dispose(); } - _history.clear(); super.dispose(); } diff --git a/lib/src/view.dart b/lib/src/view.dart index d13b76b..7de0cb6 100644 --- a/lib/src/view.dart +++ b/lib/src/view.dart @@ -61,6 +61,10 @@ class NavigationView extends StatefulWidget { /// press reach the view that contains it. A nested view is visible while /// the route that contains it is the current route of the outer /// navigator. Disabling this flag disables the nested views as well. + /// + /// A disabled view does not tell the platform that it could handle the + /// press either, so on Android the predictive back gesture is not claimed + /// for its routes. final bool interceptBackButton; /// Overrides the reaction to the system back button. @@ -116,6 +120,10 @@ class _NavigationViewState extends State /// Results passed to `Navigator.pop`, waiting for [_handleDidRemovePage]. final Map _popResults = {}; + /// The top level views, which receive the back button from the + /// [WidgetsBinding]. + static final Set<_NavigationViewState> _roots = <_NavigationViewState>{}; + /// The closest [NavigationView] above this one, `null` for a top level /// view, which receives the back button from the [WidgetsBinding]. _NavigationViewState? _parent; @@ -157,11 +165,15 @@ class _NavigationViewState extends State void activate() { super.activate(); _register(); + _reportBackButton(); } @override void deactivate() { + final parent = _parent; _unregister(); + // The views left behind may now be the ones handling the back button. + (parent ?? _roots.firstOrNull)?._reportBackButton(); super.deactivate(); } @@ -174,6 +186,7 @@ class _NavigationViewState extends State if (parent is _NavigationViewMarker) { _parent = parent.state.._children.add(this); } else { + _roots.add(this); WidgetsBinding.instance.addObserver(this); } } @@ -181,6 +194,7 @@ class _NavigationViewState extends State void _unregister() { final parent = _parent; if (parent == null) { + _roots.remove(this); WidgetsBinding.instance.removeObserver(this); } else { parent._children.remove(this); @@ -200,10 +214,9 @@ class _NavigationViewState extends State // When no guard reads anything, this element has no dependencies at all // and the callback is invoked exactly once, right after the mount. // - // `NavigationRoute.page` is built with this context too: a page reading - // the locale or the theme has to be built again, not taken from the - // cache. - _cache.clear(); + // The pages are built with the context of [_PagesBuilder] instead, so a + // guard reading e.g. the size of the window on every frame of the + // keyboard animation does not build all the pages again. widget.controller.revalidateDuringBuild(); } @@ -256,13 +269,15 @@ class _NavigationViewState extends State /// navigator that contains it is not covered by anything. bool get _visible => mounted && (_route?.isCurrent ?? true); + /// Whether this view reacts to the back button at all: it is not disabled + /// and its controller is not an inactive tab. + bool get _enabled => + widget.interceptBackButton && widget.controller.isActiveInGroup; + /// Offers the back button to the visible nested views first, then handles /// it with the controller of this view. Future _handleBackButton() async { - final controller = widget.controller; - if (!widget.interceptBackButton || !controller.isActiveInGroup) { - return false; - } + if (!_enabled) return false; for (final child in _children.reversed.toList(growable: false)) { if (!child._visible) continue; if (await child._handleBackButton()) return true; @@ -274,48 +289,75 @@ class _NavigationViewState extends State return widget.controller.maybePop(); } - /// Whether this view handles the back button even when its navigator - /// cannot pop: it has its own handler or its tabs return to the previous - /// one. - bool get _claimsBackButton => - widget.interceptBackButton && - widget.controller.isActiveInGroup && - (widget.onBackButtonPressed != null || - widget.controller.canPopGroupMember); - - /// Tells the platform whether the framework handles the back button. + /// Whether a press reaching this view would be handled, decided the same + /// way as [_handleBackButton] does it. /// - /// The navigator reports `canHandlePop: false` when it has a single route, - /// so on Android with the predictive back gesture the press would close - /// the application without ever reaching [didPopRoute]. - bool _handleNavigationNotification(NavigationNotification notification) { - // An inactive tab kept alive by an `IndexedStack` must not speak for - // the visible one. - if (widget.interceptBackButton && !widget.controller.isActiveInGroup) { + /// It is handled by a visible nested view that can go back, by closing a + /// route, by a [PopScope] blocking the visible route, by a handler of the + /// view or by returning to the previous tab. + bool get _canHandlePop { + if (!mounted || !_enabled) return false; + for (final child in _children) { + if (child._visible && child._canHandlePop) return true; + } + final navigator = this.navigator; + if (navigator != null && + (navigator.canPop() || + _spy.top?.popDisposition == RoutePopDisposition.doNotPop)) { return true; } - if (notification.canHandlePop || !_claimsBackButton) return false; - const NavigationNotification(canHandlePop: true).dispatch(context); + return widget.onBackButtonPressed != null || + widget.controller.canPopGroupMember; + } + + /// Replaces the notifications of the navigator, of its routes and of the + /// nested views with the state of the whole application, see + /// [_reportBackButton]. + /// + /// The navigator alone gets it wrong: it reports `canHandlePop: false` + /// with a single route even when the tabs return to the previous one, and + /// `true` with several routes even when this view is disabled or is an + /// inactive tab kept alive by an `IndexedStack`. + bool _handleNavigationNotification(NavigationNotification notification) { + _reportBackButton(); return true; } - /// Reports the current state of the back button, when it may have changed - /// without a change of the navigator, e.g. another tab has been selected. + /// Tells the platform whether the framework handles the back button. + /// + /// Without it, on Android with the predictive back gesture the press + /// would close the application without ever reaching [didPopRoute]. + /// + /// Every report describes all the top level views at once, so that two of + /// them, e.g. the tabs of an `IndexedStack`, never overwrite each other + /// whatever the order of their reports is. void _reportBackButton() { + // The navigator dispatches its notifications from the post frame + // callbacks: answer in the same frame. + if (SchedulerBinding.instance.schedulerPhase == + SchedulerPhase.postFrameCallbacks) { + _dispatchBackButton(); + return; + } if (_reportScheduled) return; _reportScheduled = true; - SchedulerBinding.instance.addPostFrameCallback((_) { - _reportScheduled = false; - if (!mounted || !_visible) return; - if (!widget.interceptBackButton || !widget.controller.isActiveInGroup) { - return; - } - final canPop = navigator?.canPop() ?? false; - NavigationNotification( - canHandlePop: canPop || _claimsBackButton, - ).dispatch(context); - }, debugLabel: 'NavigationView.reportBackButton'); - SchedulerBinding.instance.ensureVisualUpdate(); + SchedulerBinding.instance + ..addPostFrameCallback((_) { + _reportScheduled = false; + _dispatchBackButton(); + }, debugLabel: 'NavigationView.reportBackButton') + ..ensureVisualUpdate(); + } + + void _dispatchBackButton() { + if (!mounted) return; + final parent = _parent; + if (parent != null) return parent._dispatchBackButton(); + // Deactivated, e.g. while it is moved with a global key. + if (!_roots.contains(this)) return; + NavigationNotification( + canHandlePop: _roots.any((view) => view._visible && view._canHandlePop), + ).dispatch(context); } @override @@ -335,6 +377,9 @@ class _NavigationViewState extends State } /// Converts the routes into pages, reusing the unchanged ones. + /// + /// [context] belongs to [_PagesBuilder], which clears the cache when an + /// inherited widget read by a page changes. List> _buildPages(BuildContext context) { final stack = _built = widget.controller.stack; final keys = {}; @@ -361,24 +406,34 @@ class _NavigationViewState extends State return pages; } + void _handleRouteChanged(ModalRoute? route) { + _route = route; + // The route may have been covered or uncovered: a nested view starts or + // stops speaking for the back button. + _reportBackButton(); + } + @override - Widget build(BuildContext context) => _NavigationViewMarker( - state: this, - child: _RouteProbe( - onRoute: (route) => _route = route, - child: NavigationScope( - controller: widget.controller, - child: NotificationListener( - onNotification: _handleNavigationNotification, - child: Navigator( - pages: _buildPages(context), - onDidRemovePage: _handleDidRemovePage, - transitionDelegate: widget.transitionDelegate, - observers: _observers, - restorationScopeId: widget.restorationScopeId, - requestFocus: widget.requestFocus, - clipBehavior: widget.clipBehavior, - reportsRouteUpdateToEngine: widget.reportsRouteUpdateToEngine, + Widget build(BuildContext context) => _PagesBuilder( + onDependenciesChanged: _cache.clear, + builder: (context) => _NavigationViewMarker( + state: this, + child: _RouteProbe( + onRoute: _handleRouteChanged, + child: NavigationScope( + controller: widget.controller, + child: NotificationListener( + onNotification: _handleNavigationNotification, + child: Navigator( + pages: _buildPages(context), + onDidRemovePage: _handleDidRemovePage, + transitionDelegate: widget.transitionDelegate, + observers: _observers, + restorationScopeId: widget.restorationScopeId, + requestFocus: widget.requestFocus, + clipBehavior: widget.clipBehavior, + reportsRouteUpdateToEngine: widget.reportsRouteUpdateToEngine, + ), ), ), ), @@ -386,6 +441,37 @@ class _NavigationViewState extends State ); } +/// Builds the pages of a [NavigationView] with a context of its own. +/// +/// A separate element, so that the inherited widgets read by the guards and +/// by the pages are tracked separately: a page reading the locale or the +/// theme is built again when it changes, but not when only a guard depends +/// on the changed widget. +class _PagesBuilder extends StatefulWidget { + const _PagesBuilder({ + required this.onDependenciesChanged, + required this.builder, + }); + + final VoidCallback onDependenciesChanged; + + final WidgetBuilder builder; + + @override + State<_PagesBuilder> createState() => _PagesBuilderState(); +} + +class _PagesBuilderState extends State<_PagesBuilder> { + @override + void didChangeDependencies() { + super.didChangeDependencies(); + widget.onDependenciesChanged(); + } + + @override + Widget build(BuildContext context) => widget.builder(context); +} + /// Links a [NavigationView] with the views nested into its routes. class _NavigationViewMarker extends InheritedWidget { const _NavigationViewMarker({required this.state, required super.child}); @@ -430,6 +516,16 @@ class _NavigationViewObserver extends NavigatorObserver { final void Function(LocalKey key, Object? result) _onPop; + /// The visible route of the navigator, to see whether a [PopScope] + /// blocks the back button. + Route? top; + + @override + void didChangeTop( + Route topRoute, + Route? previousTopRoute, + ) => top = topRoute; + @override void didPush(Route route, Route? previousRoute) => _track(route); diff --git a/test/back_button_test.dart b/test/back_button_test.dart new file mode 100644 index 0000000..59db73a --- /dev/null +++ b/test/back_button_test.dart @@ -0,0 +1,390 @@ +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:squid/squid.dart'; + +import 'src/routes.dart'; + +enum _Tab { feed, cart } + +/// What the application last told the platform about the back button. +/// +/// On Android with the predictive back gesture the system closes the +/// application on its own unless the framework claims the press. +class _Platform { + bool? handlesBack; +} + +void main() => group('back button', () { + Future<_Platform> pumpApp(WidgetTester tester, Widget home) async { + final platform = _Platform(); + await tester.pumpWidget( + MaterialApp( + onNavigationNotification: (notification) { + platform.handlesBack = notification.canHandlePop; + return true; + }, + home: home, + ), + ); + await tester.pumpAndSettle(); + return platform; + } + + /// A shell rebuilt on every change of [tabs], one view per tab. + Widget shell(NavigationTabsController<_Tab> tabs) => ListenableBuilder( + listenable: tabs, + builder: (context, _) => IndexedStack( + index: tabs.activeIndex, + children: [ + for (final controller in tabs.tabs.values) + NavigationView(controller: controller), + ], + ), + ); + + testWidgets('a nested view is reported again when its route is on top', ( + tester, + ) async { + final inner = NavigationController([ + Routes.home, + Routes.catalog, + ]); + addTearDown(inner.dispose); + final outer = NavigationController([_ShellRoute(inner)]); + addTearDown(outer.dispose); + final platform = await pumpApp(tester, NavigationView(controller: outer)); + expect(platform.handlesBack, isTrue, reason: 'the nested view can pop'); + + outer.push(Routes.settings); + await tester.pumpAndSettle(); + expect(platform.handlesBack, isTrue); + + outer.pop(); + await tester.pumpAndSettle(); + expect(platform.handlesBack, isTrue, reason: 'the nested view is back'); + + // The press goes to the nested view. + expect(await tester.binding.handlePopRoute(), isTrue); + await tester.pumpAndSettle(); + expect(inner.stack, equals([Routes.home])); + expect(outer.length, equals(1)); + expect(platform.handlesBack, isFalse, reason: 'nothing left to pop'); + + // Nothing to pop: the application may be closed. + expect(await tester.binding.handlePopRoute(), isFalse); + }); + + testWidgets('a tab is reported with the nested view it hosts', ( + tester, + ) async { + final inner = NavigationController([ + Routes.home, + Routes.catalog, + ]); + addTearDown(inner.dispose); + final tabs = NavigationTabsController<_Tab>( + tabs: <_Tab, NavigationController>{ + _Tab.feed: NavigationController([_ShellRoute(inner)]), + _Tab.cart: NavigationController([Routes.catalog]), + }, + ); + addTearDown(tabs.dispose); + final platform = await pumpApp(tester, shell(tabs)); + expect(platform.handlesBack, isTrue); + + tabs.select(_Tab.cart); + await tester.pumpAndSettle(); + expect(platform.handlesBack, isTrue, reason: 'returns to the feed'); + + tabs.back(); + await tester.pumpAndSettle(); + expect(platform.handlesBack, isTrue, reason: 'the nested view can pop'); + + expect(await tester.binding.handlePopRoute(), isTrue); + await tester.pumpAndSettle(); + expect(inner.stack, equals([Routes.home])); + expect(tabs.active, equals(_Tab.feed)); + expect(platform.handlesBack, isFalse); + + expect(await tester.binding.handlePopRoute(), isFalse); + }); + + testWidgets('a PopScope on the root of a tab keeps the back button', ( + tester, + ) async { + var blocked = 0; + final tabs = NavigationTabsController<_Tab>( + tabs: <_Tab, NavigationController>{ + _Tab.feed: NavigationController([ + _GuardedRoute(() => blocked++), + ]), + _Tab.cart: NavigationController([Routes.catalog]), + }, + ); + addTearDown(tabs.dispose); + // The views are created once: nothing rebuilds them on a switch. + final views = [ + for (final controller in tabs.tabs.values) + NavigationView(controller: controller), + ]; + final platform = await pumpApp( + tester, + ListenableBuilder( + listenable: tabs, + builder: (context, _) => + IndexedStack(index: tabs.activeIndex, children: views), + ), + ); + expect(platform.handlesBack, isTrue, reason: 'the PopScope blocks it'); + + tabs.select(_Tab.cart); + await tester.pumpAndSettle(); + expect(platform.handlesBack, isTrue); + + tabs.back(); + await tester.pumpAndSettle(); + expect(platform.handlesBack, isTrue, reason: 'the PopScope blocks it'); + + // The press is handled by the PopScope, nothing is closed. + expect(await tester.binding.handlePopRoute(), isTrue); + await tester.pumpAndSettle(); + expect(blocked, equals(1)); + expect(tabs.active, equals(_Tab.feed)); + expect(find.text('guarded'), findsOneWidget); + }); + + testWidgets('a disabled view does not claim the back button', (tester) async { + final controller = NavigationController([ + Routes.home, + Routes.catalog, + ]); + addTearDown(controller.dispose); + final platform = await pumpApp( + tester, + NavigationView(controller: controller, interceptBackButton: false), + ); + expect(platform.handlesBack, isFalse); + + expect(await tester.binding.handlePopRoute(), isFalse); + expect(controller.length, equals(2)); + }); + + testWidgets('disabling the back button is reported', (tester) async { + final controller = NavigationController([ + Routes.home, + Routes.catalog, + ]); + addTearDown(controller.dispose); + final enabled = ValueNotifier(true); + addTearDown(enabled.dispose); + final platform = await pumpApp( + tester, + ValueListenableBuilder( + valueListenable: enabled, + builder: (context, value, _) => + NavigationView(controller: controller, interceptBackButton: value), + ), + ); + expect(platform.handlesBack, isTrue); + + enabled.value = false; + await tester.pumpAndSettle(); + expect(platform.handlesBack, isFalse); + expect(await tester.binding.handlePopRoute(), isFalse); + + enabled.value = true; + await tester.pumpAndSettle(); + expect(platform.handlesBack, isTrue); + expect(await tester.binding.handlePopRoute(), isTrue); + await tester.pumpAndSettle(); + expect(controller.length, equals(1)); + expect(platform.handlesBack, isFalse); + }); + + testWidgets('hand-made tabs disabled with interceptBackButton', ( + tester, + ) async { + final first = NavigationController([ + Routes.home, + Routes.catalog, + ]); + addTearDown(first.dispose); + final second = NavigationController([Routes.settings]); + addTearDown(second.dispose); + final index = ValueNotifier(0); + addTearDown(index.dispose); + final platform = await pumpApp( + tester, + ValueListenableBuilder( + valueListenable: index, + builder: (context, value, _) => IndexedStack( + index: value, + children: [ + NavigationView(controller: first, interceptBackButton: value == 0), + NavigationView(controller: second, interceptBackButton: value == 1), + ], + ), + ), + ); + expect(platform.handlesBack, isTrue); + + index.value = 1; + await tester.pumpAndSettle(); + expect(platform.handlesBack, isFalse, reason: 'a single visible route'); + expect(await tester.binding.handlePopRoute(), isFalse); + expect(first.length, equals(2), reason: 'the hidden tab is untouched'); + + index.value = 0; + await tester.pumpAndSettle(); + expect(platform.handlesBack, isTrue); + expect(await tester.binding.handlePopRoute(), isTrue); + await tester.pumpAndSettle(); + expect(first.length, equals(1)); + expect(platform.handlesBack, isFalse); + }); + + testWidgets('disposing the tabs drops the claim of the previous tab', ( + tester, + ) async { + final feed = NavigationController([Routes.home]); + addTearDown(feed.dispose); + final cart = NavigationController([Routes.catalog]); + addTearDown(cart.dispose); + final tabs = NavigationTabsController<_Tab>( + tabs: <_Tab, NavigationController>{_Tab.feed: feed, _Tab.cart: cart}, + disposeTabs: false, + ); + final platform = await pumpApp(tester, NavigationView(controller: cart)); + expect(platform.handlesBack, isFalse, reason: 'an inactive tab is silent'); + + tabs.select(_Tab.cart); + await tester.pumpAndSettle(); + expect(platform.handlesBack, isTrue, reason: 'returns to the feed'); + + tabs.dispose(); + await tester.pumpAndSettle(); + expect(platform.handlesBack, isFalse, reason: 'no tabs to return to'); + expect(await tester.binding.handlePopRoute(), isFalse); + }); + + testWidgets('tabs created around a mounted view are reported', ( + tester, + ) async { + final feed = NavigationController([Routes.home]); + addTearDown(feed.dispose); + final cart = NavigationController([ + Routes.catalog, + Routes.settings, + ]); + addTearDown(cart.dispose); + final platform = await pumpApp(tester, NavigationView(controller: cart)); + expect(platform.handlesBack, isTrue); + + final tabs = NavigationTabsController<_Tab>( + tabs: <_Tab, NavigationController>{_Tab.feed: feed, _Tab.cart: cart}, + disposeTabs: false, + ); + await tester.pumpAndSettle(); + expect(platform.handlesBack, isFalse, reason: 'an inactive tab'); + expect(await tester.binding.handlePopRoute(), isFalse); + expect(cart.length, equals(2)); + + tabs.dispose(); + await tester.pumpAndSettle(); + expect(platform.handlesBack, isTrue, reason: 'out of the tabs again'); + }); + + testWidgets('a guard reading the context does not rebuild the pages', ( + tester, + ) async { + var builds = 0; + final controller = NavigationController( + [_CountedRoute(() => builds++)], + guards: [const _SizeGuard()], + ); + addTearDown(controller.dispose); + final width = ValueNotifier(400); + addTearDown(width.dispose); + await tester.pumpWidget( + MaterialApp( + home: ValueListenableBuilder( + valueListenable: width, + builder: (context, value, child) => MediaQuery( + data: MediaQuery.of(context).copyWith(size: Size(value, 600)), + child: child!, + ), + child: NavigationView(controller: controller), + ), + ), + ); + await tester.pumpAndSettle(); + final initial = builds; + expect(initial, isPositive); + + width.value = 300; + await tester.pumpAndSettle(); + width.value = 200; + await tester.pumpAndSettle(); + expect(builds, equals(initial), reason: 'the routes have not changed'); + expect(find.text('counted'), findsOneWidget); + }); +}); + +final class _ShellRoute with NavigationRoute { + const _ShellRoute(this.controller); + + final NavigationController controller; + + @override + String get name => 'shell'; + + @override + Widget build(BuildContext context) => NavigationView(controller: controller); +} + +final class _GuardedRoute with NavigationRoute { + const _GuardedRoute(this.onBlocked); + + final VoidCallback onBlocked; + + @override + String get name => 'guarded'; + + @override + Widget build(BuildContext context) => PopScope( + canPop: false, + onPopInvokedWithResult: (didPop, _) { + if (!didPop) onBlocked(); + }, + child: const Text('guarded'), + ); +} + +final class _CountedRoute with NavigationRoute { + const _CountedRoute(this.onPage); + + final VoidCallback onPage; + + @override + String get name => 'counted'; + + @override + Page page(BuildContext context) { + onPage(); + return MaterialPage(key: key, name: name, child: build(context)); + } + + @override + Widget build(BuildContext context) => const Text('counted'); +} + +/// Subscribes the view to the size of the window, keeps the stack. +class _SizeGuard extends ContextGuard { + const _SizeGuard(); + + @override + NavigationStack guard(BuildContext context, NavigationStack stack) { + MediaQuery.sizeOf(context); + return stack; + } +} diff --git a/test/squid_test.dart b/test/squid_test.dart index b36848c..4e36e51 100644 --- a/test/squid_test.dart +++ b/test/squid_test.dart @@ -1,3 +1,4 @@ +import 'back_button_test.dart' as back_button_test; import 'context_test.dart' as context_test; import 'controller_test.dart' as controller_test; import 'extensions_test.dart' as extensions_test; @@ -26,4 +27,5 @@ void main() { pages_edge_test.main(); routes_edge_test.main(); tabs_edge_test.main(); + back_button_test.main(); } From cd13e7570c9766ed246c522f525e4259ad7c3e1b Mon Sep 17 00:00:00 2001 From: Mike Matiunin Date: Thu, 1 Oct 2026 19:21:36 +0400 Subject: [PATCH 5/8] Describe the new fixes of 0.1.1 in the changelog --- CHANGELOG.md | 60 ++++++++++++++++++++++++++++++++++------------------ 1 file changed, 40 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8fe69e5..17b3e9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,21 +5,33 @@ All notable changes to this project will be documented in this file. ## 0.1.1 - **Fixed**: on Android with the predictive back gesture, the back button - no longer closes the application when a `NavigationTabsController` can - return to the previous tab or a `NavigationView` has its own - `onBackButtonPressed`: the view tells the platform that the framework - handles the press, and an inactive tab no longer speaks for the visible - one. + no longer closes the application while the framework can handle the + press. The top level views report one state for the whole application: + a route to close, a visible nested view that can go back, a `PopScope` + blocking the visible route, a previous tab of a + `NavigationTabsController` or an `onBackButtonPressed` of the view. A + nested view reports again when its route becomes current again, an + inactive tab no longer speaks for the visible one, and a view with + `interceptBackButton: false` no longer claims the press. +- **Fixed**: a `NavigationTabsController` reports the back button when it + is created around mounted views and when it is disposed, so no stale + claim of a previous tab remains. - **Fixed**: a page built from the context of the view (the locale, the theme, an inherited scope read in `NavigationRoute.page`) is built again - when that context changes instead of being taken from the cache. -- **Fixed**: the default `key` of a route escapes `%`, `&` and `=` in the - arguments and writes a `null` argument without a value, so different - arguments, e.g. `{'q': 'a&b=c'}` and `{'q': 'a', 'b': 'c'}`, or `null` and - `'null'`, no longer produce the same key. + when that context changes instead of being taken from the cache. A guard + reading the context, e.g. the size of the window, no longer rebuilds the + pages. +- **Fixed**: the default `key` of a route escapes `%`, `&`, `=` and `?` in + the name and in the arguments and writes a `null` argument without a + value, so different routes, e.g. `{'q': 'a&b=c'}` and + `{'q': 'a', 'b': 'c'}`, `null` and `'null'`, or the name `'a?b'` and the + name `'a'` with `{'b': null}`, no longer produce the same key. - **Fixed**: observers are notified at the end of the frame, together with the listeners, when a `ContextGuard` changes the stack during the build, - so an observer calling `setState` no longer throws. + so an observer calling `setState` no longer throws. The events keep + their order when an observer changes the stack while they are delivered, + and a change made by a listener at that moment no longer notifies the + listeners once more on the next frame. - **Fixed**: an exception in one callback of an observer no longer swallows the remaining events of the same transition for that observer. - **Fixed**: a guard returning an empty stack is reported to `FlutterError` @@ -28,19 +40,27 @@ All notable changes to this project will be documented in this file. - **Fixed**: `pop` and `removeKey` called from a guard, an observer or a listener are postponed instead of being checked against the stack that is about to change, e.g. `push(route)` followed by `remove(route)`. -- **Fixed**: the result of a `pop` cancelled by a guard is no longer passed - to a later removal of the same route. -- **Fixed**: a guard disposing the controller no longer leads to a - notification of a disposed controller. + `maybePop` called from there decides against the stack it applies to and + no longer reports a press as handled when nothing is popped. +- **Fixed**: the result of a `pop` cancelled by a guard is dropped as soon + as the guard cancels it, instead of waiting for the end of the queued + changes. +- **Fixed**: a guard can dispose the controller: the remaining guards do + not run, the observers are not notified, and a controller disposed during + its initial validation does not subscribe to `revalidate`. Observers are + also not notified when a listener disposes the controller. - **Docs**: a guard keeping a route closed by the user brings back a new instance of it, use a `PopScope` to prevent the closing. - **Docs**: the name of a class route is minified in an obfuscated build, routes compare by instance in a plain `List`, overridden `tags` must keep - `kModalTag`, `pushForResult` completes the previous waiter of the same - route with `null`, `controller.stack` while the guards run. -- **CI**: a manual publication must start from the tag of the version, the - pub cache depends on the pubspecs, a `.pubignore` keeps the development - files out of the package. + `kModalTag`, calling `pushForResult` again with the same key completes + the previous waiter with `null`, `controller.stack` while the guards run, + changes made from a guard are queued, observer events may be delivered + at the end of the frame, what the default `key` guarantees. +- **CI**: a manual publication must start from the tag of the version and + be enabled on pub.dev, the pub cache depends on the pubspecs, the Flutter + version and the week and covers the example, a `.pubignore` keeps the + development files out of the package. ## 0.1.0 From 600975707f690e223736c5c82aaba57f82b4aebd Mon Sep 17 00:00:00 2001 From: Mike Matiunin Date: Thu, 1 Oct 2026 19:25:26 +0400 Subject: [PATCH 6/8] Use cascades in the controller tests for the analyzer of Flutter 3.35 --- test/controller_test.dart | 49 +++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/test/controller_test.dart b/test/controller_test.dart index 8e60b1f..b851f67 100644 --- a/test/controller_test.dart +++ b/test/controller_test.dart @@ -524,14 +524,15 @@ void main() => group('controller', () { addTearDown(controller.dispose); var queued = false; - controller.addListener(() { - if (queued) return; - queued = true; - controller - ..push(Routes.signIn) - ..push(Routes.settings); - }); - controller.push(Routes.catalog); + controller + ..addListener(() { + if (queued) return; + queued = true; + controller + ..push(Routes.signIn) + ..push(Routes.settings); + }) + ..push(Routes.catalog); expect(errors, hasLength(1)); expect(errors.single, isStateError); @@ -546,14 +547,15 @@ void main() => group('controller', () { addTearDown(controller.dispose); var queued = false; - controller.addListener(() { - if (queued) return; - queued = true; - controller.push(const ProductRoute(1)); - expect(controller.remove(const ProductRoute(1)), isTrue); - controller.push(Routes.settings); - }); - controller.push(Routes.catalog); + controller + ..addListener(() { + if (queued) return; + queued = true; + controller.push(const ProductRoute(1)); + expect(controller.remove(const ProductRoute(1)), isTrue); + controller.push(Routes.settings); + }) + ..push(Routes.catalog); expect( controller.stack, @@ -617,13 +619,13 @@ void main() => group('controller', () { addTearDown(() => FlutterError.onError = previous); final observer = _RecordingObserver(); - late final NavigationController controller; - controller = NavigationController( + final controller = NavigationController( [Routes.home], observers: [observer], - )..addListener(() => controller.dispose()); - - controller.push(Routes.catalog); + ); + controller + ..addListener(controller.dispose) + ..push(Routes.catalog); expect(controller.isDisposed, isTrue); expect(observer.changes, isZero); expect(observer.added, isEmpty); @@ -651,8 +653,9 @@ void main() => group('controller', () { addTearDown(controller.dispose); Future? popped; - controller.addListener(() => popped ??= controller.maybePop()); - controller.pushAll([Routes.catalog, Routes.settings]); + controller + ..addListener(() => popped ??= controller.maybePop()) + ..pushAll([Routes.catalog, Routes.settings]); expect(await popped, isTrue); expect( From 3332bdd90179419d891d43ab8fa671ad91b53831 Mon Sep 17 00:00:00 2001 From: Mike Matiunin Date: Thu, 1 Oct 2026 19:31:36 +0400 Subject: [PATCH 7/8] Ask the navigator in maybePop after the new pages are built and withdraw the back button of the last view - maybePop called from a guard, an observer or a listener with a mounted view waits for the change to be committed and the pages to be rebuilt, instead of asking a navigator that still shows the previous stack. - Removing the last top level view reports the back button of the navigator it was shown in, so the platform does not keep the claim of a view that no longer exists. - The documentation of NavigationStack no longer promises identity comparison for route classes that override ==. --- CHANGELOG.md | 5 +++-- lib/src/controller.dart | 29 +++++++++++++++++++++++- lib/src/route.dart | 7 +++--- lib/src/view.dart | 21 +++++++++++++++++ test/back_button_test.dart | 46 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 102 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 17b3e9d..34df33a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,8 +11,9 @@ All notable changes to this project will be documented in this file. blocking the visible route, a previous tab of a `NavigationTabsController` or an `onBackButtonPressed` of the view. A nested view reports again when its route becomes current again, an - inactive tab no longer speaks for the visible one, and a view with - `interceptBackButton: false` no longer claims the press. + inactive tab no longer speaks for the visible one, a view with + `interceptBackButton: false` no longer claims the press, and removing + the last view withdraws its claim. - **Fixed**: a `NavigationTabsController` reports the back button when it is created around mounted views and when it is disposed, so no stale claim of a previous tab remains. diff --git a/lib/src/controller.dart b/lib/src/controller.dart index 07041e5..124188a 100644 --- a/lib/src/controller.dart +++ b/lib/src/controller.dart @@ -142,6 +142,9 @@ class NavigationController /// committed, completed with `false` when the queue is dropped. final List> _pendingPops = >[]; + /// The [maybePop] calls waiting for the current change to be committed. + final List> _commitWaiters = >[]; + /// Maximum number of changes requested from within a single transition. /// /// The changes over the limit are reported to `FlutterError` and dropped: @@ -276,6 +279,13 @@ class NavigationController _processing = false; _settleResults(); _settlePendingPops(); + if (_commitWaiters.isNotEmpty) { + final waiters = List>.of(_commitWaiters); + _commitWaiters.clear(); + for (final completer in waiters) { + completer.complete(); + } + } } } @@ -376,9 +386,18 @@ class NavigationController /// /// Returns `true` when the back navigation was handled. When called from a /// guard, an observer or a listener, the decision is made against the - /// stack the pop applies to, once the current change is committed. + /// stack the pop applies to, once the current change is committed (and, + /// with a mounted [NavigationView], once its pages are rebuilt on the + /// next frame). Future maybePop() async { if (_disposed) return false; + if (_processing && this.navigator != null) { + // The navigator shows the stack being committed only once the view + // has been rebuilt: ask it after the next frame. + await _whenCommitted(); + await SchedulerBinding.instance.endOfFrame; + return maybePop(); + } final group = _group; if (group != null && !group.isActiveMember(this)) return false; final navigator = this.navigator; @@ -390,6 +409,14 @@ class NavigationController return group?.popGroupMember() ?? false; } + /// Completes once the current change and the changes queued after it + /// are committed. + Future _whenCommitted() { + final completer = Completer(); + _commitWaiters.add(completer); + return completer.future; + } + /// Queues a pop after the current change and completes with whether it /// has found a route to remove. Future _popPending() { diff --git a/lib/src/route.dart b/lib/src/route.dart index 3a5d2d0..05ae39e 100644 --- a/lib/src/route.dart +++ b/lib/src/route.dart @@ -6,9 +6,10 @@ import 'package:flutter/material.dart'; /// This is a plain [List], so any operation you already know works here: /// `[...stack, route]`, `stack.where(...)`, `stack.sublist(...)` and so on. /// -/// Routes do not override `==`, so `stack.contains(route)`, -/// `stack.indexOf(route)` and `stack.remove(route)` compare instances, while -/// the controller identifies a route by its [NavigationRoute.key]: prefer +/// [NavigationRoute] does not override `==`, so unless a route class +/// overrides it, `stack.contains(route)`, `stack.indexOf(route)` and +/// `stack.remove(route)` compare instances, while the controller always +/// identifies a route by its [NavigationRoute.key]: prefer /// `controller.contains(route)`, `stack.containsKey(route.key)` and /// `stack.without(route)` for the routes that are not constants. /// diff --git a/lib/src/view.dart b/lib/src/view.dart index 7de0cb6..639df07 100644 --- a/lib/src/view.dart +++ b/lib/src/view.dart @@ -128,6 +128,10 @@ class _NavigationViewState extends State /// view, which receives the back button from the [WidgetsBinding]. _NavigationViewState? _parent; + /// The navigator a top level view is shown in, e.g. the one of the + /// `MaterialApp`, which reports the back button once no view is left. + NavigatorState? _hostNavigator; + /// The nested views, in the order they were mounted. final List<_NavigationViewState> _children = <_NavigationViewState>[]; @@ -174,6 +178,7 @@ class _NavigationViewState extends State _unregister(); // The views left behind may now be the ones handling the back button. (parent ?? _roots.firstOrNull)?._reportBackButton(); + if (parent == null && _roots.isEmpty) _reportNoViewLeft(); super.deactivate(); } @@ -186,11 +191,27 @@ class _NavigationViewState extends State if (parent is _NavigationViewMarker) { _parent = parent.state.._children.add(this); } else { + _hostNavigator = Navigator.maybeOf(context); _roots.add(this); WidgetsBinding.instance.addObserver(this); } } + /// Withdraws the claim of the last top level view: nothing else would + /// replace it, so the platform would keep sending the back button to a + /// framework that no longer handles it. + void _reportNoViewLeft() { + final host = _hostNavigator; + if (host == null) return; + // After the frame: the view may only be moved with a global key. + SchedulerBinding.instance.addPostFrameCallback((_) { + if (!host.mounted || _roots.isNotEmpty) return; + NavigationNotification( + canHandlePop: host.canPop(), + ).dispatch(host.context); + }, debugLabel: 'NavigationView.reportNoViewLeft'); + } + void _unregister() { final parent = _parent; if (parent == null) { diff --git a/test/back_button_test.dart b/test/back_button_test.dart index 59db73a..4838df9 100644 --- a/test/back_button_test.dart +++ b/test/back_button_test.dart @@ -294,6 +294,52 @@ void main() => group('back button', () { expect(platform.handlesBack, isTrue, reason: 'out of the tabs again'); }); + testWidgets('removing the last view is reported', (tester) async { + final controller = NavigationController([ + Routes.home, + Routes.catalog, + ]); + addTearDown(controller.dispose); + final visible = ValueNotifier(true); + addTearDown(visible.dispose); + final platform = await pumpApp( + tester, + ValueListenableBuilder( + valueListenable: visible, + builder: (context, value, _) => value + ? NavigationView(controller: controller) + : const Text('no navigation'), + ), + ); + expect(platform.handlesBack, isTrue); + + visible.value = false; + await tester.pumpAndSettle(); + expect(find.text('no navigation'), findsOneWidget); + expect(platform.handlesBack, isFalse, reason: 'no view is left'); + expect(await tester.binding.handlePopRoute(), isFalse); + }); + + testWidgets('maybePop from a listener pops the pages of the new stack', ( + tester, + ) async { + final controller = NavigationController([Routes.home]); + addTearDown(controller.dispose); + await pumpApp(tester, NavigationView(controller: controller)); + + Future? popped; + controller + ..addListener(() => popped ??= controller.maybePop()) + ..pushAll([Routes.catalog, Routes.settings]); + await tester.pumpAndSettle(); + + expect(await popped, isTrue); + expect( + controller.stack, + equals([Routes.home, Routes.catalog]), + ); + }); + testWidgets('a guard reading the context does not rebuild the pages', ( tester, ) async { From bd448b1ca8746bac04f8d9d81caaa067422b772a Mon Sep 17 00:00:00 2001 From: Mike Matiunin Date: Thu, 1 Oct 2026 19:52:12 +0400 Subject: [PATCH 8/8] Finish disposing a controller once the listener disposing it returns A listener calling dispose made ChangeNotifier.dispose run while notifyListeners was active, which ChangeNotifier reports as an error and which left the controller half disposed. The controller is now marked as disposed at once and ChangeNotifier.dispose runs when the notification ends. The test now checks that no error is reported. Also cover a plain route pushed by the host navigator above a view: the host navigator replaces the false reported by the hidden view with its own claim, so the press reaches it. --- lib/src/controller.dart | 27 +++++++++++++++++++++++++++ test/back_button_test.dart | 30 ++++++++++++++++++++++++++++++ test/controller_test.dart | 1 + 3 files changed, 58 insertions(+) diff --git a/lib/src/controller.dart b/lib/src/controller.dart index 124188a..50e3429 100644 --- a/lib/src/controller.dart +++ b/lib/src/controller.dart @@ -762,9 +762,36 @@ class NavigationController _completers.clear(); _results.clear(); _deferredObserverEvents.clear(); + // A listener may dispose the controller while it is being notified, + // which `ChangeNotifier` does not allow: finish once it is done. + if (_notifyingListeners > 0) { + _disposePending = true; + return; + } super.dispose(); } + /// The depth of the [notifyListeners] calls in progress. + int _notifyingListeners = 0; + + /// Whether [dispose] has been called by a listener and waits for the + /// notification to end. + bool _disposePending = false; + + @override + void notifyListeners() { + _notifyingListeners++; + try { + super.notifyListeners(); + } finally { + _notifyingListeners--; + if (_notifyingListeners == 0 && _disposePending) { + _disposePending = false; + super.dispose(); + } + } + } + @override String toString() { final buffer = StringBuffer('NavigationController('); diff --git a/test/back_button_test.dart b/test/back_button_test.dart index 4838df9..75bdc81 100644 --- a/test/back_button_test.dart +++ b/test/back_button_test.dart @@ -1,3 +1,5 @@ +import 'dart:async'; + import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:squid/squid.dart'; @@ -340,6 +342,34 @@ void main() => group('back button', () { ); }); + testWidgets('a plain route pushed above a view is reported', (tester) async { + final controller = NavigationController([Routes.home]); + addTearDown(controller.dispose); + final platform = await pumpApp( + tester, + NavigationView(controller: controller), + ); + expect(platform.handlesBack, isFalse, reason: 'nothing to pop'); + + final app = tester.state(find.byType(Navigator).first); + unawaited( + app.push(MaterialPageRoute(builder: (_) => const Text('plain'))), + ); + await tester.pumpAndSettle(); + expect(find.text('plain'), findsOneWidget); + expect(platform.handlesBack, isTrue, reason: 'the app can pop the route'); + + // The hidden view reports again. + controller.replaceTop(Routes.catalog); + await tester.pumpAndSettle(); + expect(platform.handlesBack, isTrue, reason: 'the app can pop the route'); + + expect(await tester.binding.handlePopRoute(), isTrue); + await tester.pumpAndSettle(); + expect(find.text('plain'), findsNothing); + expect(platform.handlesBack, isFalse); + }); + testWidgets('a guard reading the context does not rebuild the pages', ( tester, ) async { diff --git a/test/controller_test.dart b/test/controller_test.dart index b851f67..2ae4bc0 100644 --- a/test/controller_test.dart +++ b/test/controller_test.dart @@ -626,6 +626,7 @@ void main() => group('controller', () { controller ..addListener(controller.dispose) ..push(Routes.catalog); + expect(errors, isEmpty); expect(controller.isDisposed, isTrue); expect(observer.changes, isZero); expect(observer.added, isEmpty);