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.

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

Go straight to inbox on launch, when an account available #525

Merged
merged 16 commits into from
Feb 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
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
2 changes: 2 additions & 0 deletions integration_test/unreadmarker_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import 'package:zulip/api/model/model.dart';
import 'package:zulip/model/narrow.dart';
import 'package:zulip/model/store.dart';
import 'package:zulip/widgets/message_list.dart';
import 'package:zulip/widgets/page.dart';
import 'package:zulip/widgets/store.dart';

import '../test/api/fake_api.dart';
Expand Down Expand Up @@ -38,6 +39,7 @@ void main() {
home: GlobalStoreWidget(
child: PerAccountStoreWidget(
accountId: eg.selfAccount.id,
placeholder: const LoadingPlaceholderPage(),
child: const MessageListPage(narrow: AllMessagesNarrow())))));
await tester.pumpAndSettle();
return messages;
Expand Down
7 changes: 3 additions & 4 deletions lib/notifications.dart
Original file line number Diff line number Diff line change
Expand Up @@ -389,10 +389,9 @@ class NotificationDisplayManager {

assert(debugLog(' account: $account, narrow: $narrow'));
// TODO(nav): Better interact with existing nav stack on notif open
navigator.push(MaterialWidgetRoute(
page: PerAccountStoreWidget(accountId: account.id,
// TODO(#82): Open at specific message, not just conversation
child: MessageListPage(narrow: narrow))));
navigator.push(MaterialAccountWidgetRoute(accountId: account.id,
// TODO(#82): Open at specific message, not just conversation
page: MessageListPage(narrow: narrow)));
return;
}
}
62 changes: 43 additions & 19 deletions lib/widgets/app.dart
Original file line number Diff line number Diff line change
Expand Up @@ -111,23 +111,48 @@ class ZulipApp extends StatelessWidget {
// a finger or thumb than the area above.
tooltipTheme: const TooltipThemeData(preferBelow: false),
);

return GlobalStoreWidget(
child: MaterialApp(
title: 'Zulip',
localizationsDelegates: ZulipLocalizations.localizationsDelegates,
supportedLocales: ZulipLocalizations.supportedLocales,
theme: theme,
navigatorKey: navigatorKey,
navigatorObservers: navigatorObservers ?? const [],
builder: (BuildContext context, Widget? child) {
if (!ready.value) {
SchedulerBinding.instance.addPostFrameCallback(
(_) => _declareReady());
}
GlobalLocalizations.zulipLocalizations = ZulipLocalizations.of(context);
return child!;
},
home: const ChooseAccountPage()));
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,
supportedLocales: ZulipLocalizations.supportedLocales,
theme: theme,

navigatorKey: navigatorKey,
navigatorObservers: navigatorObservers ?? const [],
builder: (BuildContext context, Widget? child) {
if (!ready.value) {
SchedulerBinding.instance.addPostFrameCallback(
(_) => _declareReady());
}
GlobalLocalizations.zulipLocalizations = ZulipLocalizations.of(context);
return child!;
},

// We use onGenerateInitialRoutes for the real work of specifying the
// initial nav state. To do that we need [MaterialApp] to decide to
// build a [Navigator]... which means specifying either `home`, `routes`,
// `onGenerateRoute`, or `onUnknownRoute`. Make it `onGenerateRoute`.
// It never actually gets called, though: `onGenerateInitialRoutes`
// handles startup, and then we always push whole routes with methods
// like [Navigator.push], never mere names as with [Navigator.pushNamed].
onGenerateRoute: (_) => null,

onGenerateInitialRoutes: (_) {
return [
MaterialWidgetRoute(page: const ChooseAccountPage()),
if (initialAccountId != null) ...[
HomePage.buildRoute(accountId: initialAccountId),
InboxPage.buildRoute(accountId: initialAccountId),
],
];
});
}));
}
}

