-
Notifications
You must be signed in to change notification settings - Fork 291
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
notif: Preserve target account context on back navigation after opening a notification #1373
base: main
Are you sure you want to change the base?
Conversation
509ba69
to
790c6df
Compare
|| (topRoute as AccountRoute).accountId != route.accountId) { | ||
HomePage.navigate(context, accountId: route.accountId); | ||
navigator = await ZulipApp.navigator; | ||
} |
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.
The popUntil
above unconditionally clears the navigator stack until there is only one route left. I believe that this is equivalent to HomePage.navigate
, which also pops all routes beforehand.
Either way, we should not pop all routes unconditionally, because it doesn't feel like great user experience when the existing routes already belong to the same account.
#F1210 suggests that we
switch to a new nav stack for the notification's account (if different) before pushing the message-list route.
I think what we need here is just a way to keep track of the currently active account, or a way to inspect the navigator stack without actually popping the routes.
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.
See discussion here: #mobile > Beta: after open via notification, back opens old server @ 💬. Feel free to ask questions if you have any!
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.
Based on my current implementation, I have addressed the following:
- Inspect the navigation stack without modifying it by using
popUntil
withreturn true
. - Only switch accounts (via
HomePage.navigate
) when the notification is for a different account than the current one (currentAccountId != route.accountId
). - When the notification is for the current account, preserve the existing navigation stack and simply push the new route on top, providing a better user experience.
…opening notification Fixes: zulip#1210
Hello @PIG208, I have pushed the revision. PTAL, Thanks! |
Hi @PIG208 , whenever you get some time, please take a look at the PR. Thanks! |
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 for the update! I think a full implementation might need to be a bit more complex, since the top-most route might not always tell us which account "context" we are currently in.
await tester.pump(); | ||
takeStartingRoutes(account: accountA); | ||
|
||
Route<dynamic>? topRoute; |
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.
nit: we can use Route<void>?
instead, to avoid dynamic
.
if (r is AccountRoute) { | ||
currentAccountId = r.accountId; | ||
} | ||
return true; |
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 popUntil
only looks at the top-most route, but that's insufficient when that route is not an AccountRoute
, e.g. switch-account page.
A way to check the status of the navigator stack without popping anything from it is through a NavigatorObserver
. _PreventEmptyStack
is an example of it.
I think we can have another observer that implements callbacks where a route is added to the navigator stack (namely didPush
and didReplace
), and keep track of the last time an AccountRoute
was pushed, and the associated account ID, to implement this idea.
navigator.popUntil((r) { | ||
topRoute = r; | ||
return true; | ||
}); |
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.
For tests, we can use TestNavigatorObserver
to keep track of the routes on the navigator stack. Search pushedRoutes
and poppedRoutes
, etc. for examples of how we do it in tests.
check(topRoute).isA<MaterialAccountWidgetRoute>() | ||
..accountId.equals(accountB.id) | ||
..page.isA<MessageListPage>(); |
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 we want to check that the route under the AccountRoute
associated with accountB
gets popped because we are opening a notification for a different account.
From the perspective of testing, it is less interesting that topRoute
matches the route for the notification.
navigator.popUntil((r) { | ||
topRoute = r; | ||
return true; | ||
}); |
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.
Similarly, let's try to structure this test with a TestNavigatorObserver
, so we have a better idea of what happened to the navigator stack.
}); | ||
check(topRoute).isA<AccountRoute>() | ||
.accountId.equals(account.id); | ||
}); | ||
}); | ||
|
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.
We should also have a test where the top-most route is a non-AccountRoute
.
return true; | ||
}); | ||
|
||
if (currentAccountId != route.accountId) { |
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.
When currentAccountId
(i.e., account ID associated with the top-most route) is null
, routes below the top-most route can still belong to the same account. HomePage.navigate
will pop those routes even though they belong to same account that receives this notification.
Fixes: #1210
Description
When opening a notification from a different account B while in account A and pressing back, the app should stay in account B's context rather than returning to account A. This PR implements this behavior by:
Video Demonstration
WhatsApp.Video.2025-02-22.at.10.34.48.PM.mp4