fixup! wip; Simplify global store by loading it only once

Signed-off-by: Zixuan James Li <zixuan@zulip.com>

Author: PIG208

lib/model/store.dart

@@ -51,14 +51,18 @@ export 'database.dart' show Account, AccountsCompanion, AccountAlreadyExistsExce
 ///  * [LiveGlobalStore], the implementation of this class that
 ///    we use outside of tests.
 abstract class GlobalStore extends ChangeNotifier {
-  GlobalStore({required Iterable<Account> accounts})
-    : _accounts = Map.fromEntries(accounts.map((a) => MapEntry(a.id, a)));
+  GlobalStore({
+    required Iterable<Account> accounts,
+    required GlobalSettingsData globalSettings,
+  })
+    : _accounts = Map.fromEntries(accounts.map((a) => MapEntry(a.id, a))),
+      _globalSettings = globalSettings;
 
   /// A cache of the [Accounts] table in the underlying data store.
   final Map<int, Account> _accounts;
 
   /// A cache of the [GlobalSettingsData] singleton in the underlying data store.
-  GlobalSettingsData? _globalSettings;
+  GlobalSettingsData _globalSettings;
 
   // TODO settings (those that are per-device rather than per-account)
   // TODO push token, and other data corresponding to GlobalSessionState
@@ -226,33 +230,16 @@ abstract class GlobalStore extends ChangeNotifier {
   /// Remove an account from the underlying data store.
   Future<void> doRemoveAccount(int accountId);
 
-  GlobalSettingsData? get globalSettingsSync => _globalSettings;
-
-  /// Get global settings from the store.
-  ///
-  /// Use the cache if already loaded.
-  /// Otherwise, load it from the underlying store.
-  ///
-  /// Consider checking [globalSettingsSync] before using this.
-  Future<GlobalSettingsData> getGlobalSettings() async {
-    if (globalSettingsSync != null) return globalSettingsSync!;
-    return await loadGlobalSettings();
-  }
-
-  /// Load global settings from the underlying data store, unconditionally.
-  ///
-  /// This should only be called from [getGlobalSettings].
-  Future<GlobalSettingsData> loadGlobalSettings();
+  GlobalSettingsData get globalSettings => _globalSettings;
 
   /// Update the global settings in the store, return the new version.
   ///
   /// The global settings must already exist in the store.
   Future<GlobalSettingsData> updateGlobalSettings(GlobalSettingsCompanion data) async {
-    assert(_globalSettings != null);
     await doUpdateGlobalSettings(data);
-    _globalSettings = _globalSettings!.copyWithCompanion(data);
+    _globalSettings = _globalSettings.copyWithCompanion(data);
     notifyListeners();
-    return _globalSettings!;
+    return _globalSettings;
   }
 
   /// Update the global settings in the underlying data store.
@@ -794,6 +781,7 @@ class LiveGlobalStore extends GlobalStore {
   LiveGlobalStore._({
     required AppDatabase db,
     required super.accounts,
+    required super.globalSettings,
   }) : _db = db;
 
   @override
@@ -810,7 +798,10 @@ class LiveGlobalStore extends GlobalStore {
   static Future<GlobalStore> load() async {
     final db = AppDatabase(NativeDatabase.createInBackground(await _dbFile()));
     final accounts = await db.select(db.accounts).get();
-    return LiveGlobalStore._(db: db, accounts: accounts);
+    final globalSettings = await db.ensureGlobalSettings();
+    return LiveGlobalStore._(db: db,
+      accounts: accounts,
+      globalSettings: globalSettings);
   }
 
   /// The file path to use for the app database.
@@ -872,12 +863,6 @@ class LiveGlobalStore extends GlobalStore {
     assert(rowsAffected == 1);
   }
 
-  @override
-  Future<GlobalSettingsData> loadGlobalSettings() async {
-    _globalSettings = await _db.ensureGlobalSettings();
-    return _globalSettings!;
-  }
-
   @override
   Future<void> doUpdateGlobalSettings(GlobalSettingsCompanion data) async {
     final rowsAffected = await _db.update(_db.globalSettings).write(data);

lib/widgets/content.dart

@@ -1343,12 +1343,12 @@ void _launchUrl(BuildContext context, String urlString) async {
     return;
   }
 
-  final globalSettings = GlobalStoreWidget.of(context).globalSettingsSync;
+  final globalSettings = GlobalStoreWidget.of(context).globalSettings;
   bool launched = false;
   String? errorMessage;
   try {
     launched = await ZulipBinding.instance.launchUrl(url,
-      mode: switch ((globalSettings!.browserPreference, defaultTargetPlatform)) {
+      mode: switch ((globalSettings.browserPreference, defaultTargetPlatform)) {
         (BrowserPreference.embedded, _) => UrlLaunchMode.inAppBrowserView,
         (BrowserPreference.external, _) => UrlLaunchMode.externalApplication,
         // On iOS we prefer LaunchMode.externalApplication because (for

lib/widgets/theme.dart

@@ -12,9 +12,9 @@ import 'text.dart';
 ThemeData zulipThemeData(BuildContext context) {
   final DesignVariables designVariables;
   final List<ThemeExtension> themeExtensions;
-  final globalSettings = GlobalStoreWidget.of(context).globalSettingsSync;
+  final globalSettings = GlobalStoreWidget.of(context).globalSettings;
   Brightness brightness;
-  switch (globalSettings!.themeSetting) {
+  switch (globalSettings.themeSetting) {
     case ThemeSetting.none:
       brightness = MediaQuery.platformBrightnessOf(context);
     case ThemeSetting.light:

test/example_data.dart

@@ -8,6 +8,7 @@ import 'package:zulip/api/model/submessage.dart';
 import 'package:zulip/api/route/messages.dart';
 import 'package:zulip/api/route/realm.dart';
 import 'package:zulip/api/route/channels.dart';
+import 'package:zulip/model/database.dart';
 import 'package:zulip/model/narrow.dart';
 import 'package:zulip/model/store.dart';
 
@@ -805,8 +806,12 @@ ChannelUpdateEvent channelUpdateEvent(
 // The entire per-account or global state.
 //
 
-TestGlobalStore globalStore({List<Account> accounts = const []}) {
-  return TestGlobalStore(accounts: accounts);
+TestGlobalStore globalStore({
+  List<Account> accounts = const [],
+  GlobalSettingsData globalSettings = const GlobalSettingsData(
+    themeSetting: ThemeSetting.none, browserPreference: BrowserPreference.none)
+}) {
+  return TestGlobalStore(accounts: accounts, globalSettings: globalSettings);
 }
 
 InitialSnapshot initialSnapshot({

test/model/binding.dart

@@ -8,6 +8,7 @@ import 'package:test/fake.dart';
 import 'package:url_launcher/url_launcher.dart' as url_launcher;
 import 'package:zulip/host/android_notifications.dart';
 import 'package:zulip/model/binding.dart';
+import 'package:zulip/model/database.dart';
 import 'package:zulip/model/store.dart';
 import 'package:zulip/widgets/app.dart';
 
@@ -85,7 +86,11 @@ class TestZulipBinding extends ZulipBinding {
   ///
   /// Tests that access this getter, or that mount a [GlobalStoreWidget],
   /// should clean up by calling [reset].
-  TestGlobalStore get globalStore => _globalStore ??= TestGlobalStore(accounts: []);
+  TestGlobalStore get globalStore => _globalStore ??= TestGlobalStore(
+    accounts: [],
+    globalSettings: const GlobalSettingsData(
+      themeSetting: ThemeSetting.none,
+      browserPreference: BrowserPreference.none));
   TestGlobalStore? _globalStore;
 
   bool _debugAlreadyLoadedStore = false;

test/model/store_test.dart

@@ -12,6 +12,7 @@ import 'package:zulip/api/model/model.dart';
 import 'package:zulip/api/route/events.dart';
 import 'package:zulip/api/route/messages.dart';
 import 'package:zulip/api/route/realm.dart';
+import 'package:zulip/model/database.dart';
 import 'package:zulip/model/message_list.dart';
 import 'package:zulip/model/narrow.dart';
 import 'package:zulip/log.dart';
@@ -406,7 +407,7 @@ void main() {
     late FakeApiConnection connection;
 
     Future<void> prepareStore({Account? account}) async {
-      globalStore = TestGlobalStore(accounts: []);
+      globalStore = eg.globalStore();
       account ??= eg.selfAccount;
       await globalStore.insertAccount(account.toCompanion(false));
       connection = (globalStore.apiConnectionFromAccount(account)
@@ -581,7 +582,7 @@ void main() {
     }
 
     Future<void> preparePoll({int? lastEventId}) async {
-      globalStore = TestGlobalStore(accounts: []);
+      globalStore = eg.globalStore();
       await globalStore.add(eg.selfAccount, eg.initialSnapshot(
         lastEventId: lastEventId));
       await globalStore.perAccount(eg.selfAccount.id);
@@ -1086,7 +1087,12 @@ void main() {
 }
 
 class LoadingTestGlobalStore extends TestGlobalStore {
-  LoadingTestGlobalStore({required super.accounts});
+  LoadingTestGlobalStore({
+    super.accounts = const [],
+    super.globalSettings = const GlobalSettingsData(
+      themeSetting: ThemeSetting.none,
+      browserPreference: BrowserPreference.none),
+  });
 
   Map<int, List<Completer<PerAccountStore>>> completers = {};
 

test/model/test_store.dart

@@ -23,7 +23,7 @@ import '../example_data.dart' as eg;
 ///
 /// See also [TestZulipBinding.globalStore], which provides one of these.
 class TestGlobalStore extends GlobalStore {
-  TestGlobalStore({required super.accounts});
+  TestGlobalStore({required super.accounts, required super.globalSettings});
 
   final Map<
     ({Uri realmUrl, int? zulipFeatureLevel, String? email, String? apiKey}),
@@ -161,18 +161,6 @@ class TestGlobalStore extends GlobalStore {
 
   GlobalSettingsData? _globalSettings;
 
-  @override
-  Future<GlobalSettingsData> loadGlobalSettings() async {
-    if (_globalSettings != null) {
-      return _globalSettings!;
-    }
-    _globalSettings = const GlobalSettingsData(
-      themeSetting: ThemeSetting.none,
-      browserPreference: BrowserPreference.none,
-    );
-    return Future.value(_globalSettings);
-  }
-
   @override
   Future<void> doUpdateGlobalSettings(GlobalSettingsCompanion data) async {
     _globalSettings = _globalSettings!.copyWithCompanion(data);