Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Handle didUpdateWidget for _StackNavigator when using a global key #318

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

josephelliot-wk
Copy link
Contributor

@josephelliot-wk josephelliot-wk commented Jan 18, 2023

Problem

We found a bug where using a GlobalKey for the PageStackNavigator.navigatorKey causes the internal _StackNavigator to fail to re-assign itself to the internal page stack. This issue presented itself as the Android back button failing to close non-routemaster routes (like a bottom sheet) and instead just closing the entire app.

Inside the PageStackNavigatorState, any time the PageStack changes, it'll invoke a function that recreates the _StackNavigator widget. Without a key, this would cause the navigator widget to run initState, thus setting itself on the PageStack. However, with a key, the state object is re-used, so initState (and dispose) are never called.

This is problematic because if you provide a navigator key for hooking into the navigator throughout your app, the PageStack can change but the _StackNavigator's state functions are never called and thus pop() calls for non-routemaster routes will ultimately return false, causing the app to close.

Solution

To solve this, all we need to do is override _StackNavigatorState.didUpdateWidget and handle the scenario of the internal stack changing -- if it changes, then drop the old navigator reference and add a new navigator reference to the new page stack.

FYI @tomgilder

@codecov
Copy link

codecov bot commented Jan 18, 2023

Codecov Report

Merging #318 (baf6850) into main (e6f04ec) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #318   +/-   ##
=======================================
  Coverage   99.40%   99.40%           
=======================================
  Files          20       20           
  Lines        1006     1015    +9     
=======================================
+ Hits         1000     1009    +9     
  Misses          6        6           
Impacted Files Coverage Δ
lib/routemaster.dart 100.00% <100.00%> (ø)
lib/src/route_history.dart 87.75% <100.00%> (+1.70%) ⬆️
lib/src/pages/stack_page.dart 100.00% <0.00%> (ø)
lib/src/pages/indexed_page.dart 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update decb11a...baf6850. Read the comment docs.

@josephelliot-wk
Copy link
Contributor Author

Bump @tomgilder

2 similar comments
@davidgiametta-wk
Copy link

Bump @tomgilder

@josephelliot-wk
Copy link
Contributor Author

Bump @tomgilder

@davidgiametta-wk
Copy link

Cc @tomgilder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants