Skip to content

Commit

Permalink
notif: Use associated account as initial account; if opened from back…
Browse files Browse the repository at this point in the history 
…ground

Previously, when two accounts (Account-1 and Account-2) were
logged in, the app always defaulted to showing the home page
of Account-1 on launch. If the app was closed and the user
opened a notification from Account-2, the navigation stack
would be:
  HomePage(Account-1) -> MessageListPage(Account-2)

This commit fixes that behaviour, now when a notification is
opened while the app is closed, the home page will correspond
to the account associated with the notification's conversation.

This addresses #1210 for background notifications.
  • Loading branch information
@rajveermalviya
rajveermalviya committed Dec 30, 2024
1 parent 3ff7096 commit 1b7b732
Show file tree
Hide file tree
Showing 3 changed files with 122 additions and 31 deletions.
42 changes: 32 additions & 10 deletions lib/notifications/display.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ import '../log.dart';
import '../model/binding.dart';
import '../model/localizations.dart';
import '../model/narrow.dart';
import '../model/store.dart';
import '../widgets/app.dart';
import '../widgets/color.dart';
import '../widgets/dialog.dart';
import '../widgets/message_list.dart';
import '../widgets/page.dart';
import '../widgets/store.dart';
import '../widgets/theme.dart';

Expand Down Expand Up @@ -456,36 +456,58 @@ class NotificationDisplayManager {

static String _personKey(Uri realmUrl, int userId) => "$realmUrl|$userId";

/// Provides the route and the account ID by parsing the notification URL.
///
/// The URL must have been generated using [NotificationOpenPayload.buildUrl]
/// while creating the notification.
///
/// Returns null if the associated account is not found in the global
/// store.
static ({Route<void> route, int accountId})? routeForNotification({
required GlobalStore globalStore,
required Uri url,
}) {
assert(debugLog('got notif: url: $url'));
assert(url.scheme == 'zulip' && url.host == 'notification');
final payload = NotificationOpenPayload.parseUrl(url);

final account = globalStore.accounts.firstWhereOrNull((account) =>
account.realmUrl == payload.realmUrl && account.userId == payload.userId);
if (account == null) return null;

final route = MessageListPage.buildRoute(
accountId: account.id,
// TODO(#82): Open at specific message, not just conversation
narrow: payload.narrow);
return (route: route, accountId: account.id);
}

/// Navigates to the [MessageListPage] of the specific conversation
/// given the `zulip://notification/…` Android intent data URL,
/// generated with [NotificationOpenPayload.buildUrl] while creating
/// the notification.
static Future<void> navigateForNotification(Uri url) async {
assert(debugLog('opened notif: url: $url'));

assert(url.scheme == 'zulip' && url.host == 'notification');
final payload = NotificationOpenPayload.parseUrl(url);

NavigatorState navigator = await ZulipApp.navigator;
final context = navigator.context;
assert(context.mounted);
if (!context.mounted) return; // TODO(linter): this is impossible as there's no actual async gap, but the use_build_context_synchronously lint doesn't see that

final zulipLocalizations = ZulipLocalizations.of(context);
final globalStore = GlobalStoreWidget.of(context);
final account = globalStore.accounts.firstWhereOrNull((account) =>
account.realmUrl == payload.realmUrl && account.userId == payload.userId);
if (account == null) { // TODO(log)

final notificationResult =
routeForNotification(globalStore: globalStore, url: url);
if (notificationResult == null) { // TODO(log)
showErrorDialog(context: context,
title: zulipLocalizations.errorNotificationOpenTitle,
message: zulipLocalizations.errorNotificationOpenAccountMissing);
return;
}

// TODO(nav): Better interact with existing nav stack on notif open
unawaited(navigator.push(MaterialAccountWidgetRoute<void>(accountId: account.id,
// TODO(#82): Open at specific message, not just conversation
page: MessageListPage(initNarrow: payload.narrow))));
unawaited(navigator.push(notificationResult.route));
}

static Future<Uint8List?> _fetchBitmap(Uri url) async {
Expand Down
60 changes: 44 additions & 16 deletions lib/widgets/app.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,18 +152,56 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
return super.didPushRouteInformation(routeInformation);
}

Future<void> _handleInitialRoute() async {
final initialRouteUrl = Uri.parse(WidgetsBinding.instance.platformDispatcher.defaultRouteName);
if (initialRouteUrl case Uri(scheme: 'zulip', host: 'notification')) {
await NotificationDisplayManager.navigateForNotification(initialRouteUrl);
InitialRouteListFactory _handleGenerateInitialRoutes(BuildContext 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);
}

final globalStore = GlobalStoreWidget.of(context);
// TODO(#524) choose initial account as last one used
final initialAccountId = globalStore.accounts.firstOrNull?.id;

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.
}
}

return [
if (initialAccountId == null)
MaterialWidgetRoute(page: const ChooseAccountPage())
else
HomePage.buildRoute(accountId: initialAccountId),
];
};
}

@override
void initState() {
super.initState();
WidgetsBinding.instance.addObserver(this);
_handleInitialRoute();
}

@override
Expand All @@ -177,9 +215,6 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
final themeData = zulipThemeData(context);
return GlobalStoreWidget(
child: Builder(builder: (context) {
final globalStore = GlobalStoreWidget.of(context);
// TODO(#524) choose initial account as last one used
final initialAccountId = globalStore.accounts.firstOrNull?.id;
return MaterialApp(
title: 'Zulip',
localizationsDelegates: ZulipLocalizations.localizationsDelegates,
Expand All @@ -206,14 +241,7 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
// like [Navigator.push], never mere names as with [Navigator.pushNamed].
onGenerateRoute: (_) => null,

onGenerateInitialRoutes: (_) {
return [
if (initialAccountId == null)
MaterialWidgetRoute(page: const ChooseAccountPage())
else
HomePage.buildRoute(accountId: initialAccountId),
];
});
onGenerateInitialRoutes: _handleGenerateInitialRoutes(context));
}));
}
}
Expand Down
51 changes: 46 additions & 5 deletions test/notifications/display_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1070,6 +1070,7 @@ void main() {

testWidgets('at app launch', (tester) async {
addTearDown(testBinding.reset);
addTearDown(tester.binding.platformDispatcher.clearDefaultRouteNameTestValue);
// Set up a value for `PlatformDispatcher.defaultRouteName` to return,
// for determining the intial route.
final account = eg.selfAccount;
Expand All @@ -1079,11 +1080,11 @@ void main() {
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();
FcmMessageChannelRecipient(:var streamId, :var topic) =>
TopicNarrow(streamId, topic),
FcmMessageDmRecipient(:var allRecipientIds) =>
DmNarrow(allRecipientIds: allRecipientIds, selfUserId: data.userId),
}).buildUrl();
tester.binding.platformDispatcher.defaultRouteNameTestValue = intentDataUrl.toString();

// Now start the app.
Expand All @@ -1096,6 +1097,46 @@ 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);
addTearDown(pushedRoutes.clear);

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();

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', () {
Expand Down

0 comments on commit 1b7b732

Please sign in to comment.