-
Notifications
You must be signed in to change notification settings - Fork 248
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: Use associated account as initial account; if opened from background #1240
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -155,7 +155,38 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver { | |
InitialRouteListFactory _handleGenerateInitialRoutes(BuildContext context) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This commit message is misleading 🙂; the change doesn't follow the common pattern of just moving some code out to a helper function for better organization. Putting the
Does that seem right, or am I missing something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be even clearer if it were split into two commits: one that changes when |
||
final globalStore = GlobalStoreWidget.of(context); | ||
|
||
void showNotificationErrorDialog() async { | ||
final navigator = await ZulipApp.navigator; | ||
final navigatorContext = navigator.context; | ||
assert(navigatorContext.mounted); | ||
// TODO(linter): this is impossible as there's no actual async gap, but | ||
// the use_build_context_synchronously lint doesn't see that. | ||
if (!navigatorContext.mounted) return; | ||
|
||
final zulipLocalizations = ZulipLocalizations.of(navigatorContext); | ||
showErrorDialog(context: navigatorContext, | ||
title: zulipLocalizations.errorNotificationOpenTitle, | ||
message: zulipLocalizations.errorNotificationOpenAccountMissing); | ||
} | ||
|
||
return (String initialRoute) { | ||
final initialRouteUrl = Uri.parse(initialRoute); | ||
if (initialRouteUrl case Uri(scheme: 'zulip', host: 'notification')) { | ||
final notificationResult = NotificationDisplayManager.routeForNotification( | ||
globalStore: globalStore, | ||
url: initialRouteUrl); | ||
|
||
if (notificationResult != null) { | ||
return [ | ||
HomePage.buildRoute(accountId: notificationResult.accountId), | ||
notificationResult.route, | ||
]; | ||
} else { | ||
showNotificationErrorDialog(); | ||
// Fallthrough to show default route below. | ||
} | ||
} | ||
|
||
// TODO(#524) choose initial account as last one used | ||
final initialAccountId = globalStore.accounts.firstOrNull?.id; | ||
return [ | ||
|
@@ -167,18 +198,10 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver { | |
}; | ||
} | ||
|
||
Future<void> _handleInitialRoute() async { | ||
final initialRouteUrl = Uri.parse(WidgetsBinding.instance.platformDispatcher.defaultRouteName); | ||
if (initialRouteUrl case Uri(scheme: 'zulip', host: 'notification')) { | ||
await NotificationDisplayManager.navigateForNotification(initialRouteUrl); | ||
} | ||
} | ||
|
||
@override | ||
void initState() { | ||
super.initState(); | ||
WidgetsBinding.instance.addObserver(this); | ||
_handleInitialRoute(); | ||
} | ||
|
||
@override | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1097,6 +1097,45 @@ void main() { | |
takeStartingRoutes(); | ||
matchesNavigation(check(pushedRoutes).single, account, message); | ||
}); | ||
|
||
testWidgets('uses associated account as initial account; if initial route', (tester) async { | ||
addTearDown(testBinding.reset); | ||
addTearDown(tester.binding.platformDispatcher.clearDefaultRouteNameTestValue); | ||
|
||
final accountA = eg.selfAccount; | ||
final accountB = eg.otherAccount; | ||
final message = eg.streamMessage(); | ||
final data = messageFcmMessage(message, account: accountB); | ||
await testBinding.globalStore.add(accountA, eg.initialSnapshot()); | ||
await testBinding.globalStore.add(accountB, eg.initialSnapshot()); | ||
|
||
final intentDataUrl = NotificationOpenPayload( | ||
realmUrl: data.realmUrl, | ||
userId: data.userId, | ||
narrow: switch (data.recipient) { | ||
FcmMessageChannelRecipient(:var streamId, :var topic) => | ||
TopicNarrow(streamId, topic), | ||
FcmMessageDmRecipient(:var allRecipientIds) => | ||
DmNarrow(allRecipientIds: allRecipientIds, selfUserId: data.userId), | ||
}).buildUrl(); | ||
tester.binding.platformDispatcher.defaultRouteNameTestValue = intentDataUrl.toString(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like it needs a corresponding teardown, with |
||
|
||
await prepare(tester, early: true); | ||
check(pushedRoutes).isEmpty(); // GlobalStore hasn't loaded yet | ||
|
||
await tester.pump(); | ||
check(pushedRoutes).deepEquals(<Condition<Object?>>[ | ||
(it) => it.isA<MaterialAccountWidgetRoute>() | ||
..accountId.equals(accountB.id) | ||
..page.isA<HomePage>(), | ||
(it) => it.isA<MaterialAccountWidgetRoute>() | ||
..accountId.equals(accountB.id) | ||
..page.isA<MessageListPage>() | ||
.initNarrow.equals(SendableNarrow.ofMessage(message, | ||
selfUserId: accountB.userId)) | ||
]); | ||
pushedRoutes.clear(); | ||
}); | ||
}); | ||
|
||
group('NotificationOpenPayload', () { | ||
|
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.
How likely do we think it is that these realm URLs could differ in boring ways that shouldn't matter? Like
https://chat.zulip.org/
vs. ``https://chat.zulip.org`. Do we need to just compare.origin
instead, like we do for most (all?) other realm-URL checks?