From 6dc15d15b56121116465a3b7a042d8267ee8f1ba Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 4 Apr 2025 12:34:56 -0700 Subject: [PATCH 01/14] store [nfc]: Tie CorePerAccountStore explicitly 1-to-1 with PerAccountStore --- lib/model/store.dart | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index 4a4a3e209a..f27bad9493 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -328,8 +328,14 @@ abstract class GlobalStore extends ChangeNotifier { class AccountNotFoundException implements Exception {} /// A bundle of items that are useful to [PerAccountStore] and its substores. +/// +/// Each instance of this class is constructed as part of constructing a +/// [PerAccountStore] instance, +/// and is shared by that [PerAccountStore] and its substores. +/// Calling [PerAccountStore.dispose] also disposes the [CorePerAccountStore] +/// (for example, it calls [ApiConnection.dispose] on [connection]). class CorePerAccountStore { - CorePerAccountStore({required this.connection}); + CorePerAccountStore._({required this.connection}); final ApiConnection connection; // TODO(#135): update zulipFeatureLevel with events } @@ -386,7 +392,7 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor } final realmUrl = account.realmUrl; - final core = CorePerAccountStore(connection: connection); + final core = CorePerAccountStore._(connection: connection); final channels = ChannelStoreImpl(initialSnapshot: initialSnapshot); return PerAccountStore._( globalStore: globalStore, From 48dd2ba3b6621ed11ad558849f03934f21b4df81 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 4 Apr 2025 12:14:44 -0700 Subject: [PATCH 02/14] typing_status [nfc]: Use PerAccountStoreBase to get connection --- lib/model/store.dart | 2 +- lib/model/typing_status.dart | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index f27bad9493..a1a549eb9c 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -412,7 +412,7 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor accountId: accountId, userSettings: initialSnapshot.userSettings, typingNotifier: TypingNotifier( - connection: connection, + core: core, typingStoppedWaitPeriod: Duration( milliseconds: initialSnapshot.serverTypingStoppedWaitPeriodMilliseconds), typingStartedWaitPeriod: Duration( diff --git a/lib/model/typing_status.dart b/lib/model/typing_status.dart index 2956564c20..03ad4dfe3f 100644 --- a/lib/model/typing_status.dart +++ b/lib/model/typing_status.dart @@ -2,11 +2,11 @@ import 'dart:async'; import 'package:flutter/foundation.dart'; -import '../api/core.dart'; import '../api/model/events.dart'; import '../api/route/typing.dart'; import 'binding.dart'; import 'narrow.dart'; +import 'store.dart'; /// The model for tracking the typing status organized by narrows. /// @@ -93,14 +93,13 @@ class TypingStatus extends ChangeNotifier { /// See also: /// * https://github.com/zulip/zulip/blob/52a9846cdf4abfbe937a94559690d508e95f4065/web/shared/src/typing_status.ts /// * https://zulip.readthedocs.io/en/latest/subsystems/typing-indicators.html -class TypingNotifier { +class TypingNotifier extends PerAccountStoreBase { TypingNotifier({ - required this.connection, + required super.core, required this.typingStoppedWaitPeriod, required this.typingStartedWaitPeriod, }); - final ApiConnection connection; final Duration typingStoppedWaitPeriod; final Duration typingStartedWaitPeriod; From 849a4f468a20bfbb0c70c47155c2ce23562683b5 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 4 Apr 2025 13:26:59 -0700 Subject: [PATCH 03/14] store [nfc]: Fix "will throw if disposed" comment on account to be more specific As far as I can see, just `dispose` won't cause this to throw. I think this related point is the one this comment was intended to make. --- lib/model/store.dart | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index a1a549eb9c..ef73626975 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -570,7 +570,8 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor /// The [Account] this store belongs to. /// - /// Will throw if called after [dispose] has been called. + /// Will throw if the account has been removed from the global store, + /// which is possible only if [dispose] has been called on this store. Account get account => _globalStore.getAccount(accountId)!; final UserSettings? userSettings; // TODO(server-5) From eba689b557e3e12e6015a44e714f0f3a83a088f9 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 4 Apr 2025 12:21:43 -0700 Subject: [PATCH 04/14] store [nfc]: Move global store up to PerAccountStoreBase too --- lib/model/store.dart | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index ef73626975..adb73dd80f 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -335,8 +335,12 @@ class AccountNotFoundException implements Exception {} /// Calling [PerAccountStore.dispose] also disposes the [CorePerAccountStore] /// (for example, it calls [ApiConnection.dispose] on [connection]). class CorePerAccountStore { - CorePerAccountStore._({required this.connection}); + CorePerAccountStore._({ + required GlobalStore globalStore, + required this.connection, + }) : _globalStore = globalStore; + final GlobalStore _globalStore; final ApiConnection connection; // TODO(#135): update zulipFeatureLevel with events } @@ -348,6 +352,8 @@ abstract class PerAccountStoreBase { final CorePerAccountStore _core; + GlobalStore get _globalStore => _core._globalStore; + ApiConnection get connection => _core.connection; } @@ -392,7 +398,10 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor } final realmUrl = account.realmUrl; - final core = CorePerAccountStore._(connection: connection); + final core = CorePerAccountStore._( + globalStore: globalStore, + connection: connection, + ); final channels = ChannelStoreImpl(initialSnapshot: initialSnapshot); return PerAccountStore._( globalStore: globalStore, @@ -465,7 +474,6 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor }) : assert(realmUrl == globalStore.getAccount(accountId)!.realmUrl), assert(realmUrl == core.connection.realmUrl), assert(emoji.realmUrl == realmUrl), - _globalStore = globalStore, _realmEmptyTopicDisplayName = realmEmptyTopicDisplayName, _emoji = emoji, _users = users, @@ -478,8 +486,6 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor //////////////////////////////// // Where data comes from in the first place. - final GlobalStore _globalStore; - final String queueId; UpdateMachine? get updateMachine => _updateMachine; UpdateMachine? _updateMachine; From 082cd1189baadfc4b609f896ac8eb51fbe602066 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 4 Apr 2025 12:25:16 -0700 Subject: [PATCH 05/14] store [nfc]: Move accountId to PerAccountStoreBase --- lib/model/store.dart | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index adb73dd80f..f35e4552f2 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -338,10 +338,12 @@ class CorePerAccountStore { CorePerAccountStore._({ required GlobalStore globalStore, required this.connection, + required this.accountId, }) : _globalStore = globalStore; final GlobalStore _globalStore; final ApiConnection connection; // TODO(#135): update zulipFeatureLevel with events + final int accountId; } /// A base class for [PerAccountStore] and its substores, @@ -352,9 +354,17 @@ abstract class PerAccountStoreBase { final CorePerAccountStore _core; + //////////////////////////////// + // Where data comes from in the first place. + GlobalStore get _globalStore => _core._globalStore; ApiConnection get connection => _core.connection; + + //////////////////////////////// + // Data attached to the self-account on the realm. + + int get accountId => _core.accountId; } /// Store for the user's data for a given Zulip account. @@ -401,6 +411,7 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor final core = CorePerAccountStore._( globalStore: globalStore, connection: connection, + accountId: accountId, ); final channels = ChannelStoreImpl(initialSnapshot: initialSnapshot); return PerAccountStore._( @@ -418,7 +429,6 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor emailAddressVisibility: initialSnapshot.emailAddressVisibility, emoji: EmojiStoreImpl( realmUrl: realmUrl, allRealmEmoji: initialSnapshot.realmEmoji), - accountId: accountId, userSettings: initialSnapshot.userSettings, typingNotifier: TypingNotifier( core: core, @@ -461,7 +471,6 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor required this.customProfileFields, required this.emailAddressVisibility, required EmojiStoreImpl emoji, - required this.accountId, required this.userSettings, required this.typingNotifier, required UserStoreImpl users, @@ -471,7 +480,7 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor required this.unreads, required this.recentDmConversationsView, required this.recentSenders, - }) : assert(realmUrl == globalStore.getAccount(accountId)!.realmUrl), + }) : assert(realmUrl == globalStore.getAccount(core.accountId)!.realmUrl), assert(realmUrl == core.connection.realmUrl), assert(emoji.realmUrl == realmUrl), _realmEmptyTopicDisplayName = realmEmptyTopicDisplayName, @@ -572,8 +581,6 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor //////////////////////////////// // Data attached to the self-account on the realm. - final int accountId; - /// The [Account] this store belongs to. /// /// Will throw if the account has been removed from the global store, From 766ff814ab473a234068de0493d4f69add36d5e1 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 4 Apr 2025 12:27:31 -0700 Subject: [PATCH 06/14] store [nfc]: Move `account` getter up to PerAccountStoreBase --- lib/model/store.dart | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index f35e4552f2..b9e4e7a38e 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -365,6 +365,13 @@ abstract class PerAccountStoreBase { // Data attached to the self-account on the realm. int get accountId => _core.accountId; + + /// The [Account] this store belongs to. + /// + /// Will throw if the account has been removed from the global store, + /// which is possible only if [PerAccountStore.dispose] has been called + /// on this store. + Account get account => _globalStore.getAccount(accountId)!; } /// Store for the user's data for a given Zulip account. @@ -581,12 +588,6 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor //////////////////////////////// // Data attached to the self-account on the realm. - /// The [Account] this store belongs to. - /// - /// Will throw if the account has been removed from the global store, - /// which is possible only if [dispose] has been called on this store. - Account get account => _globalStore.getAccount(accountId)!; - final UserSettings? userSettings; // TODO(server-5) final TypingNotifier typingNotifier; From e900b8dd0e2ddfd9a67901abaffa349416b34b58 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 4 Apr 2025 12:44:16 -0700 Subject: [PATCH 07/14] store [nfc]: Move assert up to core that connection/account agree on realmUrl --- lib/model/store.dart | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index b9e4e7a38e..e5a1c9fc46 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -339,7 +339,8 @@ class CorePerAccountStore { required GlobalStore globalStore, required this.connection, required this.accountId, - }) : _globalStore = globalStore; + }) : _globalStore = globalStore, + assert(connection.realmUrl == globalStore.getAccount(accountId)!.realmUrl); final GlobalStore _globalStore; final ApiConnection connection; // TODO(#135): update zulipFeatureLevel with events @@ -422,7 +423,6 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor ); final channels = ChannelStoreImpl(initialSnapshot: initialSnapshot); return PerAccountStore._( - globalStore: globalStore, core: core, queueId: queueId, realmUrl: realmUrl, @@ -465,7 +465,6 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor } PerAccountStore._({ - required GlobalStore globalStore, required super.core, required this.queueId, required this.realmUrl, @@ -487,8 +486,7 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor required this.unreads, required this.recentDmConversationsView, required this.recentSenders, - }) : assert(realmUrl == globalStore.getAccount(core.accountId)!.realmUrl), - assert(realmUrl == core.connection.realmUrl), + }) : assert(realmUrl == core.connection.realmUrl), assert(emoji.realmUrl == realmUrl), _realmEmptyTopicDisplayName = realmEmptyTopicDisplayName, _emoji = emoji, From d764c3ab13a462389675cd4290bc39859249e36e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 4 Apr 2025 12:42:25 -0700 Subject: [PATCH 08/14] store [nfc]: Move realmUrl up to PerAccountStoreBase --- lib/model/store.dart | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index e5a1c9fc46..241e65b1c4 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -362,6 +362,12 @@ abstract class PerAccountStoreBase { ApiConnection get connection => _core.connection; + //////////////////////////////// + // Data attached to the realm or the server. + + /// Always equal to `account.realmUrl` and `connection.realmUrl`. + Uri get realmUrl => connection.realmUrl; + //////////////////////////////// // Data attached to the self-account on the realm. @@ -425,7 +431,6 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor return PerAccountStore._( core: core, queueId: queueId, - realmUrl: realmUrl, realmWildcardMentionPolicy: initialSnapshot.realmWildcardMentionPolicy, realmMandatoryTopics: initialSnapshot.realmMandatoryTopics, realmWaitingPeriodThreshold: initialSnapshot.realmWaitingPeriodThreshold, @@ -467,7 +472,6 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor PerAccountStore._({ required super.core, required this.queueId, - required this.realmUrl, required this.realmWildcardMentionPolicy, required this.realmMandatoryTopics, required this.realmWaitingPeriodThreshold, @@ -486,8 +490,7 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor required this.unreads, required this.recentDmConversationsView, required this.recentSenders, - }) : assert(realmUrl == core.connection.realmUrl), - assert(emoji.realmUrl == realmUrl), + }) : assert(emoji.realmUrl == core.connection.realmUrl), _realmEmptyTopicDisplayName = realmEmptyTopicDisplayName, _emoji = emoji, _users = users, @@ -521,9 +524,6 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor //////////////////////////////// // Data attached to the realm or the server. - /// Always equal to `account.realmUrl` and `connection.realmUrl`. - final Uri realmUrl; - /// Resolve [reference] as a URL relative to [realmUrl]. /// /// This returns null if [reference] fails to parse as a URL. From 6706ce93671fcb8728af5e1d10dd5eb913d169ee Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 4 Apr 2025 13:01:28 -0700 Subject: [PATCH 09/14] emoji [nfc]: Use PerAccountStoreBase for realmUrl --- lib/model/emoji.dart | 7 ++----- lib/model/store.dart | 6 ++---- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index b0ec5f7324..42b8692be7 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -137,15 +137,12 @@ mixin EmojiStore { /// Generally the only code that should need this class is [PerAccountStore] /// itself. Other code accesses this functionality through [PerAccountStore], /// or through the mixin [EmojiStore] which describes its interface. -class EmojiStoreImpl with EmojiStore { +class EmojiStoreImpl extends PerAccountStoreBase with EmojiStore { EmojiStoreImpl({ - required this.realmUrl, + required super.core, required this.allRealmEmoji, }) : _serverEmojiData = null; // TODO(#974) maybe start from a hard-coded baseline - /// The same as [PerAccountStore.realmUrl]. - final Uri realmUrl; - /// The realm's custom emoji, indexed by their [RealmEmojiItem.emojiCode], /// including deactivated emoji not available for new uses. /// diff --git a/lib/model/store.dart b/lib/model/store.dart index 241e65b1c4..22a1b12797 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -421,7 +421,6 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor throw Exception("bad initial snapshot: missing queueId"); } - final realmUrl = account.realmUrl; final core = CorePerAccountStore._( globalStore: globalStore, connection: connection, @@ -440,7 +439,7 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor customProfileFields: _sortCustomProfileFields(initialSnapshot.customProfileFields), emailAddressVisibility: initialSnapshot.emailAddressVisibility, emoji: EmojiStoreImpl( - realmUrl: realmUrl, allRealmEmoji: initialSnapshot.realmEmoji), + core: core, allRealmEmoji: initialSnapshot.realmEmoji), userSettings: initialSnapshot.userSettings, typingNotifier: TypingNotifier( core: core, @@ -490,8 +489,7 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor required this.unreads, required this.recentDmConversationsView, required this.recentSenders, - }) : assert(emoji.realmUrl == core.connection.realmUrl), - _realmEmptyTopicDisplayName = realmEmptyTopicDisplayName, + }) : _realmEmptyTopicDisplayName = realmEmptyTopicDisplayName, _emoji = emoji, _users = users, _channels = channels, From 81656f40d4c954571d6f3748b93c9afa980d86d6 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 4 Apr 2025 13:10:54 -0700 Subject: [PATCH 10/14] emoji: Parse URLs the same way for validation as for use I think this change might be NFC, but I'm not sure. The situation where it could change the behavior is if there's some URL string that would be accepted by `Uri.parse`, but rejected by `Uri.resolve` with the realm URL as base URL, or vice versa. If there is any such situation, then this behavior is more accurate. If there isn't, then this is just a small simplification. Noticed this because it's one of the handful of references to the store's realmUrl, and another one is a call to tryResolveUrl. --- lib/model/emoji.dart | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index 42b8692be7..21e91159cd 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -192,19 +192,19 @@ class EmojiStoreImpl extends PerAccountStoreBase with EmojiStore { required String? stillUrl, required String emojiName, }) { - final source = Uri.tryParse(sourceUrl); - if (source == null) return TextEmojiDisplay(emojiName: emojiName); + final resolvedUrl = tryResolveUrl(realmUrl, sourceUrl); + if (resolvedUrl == null) return TextEmojiDisplay(emojiName: emojiName); - Uri? still; + Uri? resolvedStillUrl; if (stillUrl != null) { - still = Uri.tryParse(stillUrl); - if (still == null) return TextEmojiDisplay(emojiName: emojiName); + resolvedStillUrl = tryResolveUrl(realmUrl, stillUrl); + if (resolvedStillUrl == null) return TextEmojiDisplay(emojiName: emojiName); } return ImageEmojiDisplay( emojiName: emojiName, - resolvedUrl: realmUrl.resolveUri(source), - resolvedStillUrl: still == null ? null : realmUrl.resolveUri(still), + resolvedUrl: resolvedUrl, + resolvedStillUrl: resolvedStillUrl, ); } From b6f712fe197ceee7570492c8ab790bf0a6d2e326 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 4 Apr 2025 12:55:09 -0700 Subject: [PATCH 11/14] store [nfc]: Move tryResolveUrl up to base, and use it more --- lib/model/emoji.dart | 4 ++-- lib/model/store.dart | 32 ++++++++++++++++---------------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/lib/model/emoji.dart b/lib/model/emoji.dart index 21e91159cd..670c574c12 100644 --- a/lib/model/emoji.dart +++ b/lib/model/emoji.dart @@ -192,12 +192,12 @@ class EmojiStoreImpl extends PerAccountStoreBase with EmojiStore { required String? stillUrl, required String emojiName, }) { - final resolvedUrl = tryResolveUrl(realmUrl, sourceUrl); + final resolvedUrl = this.tryResolveUrl(sourceUrl); if (resolvedUrl == null) return TextEmojiDisplay(emojiName: emojiName); Uri? resolvedStillUrl; if (stillUrl != null) { - resolvedStillUrl = tryResolveUrl(realmUrl, stillUrl); + resolvedStillUrl = this.tryResolveUrl(stillUrl); if (resolvedStillUrl == null) return TextEmojiDisplay(emojiName: emojiName); } diff --git a/lib/model/store.dart b/lib/model/store.dart index 22a1b12797..77beb14ba5 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -368,6 +368,11 @@ abstract class PerAccountStoreBase { /// Always equal to `account.realmUrl` and `connection.realmUrl`. Uri get realmUrl => connection.realmUrl; + /// Resolve [reference] as a URL relative to [realmUrl]. + /// + /// This returns null if [reference] fails to parse as a URL. + Uri? tryResolveUrl(String reference) => _tryResolveUrl(realmUrl, reference); + //////////////////////////////// // Data attached to the self-account on the realm. @@ -381,6 +386,17 @@ abstract class PerAccountStoreBase { Account get account => _globalStore.getAccount(accountId)!; } +const _tryResolveUrl = tryResolveUrl; + +/// Like [Uri.resolve], but on failure return null instead of throwing. +Uri? tryResolveUrl(Uri baseUrl, String reference) { + try { + return baseUrl.resolve(reference); + } on FormatException { + return null; + } +} + /// Store for the user's data for a given Zulip account. /// /// This should always have a consistent snapshot of the state on the server, @@ -522,11 +538,6 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor //////////////////////////////// // Data attached to the realm or the server. - /// Resolve [reference] as a URL relative to [realmUrl]. - /// - /// This returns null if [reference] fails to parse as a URL. - Uri? tryResolveUrl(String reference) => _tryResolveUrl(realmUrl, reference); - /// Always equal to `connection.zulipFeatureLevel` /// and `account.zulipFeatureLevel`. int get zulipFeatureLevel => connection.zulipFeatureLevel!; @@ -899,17 +910,6 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor String toString() => '${objectRuntimeType(this, 'PerAccountStore')}#${shortHash(this)}'; } -const _tryResolveUrl = tryResolveUrl; - -/// Like [Uri.resolve], but on failure return null instead of throwing. -Uri? tryResolveUrl(Uri baseUrl, String reference) { - try { - return baseUrl.resolve(reference); - } on FormatException { - return null; - } -} - /// A [GlobalStoreBackend] that uses a live, persistent local database. /// /// Used as part of a [LiveGlobalStore]. From ffda5819da1f01d41640de8eabd8261eb272af83 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 4 Apr 2025 13:31:08 -0700 Subject: [PATCH 12/14] user [nfc]: Move selfUserId to PerAccountStoreBase This `selfUserId` getter has 16 other references across 11 files. Thankfully our architecture, and in particular this way of combining the substores: class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStore, UserStore, ChannelStore, MessageStore { has meant that those all refer to it on the main PerAccountStore type, rather than on an individual substore like UserStore. So none of those need to be touched when we rearrange like this where the data is stored. --- lib/model/store.dart | 24 ++++++++++++++++++------ lib/model/user.dart | 18 ++++-------------- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index 77beb14ba5..e0642b1fb3 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -339,12 +339,20 @@ class CorePerAccountStore { required GlobalStore globalStore, required this.connection, required this.accountId, + required this.selfUserId, }) : _globalStore = globalStore, assert(connection.realmUrl == globalStore.getAccount(accountId)!.realmUrl); final GlobalStore _globalStore; final ApiConnection connection; // TODO(#135): update zulipFeatureLevel with events final int accountId; + + // This isn't strictly needed as a field; it could be a getter + // that uses `_globalStore.getAccount(accountId)`. + // But we denormalize it here to save a hash-table lookup every time + // the self-user ID is needed, which can be often. + // It never changes on the account; see [GlobalStore.updateAccount]. + final int selfUserId; } /// A base class for [PerAccountStore] and its substores, @@ -384,6 +392,14 @@ abstract class PerAccountStoreBase { /// which is possible only if [PerAccountStore.dispose] has been called /// on this store. Account get account => _globalStore.getAccount(accountId)!; + + /// The user ID of the "self-user", + /// i.e. the account the person using this app is logged into. + /// + /// This always equals the [Account.userId] on [account]. + /// + /// For the corresponding [User] object, see [UserStore.selfUser]. + int get selfUserId => _core.selfUserId; } const _tryResolveUrl = tryResolveUrl; @@ -441,6 +457,7 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor globalStore: globalStore, connection: connection, accountId: accountId, + selfUserId: account.userId, ); final channels = ChannelStoreImpl(initialSnapshot: initialSnapshot); return PerAccountStore._( @@ -464,9 +481,7 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor typingStartedWaitPeriod: Duration( milliseconds: initialSnapshot.serverTypingStartedWaitPeriodMilliseconds), ), - users: UserStoreImpl( - selfUserId: account.userId, - initialSnapshot: initialSnapshot), + users: UserStoreImpl(core: core, initialSnapshot: initialSnapshot), typingStatus: TypingStatus( selfUserId: account.userId, typingStartedExpiryPeriod: Duration(milliseconds: initialSnapshot.serverTypingStartedExpiryPeriodMilliseconds), @@ -602,9 +617,6 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor //////////////////////////////// // Users and data about them. - @override - int get selfUserId => _users.selfUserId; - @override User? getUser(int userId) => _users.getUser(userId); diff --git a/lib/model/user.dart b/lib/model/user.dart index 1577e21048..05ab2747df 100644 --- a/lib/model/user.dart +++ b/lib/model/user.dart @@ -2,17 +2,10 @@ import '../api/model/events.dart'; import '../api/model/initial_snapshot.dart'; import '../api/model/model.dart'; import 'localizations.dart'; +import 'store.dart'; /// The portion of [PerAccountStore] describing the users in the realm. -mixin UserStore { - /// The user ID of the "self-user", - /// i.e. the account the person using this app is logged into. - /// - /// This always equals the [Account.userId] on [PerAccountStore.account]. - /// - /// For the corresponding [User] object, see [selfUser]. - int get selfUserId; - +mixin UserStore on PerAccountStoreBase { /// The user with the given ID, if that user is known. /// /// There may be other users that are perfectly real but are @@ -80,9 +73,9 @@ mixin UserStore { /// Generally the only code that should need this class is [PerAccountStore] /// itself. Other code accesses this functionality through [PerAccountStore], /// or through the mixin [UserStore] which describes its interface. -class UserStoreImpl with UserStore { +class UserStoreImpl extends PerAccountStoreBase with UserStore { UserStoreImpl({ - required this.selfUserId, + required super.core, required InitialSnapshot initialSnapshot, }) : _users = Map.fromEntries( initialSnapshot.realmUsers @@ -90,9 +83,6 @@ class UserStoreImpl with UserStore { .followedBy(initialSnapshot.crossRealmBots) .map((user) => MapEntry(user.userId, user))); - @override - final int selfUserId; - final Map _users; @override From 9f752aad8ebc91e4976486957153be4efa001db4 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 4 Apr 2025 13:45:36 -0700 Subject: [PATCH 13/14] store [nfc]: Get selfUserId on substores from PerAccountStoreBase Co-authored-by: Zixuan James Li --- lib/model/recent_dm_conversations.dart | 13 ++++++------- lib/model/store.dart | 9 ++++----- lib/model/typing_status.dart | 5 ++--- lib/model/unreads.dart | 16 ++++++++-------- test/model/recent_dm_conversations_test.dart | 18 ++++++++++-------- test/model/typing_status_test.dart | 8 +++++--- test/model/unreads_test.dart | 7 ++++--- 7 files changed, 39 insertions(+), 37 deletions(-) diff --git a/lib/model/recent_dm_conversations.dart b/lib/model/recent_dm_conversations.dart index b38610be15..8428ecdf63 100644 --- a/lib/model/recent_dm_conversations.dart +++ b/lib/model/recent_dm_conversations.dart @@ -7,18 +7,19 @@ import '../api/model/initial_snapshot.dart'; import '../api/model/model.dart'; import '../api/model/events.dart'; import 'narrow.dart'; +import 'store.dart'; /// A view-model for the recent-DM-conversations UI. /// /// This maintains the list of recent DM conversations, /// plus additional data in order to efficiently maintain the list. -class RecentDmConversationsView extends ChangeNotifier { +class RecentDmConversationsView extends PerAccountStoreBase with ChangeNotifier { factory RecentDmConversationsView({ + required CorePerAccountStore core, required List initial, - required int selfUserId, }) { final entries = initial.map((conversation) => MapEntry( - DmNarrow.ofRecentDmConversation(conversation, selfUserId: selfUserId), + DmNarrow.ofRecentDmConversation(conversation, selfUserId: core.selfUserId), conversation.maxMessageId, )).toList()..sort((a, b) => -a.value.compareTo(b.value)); @@ -33,18 +34,18 @@ class RecentDmConversationsView extends ChangeNotifier { } return RecentDmConversationsView._( + core: core, map: Map.fromEntries(entries), sorted: QueueList.from(entries.map((e) => e.key)), latestMessagesByRecipient: latestMessagesByRecipient, - selfUserId: selfUserId, ); } RecentDmConversationsView._({ + required super.core, required this.map, required this.sorted, required this.latestMessagesByRecipient, - required this.selfUserId, }); /// The latest message ID in each conversation. @@ -62,8 +63,6 @@ class RecentDmConversationsView extends ChangeNotifier { /// it might have been sent by anyone in its conversation.) final Map latestMessagesByRecipient; - final int selfUserId; - /// Insert the key at the proper place in [sorted]. /// /// Optimized, taking O(1) time, for the case where that place is the start, diff --git a/lib/model/store.dart b/lib/model/store.dart index e0642b1fb3..bd6e359045 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -482,19 +482,18 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor milliseconds: initialSnapshot.serverTypingStartedWaitPeriodMilliseconds), ), users: UserStoreImpl(core: core, initialSnapshot: initialSnapshot), - typingStatus: TypingStatus( - selfUserId: account.userId, + typingStatus: TypingStatus(core: core, typingStartedExpiryPeriod: Duration(milliseconds: initialSnapshot.serverTypingStartedExpiryPeriodMilliseconds), ), channels: channels, messages: MessageStoreImpl(core: core), unreads: Unreads( initial: initialSnapshot.unreadMsgs, - selfUserId: account.userId, + core: core, channelStore: channels, ), - recentDmConversationsView: RecentDmConversationsView( - initial: initialSnapshot.recentPrivateConversations, selfUserId: account.userId), + recentDmConversationsView: RecentDmConversationsView(core: core, + initial: initialSnapshot.recentPrivateConversations), recentSenders: RecentSenders(), ); } diff --git a/lib/model/typing_status.dart b/lib/model/typing_status.dart index 03ad4dfe3f..1ddd72c48b 100644 --- a/lib/model/typing_status.dart +++ b/lib/model/typing_status.dart @@ -11,13 +11,12 @@ import 'store.dart'; /// The model for tracking the typing status organized by narrows. /// /// Listeners are notified when a typist is added or removed from any narrow. -class TypingStatus extends ChangeNotifier { +class TypingStatus extends PerAccountStoreBase with ChangeNotifier { TypingStatus({ - required this.selfUserId, + required super.core, required this.typingStartedExpiryPeriod, }); - final int selfUserId; final Duration typingStartedExpiryPeriod; Iterable get debugActiveNarrows => _timerMapsByNarrow.keys; diff --git a/lib/model/unreads.dart b/lib/model/unreads.dart index 34b4492013..254b615452 100644 --- a/lib/model/unreads.dart +++ b/lib/model/unreads.dart @@ -10,6 +10,7 @@ import '../log.dart'; import 'algorithms.dart'; import 'narrow.dart'; import 'channel.dart'; +import 'store.dart'; /// The view-model for unread messages. /// @@ -34,10 +35,10 @@ import 'channel.dart'; // sync to those unreads, because the user has shown an interest in them. // TODO When loading a message list with stream messages, check all the stream // messages and refresh [mentions] (see [mentions] dartdoc). -class Unreads extends ChangeNotifier { +class Unreads extends PerAccountStoreBase with ChangeNotifier { factory Unreads({ required UnreadMessagesSnapshot initial, - required int selfUserId, + required CorePerAccountStore core, required ChannelStore channelStore, }) { final streams = >>{}; @@ -52,32 +53,33 @@ class Unreads extends ChangeNotifier { for (final unreadDmSnapshot in initial.dms) { final otherUserId = unreadDmSnapshot.otherUserId; - final narrow = DmNarrow.withUser(otherUserId, selfUserId: selfUserId); + final narrow = DmNarrow.withUser(otherUserId, selfUserId: core.selfUserId); dms[narrow] = QueueList.from(unreadDmSnapshot.unreadMessageIds); } for (final unreadHuddleSnapshot in initial.huddles) { - final narrow = DmNarrow.ofUnreadHuddleSnapshot(unreadHuddleSnapshot, selfUserId: selfUserId); + final narrow = DmNarrow.ofUnreadHuddleSnapshot(unreadHuddleSnapshot, + selfUserId: core.selfUserId); dms[narrow] = QueueList.from(unreadHuddleSnapshot.unreadMessageIds); } return Unreads._( + core: core, channelStore: channelStore, streams: streams, dms: dms, mentions: mentions, oldUnreadsMissing: initial.oldUnreadsMissing, - selfUserId: selfUserId, ); } Unreads._({ + required super.core, required this.channelStore, required this.streams, required this.dms, required this.mentions, required this.oldUnreadsMissing, - required this.selfUserId, }); final ChannelStore channelStore; @@ -125,8 +127,6 @@ class Unreads extends ChangeNotifier { /// Is set to false when the user clears out all unreads. bool oldUnreadsMissing; - final int selfUserId; - // TODO(#370): maintain this count incrementally, rather than recomputing from scratch int countInCombinedFeedNarrow() { int c = 0; diff --git a/test/model/recent_dm_conversations_test.dart b/test/model/recent_dm_conversations_test.dart index da487304e6..8905460e66 100644 --- a/test/model/recent_dm_conversations_test.dart +++ b/test/model/recent_dm_conversations_test.dart @@ -6,6 +6,7 @@ import 'package:zulip/model/recent_dm_conversations.dart'; import '../example_data.dart' as eg; import 'recent_dm_conversations_checks.dart'; +import 'store_checks.dart'; void main() { group('RecentDmConversationsView', () { @@ -18,18 +19,19 @@ void main() { } test('construct from initial data', () { - check(RecentDmConversationsView(selfUserId: eg.selfUser.userId, - initial: [])) + check(eg.store(initialSnapshot: eg.initialSnapshot( + recentPrivateConversations: [], + ))).recentDmConversationsView ..map.isEmpty() ..sorted.isEmpty() ..latestMessagesByRecipient.isEmpty(); - check(RecentDmConversationsView(selfUserId: eg.selfUser.userId, - initial: [ + check(eg.store(initialSnapshot: eg.initialSnapshot( + recentPrivateConversations: [ RecentDmConversation(userIds: [], maxMessageId: 200), RecentDmConversation(userIds: [1], maxMessageId: 100), RecentDmConversation(userIds: [2, 1], maxMessageId: 300), // userIds out of order - ])) + ]))).recentDmConversationsView ..map.deepEquals({ key([1, 2]): 300, key([]): 200, @@ -41,11 +43,11 @@ void main() { group('message event (new message)', () { RecentDmConversationsView setupView() { - return RecentDmConversationsView(selfUserId: eg.selfUser.userId, - initial: [ + return eg.store(initialSnapshot: eg.initialSnapshot( + recentPrivateConversations: [ RecentDmConversation(userIds: [1], maxMessageId: 200), RecentDmConversation(userIds: [1, 2], maxMessageId: 100), - ]); + ])).recentDmConversationsView; } test('(check base state)', () { diff --git a/test/model/typing_status_test.dart b/test/model/typing_status_test.dart index 4061301366..01d817680a 100644 --- a/test/model/typing_status_test.dart +++ b/test/model/typing_status_test.dart @@ -77,9 +77,11 @@ void main() { int? selfUserId, Map> typistsByNarrow = const {}, }) { - model = TypingStatus( - selfUserId: selfUserId ?? eg.selfUser.userId, - typingStartedExpiryPeriod: const Duration(milliseconds: 15000)); + final store = eg.store( + account: eg.selfAccount.copyWith(id: selfUserId), + initialSnapshot: eg.initialSnapshot( + serverTypingStartedExpiryPeriodMilliseconds: 15000)); + model = store.typingStatus; check(model.debugActiveNarrows).isEmpty(); notifiedCount = 0; model.addListener(() => notifiedCount += 1); diff --git a/test/model/unreads_test.dart b/test/model/unreads_test.dart index e8f7a0f850..42b9121f93 100644 --- a/test/model/unreads_test.dart +++ b/test/model/unreads_test.dart @@ -37,10 +37,11 @@ void main() { oldUnreadsMissing: false, ), }) { - channelStore = eg.store(); + final store = eg.store( + initialSnapshot: eg.initialSnapshot(unreadMsgs: initial)); + channelStore = store; notifiedCount = 0; - model = Unreads(initial: initial, - selfUserId: eg.selfUser.userId, channelStore: channelStore) + model = store.unreads ..addListener(() { notifiedCount++; }); From ec9aa351b3050a0fa3e467007826576110fdbf99 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 4 Apr 2025 15:01:59 -0700 Subject: [PATCH 14/14] unreads test [nfc]: Accept that a whole PerAccountStore is used for tests We've had a couple of approaches we've experimented with for how to write the tests for a given substore within our data model: * Operate on the given substore's individual API. * Operate on the API of the overall PerAccountStore, though focusing on the particular portion of that API provided by the given substore. In general I think the outcome of these experiments is that we want to be writing our tests against the API of the whole PerAccountStore. Fundamentally that's for the reasons described here: https://zulip.readthedocs.io/en/latest/testing/philosophy.html#integration-testing-or-unit-testing because the overall PerAccountStore is the layer whose API corresponds naturally to the Zulip server API, which is an interface that's external to the app and so is the real interface we're committed to supporting. This particular test file is one which we originally wrote on the other side of the experiment, operating on the individual `Unreads` substore. But it already had some tension with that approach, ever since we realized that that substore needed access to another substore, the ChannelStore, in order to properly handle muting (ffa2d323f, 0d03c8ec7). To handle that, these tests ended up making a whole PerAccountStore after all, and had a TODO comment for trying to fully make the substore-only approach work. With the advent of CorePerAccountStore, there's no longer a realistic prospect that we'd want to end up carrying that through. That's because the Unreads now gets its value of `selfUserId` from a CorePerAccountStore, and the only reasonable way to construct one of those is as part of a whole PerAccountStore. So resolve the experiment, at least as to this file: remove the TODO comment, and rename the variable to just `store` accordingly. --- test/model/unreads_test.dart | 44 +++++++++++++++++------------------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/test/model/unreads_test.dart b/test/model/unreads_test.dart index 42b9121f93..42e799624f 100644 --- a/test/model/unreads_test.dart +++ b/test/model/unreads_test.dart @@ -16,7 +16,7 @@ void main() { // These variables are the common state operated on by each test. // Each test case calls [prepare] to initialize them. late Unreads model; - late PerAccountStore channelStore; // TODO reduce this to ChannelStore + late PerAccountStore store; late int notifiedCount; void checkNotified({required int count}) { @@ -37,9 +37,7 @@ void main() { oldUnreadsMissing: false, ), }) { - final store = eg.store( - initialSnapshot: eg.initialSnapshot(unreadMsgs: initial)); - channelStore = store; + store = eg.store(initialSnapshot: eg.initialSnapshot(unreadMsgs: initial)); notifiedCount = 0; model = store.unreads ..addListener(() { @@ -158,11 +156,11 @@ void main() { final stream2 = eg.stream(); final stream3 = eg.stream(); prepare(); - await channelStore.addStreams([stream1, stream2, stream3]); - await channelStore.addSubscription(eg.subscription(stream1)); - await channelStore.addSubscription(eg.subscription(stream2)); - await channelStore.addSubscription(eg.subscription(stream3, isMuted: true)); - await channelStore.addUserTopic(stream1, 'a', UserTopicVisibilityPolicy.muted); + await store.addStreams([stream1, stream2, stream3]); + await store.addSubscription(eg.subscription(stream1)); + await store.addSubscription(eg.subscription(stream2)); + await store.addSubscription(eg.subscription(stream3, isMuted: true)); + await store.addUserTopic(stream1, 'a', UserTopicVisibilityPolicy.muted); fillWithMessages([ eg.streamMessage(stream: stream1, topic: 'a', flags: []), eg.streamMessage(stream: stream1, topic: 'b', flags: []), @@ -178,10 +176,10 @@ void main() { test('countInChannel/Narrow', () async { final stream = eg.stream(); prepare(); - await channelStore.addStream(stream); - await channelStore.addSubscription(eg.subscription(stream)); - await channelStore.addUserTopic(stream, 'a', UserTopicVisibilityPolicy.unmuted); - await channelStore.addUserTopic(stream, 'c', UserTopicVisibilityPolicy.muted); + await store.addStream(stream); + await store.addSubscription(eg.subscription(stream)); + await store.addUserTopic(stream, 'a', UserTopicVisibilityPolicy.unmuted); + await store.addUserTopic(stream, 'c', UserTopicVisibilityPolicy.muted); fillWithMessages([ eg.streamMessage(stream: stream, topic: 'a', flags: []), eg.streamMessage(stream: stream, topic: 'a', flags: []), @@ -193,7 +191,7 @@ void main() { check(model.countInChannel (stream.streamId)).equals(5); check(model.countInChannelNarrow(stream.streamId)).equals(5); - await channelStore.handleEvent(SubscriptionUpdateEvent(id: 1, + await store.handleEvent(SubscriptionUpdateEvent(id: 1, streamId: stream.streamId, property: SubscriptionProperty.isMuted, value: true)); check(model.countInChannel (stream.streamId)).equals(2); @@ -220,7 +218,7 @@ void main() { test('countInMentionsNarrow', () async { final stream = eg.stream(); prepare(); - await channelStore.addStream(stream); + await store.addStream(stream); fillWithMessages([ eg.streamMessage(stream: stream, flags: []), eg.streamMessage(stream: stream, flags: [MessageFlag.mentioned]), @@ -232,7 +230,7 @@ void main() { test('countInStarredMessagesNarrow', () async { final stream = eg.stream(); prepare(); - await channelStore.addStream(stream); + await store.addStream(stream); fillWithMessages([ eg.streamMessage(stream: stream, flags: []), eg.streamMessage(stream: stream, flags: [MessageFlag.starred]), @@ -481,8 +479,8 @@ void main() { Future prepareStore() async { prepare(); - await channelStore.addStream(origChannel); - await channelStore.addSubscription(eg.subscription(origChannel)); + await store.addStream(origChannel); + await store.addSubscription(eg.subscription(origChannel)); readMessages = List.generate(10, (_) => eg.streamMessage(stream: origChannel, topic: origTopic, flags: [MessageFlag.read])); @@ -505,8 +503,8 @@ void main() { test('moved messages = unread messages', () async { await prepareStore(); final newChannel = eg.stream(); - await channelStore.addStream(newChannel); - await channelStore.addSubscription(eg.subscription(newChannel)); + await store.addStream(newChannel); + await store.addSubscription(eg.subscription(newChannel)); fillWithMessages(unreadMessages); final originalMessageIds = model.streams[origChannel.streamId]![TopicName(origTopic)]!; @@ -587,8 +585,8 @@ void main() { test('moving to unsubscribed channels drops the unreads', () async { await prepareStore(); final unsubscribedChannel = eg.stream(); - await channelStore.addStream(unsubscribedChannel); - assert(!channelStore.subscriptions.containsKey( + await store.addStream(unsubscribedChannel); + assert(!store.subscriptions.containsKey( unsubscribedChannel.streamId)); fillWithMessages(unreadMessages); @@ -618,7 +616,7 @@ void main() { fillWithMessages(unreadMessages); final unknownChannel = eg.stream(); - assert(!channelStore.streams.containsKey(unknownChannel.streamId)); + assert(!store.streams.containsKey(unknownChannel.streamId)); final unknownUnreadMessage = eg.streamMessage( stream: unknownChannel, topic: origTopic);