store: Have GlobalStore manage one PerAccountStore per account

lib/model/store.dart

@@ -13,30 +13,88 @@ import '../api/route/messages.dart';
 import '../credential_fixture.dart' as credentials;
 import 'message_list.dart';
 
-/// Store for the user's cross-account data.
+/// Store for all the user's data.
 ///
-/// This includes data that is independent of the account, like some settings.
-/// It also includes a small amount of data for each account: enough to
-/// authenticate as the active account, if there is one.
-class GlobalStore extends ChangeNotifier {
-  GlobalStore._({required Map<int, Account> accounts})
+/// From UI code, use [GlobalStoreWidget.of] to get hold of an appropriate
+/// instance of this class.
+///
+/// This object carries data that is independent of the account, like some
+/// settings. It also includes a small amount of data for each account: enough
+/// to authenticate as the active account, if there is one.
+///
+/// For other data associated with a particular account, a [GlobalStore]
+/// provides a [PerAccountStore] for each account, which can be reached with
+/// [GlobalStore.perAccount] or [GlobalStore.perAccountSync].
+///
+/// See also:
+///  * [LiveGlobalStore], the implementation of this class that
+///    we use outside of tests.
+abstract class GlobalStore extends ChangeNotifier {
+  GlobalStore({required Map<int, Account> accounts})
       : _accounts = accounts;
 
-  // For convenience, a number we won't use as an ID in the database table.
-  static const fixtureAccountId = -1;
-
-  // We keep the API simple and synchronous for the bulk of the app's code
-  // by doing this loading up front before constructing a [GlobalStore].
-  static Future<GlobalStore> load() async {
-    const accounts = {fixtureAccountId: _fixtureAccount};
-    return GlobalStore._(accounts: accounts);
-  }
-
   final Map<int, Account> _accounts;
 
   // TODO settings (those that are per-device rather than per-account)
   // TODO push token, and other data corresponding to GlobalSessionState
 
+  final Map<int, PerAccountStore> _perAccountStores = {};
+  final Map<int, Future<PerAccountStore>> _perAccountStoresLoading = {};
+
+  /// The store's per-account data for the given account, if already loaded.
+  ///
+  /// When not null, this is the same [PerAccountStore] that would be returned
+  /// by the asynchronous [perAccount].
+  PerAccountStore? perAccountSync(int accountId) => _perAccountStores[accountId];
+
+  /// The store's per-account data for the given account.
+  ///
+  /// If the data for this account is not already loaded, this will ensure a
+  /// request is made to load it, and the returned future will complete when
+  /// the data is ready.
+  ///
+  /// The [GlobalStore] will avoid making redundant requests for the same data,
+  /// even if this method is called many times.  The futures returned from each
+  /// call for the same account will all complete once the data is ready.
+  ///
+  /// Consider checking [perAccountSync] before calling this function, so that if
+  /// the data is already available it can be used immediately (e.g., in the
+  /// current frame.)
+  ///
+  /// See also:
+  ///  * [PerAccountStoreWidget.of], for getting the relevant [PerAccountStore]
+  ///    from UI code.
+  Future<PerAccountStore> perAccount(int accountId) async {
+    // First, see if we have the store already.
+    PerAccountStore? store = _perAccountStores[accountId];
+    if (store != null) {
+      return store;
+    }
+
+    // Next, see if another call has already started loading one.
+    Future<PerAccountStore>? future = _perAccountStoresLoading[accountId];
+    if (future != null) {
+      return future;
+    }
+
+    // It's up to us.  Start loading.
+    final account = getAccount(accountId);
+    assert(account != null, 'Account not found on global store');
+    future = loadPerAccount(account!);
+    _perAccountStoresLoading[accountId] = future;
+    store = await future;
+    _perAccountStores[accountId] = store;
+    _perAccountStoresLoading.remove(accountId);
+    return store;
+  }
+
+  /// Load per-account data for the given account, unconditionally.
+  ///
+  /// This method should be called only by the implementation of [perAccount].
+  /// Other callers interested in per-account data should use [perAccount]
+  /// and/or [perAccountSync].
+  Future<PerAccountStore> loadPerAccount(Account account);
+
   // Just an Iterable, not the actual Map, to avoid clients mutating the map.
   // Mutations should go through the setters/mutators below.
   Iterable<Account> get accounts => _accounts.values;
@@ -46,50 +104,32 @@ class GlobalStore extends ChangeNotifier {
   // TODO add setters/mutators; will want to write to database
   // Future<void> insertAccount...
   // Future<void> updateAccount...
-
-  // TODO add a registry of [PerAccountStore]s, like the latter's of [MessageListView]
-  //   That will allow us to have many [PerAccountStoreWidget]s for a given
-  //   account, e.g. at the top of each page; and to access server data from
-  //   outside any [PerAccountStoreWidget], e.g. for handling a notification.
 }
 
 /// Store for the user's data for a given Zulip account.
 ///
 /// This should always have a consistent snapshot of the state on the server,
-/// as maintained by the Zulip event system.
+/// as provided by the Zulip event system.
+///
+/// An instance directly of this class will not attempt to poll an event queue
+/// to keep the data up to date.  For that behavior, see the subclass
+/// [LivePerAccountStore].
 class PerAccountStore extends ChangeNotifier {
-  PerAccountStore._({
+  /// Create a per-account data store that does not automatically stay up to date.
+  ///
+  /// For a [PerAccountStore] that polls an event queue to keep itself up to
+  /// date, use [LivePerAccountStore.fromInitialSnapshot].
+  PerAccountStore.fromInitialSnapshot({
     required this.account,
     required this.connection,
-    required this.queue_id,
-    required this.last_event_id,
-    required this.zulip_version,
-    required this.subscriptions,
-  });
-
-  /// Load the user's data from the server, and start an event queue going.
-  ///
-  /// In the future this might load an old snapshot from local storage first.
-  static Future<PerAccountStore> load(Account account) async {
-    final connection = LiveApiConnection(auth: account);
-
-    final stopwatch = Stopwatch()..start();
-    final initialSnapshot = await registerQueue(connection); // TODO retry
-    final t = (stopwatch..stop()).elapsed;
-    // TODO log the time better
-    if (kDebugMode) print("initial fetch time: ${t.inMilliseconds}ms");
-
-    final store = processInitialSnapshot(account, connection, initialSnapshot);
-    store.poll();
-    return store;
-  }
+    required InitialSnapshot initialSnapshot,
+  })  : zulip_version = initialSnapshot.zulip_version,
+        subscriptions = Map.fromEntries(initialSnapshot.subscriptions.map(
+                (subscription) => MapEntry(subscription.stream_id, subscription)));
 
   final Account account;
   final ApiConnection connection;
 
-  final String queue_id;
-  int last_event_id;
-
   final String zulip_version;
   final Map<int, Subscription> subscriptions;
 
@@ -117,22 +157,6 @@ class PerAccountStore extends ChangeNotifier {
     }
   }
 
-  void poll() async {
-    while (true) {
-      final result = await getEvents(connection,
-          queue_id: queue_id, last_event_id: last_event_id);
-      // TODO handle errors on get-events; retry with backoff
-      // TODO abort long-poll on [dispose]
-      final events = result.events;
-      for (final event in events) {
-        handleEvent(event);
-      }
-      if (events.isNotEmpty) {
-        last_event_id = events.last.id;
-      }
-    }
-  }
-
   void handleEvent(Event event) {
     if (event is HeartbeatEvent) {
       debugPrint("server event: heartbeat");
@@ -159,16 +183,6 @@ class PerAccountStore extends ChangeNotifier {
   }
 }
 
-/// A scaffolding hack for while prototyping.
-///
-/// See "Server credentials" in the project README for how to fill in the
-/// `credential_fixture.dart` file this requires.
-const Account _fixtureAccount = Account(
-  realmUrl: credentials.realm_url,
-  email: credentials.email,
-  apiKey: credentials.api_key,
-);
-
 @immutable
 class Account implements Auth {
   const Account(
@@ -182,24 +196,86 @@ class Account implements Auth {
   final String apiKey;
 }
 
-PerAccountStore processInitialSnapshot(Account account,
-    ApiConnection connection, InitialSnapshot initialSnapshot) {
-  final queue_id = initialSnapshot.queue_id;
-  if (queue_id == null) {
-    // The queue_id is optional in the type, but should only be missing in the
-    // case of unauthenticated access to a web-public realm.  We authenticated.
-    throw Exception("bad initial snapshot: missing queue_id");
+class LiveGlobalStore extends GlobalStore {
+  LiveGlobalStore._({required super.accounts}) : super();
+
+  // For convenience, a number we won't use as an ID in the database table.
+  static const fixtureAccountId = -1;
+
+  // We keep the API simple and synchronous for the bulk of the app's code
+  // by doing this loading up front before constructing a [GlobalStore].
+  static Future<GlobalStore> load() async {
+    const accounts = {fixtureAccountId: _fixtureAccount};
+    return LiveGlobalStore._(accounts: accounts);
+  }
+
+  @override
+  Future<PerAccountStore> loadPerAccount(Account account) {
+    return LivePerAccountStore.load(account);
+  }
+}
+
+/// A scaffolding hack for while prototyping.
+///
+/// See "Server credentials" in the project README for how to fill in the
+/// `credential_fixture.dart` file this requires.
+const Account _fixtureAccount = Account(
+  realmUrl: credentials.realm_url,
+  email: credentials.email,
+  apiKey: credentials.api_key,
+);
+
+/// A [PerAccountStore] which polls an event queue to stay up to date.
+class LivePerAccountStore extends PerAccountStore {
+  LivePerAccountStore.fromInitialSnapshot({
+    required super.account,
+    required super.connection,
+    required super.initialSnapshot,
+  })  : queue_id = initialSnapshot.queue_id ?? (() {
+            // The queue_id is optional in the type, but should only be missing in the
+            // case of unauthenticated access to a web-public realm.  We authenticated.
+            throw Exception("bad initial snapshot: missing queue_id");
+          })(),
+        last_event_id = initialSnapshot.last_event_id,
+        super.fromInitialSnapshot();
+
+  /// Load the user's data from the server, and start an event queue going.
+  ///
+  /// In the future this might load an old snapshot from local storage first.
+  static Future<PerAccountStore> load(Account account) async {
+    final connection = LiveApiConnection(auth: account);
+
+    final stopwatch = Stopwatch()..start();
+    final initialSnapshot = await registerQueue(connection); // TODO retry
+    final t = (stopwatch..stop()).elapsed;
+    // TODO log the time better
+    if (kDebugMode) print("initial fetch time: ${t.inMilliseconds}ms");
+
+    final store = LivePerAccountStore.fromInitialSnapshot(
+      account: account,
+      connection: connection,
+      initialSnapshot: initialSnapshot,
+    );
+    store.poll();
+    return store;
   }
 
-  final subscriptions = Map.fromEntries(initialSnapshot.subscriptions
-      .map((subscription) => MapEntry(subscription.stream_id, subscription)));
-
-  return PerAccountStore._(
-    account: account,
-    connection: connection,
-    queue_id: queue_id,
-    last_event_id: initialSnapshot.last_event_id,
-    zulip_version: initialSnapshot.zulip_version,
-    subscriptions: subscriptions,
-  );
+  final String queue_id;
+  int last_event_id;
+
+  void poll() async {
+    while (true) {
+      final result = await getEvents(connection,
+          queue_id: queue_id, last_event_id: last_event_id);
+      // TODO handle errors on get-events; retry with backoff
+      // TODO abort long-poll on [dispose]
+      final events = result.events;
+      for (final event in events) {
+        handleEvent(event);
+      }
+      if (events.isNotEmpty) {
+        last_event_id = events.last.id;
+      }
+    }
+  }
 }

lib/widgets/app.dart

@@ -23,7 +23,7 @@ class ZulipApp extends StatelessWidget {
     return GlobalStoreWidget(
       child: PerAccountStoreWidget(
         // Just one account for now.
-        accountId: GlobalStore.fixtureAccountId,
+        accountId: LiveGlobalStore.fixtureAccountId,
         child: MaterialApp(
           title: 'Zulip',
           theme: theme,

lib/widgets/store.dart

@@ -55,7 +55,7 @@ class _GlobalStoreWidgetState extends State<GlobalStoreWidget> {
   void initState() {
     super.initState();
     (() async {
-      final store = await GlobalStore.load();
+      final store = await LiveGlobalStore.load();
       setState(() {
         this.store = store;
       });
@@ -153,23 +153,25 @@ class _PerAccountStoreWidgetState extends State<PerAccountStoreWidget> {
   void didChangeDependencies() {
     super.didChangeDependencies();
     final globalStore = GlobalStoreWidget.of(context);
-    final account = globalStore.getAccount(widget.accountId);
-    assert(account != null, 'Account not found on global store');
+    // If we already have data, get it immediately. This avoids showing one
+    // frame of loading indicator each time we have a new PerAccountStoreWidget.
+    final store = globalStore.perAccountSync(widget.accountId);
     if (store != null) {
-      // The data we use to auth to the server should be unchanged;
-      // changing those should mean a new account ID in our database.
-      assert(account!.realmUrl == store!.account.realmUrl);
-      assert(account!.email == store!.account.email);
-      assert(account!.apiKey == store!.account.apiKey);
-      // TODO if Account has anything else change, update the PerAccountStore for that
-      return;
+      _setStore(store);
+    } else {
+      // If we don't already have data, wait for it.
+      (() async {
+        _setStore(await globalStore.perAccount(widget.accountId));
+      })();
     }
-    (() async {
-      final store = await PerAccountStore.load(account!);
+  }
+
+  void _setStore(PerAccountStore store) {
+    if (store != this.store) {
       setState(() {
         this.store = store;
       });
-    })();
+    }
   }
 
   @override

test/api/fake_api.dart

@@ -7,6 +7,9 @@ class FakeApiConnection extends ApiConnection {
       : super(auth: Account(
                 realmUrl: realmUrl, email: email, apiKey: _fakeApiKey));
 
+  FakeApiConnection.fromAccount(Account account)
+      : this(realmUrl: account.realmUrl, email: account.email);
+
   String? _nextResponse;
 
   // TODO: This mocking API will need to get richer to support all the tests we need.