store [nfc]: Separate "backend" class from store itself

This commit makes GlobalSettingsStore more self-contained, taking
over from GlobalStore the responsibility for updating settings in
the database at the same time as they get updated in the in-app
cache [GlobalSettingsStore._data].

To do that, it needs to call doUpdateGlobalSettings, the abstract
method that's implemented separately to use a live database in the
real app vs. a vacuous fake backend in tests.  But that means that
method can no longer live on GlobalStore: for GlobalStore needs a
GlobalSettingsStore, but then GlobalSettingsStore would need a
GlobalStore, forming a cycle.  In particular when calling the
GlobalSettingsStore constructor from within the initializer list of
the GlobalStore constructor, we can't pass `this` as an argument
because `this` hasn't yet been constructed.

(If we were determined to make such a cycle work, we could do so by
having a late or nullable field.  But let's take the cycle as a
signal that the design can be cleaned up.)

So, make a new class GlobalStoreBackend as the home for this
abstract method.  A GlobalStore now "has a" GlobalStoreBackend,
rather than "is a" GlobalStoreBackend as it effectively was before.
As a result we can readily construct a "backend" instance first,
and pass that to the GlobalSettingsStore constructor, before going
on to finish constructing the overall GlobalStore.

Probably most or all of the other functionality that LiveGlobalStore
implements should get moved to the new "backend" class too, but
we'll leave that for possible follow-up changes.

Author: gnprice

lib/model/settings.dart

@@ -3,6 +3,7 @@ import 'package:flutter/foundation.dart';
 import '../generated/l10n/zulip_localizations.dart';
 import 'binding.dart';
 import 'database.dart';
+import 'store.dart';
 
 /// The user's choice of visual theme for the app.
 ///
