-
Notifications
You must be signed in to change notification settings - Fork 338
Fix navigation stack overflow when sharing media #5724
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
base: develop
Are you sure you want to change the base?
Conversation
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5724 +/- ##
===========================================
- Coverage 79.63% 79.63% -0.01%
===========================================
Files 2431 2431
Lines 65261 65268 +7
Branches 8332 8333 +1
===========================================
+ Hits 51972 51977 +5
- Misses 10308 10310 +2
Partials 2981 2981 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bmarty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Why not creating a SafeBackHandler in a shared module so that we can start using it?
| if (state.isSearchActive) { | ||
| state.eventSink(RoomSelectEvents.ToggleSearchActive) | ||
| } else { | ||
| canHandleBack = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is OK, but we do not check the value of canHandleBack before setting it and invoking onDismiss? Asking because navigationIcon click also invokes the fun onBackButton.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, calling onBackButton from the back navigation icon could cause a different issue (2 or more back paginations if called quickly?), but it wouldn't interfere with the BackHandler here AFAICT.
I thought you wanted to do that, so I was just adding this as a workaround in the meantime 😅 . |
|



Content
Makes sure the
BackHandlercan only navigate up once in theRoomSelectViewcomponent.This screen is a good candidate for the
SafeBackHandlerwe previously talked about @bmarty .Motivation and context
Fixes #5688.
Tests
Previously, this would get in a stack overflow where
navigateUptriggered a back event that got caught byBackHandler, which in turn navigated up, and ended up looping in that logic.Tested devices
Checklist