From 16d3961f17d16a60715fea901d529ae149375bcf Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 20 Feb 2024 15:45:59 -0800 Subject: [PATCH 01/15] store [nfc]: Have loadPerAccount take just account ID --- lib/model/store.dart | 15 +++++++-------- test/model/store_test.dart | 12 ++++++------ test/model/test_store.dart | 7 ++++--- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index aafc30aaa7..3912ff96e7 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -111,9 +111,7 @@ abstract class GlobalStore extends ChangeNotifier { } // It's up to us. Start loading. - final account = getAccount(accountId); - assert(account != null, 'Account not found on global store'); - future = loadPerAccount(account!); + future = loadPerAccount(accountId); _perAccountStoresLoading[accountId] = future; store = await future; _setPerAccount(accountId, store); @@ -125,7 +123,7 @@ abstract class GlobalStore extends ChangeNotifier { assert(identical(_accounts[account.id], account)); assert(_perAccountStores.containsKey(account.id)); assert(!_perAccountStoresLoading.containsKey(account.id)); - final store = await loadPerAccount(account); + final store = await loadPerAccount(account.id); _setPerAccount(account.id, store); } @@ -141,7 +139,7 @@ abstract class GlobalStore extends ChangeNotifier { /// This method should be called only by the implementation of [perAccount]. /// Other callers interested in per-account data should use [perAccount] /// and/or [perAccountSync]. - Future loadPerAccount(Account account); + Future loadPerAccount(int accountId); // Just the Iterables, not the actual Map, to avoid clients mutating the map. // Mutations should go through the setters/mutators below. @@ -517,8 +515,8 @@ class LiveGlobalStore extends GlobalStore { final AppDatabase _db; @override - Future loadPerAccount(Account account) async { - final updateMachine = await UpdateMachine.load(this, account); + Future loadPerAccount(int accountId) async { + final updateMachine = await UpdateMachine.load(this, accountId); return updateMachine.store; } @@ -554,7 +552,8 @@ 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(GlobalStore globalStore, Account account) async { + static Future load(GlobalStore globalStore, int accountId) async { + final account = globalStore.getAccount(accountId)!; // TODO test UpdateMachine.load, now that it uses [GlobalStore.apiConnection] final connection = globalStore.apiConnectionFromAccount(account); diff --git a/test/model/store_test.dart b/test/model/store_test.dart index cd8e13f03e..b28772f05b 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -28,7 +28,7 @@ void main() { final accounts = [account1, account2]; final globalStore = LoadingTestGlobalStore(accounts: accounts); List> completers(int accountId) => - globalStore.completers[accounts[accountId - 1]]!; + globalStore.completers[accountId]!; final future1 = globalStore.perAccount(1); final store1 = PerAccountStore.fromInitialSnapshot( @@ -61,7 +61,7 @@ void main() { final accounts = [account1, account2]; final globalStore = LoadingTestGlobalStore(accounts: accounts); List> completers(int accountId) => - globalStore.completers[accounts[accountId - 1]]!; + globalStore.completers[accountId]!; final future1a = globalStore.perAccount(1); final future1b = globalStore.perAccount(1); @@ -94,7 +94,7 @@ void main() { final accounts = [account1, account2]; final globalStore = LoadingTestGlobalStore(accounts: accounts); List> completers(int accountId) => - globalStore.completers[accounts[accountId - 1]]!; + globalStore.completers[accountId]!; check(globalStore.perAccountSync(1)).isNull(); final future1 = globalStore.perAccount(1); @@ -388,12 +388,12 @@ void main() { class LoadingTestGlobalStore extends TestGlobalStore { LoadingTestGlobalStore({required super.accounts}); - Map>> completers = {}; + Map>> completers = {}; @override - Future loadPerAccount(Account account) { + Future loadPerAccount(int accountId) { final completer = Completer(); - (completers[account] ??= []).add(completer); + (completers[accountId] ??= []).add(completer); return completer.future; } } diff --git a/test/model/test_store.dart b/test/model/test_store.dart index e16a84d932..e9f1a23b36 100644 --- a/test/model/test_store.dart +++ b/test/model/test_store.dart @@ -90,14 +90,15 @@ class TestGlobalStore extends GlobalStore { } @override - Future loadPerAccount(Account account) { - final initialSnapshot = _initialSnapshots[account.id]!; + Future loadPerAccount(int accountId) { + final account = getAccount(accountId)!; + final initialSnapshot = _initialSnapshots[accountId]!; final store = PerAccountStore.fromInitialSnapshot( globalStore: this, account: account, initialSnapshot: initialSnapshot, ); - updateMachines[account.id] = UpdateMachine.fromInitialSnapshot( + updateMachines[accountId] = UpdateMachine.fromInitialSnapshot( store: store, initialSnapshot: initialSnapshot); return Future.value(store); } From 75b4f37c3a58f462db048af4edfd1276aa9f09f3 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 5 Jun 2023 11:53:07 -0700 Subject: [PATCH 02/15] store [nfc]: Keep just one copy of Account object We're going to want to update these, when things like the server version or the acked push token change. To avoid endless bugs, it'll then be important that there be just one copy to be updated. --- lib/model/store.dart | 13 ++++++++----- test/example_data.dart | 2 +- test/model/store_test.dart | 10 +++++----- test/model/test_store.dart | 3 +-- 4 files changed, 15 insertions(+), 13 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index 3912ff96e7..bd6cd90be4 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -189,15 +189,16 @@ class PerAccountStore extends ChangeNotifier with StreamStore { /// but it may have already been used for other requests. factory PerAccountStore.fromInitialSnapshot({ required GlobalStore globalStore, - required Account account, + required int accountId, ApiConnection? connection, required InitialSnapshot initialSnapshot, }) { + final account = globalStore.getAccount(accountId)!; connection ??= globalStore.apiConnectionFromAccount(account); final streams = StreamStoreImpl(initialSnapshot: initialSnapshot); return PerAccountStore._( globalStore: globalStore, - account: account, + accountId: accountId, connection: connection, zulipVersion: initialSnapshot.zulipVersion, maxFileUploadSizeMib: initialSnapshot.maxFileUploadSizeMib, @@ -223,7 +224,7 @@ class PerAccountStore extends ChangeNotifier with StreamStore { PerAccountStore._({ required GlobalStore globalStore, - required this.account, + required this.accountId, required this.connection, required this.zulipVersion, required this.maxFileUploadSizeMib, @@ -240,7 +241,9 @@ class PerAccountStore extends ChangeNotifier with StreamStore { final GlobalStore _globalStore; - final Account account; + final int accountId; + Account get account => _globalStore.getAccount(accountId)!; + final ApiConnection connection; // TODO(#135): update zulipFeatureLevel with events // TODO(#135): Keep all this data updated by handling Zulip events from the server. @@ -565,7 +568,7 @@ class UpdateMachine { final store = PerAccountStore.fromInitialSnapshot( globalStore: globalStore, - account: account, + accountId: accountId, connection: connection, initialSnapshot: initialSnapshot, ); diff --git a/test/example_data.dart b/test/example_data.dart index ae3dfcac14..be976bce84 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -494,7 +494,7 @@ PerAccountStore store({Account? account, InitialSnapshot? initialSnapshot}) { final effectiveAccount = account ?? selfAccount; return PerAccountStore.fromInitialSnapshot( globalStore: globalStore(accounts: [effectiveAccount]), - account: effectiveAccount, + accountId: effectiveAccount.id, initialSnapshot: initialSnapshot ?? _initialSnapshot(), ); } diff --git a/test/model/store_test.dart b/test/model/store_test.dart index b28772f05b..e292002bd1 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -33,7 +33,7 @@ void main() { final future1 = globalStore.perAccount(1); final store1 = PerAccountStore.fromInitialSnapshot( globalStore: globalStore, - account: account1, + accountId: 1, initialSnapshot: eg.initialSnapshot(), ); completers(1).single.complete(store1); @@ -44,7 +44,7 @@ void main() { final future2 = globalStore.perAccount(2); final store2 = PerAccountStore.fromInitialSnapshot( globalStore: globalStore, - account: account2, + accountId: 2, initialSnapshot: eg.initialSnapshot(), ); completers(2).single.complete(store2); @@ -71,12 +71,12 @@ void main() { final future2 = globalStore.perAccount(2); final store1 = PerAccountStore.fromInitialSnapshot( globalStore: globalStore, - account: account1, + accountId: 1, initialSnapshot: eg.initialSnapshot(), ); final store2 = PerAccountStore.fromInitialSnapshot( globalStore: globalStore, - account: account2, + accountId: 2, initialSnapshot: eg.initialSnapshot(), ); completers(1).single.complete(store1); @@ -101,7 +101,7 @@ void main() { check(globalStore.perAccountSync(1)).isNull(); final store1 = PerAccountStore.fromInitialSnapshot( globalStore: globalStore, - account: account1, + accountId: 1, initialSnapshot: eg.initialSnapshot(), ); completers(1).single.complete(store1); diff --git a/test/model/test_store.dart b/test/model/test_store.dart index e9f1a23b36..f170f01900 100644 --- a/test/model/test_store.dart +++ b/test/model/test_store.dart @@ -91,11 +91,10 @@ class TestGlobalStore extends GlobalStore { @override Future loadPerAccount(int accountId) { - final account = getAccount(accountId)!; final initialSnapshot = _initialSnapshots[accountId]!; final store = PerAccountStore.fromInitialSnapshot( globalStore: this, - account: account, + accountId: accountId, initialSnapshot: initialSnapshot, ); updateMachines[accountId] = UpdateMachine.fromInitialSnapshot( From 9935f9fbeb7f2ef7f8fb4e30e25dcf4426172cd0 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 20 Feb 2024 15:20:53 -0800 Subject: [PATCH 03/15] store [nfc]: Organize the fields and getters a bit more These have been gradually proliferating, and it's time for another round of giving more structure to the list. Also remove a couple of TODO comments that are better expressed in the tracker; and an admonition about handleEvent that is probably no longer helpful because there are enough of these fields that a developer isn't likely to be looking at it when they go to add another one. --- lib/model/store.dart | 48 ++++++++++++++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index bd6cd90be4..c637eda148 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -198,25 +198,25 @@ class PerAccountStore extends ChangeNotifier with StreamStore { final streams = StreamStoreImpl(initialSnapshot: initialSnapshot); return PerAccountStore._( globalStore: globalStore, - accountId: accountId, connection: connection, zulipVersion: initialSnapshot.zulipVersion, maxFileUploadSizeMib: initialSnapshot.maxFileUploadSizeMib, realmDefaultExternalAccounts: initialSnapshot.realmDefaultExternalAccounts, realmEmoji: initialSnapshot.realmEmoji, customProfileFields: _sortCustomProfileFields(initialSnapshot.customProfileFields), + accountId: accountId, userSettings: initialSnapshot.userSettings, - unreads: Unreads( - initial: initialSnapshot.unreadMsgs, - selfUserId: account.userId, - streamStore: streams, - ), users: Map.fromEntries( initialSnapshot.realmUsers .followedBy(initialSnapshot.realmNonActiveUsers) .followedBy(initialSnapshot.crossRealmBots) .map((user) => MapEntry(user.userId, user))), streams: streams, + unreads: Unreads( + initial: initialSnapshot.unreadMsgs, + selfUserId: account.userId, + streamStore: streams, + ), recentDmConversationsView: RecentDmConversationsView( initial: initialSnapshot.recentPrivateConversations, selfUserId: account.userId), ); @@ -224,44 +224,53 @@ class PerAccountStore extends ChangeNotifier with StreamStore { PerAccountStore._({ required GlobalStore globalStore, - required this.accountId, required this.connection, required this.zulipVersion, required this.maxFileUploadSizeMib, required this.realmDefaultExternalAccounts, required this.realmEmoji, required this.customProfileFields, + required this.accountId, required this.userSettings, - required this.unreads, required this.users, required streams, + required this.unreads, required this.recentDmConversationsView, }) : _globalStore = globalStore, _streams = streams; - final GlobalStore _globalStore; + //////////////////////////////////////////////////////////////// + // Data. - final int accountId; - Account get account => _globalStore.getAccount(accountId)!; + //////////////////////////////// + // Where data comes from in the first place. + final GlobalStore _globalStore; final ApiConnection connection; // TODO(#135): update zulipFeatureLevel with events - // TODO(#135): Keep all this data updated by handling Zulip events from the server. - + //////////////////////////////// // Data attached to the realm or the server. + final String zulipVersion; // TODO get from account; update there on initial snapshot final int maxFileUploadSizeMib; // No event for this. final Map realmDefaultExternalAccounts; Map realmEmoji; List customProfileFields; + //////////////////////////////// // Data attached to the self-account on the realm. + + final int accountId; + Account get account => _globalStore.getAccount(accountId)!; + final UserSettings? userSettings; // TODO(server-5) - final Unreads unreads; + //////////////////////////////// // Users and data about them. + final Map users; + //////////////////////////////// // Streams, topics, and stuff about them. @override @@ -279,7 +288,10 @@ class PerAccountStore extends ChangeNotifier with StreamStore { @visibleForTesting StreamStoreImpl get debugStreamStore => _streams; - // TODO lots more data. When adding, be sure to update handleEvent too. + //////////////////////////////// + // Messages, and summaries of messages. + + final Unreads unreads; final RecentDmConversationsView recentDmConversationsView; @@ -295,8 +307,14 @@ class PerAccountStore extends ChangeNotifier with StreamStore { assert(removed); } + //////////////////////////////// + // Other digests of data. + final AutocompleteViewManager autocompleteViewManager = AutocompleteViewManager(); + // End of data. + //////////////////////////////////////////////////////////////// + /// Called when the app is reassembled during debugging, e.g. for hot reload. /// /// This will redo from scratch any computations we can, such as parsing From e6fa238713e71892d0f01f9781d1b3fa4ebbbddf Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 20 Feb 2024 16:25:16 -0800 Subject: [PATCH 04/15] db [nfc]: Document some fields of Accounts, and Account The field docs get helpfully propagated by Drift from the table class to the corresponding fields on the record class, which is the one most widely found around the codebase. --- lib/model/database.dart | 15 +++++++++++++++ lib/model/database.g.dart | 15 +++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/lib/model/database.dart b/lib/model/database.dart index fb13b3e6e2..bd8f243d7f 100644 --- a/lib/model/database.dart +++ b/lib/model/database.dart @@ -7,10 +7,25 @@ import 'package:path_provider/path_provider.dart'; part 'database.g.dart'; +/// The table of [Account] records in the app's database. class Accounts extends Table { + /// The ID of this account in the app's local database. + /// + /// This uniquely identifies the account within this install of the app, + /// and never changes for a given account. It has no meaning to the server, + /// though, or anywhere else outside this install of the app. Column get id => integer().autoIncrement()(); + /// The URL of the Zulip realm this account is on. + /// + /// This corresponds to [GetServerSettingsResult.realmUrl]. + /// It never changes for a given account. Column get realmUrl => text().map(const UriConverter())(); + + /// The Zulip user ID of this account. + /// + /// This is the identifier the server uses for the account. + /// It never changes for a given account. Column get userId => integer()(); Column get email => text()(); diff --git a/lib/model/database.g.dart b/lib/model/database.g.dart index d493eafae6..52d9c00c0c 100644 --- a/lib/model/database.g.dart +++ b/lib/model/database.g.dart @@ -182,8 +182,23 @@ class $AccountsTable extends Accounts with TableInfo<$AccountsTable, Account> { } class Account extends DataClass implements Insertable { + /// The ID of this account in the app's local database. + /// + /// This uniquely identifies the account within this install of the app, + /// and never changes for a given account. It has no meaning to the server, + /// though, or anywhere else outside this install of the app. final int id; + + /// The URL of the Zulip realm this account is on. + /// + /// This corresponds to [GetServerSettingsResult.realmUrl]. + /// It never changes for a given account. final Uri realmUrl; + + /// The Zulip user ID of this account. + /// + /// This is the identifier the server uses for the account. + /// It never changes for a given account. final int userId; final String email; final String apiKey; From ab10cf3f6a6fb2beb26e1d823be05f63bd88ac9e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 20 Feb 2024 15:06:19 -0800 Subject: [PATCH 05/15] store [nfc]: Add direct selfUserId field on PerAccountStore This makes use sites a bit shorter and more explicit. We could have made it a getter, but storing the answer directly here on the PerAccountStore is a space-for-time tradeoff: this is something we look up frequently, so the extra word of memory to have a copy directly here seems worth it to save the hash-map lookup each time. --- lib/model/compose.dart | 4 ++-- lib/model/internal_link.dart | 2 +- lib/model/store.dart | 8 +++++++- lib/widgets/action_sheet.dart | 3 +-- lib/widgets/emoji_reaction.dart | 5 ++--- lib/widgets/inbox.dart | 2 +- lib/widgets/message_list.dart | 4 ++-- lib/widgets/profile.dart | 2 +- lib/widgets/recent_dm_conversations.dart | 2 +- 9 files changed, 18 insertions(+), 14 deletions(-) diff --git a/lib/model/compose.dart b/lib/model/compose.dart index ec8fdcb3dd..b59a3efcc7 100644 --- a/lib/model/compose.dart +++ b/lib/model/compose.dart @@ -142,7 +142,7 @@ String quoteAndReplyPlaceholder(PerAccountStore store, { final sender = store.users[message.senderId]; assert(sender != null); final url = narrowLink(store, - SendableNarrow.ofMessage(message, selfUserId: store.account.userId), + SendableNarrow.ofMessage(message, selfUserId: store.selfUserId), nearMessageId: message.id); // See note in [quoteAndReply] about asking `mention` to omit the | part. return '${mention(sender!, silent: true)} ${inlineLink('said', url)}: ' // TODO(i18n) ? @@ -164,7 +164,7 @@ String quoteAndReply(PerAccountStore store, { final sender = store.users[message.senderId]; assert(sender != null); final url = narrowLink(store, - SendableNarrow.ofMessage(message, selfUserId: store.account.userId), + SendableNarrow.ofMessage(message, selfUserId: store.selfUserId), nearMessageId: message.id); // Could ask `mention` to omit the | part unless the mention is ambiguous… // but that would mean a linear scan through all users, and the extra noise diff --git a/lib/model/internal_link.dart b/lib/model/internal_link.dart index 15fc0cbeb7..ef50964a05 100644 --- a/lib/model/internal_link.dart +++ b/lib/model/internal_link.dart @@ -197,7 +197,7 @@ Narrow? _interpretNarrowSegments(List segments, PerAccountStore store) { if (dmElement != null) { if (streamElement != null || topicElement != null) return null; - return DmNarrow.withUsers(dmElement.operand, selfUserId: store.account.userId); + return DmNarrow.withUsers(dmElement.operand, selfUserId: store.selfUserId); } else if (streamElement != null) { final streamId = streamElement.operand; if (topicElement != null) { diff --git a/lib/model/store.dart b/lib/model/store.dart index c637eda148..3a8eee12f0 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -205,6 +205,7 @@ class PerAccountStore extends ChangeNotifier with StreamStore { realmEmoji: initialSnapshot.realmEmoji, customProfileFields: _sortCustomProfileFields(initialSnapshot.customProfileFields), accountId: accountId, + selfUserId: account.userId, userSettings: initialSnapshot.userSettings, users: Map.fromEntries( initialSnapshot.realmUsers @@ -231,12 +232,14 @@ class PerAccountStore extends ChangeNotifier with StreamStore { required this.realmEmoji, required this.customProfileFields, required this.accountId, + required this.selfUserId, required this.userSettings, required this.users, required streams, required this.unreads, required this.recentDmConversationsView, - }) : _globalStore = globalStore, + }) : assert(selfUserId == globalStore.getAccount(accountId)!.userId), + _globalStore = globalStore, _streams = streams; //////////////////////////////////////////////////////////////// @@ -263,6 +266,9 @@ class PerAccountStore extends ChangeNotifier with StreamStore { final int accountId; Account get account => _globalStore.getAccount(accountId)!; + /// Always equal to `account.userId`. + final int selfUserId; + final UserSettings? userSettings; // TODO(server-5) //////////////////////////////// diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart index 7ca86d3779..ba03831fe2 100644 --- a/lib/widgets/action_sheet.dart +++ b/lib/widgets/action_sheet.dart @@ -26,12 +26,11 @@ void showMessageActionSheet({required BuildContext context, required Message mes // any message list, so that's fine. final isComposeBoxOffered = MessageListPage.composeBoxControllerOf(context) != null; - final selfUserId = store.account.userId; final hasThumbsUpReactionVote = message.reactions ?.aggregated.any((reactionWithVotes) => reactionWithVotes.reactionType == ReactionType.unicodeEmoji && reactionWithVotes.emojiCode == '1f44d' - && reactionWithVotes.userIds.contains(selfUserId)) + && reactionWithVotes.userIds.contains(store.selfUserId)) ?? false; showDraggableScrollableModalBottomSheet( diff --git a/lib/widgets/emoji_reaction.dart b/lib/widgets/emoji_reaction.dart index 947cd226c1..a4dd8436cb 100644 --- a/lib/widgets/emoji_reaction.dart +++ b/lib/widgets/emoji_reaction.dart @@ -73,14 +73,13 @@ class ReactionChip extends StatelessWidget { final emojiset = store.userSettings?.emojiset ?? Emojiset.google; - final selfUserId = store.account.userId; - final selfVoted = userIds.contains(selfUserId); + final selfVoted = userIds.contains(store.selfUserId); final label = showName // TODO(i18n): List formatting, like you can do in JavaScript: // new Intl.ListFormat('ja').format(['Chris', 'Greg', 'Alya', 'Shu']) // // 'Chris、Greg、Alya、Shu' ? userIds.map((id) { - return id == selfUserId + return id == store.selfUserId ? 'You' : store.users[id]?.fullName ?? '(unknown user)'; // TODO(i18n) }).join(', ') diff --git a/lib/widgets/inbox.dart b/lib/widgets/inbox.dart index a1e706946f..8aa5172efc 100644 --- a/lib/widgets/inbox.dart +++ b/lib/widgets/inbox.dart @@ -335,7 +335,7 @@ class _DmItem extends StatelessWidget { @override Widget build(BuildContext context) { final store = PerAccountStoreWidget.of(context); - final selfUser = store.users[store.account.userId]!; + final selfUser = store.users[store.selfUserId]!; final title = switch (narrow.otherRecipientIds) { // TODO dedupe with [RecentDmConversationsItem] [] => selfUser.fullName, diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index a957b29cfc..3a5ca5fae5 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -745,7 +745,7 @@ class DmRecipientHeader extends StatelessWidget { final String title; if (message.allRecipientIds.length > 1) { title = zulipLocalizations.messageListGroupYouAndOthers(message.allRecipientIds - .where((id) => id != store.account.userId) + .where((id) => id != store.selfUserId) .map((id) => store.users[id]?.fullName ?? zulipLocalizations.unknownUserName) .sorted() .join(", ")); @@ -757,7 +757,7 @@ class DmRecipientHeader extends StatelessWidget { return GestureDetector( onTap: () => Navigator.push(context, MessageListPage.buildRoute(context: context, - narrow: DmNarrow.ofMessage(message, selfUserId: store.account.userId))), + narrow: DmNarrow.ofMessage(message, selfUserId: store.selfUserId))), child: ColoredBox( color: _kDmRecipientHeaderColor, child: Padding( diff --git a/lib/widgets/profile.dart b/lib/widgets/profile.dart index 055772017e..6a46e5779d 100644 --- a/lib/widgets/profile.dart +++ b/lib/widgets/profile.dart @@ -57,7 +57,7 @@ class ProfilePage extends StatelessWidget { FilledButton.icon( onPressed: () => Navigator.push(context, MessageListPage.buildRoute(context: context, - narrow: DmNarrow.withUser(userId, selfUserId: store.account.userId))), + narrow: DmNarrow.withUser(userId, selfUserId: store.selfUserId))), icon: const Icon(Icons.email), label: Text(zulipLocalizations.profileButtonSendDirectMessage)), ]; diff --git a/lib/widgets/recent_dm_conversations.dart b/lib/widgets/recent_dm_conversations.dart index dac1d638c5..aafdc8c42f 100644 --- a/lib/widgets/recent_dm_conversations.dart +++ b/lib/widgets/recent_dm_conversations.dart @@ -87,7 +87,7 @@ class RecentDmConversationsItem extends StatelessWidget { @override Widget build(BuildContext context) { final store = PerAccountStoreWidget.of(context); - final selfUser = store.users[store.account.userId]!; + final selfUser = store.users[store.selfUserId]!; final String title; final Widget avatar; From 83efdb9b785eb2a5445b462cad8e20a0c47bd2b8 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 20 Feb 2024 15:27:38 -0800 Subject: [PATCH 06/15] store [nfc]: Add direct realmUrl field on PerAccountStore --- lib/model/internal_link.dart | 4 ++-- lib/model/store.dart | 7 +++++++ lib/widgets/app.dart | 2 +- lib/widgets/content.dart | 2 +- lib/widgets/emoji_reaction.dart | 2 +- test/model/compose_test.dart | 10 +++++----- 6 files changed, 17 insertions(+), 10 deletions(-) diff --git a/lib/model/internal_link.dart b/lib/model/internal_link.dart index ef50964a05..f0ecc69dbf 100644 --- a/lib/model/internal_link.dart +++ b/lib/model/internal_link.dart @@ -95,7 +95,7 @@ Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) { fragment.write('/near/$nearMessageId'); } - return store.account.realmUrl.replace(fragment: fragment.toString()); + return store.realmUrl.replace(fragment: fragment.toString()); } /// Create a new `Uri` object in relation to a given realmUrl. @@ -124,7 +124,7 @@ Uri? tryResolveOnRealmUrl(String urlString, Uri realmUrl) { /// #narrow/stream/1-announce/stream/1-announce (duplicated operator) // TODO(#252): handle all valid narrow links, returning a search narrow Narrow? parseInternalLink(Uri url, PerAccountStore store) { - if (!_isInternalLink(url, store.account.realmUrl)) return null; + if (!_isInternalLink(url, store.realmUrl)) return null; final (category, segments) = _getCategoryAndSegmentsFromFragment(url.fragment); switch (category) { diff --git a/lib/model/store.dart b/lib/model/store.dart index 3a8eee12f0..94b698438c 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -199,6 +199,7 @@ class PerAccountStore extends ChangeNotifier with StreamStore { return PerAccountStore._( globalStore: globalStore, connection: connection, + realmUrl: account.realmUrl, zulipVersion: initialSnapshot.zulipVersion, maxFileUploadSizeMib: initialSnapshot.maxFileUploadSizeMib, realmDefaultExternalAccounts: initialSnapshot.realmDefaultExternalAccounts, @@ -226,6 +227,7 @@ class PerAccountStore extends ChangeNotifier with StreamStore { PerAccountStore._({ required GlobalStore globalStore, required this.connection, + required this.realmUrl, required this.zulipVersion, required this.maxFileUploadSizeMib, required this.realmDefaultExternalAccounts, @@ -239,6 +241,8 @@ class PerAccountStore extends ChangeNotifier with StreamStore { required this.unreads, required this.recentDmConversationsView, }) : assert(selfUserId == globalStore.getAccount(accountId)!.userId), + assert(realmUrl == globalStore.getAccount(accountId)!.realmUrl), + assert(realmUrl == connection.realmUrl), _globalStore = globalStore, _streams = streams; @@ -254,6 +258,9 @@ class PerAccountStore extends ChangeNotifier with StreamStore { //////////////////////////////// // Data attached to the realm or the server. + /// Always equal to `account.realmUrl` and `connection.realmUrl`. + final Uri realmUrl; + final String zulipVersion; // TODO get from account; update there on initial snapshot final int maxFileUploadSizeMib; // No event for this. final Map realmDefaultExternalAccounts; diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index a153125e49..75c7fd90e2 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -265,7 +265,7 @@ class HomePage extends StatelessWidget { const SizedBox(height: 12), Text.rich(TextSpan( text: 'Connected to: ', - children: [bold(store.account.realmUrl.toString())])), + children: [bold(store.realmUrl.toString())])), Text.rich(TextSpan( text: 'Zulip server version: ', children: [bold(store.zulipVersion)])), diff --git a/lib/widgets/content.dart b/lib/widgets/content.dart index e2a45923eb..d20fba567f 100644 --- a/lib/widgets/content.dart +++ b/lib/widgets/content.dart @@ -786,7 +786,7 @@ void _launchUrl(BuildContext context, String urlString) async { } final store = PerAccountStoreWidget.of(context); - final url = tryResolveOnRealmUrl(urlString, store.account.realmUrl); + final url = tryResolveOnRealmUrl(urlString, store.realmUrl); if (url == null) { // TODO(log) await showError(context, null); return; diff --git a/lib/widgets/emoji_reaction.dart b/lib/widgets/emoji_reaction.dart index a4dd8436cb..ba7fc7a4a4 100644 --- a/lib/widgets/emoji_reaction.dart +++ b/lib/widgets/emoji_reaction.dart @@ -319,7 +319,7 @@ class _ImageEmoji extends StatelessWidget { if (parsedSrc == null) { // TODO(log) return _textFallback; } - final resolved = store.account.realmUrl.resolveUri(parsedSrc); + final resolved = store.realmUrl.resolveUri(parsedSrc); // Unicode and text emoji get scaled; it would look weird if image emoji didn't. final size = _squareEmojiScalerClamped(context).scale(_squareEmojiSize); diff --git a/test/model/compose_test.dart b/test/model/compose_test.dart index ad68a70d10..202a36050e 100644 --- a/test/model/compose_test.dart +++ b/test/model/compose_test.dart @@ -225,9 +225,9 @@ hello test('AllMessagesNarrow', () { final store = eg.store(); check(narrowLink(store, const AllMessagesNarrow())) - .equals(store.account.realmUrl.resolve('#narrow')); + .equals(store.realmUrl.resolve('#narrow')); check(narrowLink(store, const AllMessagesNarrow(), nearMessageId: 1)) - .equals(store.account.realmUrl.resolve('#narrow/near/1')); + .equals(store.realmUrl.resolve('#narrow/near/1')); }); test('StreamNarrow / TopicNarrow', () { @@ -244,7 +244,7 @@ hello ? StreamNarrow(streamId) : TopicNarrow(streamId, topic); check(narrowLink(store, narrow, nearMessageId: nearMessageId)) - .equals(store.account.realmUrl.resolve(expectedFragment)); + .equals(store.realmUrl.resolve(expectedFragment)); } checkNarrow(streamId: 1, name: 'announce', '#narrow/stream/1-announce'); @@ -275,10 +275,10 @@ hello final store = eg.store(); final narrow = DmNarrow(allRecipientIds: allRecipientIds, selfUserId: selfUserId); check(narrowLink(store, narrow, nearMessageId: nearMessageId)) - .equals(store.account.realmUrl.resolve(expectedFragment)); + .equals(store.realmUrl.resolve(expectedFragment)); store.connection.zulipFeatureLevel = 176; check(narrowLink(store, narrow, nearMessageId: nearMessageId)) - .equals(store.account.realmUrl.resolve(legacyExpectedFragment)); + .equals(store.realmUrl.resolve(legacyExpectedFragment)); } checkNarrow(allRecipientIds: [1], selfUserId: 1, From 71a81f78ae611cbfcf6483deee86181ff51c39ab Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 20 Feb 2024 16:45:25 -0800 Subject: [PATCH 07/15] url [nfc]: Factor out a general tryResolveUrl, and one on the store --- lib/model/internal_link.dart | 14 ++------------ lib/model/store.dart | 15 +++++++++++++++ lib/widgets/content.dart | 2 +- test/model/internal_link_test.dart | 4 ++-- 4 files changed, 20 insertions(+), 15 deletions(-) diff --git a/lib/model/internal_link.dart b/lib/model/internal_link.dart index f0ecc69dbf..91cb1c6df1 100644 --- a/lib/model/internal_link.dart +++ b/lib/model/internal_link.dart @@ -98,20 +98,10 @@ Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) { return store.realmUrl.replace(fragment: fragment.toString()); } -/// Create a new `Uri` object in relation to a given realmUrl. -/// -/// Returns `null` if `urlString` could not be parsed as a `Uri`. -Uri? tryResolveOnRealmUrl(String urlString, Uri realmUrl) { - try { - return realmUrl.resolve(urlString); - } on FormatException { - return null; - } -} - /// A [Narrow] from a given URL, on `store`'s realm. /// -/// `url` must already be passed through [tryResolveOnRealmUrl]. +/// `url` must already be a result from [PerAccountStore.tryResolveUrl] +/// on `store`. /// /// Returns `null` if any of the operator/operand pairs are invalid. /// diff --git a/lib/model/store.dart b/lib/model/store.dart index 94b698438c..7d66bda06a 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -261,6 +261,11 @@ class PerAccountStore extends ChangeNotifier with StreamStore { /// 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. + Uri? tryResolveUrl(String reference) => _tryResolveUrl(realmUrl, reference); + final String zulipVersion; // TODO get from account; update there on initial snapshot final int maxFileUploadSizeMib; // No event for this. final Map realmDefaultExternalAccounts; @@ -496,6 +501,16 @@ class PerAccountStore extends ChangeNotifier with StreamStore { } const _apiSendMessage = sendMessage; // Bit ugly; for alternatives, see: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20PerAccountStore.20methods/near/1545809 +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 [GlobalStore] that uses a live server and live, persistent local database. /// diff --git a/lib/widgets/content.dart b/lib/widgets/content.dart index d20fba567f..f52b62f206 100644 --- a/lib/widgets/content.dart +++ b/lib/widgets/content.dart @@ -786,7 +786,7 @@ void _launchUrl(BuildContext context, String urlString) async { } final store = PerAccountStoreWidget.of(context); - final url = tryResolveOnRealmUrl(urlString, store.realmUrl); + final url = store.tryResolveUrl(urlString); if (url == null) { // TODO(log) await showError(context, null); return; diff --git a/test/model/internal_link_test.dart b/test/model/internal_link_test.dart index c80c41ca76..e06c8590af 100644 --- a/test/model/internal_link_test.dart +++ b/test/model/internal_link_test.dart @@ -38,7 +38,7 @@ void main() { store ??= setupStore(realmUrl: realmUrl, streams: streams, users: users); for (final testCase in testCases) { final String urlString = testCase.$1; - final Uri url = tryResolveOnRealmUrl(urlString, realmUrl)!; + final Uri url = store.tryResolveUrl(urlString)!; final Narrow? expected = testCase.$2; test(urlString, () { check(parseInternalLink(url, store!)).equals(expected); @@ -134,7 +134,7 @@ void main() { final Uri realmUrl = testCase.$4; test('${expected ? 'accepts': 'rejects'} $description: $urlString', () { final store = setupStore(realmUrl: realmUrl, streams: streams); - final url = tryResolveOnRealmUrl(urlString, realmUrl)!; + final url = store.tryResolveUrl(urlString)!; final result = parseInternalLink(url, store); check(result != null).equals(expected); }); From 87c17bbca83e64226d11b740babe883b5d9298bd Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 20 Feb 2024 17:09:01 -0800 Subject: [PATCH 08/15] content test [nfc]: Fix name and ordering of MessageImage tests --- test/widgets/content_test.dart | 186 ++++++++++++++++----------------- 1 file changed, 93 insertions(+), 93 deletions(-) diff --git a/test/widgets/content_test.dart b/test/widgets/content_test.dart index 0f130f9229..4a7d78f215 100644 --- a/test/widgets/content_test.dart +++ b/test/widgets/content_test.dart @@ -73,6 +73,99 @@ void main() { testContentSmoke(ContentExample.quotation); + group('MessageImage, MessageImageList', () { + final message = eg.streamMessage(); + + Future prepareContent(WidgetTester tester, String html) async { + addTearDown(testBinding.reset); + + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + final httpClient = FakeImageHttpClient(); + + debugNetworkImageHttpClientProvider = () => httpClient; + httpClient.request.response + ..statusCode = HttpStatus.ok + ..content = kSolidBlueAvatar; + + await tester.pumpWidget( + MaterialApp( + home: Directionality( + textDirection: TextDirection.ltr, + child: GlobalStoreWidget( + child: PerAccountStoreWidget( + accountId: eg.selfAccount.id, + child: MessageContent( + message: message, + content: parseContent(html))))))); + await tester.pump(); // global store + await tester.pump(); // per-account store + debugNetworkImageHttpClientProvider = null; + } + + testWidgets('single image', (tester) async { + const example = ContentExample.imageSingle; + await prepareContent(tester, example.html); + final expectedImages = (example.expectedNodes[0] as ImageNodeList).images; + final images = tester.widgetList( + find.byType(RealmContentNetworkImage)); + check(images.map((i) => i.src.toString()).toList()) + .deepEquals(expectedImages.map((n) => n.srcUrl)); + }); + + testWidgets('multiple images', (tester) async { + const example = ContentExample.imageCluster; + await prepareContent(tester, example.html); + final expectedImages = (example.expectedNodes[1] as ImageNodeList).images; + final images = tester.widgetList( + find.byType(RealmContentNetworkImage)); + check(images.map((i) => i.src.toString()).toList()) + .deepEquals(expectedImages.map((n) => n.srcUrl)); + }); + + testWidgets('content after image cluster', (tester) async { + const example = ContentExample.imageClusterThenContent; + await prepareContent(tester, example.html); + final expectedImages = (example.expectedNodes[1] as ImageNodeList).images; + final images = tester.widgetList( + find.byType(RealmContentNetworkImage)); + check(images.map((i) => i.src.toString()).toList()) + .deepEquals(expectedImages.map((n) => n.srcUrl)); + }); + + testWidgets('multiple clusters of images', (tester) async { + const example = ContentExample.imageMultipleClusters; + await prepareContent(tester, example.html); + final expectedImages = (example.expectedNodes[1] as ImageNodeList).images + + (example.expectedNodes[4] as ImageNodeList).images; + final images = tester.widgetList( + find.byType(RealmContentNetworkImage)); + check(images.map((i) => i.src.toString()).toList()) + .deepEquals(expectedImages.map((n) => n.srcUrl)); + }); + + testWidgets('image as immediate child in implicit paragraph', (tester) async { + const example = ContentExample.imageInImplicitParagraph; + await prepareContent(tester, example.html); + final expectedImages = ((example.expectedNodes[0] as ListNode) + .items[0][0] as ImageNodeList).images; + final images = tester.widgetList( + find.byType(RealmContentNetworkImage)); + check(images.map((i) => i.src.toString()).toList()) + .deepEquals(expectedImages.map((n) => n.srcUrl)); + }); + + testWidgets('image cluster in implicit paragraph', (tester) async { + const example = ContentExample.imageClusterInImplicitParagraph; + await prepareContent(tester, example.html); + final expectedImages = ((example.expectedNodes[0] as ListNode) + .items[0][1] as ImageNodeList).images; + final images = tester.widgetList( + find.byType(RealmContentNetworkImage)); + check(images.map((i) => i.src.toString()).toList()) + .deepEquals(expectedImages.map((n) => n.srcUrl)); + }); + }); + group("CodeBlock", () { testContentSmoke(ContentExample.codeBlockPlain); testContentSmoke(ContentExample.codeBlockHighlightedShort); @@ -252,99 +345,6 @@ void main() { tester.widget(find.textContaining(RegExp(r'^(Tue, Jan 30|Wed, Jan 31), 2024, \d+:\d\d [AP]M$'))); }); - group('MessageImages', () { - final message = eg.streamMessage(); - - Future prepareContent(WidgetTester tester, String html) async { - addTearDown(testBinding.reset); - - await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); - final httpClient = FakeImageHttpClient(); - - debugNetworkImageHttpClientProvider = () => httpClient; - httpClient.request.response - ..statusCode = HttpStatus.ok - ..content = kSolidBlueAvatar; - - await tester.pumpWidget( - MaterialApp( - home: Directionality( - textDirection: TextDirection.ltr, - child: GlobalStoreWidget( - child: PerAccountStoreWidget( - accountId: eg.selfAccount.id, - child: MessageContent( - message: message, - content: parseContent(html))))))); - await tester.pump(); // global store - await tester.pump(); // per-account store - debugNetworkImageHttpClientProvider = null; - } - - testWidgets('single image', (tester) async { - const example = ContentExample.imageSingle; - await prepareContent(tester, example.html); - final expectedImages = (example.expectedNodes[0] as ImageNodeList).images; - final images = tester.widgetList( - find.byType(RealmContentNetworkImage)); - check(images.map((i) => i.src.toString()).toList()) - .deepEquals(expectedImages.map((n) => n.srcUrl)); - }); - - testWidgets('multiple images', (tester) async { - const example = ContentExample.imageCluster; - await prepareContent(tester, example.html); - final expectedImages = (example.expectedNodes[1] as ImageNodeList).images; - final images = tester.widgetList( - find.byType(RealmContentNetworkImage)); - check(images.map((i) => i.src.toString()).toList()) - .deepEquals(expectedImages.map((n) => n.srcUrl)); - }); - - testWidgets('content after image cluster', (tester) async { - const example = ContentExample.imageClusterThenContent; - await prepareContent(tester, example.html); - final expectedImages = (example.expectedNodes[1] as ImageNodeList).images; - final images = tester.widgetList( - find.byType(RealmContentNetworkImage)); - check(images.map((i) => i.src.toString()).toList()) - .deepEquals(expectedImages.map((n) => n.srcUrl)); - }); - - testWidgets('multiple clusters of images', (tester) async { - const example = ContentExample.imageMultipleClusters; - await prepareContent(tester, example.html); - final expectedImages = (example.expectedNodes[1] as ImageNodeList).images - + (example.expectedNodes[4] as ImageNodeList).images; - final images = tester.widgetList( - find.byType(RealmContentNetworkImage)); - check(images.map((i) => i.src.toString()).toList()) - .deepEquals(expectedImages.map((n) => n.srcUrl)); - }); - - testWidgets('image as immediate child in implicit paragraph', (tester) async { - const example = ContentExample.imageInImplicitParagraph; - await prepareContent(tester, example.html); - final expectedImages = ((example.expectedNodes[0] as ListNode) - .items[0][0] as ImageNodeList).images; - final images = tester.widgetList( - find.byType(RealmContentNetworkImage)); - check(images.map((i) => i.src.toString()).toList()) - .deepEquals(expectedImages.map((n) => n.srcUrl)); - }); - - testWidgets('image cluster in implicit paragraph', (tester) async { - const example = ContentExample.imageClusterInImplicitParagraph; - await prepareContent(tester, example.html); - final expectedImages = ((example.expectedNodes[0] as ListNode) - .items[0][1] as ImageNodeList).images; - final images = tester.widgetList( - find.byType(RealmContentNetworkImage)); - check(images.map((i) => i.src.toString()).toList()) - .deepEquals(expectedImages.map((n) => n.srcUrl)); - }); - }); - group('RealmContentNetworkImage', () { final authHeaders = authHeader(email: eg.selfAccount.email, apiKey: eg.selfAccount.apiKey); From 037295990ead2cee9b3635de12c8da832bb58108 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 20 Feb 2024 17:31:08 -0800 Subject: [PATCH 09/15] content test: Fix bogus data in message-images tests, and simplify The `message` passed to MessageContent should have the same content that was parsed to produce `content`. In the app we don't insert a Directionality widget of our own, and the GlobalStoreWidget is outside the MaterialApp, not inside. --- test/widgets/content_test.dart | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/test/widgets/content_test.dart b/test/widgets/content_test.dart index 4a7d78f215..3192c2c851 100644 --- a/test/widgets/content_test.dart +++ b/test/widgets/content_test.dart @@ -74,8 +74,6 @@ void main() { testContentSmoke(ContentExample.quotation); group('MessageImage, MessageImageList', () { - final message = eg.streamMessage(); - Future prepareContent(WidgetTester tester, String html) async { addTearDown(testBinding.reset); @@ -87,16 +85,11 @@ void main() { ..statusCode = HttpStatus.ok ..content = kSolidBlueAvatar; - await tester.pumpWidget( - MaterialApp( - home: Directionality( - textDirection: TextDirection.ltr, - child: GlobalStoreWidget( - child: PerAccountStoreWidget( - accountId: eg.selfAccount.id, - child: MessageContent( - message: message, - content: parseContent(html))))))); + await tester.pumpWidget(GlobalStoreWidget(child: MaterialApp( + home: PerAccountStoreWidget(accountId: eg.selfAccount.id, + child: MessageContent( + message: eg.streamMessage(content: html), + content: parseContent(html)))))); await tester.pump(); // global store await tester.pump(); // per-account store debugNetworkImageHttpClientProvider = null; From d5058bb8979112100d3d1375284c789a11527d80 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 20 Feb 2024 17:24:47 -0800 Subject: [PATCH 10/15] content test [nfc]: Factor out prepareBoringImageHttpClient --- test/widgets/content_test.dart | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/test/widgets/content_test.dart b/test/widgets/content_test.dart index 3192c2c851..8ee7376f7c 100644 --- a/test/widgets/content_test.dart +++ b/test/widgets/content_test.dart @@ -55,6 +55,21 @@ void main() { }); } + /// Set [debugNetworkImageHttpClientProvider] to return a constant image. + /// + /// Returns the [FakeImageHttpClient] that handles the requests. + /// + /// The caller must set [debugNetworkImageHttpClientProvider] back to null + /// before the end of the test. + FakeImageHttpClient prepareBoringImageHttpClient() { + final httpClient = FakeImageHttpClient(); + debugNetworkImageHttpClientProvider = () => httpClient; + httpClient.request.response + ..statusCode = HttpStatus.ok + ..content = kSolidBlueAvatar; + return httpClient; + } + group('Heading', () { testWidgets('plain h6', (tester) async { await prepareContentBare(tester, @@ -76,14 +91,8 @@ void main() { group('MessageImage, MessageImageList', () { Future prepareContent(WidgetTester tester, String html) async { addTearDown(testBinding.reset); - await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); - final httpClient = FakeImageHttpClient(); - - debugNetworkImageHttpClientProvider = () => httpClient; - httpClient.request.response - ..statusCode = HttpStatus.ok - ..content = kSolidBlueAvatar; + prepareBoringImageHttpClient(); await tester.pumpWidget(GlobalStoreWidget(child: MaterialApp( home: PerAccountStoreWidget(accountId: eg.selfAccount.id, @@ -342,14 +351,10 @@ void main() { final authHeaders = authHeader(email: eg.selfAccount.email, apiKey: eg.selfAccount.apiKey); Future>> actualHeaders(WidgetTester tester, Uri src) async { - await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); addTearDown(testBinding.reset); + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); - final httpClient = FakeImageHttpClient(); - debugNetworkImageHttpClientProvider = () => httpClient; - httpClient.request.response - ..statusCode = HttpStatus.ok - ..content = kSolidBlueAvatar; + final httpClient = prepareBoringImageHttpClient(); await tester.pumpWidget(GlobalStoreWidget( child: PerAccountStoreWidget(accountId: eg.selfAccount.id, From ed256dc80b4dc7fc6ceeb45ab8a010bf844bba71 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 20 Feb 2024 17:42:02 -0800 Subject: [PATCH 11/15] content test: Test MessageImageEmoji widget --- test/widgets/content_test.dart | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/test/widgets/content_test.dart b/test/widgets/content_test.dart index 8ee7376f7c..8521f4dc75 100644 --- a/test/widgets/content_test.dart +++ b/test/widgets/content_test.dart @@ -347,6 +347,32 @@ void main() { tester.widget(find.textContaining(RegExp(r'^(Tue, Jan 30|Wed, Jan 31), 2024, \d+:\d\d [AP]M$'))); }); + group('MessageImageEmoji', () { + Future prepareContent(WidgetTester tester, String html) async { + addTearDown(testBinding.reset); + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + prepareBoringImageHttpClient(); + + await tester.pumpWidget(GlobalStoreWidget(child: MaterialApp( + home: PerAccountStoreWidget(accountId: eg.selfAccount.id, + child: BlockContentList(nodes: parseContent(html).nodes))))); + await tester.pump(); // global store + await tester.pump(); // per-account store + } + + testWidgets('smoke: custom emoji', (tester) async { + await prepareContent(tester, ContentExample.emojiCustom.html); + tester.widget(find.byType(MessageImageEmoji)); + debugNetworkImageHttpClientProvider = null; + }); + + testWidgets('smoke: Zulip extra emoji', (tester) async { + await prepareContent(tester, ContentExample.emojiZulipExtra.html); + tester.widget(find.byType(MessageImageEmoji)); + debugNetworkImageHttpClientProvider = null; + }); + }); + group('RealmContentNetworkImage', () { final authHeaders = authHeader(email: eg.selfAccount.email, apiKey: eg.selfAccount.apiKey); From 962ca5ec881d6d342c474506ef68e70a728e6ad4 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 20 Feb 2024 17:57:49 -0800 Subject: [PATCH 12/15] content test: Test AvatarImage widget --- test/widgets/content_test.dart | 40 ++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/test/widgets/content_test.dart b/test/widgets/content_test.dart index 8521f4dc75..aba2e97ff5 100644 --- a/test/widgets/content_test.dart +++ b/test/widgets/content_test.dart @@ -8,6 +8,7 @@ import 'package:url_launcher/url_launcher.dart'; import 'package:zulip/api/core.dart'; import 'package:zulip/model/content.dart'; import 'package:zulip/model/narrow.dart'; +import 'package:zulip/model/store.dart'; import 'package:zulip/widgets/content.dart'; import 'package:zulip/widgets/message_list.dart'; import 'package:zulip/widgets/page.dart'; @@ -16,6 +17,8 @@ import 'package:zulip/widgets/store.dart'; import '../example_data.dart' as eg; import '../model/binding.dart'; import '../model/content_test.dart'; +import '../model/test_store.dart'; +import '../stdlib_checks.dart'; import '../test_images.dart'; import '../test_navigation.dart'; import 'dialog_checks.dart'; @@ -412,4 +415,41 @@ void main() { check(tester.takeException()).isA(); }); }); + + group('AvatarImage', () { + late PerAccountStore store; + + Future actualUrl(WidgetTester tester, String avatarUrl) async { + addTearDown(testBinding.reset); + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + store = await testBinding.globalStore.perAccount(eg.selfAccount.id); + final user = eg.user(avatarUrl: avatarUrl); + store.addUser(user); + + prepareBoringImageHttpClient(); + await tester.pumpWidget(GlobalStoreWidget( + child: PerAccountStoreWidget(accountId: eg.selfAccount.id, + child: AvatarImage(userId: user.userId)))); + await tester.pump(); + await tester.pump(); + tester.widget(find.byType(AvatarImage)); + final widgets = tester.widgetList( + find.byType(RealmContentNetworkImage)); + return widgets.firstOrNull?.src; + } + + testWidgets('smoke with absolute URL', (tester) async { + const avatarUrl = 'https://example/avatar.png'; + check(await actualUrl(tester, avatarUrl)).isNotNull() + .asString.equals(avatarUrl); + debugNetworkImageHttpClientProvider = null; + }); + + testWidgets('smoke with relative URL', (tester) async { + const avatarUrl = '/avatar.png'; + check(await actualUrl(tester, avatarUrl)) + .equals(store.tryResolveUrl(avatarUrl)!); + debugNetworkImageHttpClientProvider = null; + }); + }); } From d1b90eaa9229954d1f869d69fcdf91933704d648 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 20 Feb 2024 16:51:45 -0800 Subject: [PATCH 13/15] content: Explicitly handle invalid image URLs, using tryResolveUrl --- lib/widgets/content.dart | 33 ++++++++++++----------------- test/model/content_test.dart | 20 +++++++++++++++++ test/widgets/content_test.dart | 25 ++++++++++++++++++++++ test/widgets/message_list_test.dart | 2 +- 4 files changed, 59 insertions(+), 21 deletions(-) diff --git a/lib/widgets/content.dart b/lib/widgets/content.dart index f52b62f206..1f70556f86 100644 --- a/lib/widgets/content.dart +++ b/lib/widgets/content.dart @@ -9,7 +9,6 @@ import '../api/model/model.dart'; import '../model/binding.dart'; import '../model/content.dart'; import '../model/internal_link.dart'; -import '../model/store.dart'; import 'code_block.dart'; import 'dialog.dart'; import 'icons.dart'; @@ -261,10 +260,11 @@ class MessageImage extends StatelessWidget { final src = node.srcUrl; final store = PerAccountStoreWidget.of(context); - final resolvedSrc = resolveUrl(src, store.account); + final resolvedSrc = store.tryResolveUrl(src); + // TODO if src fails to parse, show an explicit "broken image" return GestureDetector( - onTap: () { + onTap: resolvedSrc == null ? null : () { // TODO(log) Navigator.of(context).push(getLightboxRoute( context: context, message: message, src: resolvedSrc)); }, @@ -281,7 +281,7 @@ class MessageImage extends StatelessWidget { child: SizedBox( height: 100, width: 150, - child: LightboxHero( + child: resolvedSrc == null ? null : LightboxHero( message: message, src: resolvedSrc, child: RealmContentNetworkImage( @@ -717,7 +717,7 @@ class MessageImageEmoji extends StatelessWidget { @override Widget build(BuildContext context) { final store = PerAccountStoreWidget.of(context); - final resolvedSrc = resolveUrl(node.src, store.account); + final resolvedSrc = store.tryResolveUrl(node.src); const size = 20.0; @@ -730,12 +730,13 @@ class MessageImageEmoji extends StatelessWidget { // Web's css makes this seem like it should be -0.5, but that looks // too low. top: -1.5, - child: RealmContentNetworkImage( - resolvedSrc, - filterQuality: FilterQuality.medium, - width: size, - height: size, - )), + child: resolvedSrc == null ? const SizedBox.shrink() // TODO(log) + : RealmContentNetworkImage( + resolvedSrc, + filterQuality: FilterQuality.medium, + width: size, + height: size, + )), ]); } } @@ -964,7 +965,7 @@ class AvatarImage extends StatelessWidget { final resolvedUrl = switch (user.avatarUrl) { null => null, // TODO(#255): handle computing gravatars - var avatarUrl => resolveUrl(avatarUrl, store.account), + var avatarUrl => store.tryResolveUrl(avatarUrl), }; return (resolvedUrl == null) ? const SizedBox.shrink() @@ -1000,14 +1001,6 @@ class AvatarShape extends StatelessWidget { // Small helpers. // -/// Resolve `url` to `account`'s realm, if relative -// This may dissolve when we start passing around URLs as [Uri] objects instead -// of strings. -Uri resolveUrl(String url, Account account) { - final realmUrl = account.realmUrl; - return realmUrl.resolve(url); // TODO handle if fails to parse -} - InlineSpan _errorUnimplemented(UnimplementedNode node) { // For now this shows error-styled HTML code even in release mode, // because release mode isn't yet about general users but developer demos, diff --git a/test/model/content_test.dart b/test/model/content_test.dart index 4b00a42510..572e176157 100644 --- a/test/model/content_test.dart +++ b/test/model/content_test.dart @@ -111,6 +111,13 @@ class ContentExample { const ImageEmojiNode( src: '/user_avatars/2/emoji/images/204.png', alt: ':flutter:')); + static final emojiCustomInvalidUrl = ContentExample.inline( + 'custom emoji with invalid URL', + null, // hypothetical, to test for a risk of crashing + '