@@ -49,7 +50,12 @@ enum BrowserPreference {
 /// From UI code, use [GlobalStoreWidget.settingsOf] to get hold of
 /// an appropriate instance of this class.
 class GlobalSettingsStore extends ChangeNotifier {
-  GlobalSettingsStore({required GlobalSettingsData data}) : _data = data;
+  GlobalSettingsStore({
+    required GlobalStoreBackend backend,
+    required GlobalSettingsData data,
+  }) : _backend = backend, _data = data;
+
+  final GlobalStoreBackend _backend;
 
   /// A cache of the [GlobalSettingsData] singleton in the underlying data store.
   GlobalSettingsData _data;
@@ -104,9 +110,9 @@ class GlobalSettingsStore extends ChangeNotifier {
     }
   }
 
-  /// (Should only be called by [GlobalStore].)
-  void update(GlobalSettingsCompanion data) {
-    // TODO move responsibility for updating the DB to this class too
+  /// Update the global settings in the store.
+  Future<void> update(GlobalSettingsCompanion data) async {
+    await _backend.doUpdateGlobalSettings(data);
     _data = _data.copyWithCompanion(data);
     notifyListeners();
   }

lib/model/store.dart

@@ -37,6 +37,22 @@ import 'user.dart';
 export 'package:drift/drift.dart' show Value;
 export 'database.dart' show Account, AccountsCompanion, AccountAlreadyExistsException;
 
+/// An underlying data store that can support a [GlobalStore],
+/// possibly storing the data to persist between runs of the app.
+///
+/// In the real app, the implementation used is [LiveGlobalStoreBackend],
+/// which stores data persistently in a database on the user's device.
+/// This interface enables tests to use a different implementation.
+abstract class GlobalStoreBackend {
+  /// Update the global settings in the underlying data store.
+  ///
+  /// This should only be called from [GlobalSettingsStore].
+  Future<void> doUpdateGlobalSettings(GlobalSettingsCompanion data);
+
+  // TODO move here the similar methods for accounts;
+  //   perhaps the rest of the GlobalStore abstract methods, too.
+}
+
 /// Store for all the user's data.
 ///
 /// From UI code, use [GlobalStoreWidget.of] to get hold of an appropriate
@@ -55,10 +71,11 @@ export 'database.dart' show Account, AccountsCompanion, AccountAlreadyExistsExce
 ///    we use outside of tests.
 abstract class GlobalStore extends ChangeNotifier {
   GlobalStore({
+    required GlobalStoreBackend backend,
     required GlobalSettingsData globalSettings,
     required Iterable<Account> accounts,
   })
-    : settings = GlobalSettingsStore(data: globalSettings),
+    : settings = GlobalSettingsStore(backend: backend, data: globalSettings),
       _accounts = Map.fromEntries(accounts.map((a) => MapEntry(a.id, a)));
 
   /// The store for the user's account-independent settings.
@@ -70,16 +87,11 @@ abstract class GlobalStore extends ChangeNotifier {
   final GlobalSettingsStore settings;
 
   /// Update the global settings in the store.
+  // TODO inline this out
   Future<void> updateGlobalSettings(GlobalSettingsCompanion data) async {
-    await doUpdateGlobalSettings(data);
-    settings.update(data);
+    await settings.update(data);
   }
 
-  /// Update the global settings in the underlying data store.
-  ///
-  /// This should only be called from [updateGlobalSettings].
-  Future<void> doUpdateGlobalSettings(GlobalSettingsCompanion data);
-
   /// A cache of the [Accounts] table in the underlying data store.
   final Map<int, Account> _accounts;
 
@@ -832,6 +844,23 @@ Uri? tryResolveUrl(Uri baseUrl, String reference) {
   }
 }
 
+/// A [GlobalStoreBackend] that uses a live, persistent local database.
+///
+/// Used as part of a [LiveGlobalStore].
+/// The underlying data store is an [AppDatabase] corresponding to a
+/// SQLite database file in the app's persistent storage on the device.
+class LiveGlobalStoreBackend implements GlobalStoreBackend {
+  LiveGlobalStoreBackend._({required AppDatabase db}) : _db = db;
+
+  final AppDatabase _db;
+
+  @override
+  Future<void> doUpdateGlobalSettings(GlobalSettingsCompanion data) async {
+    final rowsAffected = await _db.update(_db.globalSettings).write(data);
+    assert(rowsAffected == 1);
+  }
+}
+
 /// A [GlobalStore] that uses a live server and live, persistent local database.
 ///
 /// The underlying data store is an [AppDatabase] corresponding to a SQLite
@@ -841,10 +870,11 @@ Uri? tryResolveUrl(Uri baseUrl, String reference) {
 /// and will have an associated [UpdateMachine].
 class LiveGlobalStore extends GlobalStore {
   LiveGlobalStore._({
-    required AppDatabase db,
+    required LiveGlobalStoreBackend backend,
     required super.globalSettings,
     required super.accounts,
-  }) : _db = db;
+  }) : _backend = backend,
+       super(backend: backend);
 
   @override
   ApiConnection apiConnection({
@@ -861,7 +891,8 @@ class LiveGlobalStore extends GlobalStore {
     final db = AppDatabase(NativeDatabase.createInBackground(await _dbFile()));
     final globalSettings = await db.getGlobalSettings();
     final accounts = await db.select(db.accounts).get();
-    return LiveGlobalStore._(db: db,
+    return LiveGlobalStore._(
+      backend: LiveGlobalStoreBackend._(db: db),
       globalSettings: globalSettings,
       accounts: accounts);
   }
@@ -885,13 +916,13 @@ class LiveGlobalStore extends GlobalStore {
     return File(p.join(dir.path, 'zulip.db'));
   }
 
-  final AppDatabase _db;
+  final LiveGlobalStoreBackend _backend;
 
-  @override
-  Future<void> doUpdateGlobalSettings(GlobalSettingsCompanion data) async {
-    final rowsAffected = await _db.update(_db.globalSettings).write(data);
-    assert(rowsAffected == 1);
-  }
+  // The methods that use this should probably all move to [GlobalStoreBackend]
+  // and [LiveGlobalStoreBackend] anyway (see comment on the former);
+  // so let the latter be the canonical home of the [AppDatabase].
+  // This getter just simplifies the transition.
+  AppDatabase get _db => _backend._db;
 
   @override
   Future<PerAccountStore> doLoadPerAccount(int accountId) async {

test/model/test_store.dart

@@ -62,12 +62,14 @@ mixin _ApiConnectionsMixin on GlobalStore {
   }
 }
 
-mixin _DatabaseMixin on GlobalStore {
+class _TestGlobalStoreBackend implements GlobalStoreBackend {
   @override
   Future<void> doUpdateGlobalSettings(GlobalSettingsCompanion data) async {
     // Nothing to do.
   }
+}
 
+mixin _DatabaseMixin on GlobalStore {
   int _nextAccountId = 1;
 
   @override
@@ -138,7 +140,8 @@ class TestGlobalStore extends GlobalStore with _ApiConnectionsMixin, _DatabaseMi
   TestGlobalStore({
     GlobalSettingsData? globalSettings,
     required super.accounts,
-  }) : super(globalSettings: globalSettings ?? GlobalSettingsData());
+  }) : super(backend: _TestGlobalStoreBackend(),
+         globalSettings: globalSettings ?? GlobalSettingsData());
 
   final Map<int, InitialSnapshot> _initialSnapshots = {};
 
@@ -202,7 +205,8 @@ class UpdateMachineTestGlobalStore extends GlobalStore with _ApiConnectionsMixin
   UpdateMachineTestGlobalStore({
     GlobalSettingsData? globalSettings,
     required super.accounts,
-  }) : super(globalSettings: globalSettings ?? GlobalSettingsData());
+  }) : super(backend: _TestGlobalStoreBackend(),
+         globalSettings: globalSettings ?? GlobalSettingsData());
 
   // [doLoadPerAccount] depends on the cache to prepare the API responses.
   // Calling [clearCachedApiConnections] is permitted, though.