store: Have GlobalStore manage one PerAccountStore per account #27

gnprice · 2023-03-14T22:13:31Z

This is stacked atop #26.

As part of #21, we'll want to be able to freely instantiate a PerAccountStoreWidget whenever we want to mark some subtree as being for a particular account, and have that cheaply just refer to the already-loaded data for that account. This PR does that by having a GlobalStore maintain a PerAccountStore for each account, which PerAccountStoreWidget asks it for when it's first mounted in the element tree.

Because the logic in GlobalStore to maintain that, and in particular to avoid redundant requests if several PerAccountStoreWidgets ask about the same account at the beginning, is a bit complex, we want tests for it. To make those possible, we split both GlobalStore and PerAccountStore into a base class (which keeps the name) and a subclass that adds the using-real-data part which a unit test will want to leave out.

We also make sure to accommodate this fun trick (in _PerAccountStoreWidgetState):

    // 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.ofAccountSync(widget.accountId);
    if (store != null) {
      _setStore(store);
    } else {
      // If we don't already have data, wait for it.
      (() async {
        _setStore(await globalStore.ofAccount(widget.accountId));
      })();
    }

Fortunately the nature of that trick means that other code generally won't have to think about it; just use PerAccountWidget.

(We might eventually want to play the same kind of trick in a small number of other places, where we depend on data that's loaded outside of the initial fetch (and so isn't guaranteed by simply having a PerAccountState) but that we'll sometimes have already loaded. The message list is the main example that comes to mind.)

This is how it's really functioning anyway. This arrangement will be helpful as we split PerAccountStore into a base class and an implementation with more details, as the latter can then have a constructor that cleanly delegates most of the work to the former's constructor. This way also reduces the amount of boilerplate required to add another piece of data to the model.

This lets a test subclass GlobalStore to provide a different way to load a PerAccountStore. (Widgets still hardcode the "live" GlobalStore, so we'll need to make some further changes when we go to write tests for those widgets. But this is already enough to write tests for any logic we put on GlobalStore.)

…shot

chrisbobbe

Thanks, LGTM! Please merge at will after seeing the small thing below.

chrisbobbe · 2023-03-15T20:58:02Z

lib/model/store.dart

+  /// 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 [ofAccount].
+  PerAccountStore? ofAccountSync(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 [ofAccountSync] 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> ofAccount(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;
+  }


A small thing we discussed in the office: perhaps there's a better name than ofAccount / ofAccountSync? It's easy to misunderstand globalStore.ofAccount(2) as something like "get the global store of account 2", when really the store you get is a per-account store (PerAccountStore), not a global one (GlobalStore).

Not a blocker though, I think. 🙂

Sure — renaming to perAccount and perAccountSync.

And make use of the testability refactors we've made in the preceding series of commits, in order to test this code because it needs to deal with concurrency and so is a bit subtle.

gnprice · 2023-03-16T00:16:26Z

Thanks for the review! Merging, with that rename.

gnprice added 5 commits March 15, 2023 09:47

store [nfc]: Let GlobalStore say how to load a PerAccountStore

403d1ad

store [nfc]: Split PerAccountStore for easier testing

50a1ba9

test: Add example data for realm URL, version, accounts, initial snap…

39f3710

…shot

gnprice force-pushed the pr-manage-store branch from 4e95a46 to 9fef8cf Compare March 15, 2023 16:49

chrisbobbe approved these changes Mar 15, 2023

View reviewed changes

gnprice added 3 commits March 15, 2023 17:13

store: Have GlobalStore manage one PerAccountStore per account

3116740

And make use of the testability refactors we've made in the preceding series of commits, in order to test this code because it needs to deal with concurrency and so is a bit subtle.

store: Get the per-account store synchronously when available

4c82af6

store [nfc]: Write more dartdoc

2896360

gnprice force-pushed the pr-manage-store branch from 9fef8cf to 2896360 Compare March 16, 2023 00:15

gnprice merged commit 2896360 into zulip:main Mar 16, 2023

gnprice deleted the pr-manage-store branch March 22, 2023 03:41

gnprice mentioned this pull request Apr 7, 2023

Add personal user_id & full_name data, updating latter via events & in UI #25

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

store: Have GlobalStore manage one PerAccountStore per account #27

store: Have GlobalStore manage one PerAccountStore per account #27

Uh oh!

gnprice commented Mar 14, 2023 •

edited

Loading

Uh oh!

chrisbobbe left a comment

Uh oh!

chrisbobbe Mar 15, 2023

Uh oh!

gnprice Mar 16, 2023

Uh oh!

gnprice commented Mar 16, 2023

Uh oh!

Uh oh!

store: Have GlobalStore manage one PerAccountStore per account #27

store: Have GlobalStore manage one PerAccountStore per account #27

Uh oh!

Conversation

gnprice commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

gnprice Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

gnprice commented Mar 16, 2023

Uh oh!

Uh oh!

gnprice commented Mar 14, 2023 •

edited

Loading