From fdb960500fe34b3c6eedf2c7f0a21f261ded8e3f Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 26 Dec 2023 11:38:46 -0800 Subject: [PATCH 01/10] store [nfc]: Update comments for UpdateMachine replacing LivePerAccountStore This change happened in 5c778bdab, but some comments didn't get updated at the same time, oops. --- lib/model/store.dart | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index 39a9320552..a0f29313b2 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -140,14 +140,10 @@ abstract class GlobalStore extends ChangeNotifier { /// This should always have a consistent snapshot of the state on the server, /// as provided by the Zulip event system. /// -/// An instance directly of this class will not attempt to poll an event queue -/// to keep the data up to date. For that behavior, see the subclass -/// [LivePerAccountStore]. +/// This class does not attempt to poll an event queue +/// to keep the data up to date. For that behavior, see +/// [UpdateMachine]. class PerAccountStore extends ChangeNotifier with StreamStore { - /// Create a per-account data store that does not automatically stay up to date. - /// - /// For a [PerAccountStore] that polls an event queue to keep itself up to - /// date, use [LivePerAccountStore.fromInitialSnapshot]. factory PerAccountStore.fromInitialSnapshot({ required Account account, required ApiConnection connection, @@ -410,8 +406,8 @@ const _apiSendMessage = sendMessage; // Bit ugly; for alternatives, see: https:/ /// The underlying data store is an [AppDatabase] corresponding to a SQLite /// database file in the app's persistent storage on the device. /// -/// The per-account stores will be instances of [LivePerAccountStore], -/// with data loaded through a live [ApiConnection]. +/// The per-account stores will use a live [ApiConnection], +/// and will have an associated [UpdateMachine]. class LiveGlobalStore extends GlobalStore { LiveGlobalStore._({ required AppDatabase db, From 2d97b55fe996765f461c482696e0cf798f740dba Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 27 Dec 2023 13:57:30 -0800 Subject: [PATCH 02/10] store: Add debug-only timing control to polling loop This will be useful for tests. --- lib/model/store.dart | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/lib/model/store.dart b/lib/model/store.dart index a0f29313b2..bbabafea19 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -1,3 +1,4 @@ +import 'dart:async'; import 'dart:convert'; import 'dart:io'; @@ -509,8 +510,37 @@ class UpdateMachine { final String queueId; int lastEventId; + Completer? _debugLoopSignal; + + /// In debug mode, causes the polling loop to pause before the next + /// request and wait for [debugAdvanceLoop] to be called. + void debugPauseLoop() { + assert((){ + assert(_debugLoopSignal == null); + _debugLoopSignal = Completer(); + return true; + }()); + } + + /// In debug mode, after a call to [debugPauseLoop], causes the + /// polling loop to make one more request and then pause again. + void debugAdvanceLoop() { + assert((){ + _debugLoopSignal!.complete(); + return true; + }()); + } + void poll() async { while (true) { + if (_debugLoopSignal != null) { + await _debugLoopSignal!.future; + assert(() { + _debugLoopSignal = Completer(); + return true; + }()); + } + final result = await getEvents(store.connection, queueId: queueId, lastEventId: lastEventId); // TODO handle errors on get-events; retry with backoff From 6a6669906b4482456e657215cf115ec4f0a05691 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 26 Dec 2023 16:19:07 -0800 Subject: [PATCH 03/10] store test: Test UpdateMachine.poll in happy cases --- test/model/store_test.dart | 72 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/test/model/store_test.dart b/test/model/store_test.dart index 07ce1beedf..7967da6e47 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -4,6 +4,9 @@ import 'package:checks/checks.dart'; import 'package:flutter/foundation.dart'; import 'package:http/http.dart' as http; import 'package:test/scaffolding.dart'; +import 'package:zulip/api/model/events.dart'; +import 'package:zulip/api/model/model.dart'; +import 'package:zulip/api/route/events.dart'; import 'package:zulip/api/route/messages.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/notifications.dart'; @@ -131,6 +134,75 @@ void main() { }); }); + group('UpdateMachine.poll', () { + late UpdateMachine updateMachine; + late PerAccountStore store; + late FakeApiConnection connection; + + void prepareStore({int? lastEventId}) { + updateMachine = eg.updateMachine(initialSnapshot: eg.initialSnapshot( + lastEventId: lastEventId, + )); + store = updateMachine.store; + connection = store.connection as FakeApiConnection; + } + + void checkLastRequest({required int lastEventId}) { + check(connection.lastRequest).isA() + ..method.equals('GET') + ..url.path.equals('/api/v1/events') + ..url.queryParameters.deepEquals({ + 'queue_id': updateMachine.queueId, + 'last_event_id': lastEventId.toString(), + }); + } + + test('loops on success', () async { + prepareStore(lastEventId: 1); + check(updateMachine.lastEventId).equals(1); + + updateMachine.debugPauseLoop(); + updateMachine.poll(); + + // Loop makes first request, and processes result. + connection.prepare(json: GetEventsResult(events: [ + HeartbeatEvent(id: 2), + ], queueId: null).toJson()); + updateMachine.debugAdvanceLoop(); + await null; + checkLastRequest(lastEventId: 1); + await Future.delayed(Duration.zero); + check(updateMachine.lastEventId).equals(2); + + // Loop makes second request, and processes result. + connection.prepare(json: GetEventsResult(events: [ + HeartbeatEvent(id: 3), + ], queueId: null).toJson()); + updateMachine.debugAdvanceLoop(); + await null; + checkLastRequest(lastEventId: 2); + await Future.delayed(Duration.zero); + check(updateMachine.lastEventId).equals(3); + }); + + test('handles events', () async { + prepareStore(); + updateMachine.debugPauseLoop(); + updateMachine.poll(); + + // Pick some arbitrary event and check it gets processed on the store. + check(store.userSettings!.twentyFourHourTime).isFalse(); + connection.prepare(json: GetEventsResult(events: [ + UserSettingsUpdateEvent(id: 2, + property: UserSettingName.twentyFourHourTime, value: true), + ], queueId: null).toJson()); + updateMachine.debugAdvanceLoop(); + await null; + await Future.delayed(Duration.zero); + check(store.userSettings!.twentyFourHourTime).isTrue(); + }); + }); + group('UpdateMachine.registerNotificationToken', () { late UpdateMachine updateMachine; late FakeApiConnection connection; From 35a9bb0e199f4996836bab2bb1c1fae3fdb55e42 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 26 Dec 2023 15:51:23 -0800 Subject: [PATCH 04/10] store: Implement toString on PerAccountStore and friends The `shortHash` identity tags can be helpful in debugging. --- lib/model/store.dart | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/model/store.dart b/lib/model/store.dart index bbabafea19..ddd4278e5f 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -134,6 +134,9 @@ abstract class GlobalStore extends ChangeNotifier { // More mutators as needed: // Future updateAccount... + + @override + String toString() => '${objectRuntimeType(this, 'GlobalStore')}#${shortHash(this)}'; } /// Store for the user's data for a given Zulip account. @@ -398,6 +401,9 @@ class PerAccountStore extends ChangeNotifier with StreamStore { final nonDisplayFields = initialCustomProfileFields.where((e) => e.displayInProfileSummary != true); return displayFields.followedBy(nonDisplayFields).toList(); } + + @override + String toString() => '${objectRuntimeType(this, 'PerAccountStore')}#${shortHash(this)}'; } const _apiSendMessage = sendMessage; // Bit ugly; for alternatives, see: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20PerAccountStore.20methods/near/1545809 @@ -462,6 +468,9 @@ class LiveGlobalStore extends GlobalStore { ..where((a) => a.id.equals(accountId)) ).getSingle(); } + + @override + String toString() => '${objectRuntimeType(this, 'LiveGlobalStore')}#${shortHash(this)}'; } /// A [PerAccountStore] plus an event-polling loop to stay up to date. @@ -573,4 +582,7 @@ class UpdateMachine { if (token == null) return; await NotificationService.registerToken(store.connection, token: token); } + + @override + String toString() => '${objectRuntimeType(this, 'UpdateMachine')}#${shortHash(this)}'; } From 6ef1996928facfe5a16343e7f46cee1d8f717f5f Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 26 Dec 2023 15:42:59 -0800 Subject: [PATCH 05/10] store [nfc]: On loading account, notify global listeners This makes GlobalStore behave a bit more conventionally for a ChangeNotifier. --- lib/model/store.dart | 1 + lib/widgets/store.dart | 6 ++---- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index ddd4278e5f..ff447031b4 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -98,6 +98,7 @@ abstract class GlobalStore extends ChangeNotifier { store = await future; _perAccountStores[accountId] = store; _perAccountStoresLoading.remove(accountId); + notifyListeners(); return store; } diff --git a/lib/widgets/store.dart b/lib/widgets/store.dart index b820fd661e..61fd9f6e4e 100644 --- a/lib/widgets/store.dart +++ b/lib/widgets/store.dart @@ -192,10 +192,8 @@ class _PerAccountStoreWidgetState extends State { if (store != null) { _setStore(store); } else { - // If we don't already have data, wait for it. - (() async { - _setStore(await globalStore.perAccount(widget.accountId)); - })(); + // If we don't already have data, request it. + globalStore.perAccount(widget.accountId); } } From 9eafed40d2f5121802feb3467b5b78f534f3f172 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 26 Dec 2023 15:06:09 -0800 Subject: [PATCH 06/10] store [nfc]: Add GlobalStore reference on PerAccountStore --- lib/model/store.dart | 15 ++++++++++++--- test/example_data.dart | 13 ++++++++++--- test/model/store_test.dart | 5 +++++ test/model/test_store.dart | 1 + 4 files changed, 28 insertions(+), 6 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index ff447031b4..d9585f03af 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -150,12 +150,14 @@ abstract class GlobalStore extends ChangeNotifier { /// [UpdateMachine]. class PerAccountStore extends ChangeNotifier with StreamStore { factory PerAccountStore.fromInitialSnapshot({ + required GlobalStore globalStore, required Account account, required ApiConnection connection, required InitialSnapshot initialSnapshot, }) { final streams = StreamStoreImpl(initialSnapshot: initialSnapshot); return PerAccountStore._( + globalStore: globalStore, account: account, connection: connection, zulipVersion: initialSnapshot.zulipVersion, @@ -181,6 +183,7 @@ class PerAccountStore extends ChangeNotifier with StreamStore { } PerAccountStore._({ + required GlobalStore globalStore, required this.account, required this.connection, required this.zulipVersion, @@ -193,7 +196,12 @@ class PerAccountStore extends ChangeNotifier with StreamStore { required this.users, required streams, required this.recentDmConversationsView, - }) : _streams = streams; + }) : _globalStore = globalStore, + _streams = streams; + + // We'll use this in an upcoming commit. + // ignore: unused_field + final GlobalStore _globalStore; final Account account; final ApiConnection connection; // TODO(#135): update zulipFeatureLevel with events @@ -453,7 +461,7 @@ class LiveGlobalStore extends GlobalStore { @override Future loadPerAccount(Account account) async { - final updateMachine = await UpdateMachine.load(account); + final updateMachine = await UpdateMachine.load(this, account); return updateMachine.store; } @@ -491,7 +499,7 @@ class UpdateMachine { /// Load the user's data from the server, and start an event queue going. /// /// In the future this might load an old snapshot from local storage first. - static Future load(Account account) async { + static Future load(GlobalStore globalStore, Account account) async { final connection = ApiConnection.live( realmUrl: account.realmUrl, zulipFeatureLevel: account.zulipFeatureLevel, email: account.email, apiKey: account.apiKey); @@ -503,6 +511,7 @@ class UpdateMachine { if (kDebugMode) print("initial fetch time: ${t.inMilliseconds}ms"); final store = PerAccountStore.fromInitialSnapshot( + globalStore: globalStore, account: account, connection: connection, initialSnapshot: initialSnapshot, diff --git a/test/example_data.dart b/test/example_data.dart index 5c66d2e871..03667ebcfd 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -7,6 +7,7 @@ import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; import 'api/fake_api.dart'; +import 'model/test_store.dart'; import 'stdlib_checks.dart'; //////////////////////////////////////////////////////////////// @@ -414,9 +415,13 @@ UpdateMessageFlagsRemoveEvent updateMessageFlagsRemoveEvent( } //////////////////////////////////////////////////////////////// -// The entire per-account state. +// The entire per-account or global state. // +GlobalStore globalStore({List accounts = const []}) { + return TestGlobalStore(accounts: accounts); +} + InitialSnapshot initialSnapshot({ String? queueId, int? lastEventId, @@ -467,9 +472,11 @@ InitialSnapshot initialSnapshot({ const _initialSnapshot = initialSnapshot; PerAccountStore store({Account? account, InitialSnapshot? initialSnapshot}) { + final effectiveAccount = account ?? selfAccount; return PerAccountStore.fromInitialSnapshot( - account: account ?? selfAccount, - connection: FakeApiConnection.fromAccount(account ?? selfAccount), + globalStore: globalStore(accounts: [effectiveAccount]), + account: effectiveAccount, + connection: FakeApiConnection.fromAccount(effectiveAccount), initialSnapshot: initialSnapshot ?? _initialSnapshot(), ); } diff --git a/test/model/store_test.dart b/test/model/store_test.dart index 7967da6e47..76087069f3 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -31,6 +31,7 @@ void main() { final future1 = globalStore.perAccount(1); final store1 = PerAccountStore.fromInitialSnapshot( + globalStore: globalStore, account: account1, connection: FakeApiConnection.fromAccount(account1), initialSnapshot: eg.initialSnapshot(), @@ -42,6 +43,7 @@ void main() { final future2 = globalStore.perAccount(2); final store2 = PerAccountStore.fromInitialSnapshot( + globalStore: globalStore, account: account2, connection: FakeApiConnection.fromAccount(account2), initialSnapshot: eg.initialSnapshot(), @@ -69,11 +71,13 @@ void main() { final future2 = globalStore.perAccount(2); final store1 = PerAccountStore.fromInitialSnapshot( + globalStore: globalStore, account: account1, connection: FakeApiConnection.fromAccount(account1), initialSnapshot: eg.initialSnapshot(), ); final store2 = PerAccountStore.fromInitialSnapshot( + globalStore: globalStore, account: account2, connection: FakeApiConnection.fromAccount(account2), initialSnapshot: eg.initialSnapshot(), @@ -99,6 +103,7 @@ void main() { final future1 = globalStore.perAccount(1); check(globalStore.perAccountSync(1)).isNull(); final store1 = PerAccountStore.fromInitialSnapshot( + globalStore: globalStore, account: account1, connection: FakeApiConnection.fromAccount(account1), initialSnapshot: eg.initialSnapshot(), diff --git a/test/model/test_store.dart b/test/model/test_store.dart index a6522b8eb1..62881852ff 100644 --- a/test/model/test_store.dart +++ b/test/model/test_store.dart @@ -51,6 +51,7 @@ class TestGlobalStore extends GlobalStore { @override Future loadPerAccount(Account account) { return Future.value(PerAccountStore.fromInitialSnapshot( + globalStore: this, account: account, connection: FakeApiConnection.fromAccount(account), initialSnapshot: _initialSnapshots[account.id]!, From fcb05a484f8ded45bac8fcff507220ac123ff1ff Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 26 Dec 2023 15:21:06 -0800 Subject: [PATCH 07/10] store: Implement PerAccountStore.dispose --- lib/model/store.dart | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index d9585f03af..a167f1bb0b 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -241,7 +241,6 @@ class PerAccountStore extends ChangeNotifier with StreamStore { // TODO lots more data. When adding, be sure to update handleEvent too. - // TODO call [RecentDmConversationsView.dispose] in [dispose] final RecentDmConversationsView recentDmConversationsView; final Set _messageListViews = {}; @@ -269,6 +268,16 @@ class PerAccountStore extends ChangeNotifier with StreamStore { autocompleteViewManager.reassemble(); } + @override + void dispose() { + unreads.dispose(); + recentDmConversationsView.dispose(); + for (final view in _messageListViews.toList()) { + view.dispose(); + } + super.dispose(); + } + void handleEvent(Event event) { if (event is HeartbeatEvent) { assert(debugLog("server event: heartbeat")); From f0c76da6d40771fbd5f374648c5b86a848806a99 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 26 Dec 2023 16:05:39 -0800 Subject: [PATCH 08/10] store: Implement UpdateMachine.dispose --- lib/model/store.dart | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index a167f1bb0b..693a5f2729 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -572,7 +572,6 @@ class UpdateMachine { final result = await getEvents(store.connection, queueId: queueId, lastEventId: lastEventId); // TODO handle errors on get-events; retry with backoff - // TODO abort long-poll and close ApiConnection on [dispose] final events = result.events; for (final event in events) { store.handleEvent(event); @@ -591,7 +590,6 @@ class UpdateMachine { // TODO(#322) save acked token, to dedupe updating it on the server // TODO(#323) track the registerFcmToken/etc request, warn if not succeeding Future registerNotificationToken() async { - // TODO call removeListener on [dispose] NotificationService.instance.token.addListener(_registerNotificationToken); await _registerNotificationToken(); } @@ -602,6 +600,10 @@ class UpdateMachine { await NotificationService.registerToken(store.connection, token: token); } + void dispose() { // TODO abort long-poll and close ApiConnection + NotificationService.instance.token.removeListener(_registerNotificationToken); + } + @override String toString() => '${objectRuntimeType(this, 'UpdateMachine')}#${shortHash(this)}'; } From 7465951ddbbe2a6e69ecb34207a23d35292348eb Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 26 Dec 2023 20:08:46 -0800 Subject: [PATCH 09/10] test: Track update machines in TestGlobalStore --- test/model/test_store.dart | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/test/model/test_store.dart b/test/model/test_store.dart index 62881852ff..58acce64b9 100644 --- a/test/model/test_store.dart +++ b/test/model/test_store.dart @@ -13,10 +13,19 @@ import '../api/fake_api.dart'; /// /// The per-account stores will use [FakeApiConnection]. /// +/// Unlike with [LiveGlobalStore] and the associated [UpdateMachine.load], +/// there is no automatic event-polling loop or other automated requests. +/// For each account loaded, there is a corresponding [UpdateMachine] +/// in [updateMachines], which tests can use for invoking that logic +/// explicitly when desired. +/// /// See also [TestZulipBinding.globalStore], which provides one of these. class TestGlobalStore extends GlobalStore { TestGlobalStore({required super.accounts}); + /// A corresponding [UpdateMachine] for each loaded account. + final Map updateMachines = {}; + final Map _initialSnapshots = {}; /// Add an account and corresponding server data to the test data. @@ -50,12 +59,16 @@ class TestGlobalStore extends GlobalStore { @override Future loadPerAccount(Account account) { - return Future.value(PerAccountStore.fromInitialSnapshot( + final initialSnapshot = _initialSnapshots[account.id]!; + final store = PerAccountStore.fromInitialSnapshot( globalStore: this, account: account, connection: FakeApiConnection.fromAccount(account), - initialSnapshot: _initialSnapshots[account.id]!, - )); + initialSnapshot: initialSnapshot, + ); + updateMachines[account.id] = UpdateMachine.fromInitialSnapshot( + store: store, initialSnapshot: initialSnapshot); + return Future.value(store); } } From 30693a8f03a3b72b6a183e8d793daf145fa34b75 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 26 Dec 2023 15:46:19 -0800 Subject: [PATCH 10/10] store: Replace event queue on expiry Fixes: #185 --- lib/model/store.dart | 44 +++++++++++++++++++++++----- lib/widgets/message_list.dart | 3 +- lib/widgets/store.dart | 20 +++++++------ test/model/store_test.dart | 54 +++++++++++++++++++++++++++++++---- 4 files changed, 97 insertions(+), 24 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index 693a5f2729..dbbdf20864 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -8,6 +8,7 @@ import 'package:path/path.dart' as p; import 'package:path_provider/path_provider.dart'; import '../api/core.dart'; +import '../api/exception.dart'; import '../api/model/events.dart'; import '../api/model/initial_snapshot.dart'; import '../api/model/model.dart'; @@ -96,12 +97,26 @@ abstract class GlobalStore extends ChangeNotifier { future = loadPerAccount(account!); _perAccountStoresLoading[accountId] = future; store = await future; - _perAccountStores[accountId] = store; + _setPerAccount(accountId, store); _perAccountStoresLoading.remove(accountId); - notifyListeners(); return store; } + Future _reloadPerAccount(Account account) async { + assert(identical(_accounts[account.id], account)); + assert(_perAccountStores.containsKey(account.id)); + assert(!_perAccountStoresLoading.containsKey(account.id)); + final store = await loadPerAccount(account); + _setPerAccount(account.id, store); + } + + void _setPerAccount(int accountId, PerAccountStore store) { + final oldStore = _perAccountStores[accountId]; + _perAccountStores[accountId] = store; + notifyListeners(); + oldStore?.dispose(); + } + /// Load per-account data for the given account, unconditionally. /// /// This method should be called only by the implementation of [perAccount]. @@ -199,8 +214,6 @@ class PerAccountStore extends ChangeNotifier with StreamStore { }) : _globalStore = globalStore, _streams = streams; - // We'll use this in an upcoming commit. - // ignore: unused_field final GlobalStore _globalStore; final Account account; @@ -569,9 +582,26 @@ class UpdateMachine { }()); } - final result = await getEvents(store.connection, - queueId: queueId, lastEventId: lastEventId); - // TODO handle errors on get-events; retry with backoff + final GetEventsResult result; + try { + result = await getEvents(store.connection, + queueId: queueId, lastEventId: lastEventId); + } catch (e) { + switch (e) { + case ZulipApiException(code: 'BAD_EVENT_QUEUE_ID'): + assert(debugLog('Lost event queue for $store. Replacing…')); + await store._globalStore._reloadPerAccount(store.account); + dispose(); + debugLog('… Event queue replaced.'); + return; + + default: + assert(debugLog('Error polling event queue for $store: $e')); + // TODO(#184) handle errors on get-events; retry with backoff + rethrow; + } + } + final events = result.events; for (final event in events) { store.handleEvent(event); diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 504fe0081e..6a644d2493 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -186,8 +186,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat } @override - void onNewStore() { - model?.dispose(); + void onNewStore() { // TODO(#464) try to keep using old model until new one gets messages _initModel(PerAccountStoreWidget.of(context)); } diff --git a/lib/widgets/store.dart b/lib/widgets/store.dart index 61fd9f6e4e..f69a2c7d6b 100644 --- a/lib/widgets/store.dart +++ b/lib/widgets/store.dart @@ -135,14 +135,17 @@ class PerAccountStoreWidget extends StatefulWidget { /// use [State.didChangeDependencies] instead. For discussion, see /// [BuildContext.dependOnInheritedWidgetOfExactType]. /// + /// A [State] that calls this method in [State.didChangeDependencies] + /// should typically also mix in [PerAccountStoreAwareStateMixin], + /// in order to handle the store itself being replaced with a new store. + /// This happens in particular if the old store's event queue expires + /// on the server. + /// /// See also: /// * [accountIdOf], for the account ID corresponding to the same data. /// * [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'); @@ -253,13 +256,11 @@ class LoadingPage extends StatelessWidget { /// 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 +/// - stop using the old [PerAccountStore], which will already have +/// been disposed; /// - 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; @@ -270,8 +271,9 @@ mixin PerAccountStoreAwareStateMixin on State { /// 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. + /// In this, add any needed listeners on the new store + /// and drop any references to the old store, which will already + /// have been disposed. void onNewStore(); @override diff --git a/test/model/store_test.dart b/test/model/store_test.dart index 76087069f3..cde30b2c56 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -140,18 +140,26 @@ void main() { }); group('UpdateMachine.poll', () { + late TestGlobalStore globalStore; late UpdateMachine updateMachine; late PerAccountStore store; late FakeApiConnection connection; - void prepareStore({int? lastEventId}) { - updateMachine = eg.updateMachine(initialSnapshot: eg.initialSnapshot( - lastEventId: lastEventId, - )); + void updateFromGlobalStore() { + updateMachine = globalStore.updateMachines[eg.selfAccount.id]!; store = updateMachine.store; + assert(identical(store, globalStore.perAccountSync(eg.selfAccount.id))); connection = store.connection as FakeApiConnection; } + Future prepareStore({int? lastEventId}) async { + globalStore = TestGlobalStore(accounts: []); + await globalStore.add(eg.selfAccount, eg.initialSnapshot( + lastEventId: lastEventId)); + await globalStore.perAccount(eg.selfAccount.id); + updateFromGlobalStore(); + } + void checkLastRequest({required int lastEventId}) { check(connection.lastRequest).isA() ..method.equals('GET') @@ -163,7 +171,7 @@ void main() { } test('loops on success', () async { - prepareStore(lastEventId: 1); + await prepareStore(lastEventId: 1); check(updateMachine.lastEventId).equals(1); updateMachine.debugPauseLoop(); @@ -191,7 +199,7 @@ void main() { }); test('handles events', () async { - prepareStore(); + await prepareStore(); updateMachine.debugPauseLoop(); updateMachine.poll(); @@ -206,6 +214,40 @@ void main() { await Future.delayed(Duration.zero); check(store.userSettings!.twentyFourHourTime).isTrue(); }); + + test('handles expired queue', () async { + await prepareStore(); + updateMachine.debugPauseLoop(); + updateMachine.poll(); + check(globalStore.perAccountSync(store.account.id)).identicalTo(store); + + // Let the server expire the event queue. + connection.prepare(httpStatus: 400, json: { + 'result': 'error', 'code': 'BAD_EVENT_QUEUE_ID', + 'queue_id': updateMachine.queueId, + 'msg': 'Bad event queue ID: ${updateMachine.queueId}', + }); + updateMachine.debugAdvanceLoop(); + await null; + await Future.delayed(Duration.zero); + + // The global store has a new store. + check(globalStore.perAccountSync(store.account.id)).not(it()..identicalTo(store)); + updateFromGlobalStore(); + + // The new UpdateMachine updates the new store. + updateMachine.debugPauseLoop(); + updateMachine.poll(); + check(store.userSettings!.twentyFourHourTime).isFalse(); + connection.prepare(json: GetEventsResult(events: [ + UserSettingsUpdateEvent(id: 2, + property: UserSettingName.twentyFourHourTime, value: true), + ], queueId: null).toJson()); + updateMachine.debugAdvanceLoop(); + await null; + await Future.delayed(Duration.zero); + check(store.userSettings!.twentyFourHourTime).isTrue(); + }); }); group('UpdateMachine.registerNotificationToken', () {