Expand Down Expand Up @@ -211,9 +236,8 @@ class HomePage extends StatelessWidget {
const HomePage({super.key});

static Route<void> buildRoute({required int accountId}) {
return MaterialWidgetRoute(
page: PerAccountStoreWidget(accountId: accountId,
child: const HomePage()));
return MaterialAccountWidgetRoute(accountId: accountId,
page: const HomePage());
}

@override
Expand Down
4 changes: 2 additions & 2 deletions lib/widgets/inbox.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ import 'unread_count_badge.dart';
class InboxPage extends StatefulWidget {
const InboxPage({super.key});

static Route<void> buildRoute({required BuildContext context}) {
return MaterialAccountWidgetRoute(context: context,
static Route<void> buildRoute({int? accountId, BuildContext? context}) {
return MaterialAccountWidgetRoute(accountId: accountId, context: context,
page: const InboxPage());
}

Expand Down
4 changes: 3 additions & 1 deletion lib/widgets/lightbox.dart
Original file line number Diff line number Diff line change
Expand Up @@ -208,11 +208,13 @@ class _LightboxPageState extends State<_LightboxPage> {
}

Route getLightboxRoute({
required BuildContext context,
int? accountId,
BuildContext? context,
required Message message,
required Uri src,
}) {
return AccountPageRouteBuilder(
accountId: accountId,
context: context,
fullscreenDialog: true,
pageBuilder: (
Expand Down
5 changes: 3 additions & 2 deletions lib/widgets/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@ import 'text.dart';
class MessageListPage extends StatefulWidget {
const MessageListPage({super.key, required this.narrow});

static Route<void> buildRoute({required BuildContext context, required Narrow narrow}) {
return MaterialAccountWidgetRoute(context: context,
static Route<void> buildRoute({int? accountId, BuildContext? context,
required Narrow narrow}) {
return MaterialAccountWidgetRoute(accountId: accountId, context: context,
page: MessageListPage(narrow: narrow));
}

Expand Down
89 changes: 82 additions & 7 deletions lib/widgets/page.dart
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ abstract class WidgetRoute<T> extends PageRoute<T> {
/// A [MaterialPageRoute] that always builds the same widget.
///
/// This is useful for making the route more transparent for a test to inspect.
///
/// See also:
/// * [MaterialAccountWidgetRoute], a subclass which automates providing a
/// per-account store on the new route.
class MaterialWidgetRoute<T> extends MaterialPageRoute<T> implements WidgetRoute<T> {
MaterialWidgetRoute({
required this.page,
Expand All @@ -27,37 +31,79 @@ class MaterialWidgetRoute<T> extends MaterialPageRoute<T> implements WidgetRoute
final Widget page;
}

/// A mixin for providing a given account's per-account store on a page route.
mixin AccountPageRouteMixin<T> on PageRoute<T> {
int get accountId;

@override
Widget buildPage(BuildContext context, Animation<double> animation, Animation<double> secondaryAnimation) {
return PerAccountStoreWidget(
accountId: accountId,
placeholder: const LoadingPlaceholderPage(),
child: super.buildPage(context, animation, secondaryAnimation));
}
}

/// A [MaterialPageRoute] providing a per-account store for a given account.
///
/// See also:
/// * [MaterialAccountWidgetRoute], a subclass which is more transparent
/// for tests.
/// * [AccountPageRouteBuilder], for defining one-off page routes
/// in terms of callbacks.
class MaterialAccountPageRoute<T> extends MaterialPageRoute<T> with AccountPageRouteMixin<T> {
/// Construct a [MaterialAccountPageRoute] using either the given account ID,
/// or the ambient one from the given context.
///
/// The account ID used is [accountId] if specified,
/// else the ambient account ID from [context].
/// One of those parameters must be specified, and not both.
///
/// Generally most navigation in the app is within a given account,
/// and should use [context]. Using [accountId] is appropriate for
/// navigating across accounts, or navigating into an account from contexts
/// (like login or the choose-account page) that don't have an ambient account.
MaterialAccountPageRoute({
required BuildContext context,
int? accountId,
BuildContext? context,
required super.builder,
super.settings,
super.maintainState,
super.fullscreenDialog,
super.allowSnapshotting,
}) : accountId = PerAccountStoreWidget.accountIdOf(context);
}) : assert((accountId != null) ^ (context != null),
"exactly one of accountId or context must be specified"),
accountId = accountId ?? PerAccountStoreWidget.accountIdOf(context!);

@override
final int accountId;
}

/// A [MaterialAccountPageRoute] that always builds the same widget.
/// A [MaterialPageRoute] that provides a per-account store for a given account
/// and always builds the same widget.
///
/// This is useful for making the route more transparent for a test to inspect.
/// This is the [PageRoute] subclass to use for most navigation in the app.
///
/// Always building the same widget is useful for making the route
/// more transparent for a test to inspect.
///
/// See also:
/// * [MaterialWidgetRoute], for routes that need no per-account store.
class MaterialAccountWidgetRoute<T> extends MaterialAccountPageRoute<T> implements WidgetRoute<T> {
/// Construct a [MaterialAccountWidgetRoute] using either the given account ID,
/// or the ambient one from the given context.
///
/// The account ID used is [accountId] if specified,
/// else the ambient account ID from [context].
/// One of those parameters must be specified, and not both.
///
/// Generally most navigation in the app is within a given account,
/// and should use [context]. Using [accountId] is appropriate for
/// navigating across accounts, or navigating into an account from contexts
/// (like login or the choose-account page) that don't have an ambient account.
MaterialAccountWidgetRoute({
required super.context,
super.accountId,
super.context,
required this.page,
super.settings,
super.maintainState,
Expand All @@ -69,9 +115,24 @@ class MaterialAccountWidgetRoute<T> extends MaterialAccountPageRoute<T> implemen
final Widget page;
}

/// A [PageRouteBuilder] providing a per-account store for a given account.
///
/// This is the [PageRouteBuilder] analogue of [MaterialAccountPageRoute].
class AccountPageRouteBuilder<T> extends PageRouteBuilder<T> with AccountPageRouteMixin<T> {
/// Construct an [AccountPageRouteBuilder] using either the given account ID,
/// or the ambient one from the given context.
///
/// The account ID used is [accountId] if specified,
/// else the ambient account ID from [context].
/// One of those parameters must be specified, and not both.
///
/// Generally most navigation in the app is within a given account,
/// and should use [context]. Using [accountId] is appropriate for
/// navigating across accounts, or navigating into an account from contexts
/// (like login or the choose-account page) that don't have an ambient account.
AccountPageRouteBuilder({
required BuildContext context,
int? accountId,
BuildContext? context,
super.settings,
required super.pageBuilder,
super.transitionsBuilder,
Expand All @@ -84,8 +145,22 @@ class AccountPageRouteBuilder<T> extends PageRouteBuilder<T> with AccountPageRou
super.maintainState,
super.fullscreenDialog,
super.allowSnapshotting,
}) : accountId = PerAccountStoreWidget.accountIdOf(context);
}) : assert((accountId != null) ^ (context != null),
"exactly one of accountId or context must be specified"),
accountId = accountId ?? PerAccountStoreWidget.accountIdOf(context!);

@override
final int accountId;
}

class LoadingPlaceholderPage extends StatelessWidget {
const LoadingPlaceholderPage({super.key});

@override
Widget build(BuildContext context) {
return Scaffold(
appBar: AppBar(),
body: const LoadingPlaceholder(),
);
}
}
5 changes: 3 additions & 2 deletions lib/widgets/profile.dart
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@ class ProfilePage extends StatelessWidget {

final int userId;

static Route<void> buildRoute({required BuildContext context, required int userId}) {
return MaterialAccountWidgetRoute(context: context,
static Route<void> buildRoute({int? accountId, BuildContext? context,
required int userId}) {
return MaterialAccountWidgetRoute(accountId: accountId, context: context,
page: ProfilePage(userId: userId));
}

Expand Down
4 changes: 2 additions & 2 deletions lib/widgets/recent_dm_conversations.dart
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ import 'unread_count_badge.dart';
class RecentDmConversationsPage extends StatefulWidget {
const RecentDmConversationsPage({super.key});

static Route<void> buildRoute({required BuildContext context}) {
return MaterialAccountWidgetRoute(context: context,
static Route<void> buildRoute({int? accountId, BuildContext? context}) {
return MaterialAccountWidgetRoute(accountId: accountId, context: context,
page: const RecentDmConversationsPage());
}

Expand Down
19 changes: 12 additions & 7 deletions lib/widgets/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,13 @@ import '../model/store.dart';
/// * [PerAccountStoreWidget], for the user's data associated with a
/// particular Zulip account.
class GlobalStoreWidget extends StatefulWidget {
const GlobalStoreWidget({super.key, required this.child});
const GlobalStoreWidget({
super.key,
this.placeholder = const LoadingPlaceholder(),
required this.child,
});

final Widget placeholder;
final Widget child;

/// The app's global data store.
Expand Down Expand Up @@ -65,8 +70,7 @@ class _GlobalStoreWidgetState extends State<GlobalStoreWidget> {
@override
Widget build(BuildContext context) {
final store = this.store;
// TODO: factor out the use of LoadingPage to be configured by the widget, like [widget.child] is
if (store == null) return const LoadingPage();
if (store == null) return widget.placeholder;
return _GlobalStoreInheritedWidget(store: store, child: widget.child);
}
}
Expand Down Expand Up @@ -107,10 +111,12 @@ class PerAccountStoreWidget extends StatefulWidget {
const PerAccountStoreWidget({
super.key,
required this.accountId,
this.placeholder = const LoadingPlaceholder(),
required this.child,
});

final int accountId;
final Widget placeholder;
final Widget child;

/// The user's data for the relevant Zulip account for this widget.
Expand Down Expand Up @@ -220,8 +226,7 @@ class _PerAccountStoreWidgetState extends State<PerAccountStoreWidget> {

@override
Widget build(BuildContext context) {
// TODO: factor out the use of LoadingPage to be configured by the widget, like [widget.child] is
if (store == null) return const LoadingPage();
if (store == null) return widget.placeholder;
return _PerAccountStoreInheritedWidget(store: store!, child: widget.child);
}
}
Expand All @@ -242,8 +247,8 @@ class _PerAccountStoreInheritedWidget extends InheritedNotifier<PerAccountStore>
store != oldWidget.store;
}

class LoadingPage extends StatelessWidget {
const LoadingPage({super.key});
class LoadingPlaceholder extends StatelessWidget {
const LoadingPlaceholder({super.key});

@override
Widget build(BuildContext context) {
Expand Down
4 changes: 2 additions & 2 deletions lib/widgets/subscription_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ import 'unread_count_badge.dart';
class SubscriptionListPage extends StatefulWidget {
const SubscriptionListPage({super.key});

static Route<void> buildRoute({required BuildContext context}) {
return MaterialAccountWidgetRoute(context: context,
static Route<void> buildRoute({int? accountId, BuildContext? context}) {
return MaterialAccountWidgetRoute(accountId: accountId, context: context,
page: const SubscriptionListPage());
}

Expand Down
Loading
Loading