:invalid:

', + const ImageEmojiNode( + src: '::not a URL::', alt: ':invalid:')); + static final emojiZulipExtra = ContentExample.inline( 'Zulip extra emoji', ":zulip:", @@ -268,6 +275,17 @@ class ContentExample { ]), ]); + static const imageInvalidUrl = ContentExample( + 'single image with invalid URL', + null, // hypothetical, to test for a risk of crashing + '
' + '' + '
', [ + ImageNodeList([ + ImageNode(srcUrl: '::not a URL::'), + ]), + ]); + static const imageCluster = ContentExample( 'multiple images', "https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3\nhttps://chat.zulip.org/user_avatars/2/realm/icon.png?version=4", @@ -580,6 +598,7 @@ void main() { testParseExample(ContentExample.emojiUnicodeMultiCodepoint); testParseExample(ContentExample.emojiUnicodeLiteral); testParseExample(ContentExample.emojiCustom); + testParseExample(ContentExample.emojiCustomInvalidUrl); testParseExample(ContentExample.emojiZulipExtra); testParseExample(ContentExample.mathInline); @@ -751,6 +770,7 @@ void main() { testParseExample(ContentExample.mathBlockInQuote); testParseExample(ContentExample.imageSingle); + testParseExample(ContentExample.imageInvalidUrl); testParseExample(ContentExample.imageCluster); testParseExample(ContentExample.imageClusterThenContent); testParseExample(ContentExample.imageMultipleClusters); diff --git a/test/widgets/content_test.dart b/test/widgets/content_test.dart index aba2e97ff5..f408854442 100644 --- a/test/widgets/content_test.dart +++ b/test/widgets/content_test.dart @@ -117,6 +117,18 @@ void main() { .deepEquals(expectedImages.map((n) => n.srcUrl)); }); + testWidgets('image with invalid src URL', (tester) async { + const example = ContentExample.imageInvalidUrl; + await prepareContent(tester, example.html); + // The image indeed has an invalid URL. + final expectedImages = (example.expectedNodes[0] as ImageNodeList).images; + check(() => Uri.parse(expectedImages.single.srcUrl)).throws(); + check(tryResolveUrl(eg.realmUrl, expectedImages.single.srcUrl)).isNull(); + // The MessageImage has shown up, but it doesn't attempt a RealmContentNetworkImage. + check(tester.widgetList(find.byType(MessageImage))).isNotEmpty(); + check(tester.widgetList(find.byType(RealmContentNetworkImage))).isEmpty(); + }); + testWidgets('multiple images', (tester) async { const example = ContentExample.imageCluster; await prepareContent(tester, example.html); @@ -369,6 +381,13 @@ void main() { debugNetworkImageHttpClientProvider = null; }); + testWidgets('smoke: custom emoji with invalid URL', (tester) async { + await prepareContent(tester, ContentExample.emojiCustomInvalidUrl.html); + final url = tester.widget(find.byType(MessageImageEmoji)).node.src; + check(() => Uri.parse(url)).throws(); + debugNetworkImageHttpClientProvider = null; + }); + testWidgets('smoke: Zulip extra emoji', (tester) async { await prepareContent(tester, ContentExample.emojiZulipExtra.html); tester.widget(find.byType(MessageImageEmoji)); @@ -451,5 +470,11 @@ void main() { .equals(store.tryResolveUrl(avatarUrl)!); debugNetworkImageHttpClientProvider = null; }); + + testWidgets('smoke with invalid URL', (tester) async { + const avatarUrl = '::not a URL::'; + check(await actualUrl(tester, avatarUrl)).isNull(); + debugNetworkImageHttpClientProvider = null; + }); }); } diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 33202bf3e1..b8ee2e0e84 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -458,7 +458,7 @@ void main() { check(findAvatarImageWidget(tester)).isNull(); } else { check(findAvatarImageWidget(tester)).isNotNull() - .src.equals(resolveUrl(avatarUrl, eg.selfAccount)); + .src.equals(eg.selfAccount.realmUrl.resolve(avatarUrl)); } } From 6765527558c397cd8ea29ccd10dcb85b026d4c73 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 20 Feb 2024 15:36:05 -0800 Subject: [PATCH 14/15] store [nfc]: Use accountId directly rather than store.account.id --- test/model/store_test.dart | 4 ++-- test/widgets/store_test.dart | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/test/model/store_test.dart b/test/model/store_test.dart index e292002bd1..38eff362a8 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -215,7 +215,7 @@ void main() { await prepareStore(); updateMachine.debugPauseLoop(); updateMachine.poll(); - check(globalStore.perAccountSync(store.account.id)).identicalTo(store); + check(globalStore.perAccountSync(store.accountId)).identicalTo(store); // Let the server expire the event queue. connection.prepare(httpStatus: 400, json: { @@ -228,7 +228,7 @@ void main() { await Future.delayed(Duration.zero); // The global store has a new store. - check(globalStore.perAccountSync(store.account.id)).not((it) => it.identicalTo(store)); + check(globalStore.perAccountSync(store.accountId)).not((it) => it.identicalTo(store)); updateFromGlobalStore(); // The new UpdateMachine updates the new store. diff --git a/test/widgets/store_test.dart b/test/widgets/store_test.dart index 580ab9fd8e..a4b8500ba4 100644 --- a/test/widgets/store_test.dart +++ b/test/widgets/store_test.dart @@ -35,7 +35,7 @@ class MyWidgetWithMixinState extends State with PerAccountSto @override Widget build(BuildContext context) { final brightness = Theme.of(context).brightness; - final accountId = PerAccountStoreWidget.of(context).account.id; + final accountId = PerAccountStoreWidget.of(context).accountId; return Text('brightness: $brightness; accountId: $accountId'); } } @@ -87,7 +87,7 @@ void main() { child: Builder( builder: (context) { final store = PerAccountStoreWidget.of(context); - return Text('found store, account: ${store.account.id}'); + return Text('found store, account: ${store.accountId}'); }))))); await tester.pump(); await tester.pump(); @@ -109,7 +109,7 @@ void main() { child: Builder( builder: (context) { final store = PerAccountStoreWidget.of(context); - return Text('found store, account: ${store.account.id}'); + return Text('found store, account: ${store.accountId}'); }))))); // First, the global store has to load. @@ -137,7 +137,7 @@ void main() { child: Builder( builder: (context) { final store = PerAccountStoreWidget.of(context); - return Text('found store, account: ${store.account.id}'); + return Text('found store, account: ${store.accountId}'); }))))); // (... even one that really is separate, with its own fresh state node ...) From abc65a29e8800bab4dd528ec9e6e4d5efbfb2475 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 20 Feb 2024 15:37:36 -0800 Subject: [PATCH 15/15] store [nfc]: Take account ID for _reloadPerAccount Now that the caller is getting the Account object by looking it up on the GlobalStore anyway, there's no checking to be gained from passing the whole Account here. --- lib/model/store.dart | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index 7d66bda06a..1aed416810 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -119,12 +119,11 @@ abstract class GlobalStore extends ChangeNotifier { 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.id); - _setPerAccount(account.id, store); + Future _reloadPerAccount(int accountId) async { + assert(_perAccountStores.containsKey(accountId)); + assert(!_perAccountStoresLoading.containsKey(accountId)); + final store = await loadPerAccount(accountId); + _setPerAccount(accountId, store); } void _setPerAccount(int accountId, PerAccountStore store) { @@ -672,7 +671,7 @@ class UpdateMachine { switch (e) { case ZulipApiException(code: 'BAD_EVENT_QUEUE_ID'): assert(debugLog('Lost event queue for $store. Replacing…')); - await store._globalStore._reloadPerAccount(store.account); + await store._globalStore._reloadPerAccount(store.accountId); dispose(); debugLog('… Event queue replaced.'); return;