diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 2afbbd4946..fda2cba197 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -192,18 +192,21 @@ class MentionAutocompleteView extends ChangeNotifier { final PerAccountStore store; final Narrow narrow; - MentionAutocompleteQuery? _currentQuery; - set query(MentionAutocompleteQuery query) { - _currentQuery = query; - _startSearch(query); + MentionAutocompleteQuery? get query => _query; + MentionAutocompleteQuery? _query; + set query(MentionAutocompleteQuery? query) { + _query = query; + if (query != null) { + _startSearch(query); + } } /// Recompute user results for the current query, if any. /// /// Called in particular when we get a [RealmUserEvent]. void refreshStaleUserResults() { - if (_currentQuery != null) { - _startSearch(_currentQuery!); + if (_query != null) { + _startSearch(_query!); } } @@ -211,8 +214,8 @@ class MentionAutocompleteView extends ChangeNotifier { /// /// This will redo the search from scratch for the current query, if any. void reassemble() { - if (_currentQuery != null) { - _startSearch(_currentQuery!); + if (_query != null) { + _startSearch(_query!); } } @@ -251,7 +254,7 @@ class MentionAutocompleteView extends ChangeNotifier { // CPU perf: End this task; enqueue a new one for resuming this work await Future(() {}); - if (query != _currentQuery || !hasListeners) { // false if [dispose] has been called. + if (query != _query || !hasListeners) { // false if [dispose] has been called. return null; } diff --git a/lib/widgets/autocomplete.dart b/lib/widgets/autocomplete.dart index 6e538e671a..773a741efe 100644 --- a/lib/widgets/autocomplete.dart +++ b/lib/widgets/autocomplete.dart @@ -26,15 +26,21 @@ class ComposeAutocomplete extends StatefulWidget { State createState() => _ComposeAutocompleteState(); } -class _ComposeAutocompleteState extends State { +class _ComposeAutocompleteState extends State with PerAccountStoreAwareStateMixin { MentionAutocompleteView? _viewModel; // TODO different autocomplete view types + void _initViewModel() { + final store = PerAccountStoreWidget.of(context); + _viewModel = MentionAutocompleteView.init(store: store, narrow: widget.narrow) + ..addListener(_viewModelChanged); + } + void _composeContentChanged() { final newAutocompleteIntent = widget.controller.autocompleteIntent(); if (newAutocompleteIntent != null) { - final store = PerAccountStoreWidget.of(context); - _viewModel ??= MentionAutocompleteView.init(store: store, narrow: widget.narrow) - ..addListener(_viewModelChanged); + if (_viewModel == null) { + _initViewModel(); + } _viewModel!.query = newAutocompleteIntent.query; } else { if (_viewModel != null) { @@ -51,6 +57,16 @@ class _ComposeAutocompleteState extends State { widget.controller.addListener(_composeContentChanged); } + @override + void onNewStore() { + if (_viewModel != null) { + final query = _viewModel!.query; + _viewModel!.dispose(); + _initViewModel(); + _viewModel!.query = query; + } + } + @override void didUpdateWidget(covariant ComposeAutocomplete oldWidget) { super.didUpdateWidget(oldWidget); diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index ed077b9e77..416037b869 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -107,7 +107,7 @@ class MessageList extends StatefulWidget { State createState() => _MessageListState(); } -class _MessageListState extends State { +class _MessageListState extends State with PerAccountStoreAwareStateMixin { MessageListView? model; final ScrollController scrollController = ScrollController(); final ValueNotifier _scrollToBottomVisibleValue = ValueNotifier(false); @@ -119,16 +119,9 @@ class _MessageListState extends State { } @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 diff --git a/lib/widgets/store.dart b/lib/widgets/store.dart index 9ff702189f..b820fd661e 100644 --- a/lib/widgets/store.dart +++ b/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 on State { + 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(); + } + } +} diff --git a/test/flutter_checks.dart b/test/flutter_checks.dart index fbafce6d3a..c693fe831d 100644 --- a/test/flutter_checks.dart +++ b/test/flutter_checks.dart @@ -2,14 +2,19 @@ import 'dart:ui'; import 'package:checks/checks.dart'; -import 'package:flutter/foundation.dart'; -import 'package:flutter/painting.dart'; import 'package:flutter/services.dart'; +import 'package:flutter/widgets.dart'; extension ClipboardDataChecks on Subject { Subject get text => has((d) => d.text, 'text'); } +extension GlobalKeyChecks> on Subject> { + Subject get currentContext => has((k) => k.currentContext, 'currentContext'); + Subject get currentWidget => has((k) => k.currentWidget, 'currentWidget'); + Subject get currentState => has((k) => k.currentState, 'currentState'); +} + extension ValueNotifierChecks on Subject> { Subject get value => has((c) => c.value, 'value'); } diff --git a/test/widgets/store_test.dart b/test/widgets/store_test.dart index 3c26f6c180..661f80b0ff 100644 --- a/test/widgets/store_test.dart +++ b/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 createState() => MyWidgetWithMixinState(); +} + +class MyWidgetWithMixinState extends State with PerAccountStoreAwareStateMixin { + 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 { + Subject get anyDepChangeCounter => has((w) => w.anyDepChangeCounter, 'anyDepChangeCounter'); + Subject 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(); + final accountId = eg.selfAccount.id; + + await TestZulipBinding.instance.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + + Future 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); + }); }