store [nfc]: Add and use PerAccountStoreAwareStateMixin

Author: chrisbobbe

lib/widgets/message_list.dart

@@ -107,20 +107,13 @@ class MessageList extends StatefulWidget {
   State<StatefulWidget> createState() => _MessageListState();
 }
 
-class _MessageListState extends State<MessageList> {
+class _MessageListState extends State<MessageList> with PerAccountStoreAwareStateMixin<MessageList> {
   MessageListView? model;
 
   @override
-  void didChangeDependencies() {
-    super.didChangeDependencies();
-    final store = PerAccountStoreWidget.of(context);
-    if (model != null && model!.store == store) {
-      // We already have a model, and it's for the right store.
-      return;
-    }
-    // Otherwise, set up the model.  Dispose of any old model.
+  void onNewStore() {
     model?.dispose();
-    _initModel(store);
+    _initModel(PerAccountStoreWidget.of(context));
   }
 
   @override

lib/widgets/store.dart

@@ -140,6 +140,9 @@ class PerAccountStoreWidget extends StatefulWidget {
   ///  * [GlobalStoreWidget.of], for the app's data beyond that of a
   ///    particular account.
   ///  * [InheritedNotifier], which provides the "dependency" mechanism.
+  // TODO(#185): Explain in dartdoc that the returned [PerAccountStore] might
+  //   differ from one call to the next, and to handle that with
+  //   [PerAccountStoreAwareStateMixin].
   static PerAccountStore of(BuildContext context) {
     final widget = context.dependOnInheritedWidgetOfExactType<_PerAccountStoreInheritedWidget>();
     assert(widget != null, 'No PerAccountStoreWidget ancestor');
@@ -246,3 +249,41 @@ class LoadingPage extends StatelessWidget {
     return const Center(child: CircularProgressIndicator());
   }
 }
+
+/// A [State] that uses the ambient [PerAccountStore].
+///
+/// The ambient [PerAccountStore] can be replaced in some circumstances,
+/// such as when an event queue expires. See [PerAccountStoreWidget.of].
+/// When that happens, stateful widgets should
+/// - remove listeners on the old [PerAccountStore], and
+/// - add listeners on the new one.
+///
+/// Use this mixin, overriding [onNewStore], to do that concisely.
+// TODO(#185): Until #185, I think [PerAccountStoreWidget.of] never actually
+//   returns a different [PerAccountStore] from one call to the next.
+//   But it will, and when it does, we want our [StatefulWidgets] to handle it.
+mixin PerAccountStoreAwareStateMixin<T extends StatefulWidget> on State<T> {
+  PerAccountStore? _store;
+
+  /// Called when there is a new ambient [PerAccountStore].
+  ///
+  /// Specifically this is called when this element is first inserted into the tree
+  /// (so that it has an ambient [PerAccountStore] for the first time),
+  /// and again whenever dependencies change so that [PerAccountStoreWidget.of]
+  /// would return a different store from previously.
+  ///
+  /// In this, remove any listeners on the old store
+  /// and add them on the new store.
+  void onNewStore();
+
+  @override
+  void didChangeDependencies() {
+    super.didChangeDependencies();
+
+    final storeNow = PerAccountStoreWidget.of(context);
+    if (_store != storeNow) {
+      _store = storeNow;
+      onNewStore();
+    }
+  }
+}

test/flutter_checks.dart

@@ -2,9 +2,13 @@
 import 'dart:ui';
 
 import 'package:checks/checks.dart';
-import 'package:flutter/foundation.dart';
-import 'package:flutter/painting.dart';
+import 'package:flutter/widgets.dart';
 
+extension GlobalKeyChecks<T extends State<StatefulWidget>> on Subject<GlobalKey<T>> {
+  Subject<BuildContext?> get currentContext => has((k) => k.currentContext, 'currentContext');
+  Subject<Widget?> get currentWidget => has((k) => k.currentWidget, 'currentWidget');
+  Subject<T?> get currentState => has((k) => k.currentState, 'currentState');
+}
 
 extension ValueNotifierChecks<T> on Subject<ValueNotifier<T>> {
   Subject<T> get value => has((c) => c.value, 'value');

test/widgets/store_test.dart

@@ -4,10 +4,47 @@ import 'package:flutter_test/flutter_test.dart';
 import 'package:zulip/model/store.dart';
 import 'package:zulip/widgets/store.dart';
 
+import '../flutter_checks.dart';
 import '../model/binding.dart';
 import '../example_data.dart' as eg;
 import '../model/store_checks.dart';
 
+/// A widget whose state uses [PerAccountStoreAwareStateMixin].
+class MyWidgetWithMixin extends StatefulWidget {
+  const MyWidgetWithMixin({super.key});
+
+  @override
+  State<MyWidgetWithMixin> createState() => MyWidgetWithMixinState();
+}
+
+class MyWidgetWithMixinState extends State<MyWidgetWithMixin> with PerAccountStoreAwareStateMixin<MyWidgetWithMixin> {
+  int anyDepChangeCounter = 0;
+  int storeChangeCounter = 0;
+
+  @override
+  void onNewStore() {
+    storeChangeCounter++;
+  }
+
+  @override
+  void didChangeDependencies() {
+    super.didChangeDependencies();
+    anyDepChangeCounter++;
+  }
+
+  @override
+  Widget build(BuildContext context) {
+    final brightness = Theme.of(context).brightness;
+    final accountId = PerAccountStoreWidget.of(context).account.id;
+    return Text('brightness: $brightness; accountId: $accountId');
+  }
+}
+
+extension MyWidgetWithMixinStateChecks on Subject<MyWidgetWithMixinState> {
+  Subject<int> get anyDepChangeCounter => has((w) => w.anyDepChangeCounter, 'anyDepChangeCounter');
+  Subject<int> get storeChangeCounter => has((w) => w.storeChangeCounter, 'storeChangeCounter');
+}
+
 void main() {
   TestZulipBinding.ensureInitialized();
 
@@ -113,4 +150,65 @@ void main() {
     check(tester.any(find.textContaining('found store'))).isTrue();
     tester.widget(find.text('found store, account: ${eg.selfAccount.id}'));
   });
+
+  testWidgets('PerAccountStoreAwareStateMixin', (tester) async {
+    final widgetWithMixinKey = GlobalKey<MyWidgetWithMixinState>();
+    final accountId = eg.selfAccount.id;
+
+    await TestZulipBinding.instance.globalStore.add(eg.selfAccount, eg.initialSnapshot());
+
+    Future<void> pumpWithParams({required bool light, required int accountId}) async {
+      await tester.pumpWidget(
+        MaterialApp(
+          theme: light ? ThemeData.light() : ThemeData.dark(),
+          home: GlobalStoreWidget(
+            child: PerAccountStoreWidget(
+              accountId: accountId,
+              child: MyWidgetWithMixin(key: widgetWithMixinKey)))));
+    }
+
+    // [onNewStore] called initially
+    await pumpWithParams(light: true, accountId: accountId);
+    await tester.pump(); // global store
+    await tester.pump(); // per-account store
+    check(widgetWithMixinKey).currentState.isNotNull()
+      ..anyDepChangeCounter.equals(1)
+      ..storeChangeCounter.equals(1);
+
+    // [onNewStore] not called on unrelated dependency change
+    await pumpWithParams(light: false, accountId: accountId);
+    await tester.pumpAndSettle(kThemeAnimationDuration);
+    check(widgetWithMixinKey).currentState.isNotNull()
+      ..anyDepChangeCounter.equals(2)
+      ..storeChangeCounter.equals(1);
+
+    // [onNewStore] called when store changes
+    //
+    // TODO: Trigger `store` change by simulating an event-queue renewal,
+    //   instead of with this hack where we change the UI's backing Account
+    //   out from under it. (That account-swapping would be suspicious in
+    //   production code, where we could reasonably add an assert against it.
+    //   If forced, we could let this test code proceed despite such an assert…)
+    // hack; the snapshot probably corresponds to selfAccount, not otherAccount.
+    await TestZulipBinding.instance.globalStore.add(eg.otherAccount, eg.initialSnapshot());
+    await pumpWithParams(light: false, accountId: eg.otherAccount.id);
+    // Nudge PerAccountStoreWidget to send its updated store to MyWidgetWithMixin.
+    //
+    // A change in PerAccountStoreWidget's [accountId] field doesn't by itself
+    // prompt dependent widgets (those using PerAccountStoreWidget.of) to update,
+    // even though it holds a new store. (See its state's didUpdateWidget,
+    // or lack thereof.) That's reasonable, since such a change is never expected;
+    // see TODO above.
+    //
+    // But when PerAccountStoreWidget gets a notification from the GlobalStore,
+    // it checks at that time whether it has a new PerAccountStore to distribute
+    // (as it will when widget.accountId has changed), and if so,
+    // it will notify dependent widgets. (See its state's didChangeDependencies.)
+    // So, take advantage of that.
+    TestZulipBinding.instance.globalStore.notifyListeners();
+    await tester.pumpAndSettle();
+    check(widgetWithMixinKey).currentState.isNotNull()
+      ..anyDepChangeCounter.equals(3)
+      ..storeChangeCounter.equals(2);
+  });
 }