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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

notif: Use associated account as initial account; if opened from background #1240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c961903
notif test [nfc]: Cleanup `TestPlatformDispatcher.defaultRouteNameTes…
rajveermalviya Jan 2, 2025
4d68974
app [nfc]: Pull out `_handleGenerateInitialRoutes`
rajveermalviya Jan 2, 2025
ae30cd3
notif: Use associated account as initial account; if opened from back…
rajveermalviya Dec 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 32 additions & 10 deletions lib/notifications/display.dart
View file
Open in desktop
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);
Copy link
Collaborator

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?

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
Open in desktop
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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

app [nfc]: Pull out _handleGenerateInitialRoutes

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 globalStore.accounts query inside the callback, instead of outside, is significant and worth mentioning; in fact it might not be NFC. The reason I gave in #1240 (comment) was:

Then in particular we don't have to think about what happens if the firstOrNull account is logged out between a call to _ZulipAppState.build and the time when Flutter decides to call this InitialRouteListFactory callback.

Does that seem right, or am I missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 globalStore.accounts is queried, and one for making the helper function (the pure refactor).

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 [
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
50 changes: 45 additions & 5 deletions test/notifications/display_test.dart
View file
Open in desktop
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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be best in its own commit, since it's about an unrelated test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this and the formating change in this test to a separate commit.

// 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,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();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it needs a corresponding teardown, with clearDefaultRouteNameTestValue. (Same with the existing place we set this, actually)


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