From fbee491fb428ba7bb3ed8eb1855382cc06d5381d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 15 Feb 2025 15:12:14 -0800 Subject: [PATCH 01/19] test [nfc]: Add the few missing awaits on handleEvent calls This is NFC because there isn't currently actually anything to wait for; the implementation of handleEvent is entirely synchronous. But in principle it's async, because in the future it will be, for the reasons described in baea6d746. Thanks to Zixuan for spotting this pattern in some newly-added tests (coming up): https://github.com/zulip/zulip-flutter/pull/1327#discussion_r1945527867 --- test/model/emoji_test.dart | 4 ++-- test/model/store_test.dart | 10 +++++----- test/widgets/message_list_test.dart | 10 +++++----- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/test/model/emoji_test.dart b/test/model/emoji_test.dart index a55f2dc36b..7916af0849 100644 --- a/test/model/emoji_test.dart +++ b/test/model/emoji_test.dart @@ -262,14 +262,14 @@ void main() { ]); }); - test('updates on RealmEmojiUpdateEvent', () { + test('updates on RealmEmojiUpdateEvent', () async { final store = prepare(); check(store.allEmojiCandidates()).deepEquals([ ...arePopularCandidates, isZulipCandidate(), ]); - store.handleEvent(RealmEmojiUpdateEvent(id: 1, realmEmoji: { + await store.handleEvent(RealmEmojiUpdateEvent(id: 1, realmEmoji: { '1': eg.realmEmojiItem(emojiCode: '1', emojiName: 'happy'), })); check(store.allEmojiCandidates()).deepEquals([ diff --git a/test/model/store_test.dart b/test/model/store_test.dart index 99235dbe67..7b3099f08a 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -375,26 +375,26 @@ void main() { group('RealmUserUpdateEvent', () { // TODO write more tests for handling RealmUserUpdateEvent - test('deliveryEmail', () { + test('deliveryEmail', () async { final user = eg.user(deliveryEmail: 'a@mail.example'); final store = eg.store(initialSnapshot: eg.initialSnapshot( realmUsers: [eg.selfUser, user])); User getUser() => store.users[user.userId]!; - store.handleEvent(RealmUserUpdateEvent(id: 1, userId: user.userId, + await store.handleEvent(RealmUserUpdateEvent(id: 1, userId: user.userId, deliveryEmail: null)); check(getUser()).deliveryEmail.equals('a@mail.example'); - store.handleEvent(RealmUserUpdateEvent(id: 1, userId: user.userId, + await store.handleEvent(RealmUserUpdateEvent(id: 1, userId: user.userId, deliveryEmail: const JsonNullable(null))); check(getUser()).deliveryEmail.isNull(); - store.handleEvent(RealmUserUpdateEvent(id: 1, userId: user.userId, + await store.handleEvent(RealmUserUpdateEvent(id: 1, userId: user.userId, deliveryEmail: const JsonNullable('b@mail.example'))); check(getUser()).deliveryEmail.equals('b@mail.example'); - store.handleEvent(RealmUserUpdateEvent(id: 1, userId: user.userId, + await store.handleEvent(RealmUserUpdateEvent(id: 1, userId: user.userId, deliveryEmail: const JsonNullable('c@mail.example'))); check(getUser()).deliveryEmail.equals('c@mail.example'); }); diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index b7384824fd..b5d3844c1a 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -799,8 +799,8 @@ void main() { foundOldest: false, messages: messages).toJson()); } - void handleMessageMoveEvent(List messages, String newTopic, {int? newChannelId}) { - store.handleEvent(eg.updateMessageEventMoveFrom( + Future handleMessageMoveEvent(List messages, String newTopic, {int? newChannelId}) async { + await store.handleEvent(eg.updateMessageEventMoveFrom( origMessages: messages, newTopicStr: newTopic, newStreamId: newChannelId, @@ -821,7 +821,7 @@ void main() { ..controller.isNotNull().text.equals('Some text'); prepareGetMessageResponse([message]); - handleMessageMoveEvent([message], 'new topic', newChannelId: otherChannel.streamId); + await handleMessageMoveEvent([message], 'new topic', newChannelId: otherChannel.streamId); await tester.pump(const Duration(seconds: 1)); check(tester.widget(channelContentInputFinder)) ..decoration.isNotNull().hintText.equals('Message #${otherChannel.name} > new topic') @@ -851,7 +851,7 @@ void main() { final existingMessage = eg.streamMessage( stream: eg.stream(), topic: 'new topic', content: 'Existing message'); prepareGetMessageResponse([existingMessage, message]); - handleMessageMoveEvent([message], 'new topic'); + await handleMessageMoveEvent([message], 'new topic'); await tester.pump(const Duration(seconds: 1)); check(find.textContaining('Existing message').evaluate()).length.equals(1); @@ -863,7 +863,7 @@ void main() { await setupMessageListPage(tester, narrow: narrow, messages: [message], streams: [channel]); prepareGetMessageResponse([message]); - handleMessageMoveEvent([message], 'new topic'); + await handleMessageMoveEvent([message], 'new topic'); await tester.pump(const Duration(seconds: 1)); check(find.descendant( From d5caa78db9a97fe81f2c426094532731f2be1363 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 4 Feb 2025 18:18:35 -0800 Subject: [PATCH 02/19] user: Split a UserStore out from PerAccountStore Like ChannelStore and others, this helps reduce the amount of complexity that's concentrated centrally in PerAccountStore. This change is all NFC except that if we get a RealmUserUpdateEvent for an unknown user, we'll now call notifyListeners, and so might cause some widgets to rebuild, when previously we wouldn't. That's pretty low-stakes, so I'm not bothering to wire through the data flow to avoid it. --- lib/model/store.dart | 49 +++++++---------------------- lib/model/user.dart | 64 ++++++++++++++++++++++++++++++++++++++ test/model/store_test.dart | 30 +----------------- test/model/user_test.dart | 37 ++++++++++++++++++++++ 4 files changed, 114 insertions(+), 66 deletions(-) create mode 100644 lib/model/user.dart create mode 100644 test/model/user_test.dart diff --git a/lib/model/store.dart b/lib/model/store.dart index 611494cc8f..65c7f0af0a 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -31,6 +31,7 @@ import 'recent_senders.dart'; import 'channel.dart'; import 'typing_status.dart'; import 'unreads.dart'; +import 'user.dart'; export 'package:drift/drift.dart' show Value; export 'database.dart' show Account, AccountsCompanion, AccountAlreadyExistsException; @@ -266,7 +267,7 @@ class AccountNotFoundException implements Exception {} /// This class does not attempt to poll an event queue /// to keep the data up to date. For that behavior, see /// [UpdateMachine]. -class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, MessageStore { +class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, ChannelStore, MessageStore { /// Construct a store for the user's data, starting from the given snapshot. /// /// The global store must already have been updated with @@ -316,11 +317,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess typingStartedWaitPeriod: Duration( milliseconds: initialSnapshot.serverTypingStartedWaitPeriodMilliseconds), ), - users: Map.fromEntries( - initialSnapshot.realmUsers - .followedBy(initialSnapshot.realmNonActiveUsers) - .followedBy(initialSnapshot.crossRealmBots) - .map((user) => MapEntry(user.userId, user))), + users: UserStoreImpl(initialSnapshot: initialSnapshot), typingStatus: TypingStatus( selfUserId: account.userId, typingStartedExpiryPeriod: Duration(milliseconds: initialSnapshot.serverTypingStartedExpiryPeriodMilliseconds), @@ -354,7 +351,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess required this.selfUserId, required this.userSettings, required this.typingNotifier, - required this.users, + required UserStoreImpl users, required this.typingStatus, required ChannelStoreImpl channels, required MessageStoreImpl messages, @@ -367,6 +364,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess assert(emoji.realmUrl == realmUrl), _globalStore = globalStore, _emoji = emoji, + _users = users, _channels = channels, _messages = messages; @@ -465,7 +463,10 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess //////////////////////////////// // Users and data about them. - final Map users; + @override + Map get users => _users.users; + + final UserStoreImpl _users; final TypingStatus typingStatus; @@ -634,44 +635,18 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess case RealmUserAddEvent(): assert(debugLog("server event: realm_user/add")); - users[event.person.userId] = event.person; + _users.handleRealmUserEvent(event); notifyListeners(); case RealmUserRemoveEvent(): assert(debugLog("server event: realm_user/remove")); - users.remove(event.userId); + _users.handleRealmUserEvent(event); autocompleteViewManager.handleRealmUserRemoveEvent(event); notifyListeners(); case RealmUserUpdateEvent(): assert(debugLog("server event: realm_user/update")); - final user = users[event.userId]; - if (user == null) { - return; // TODO log - } - if (event.fullName != null) user.fullName = event.fullName!; - if (event.avatarUrl != null) user.avatarUrl = event.avatarUrl!; - if (event.avatarVersion != null) user.avatarVersion = event.avatarVersion!; - if (event.timezone != null) user.timezone = event.timezone!; - if (event.botOwnerId != null) user.botOwnerId = event.botOwnerId!; - if (event.role != null) user.role = event.role!; - if (event.isBillingAdmin != null) user.isBillingAdmin = event.isBillingAdmin!; - if (event.deliveryEmail != null) user.deliveryEmail = event.deliveryEmail!.value; - if (event.newEmail != null) user.email = event.newEmail!; - if (event.isActive != null) user.isActive = event.isActive!; - if (event.customProfileField != null) { - final profileData = (user.profileData ??= {}); - final update = event.customProfileField!; - if (update.value != null) { - profileData[update.id] = ProfileFieldUserData(value: update.value!, renderedValue: update.renderedValue); - } else { - profileData.remove(update.id); - } - if (profileData.isEmpty) { - // null is equivalent to `{}` for efficiency; see [User._readProfileData]. - user.profileData = null; - } - } + _users.handleRealmUserEvent(event); autocompleteViewManager.handleRealmUserUpdateEvent(event); notifyListeners(); diff --git a/lib/model/user.dart b/lib/model/user.dart new file mode 100644 index 0000000000..2e829132b0 --- /dev/null +++ b/lib/model/user.dart @@ -0,0 +1,64 @@ +import '../api/model/events.dart'; +import '../api/model/initial_snapshot.dart'; +import '../api/model/model.dart'; + +/// The portion of [PerAccountStore] describing the users in the realm. +mixin UserStore { + Map get users; +} + +/// The implementation of [UserStore] that does the work. +/// +/// 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 { + UserStoreImpl({required InitialSnapshot initialSnapshot}) + : users = Map.fromEntries( + initialSnapshot.realmUsers + .followedBy(initialSnapshot.realmNonActiveUsers) + .followedBy(initialSnapshot.crossRealmBots) + .map((user) => MapEntry(user.userId, user))); + + @override + final Map users; + + void handleRealmUserEvent(RealmUserEvent event) { + switch (event) { + case RealmUserAddEvent(): + users[event.person.userId] = event.person; + + case RealmUserRemoveEvent(): + users.remove(event.userId); + + case RealmUserUpdateEvent(): + final user = users[event.userId]; + if (user == null) { + return; // TODO log + } + if (event.fullName != null) user.fullName = event.fullName!; + if (event.avatarUrl != null) user.avatarUrl = event.avatarUrl!; + if (event.avatarVersion != null) user.avatarVersion = event.avatarVersion!; + if (event.timezone != null) user.timezone = event.timezone!; + if (event.botOwnerId != null) user.botOwnerId = event.botOwnerId!; + if (event.role != null) user.role = event.role!; + if (event.isBillingAdmin != null) user.isBillingAdmin = event.isBillingAdmin!; + if (event.deliveryEmail != null) user.deliveryEmail = event.deliveryEmail!.value; + if (event.newEmail != null) user.email = event.newEmail!; + if (event.isActive != null) user.isActive = event.isActive!; + if (event.customProfileField != null) { + final profileData = (user.profileData ??= {}); + final update = event.customProfileField!; + if (update.value != null) { + profileData[update.id] = ProfileFieldUserData(value: update.value!, renderedValue: update.renderedValue); + } else { + profileData.remove(update.id); + } + if (profileData.isEmpty) { + // null is equivalent to `{}` for efficiency; see [User._readProfileData]. + user.profileData = null; + } + } + } + } +} diff --git a/test/model/store_test.dart b/test/model/store_test.dart index 7b3099f08a..5adc97a39b 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -369,36 +369,8 @@ void main() { group('PerAccountStore.handleEvent', () { // Mostly this method just dispatches to ChannelStore and MessageStore etc., - // and so most of the tests live in the test files for those + // and so its tests generally live in the test files for those // (but they call the handleEvent method because it's the entry point). - - group('RealmUserUpdateEvent', () { - // TODO write more tests for handling RealmUserUpdateEvent - - test('deliveryEmail', () async { - final user = eg.user(deliveryEmail: 'a@mail.example'); - final store = eg.store(initialSnapshot: eg.initialSnapshot( - realmUsers: [eg.selfUser, user])); - - User getUser() => store.users[user.userId]!; - - await store.handleEvent(RealmUserUpdateEvent(id: 1, userId: user.userId, - deliveryEmail: null)); - check(getUser()).deliveryEmail.equals('a@mail.example'); - - await store.handleEvent(RealmUserUpdateEvent(id: 1, userId: user.userId, - deliveryEmail: const JsonNullable(null))); - check(getUser()).deliveryEmail.isNull(); - - await store.handleEvent(RealmUserUpdateEvent(id: 1, userId: user.userId, - deliveryEmail: const JsonNullable('b@mail.example'))); - check(getUser()).deliveryEmail.equals('b@mail.example'); - - await store.handleEvent(RealmUserUpdateEvent(id: 1, userId: user.userId, - deliveryEmail: const JsonNullable('c@mail.example'))); - check(getUser()).deliveryEmail.equals('c@mail.example'); - }); - }); }); group('PerAccountStore.sendMessage', () { diff --git a/test/model/user_test.dart b/test/model/user_test.dart new file mode 100644 index 0000000000..b4e3065a19 --- /dev/null +++ b/test/model/user_test.dart @@ -0,0 +1,37 @@ +import 'package:checks/checks.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:zulip/api/model/events.dart'; +import 'package:zulip/api/model/model.dart'; + +import '../api/model/model_checks.dart'; +import '../example_data.dart' as eg; + +void main() { + group('RealmUserUpdateEvent', () { + // TODO write more tests for handling RealmUserUpdateEvent + + test('deliveryEmail', () async { + final user = eg.user(deliveryEmail: 'a@mail.example'); + final store = eg.store(initialSnapshot: eg.initialSnapshot( + realmUsers: [eg.selfUser, user])); + + User getUser() => store.users[user.userId]!; + + await store.handleEvent(RealmUserUpdateEvent(id: 1, userId: user.userId, + deliveryEmail: null)); + check(getUser()).deliveryEmail.equals('a@mail.example'); + + await store.handleEvent(RealmUserUpdateEvent(id: 1, userId: user.userId, + deliveryEmail: const JsonNullable(null))); + check(getUser()).deliveryEmail.isNull(); + + await store.handleEvent(RealmUserUpdateEvent(id: 1, userId: user.userId, + deliveryEmail: const JsonNullable('b@mail.example'))); + check(getUser()).deliveryEmail.equals('b@mail.example'); + + await store.handleEvent(RealmUserUpdateEvent(id: 1, userId: user.userId, + deliveryEmail: const JsonNullable('c@mail.example'))); + check(getUser()).deliveryEmail.equals('c@mail.example'); + }); + }); +} From af10aead988b2c8ce52572ad63a6e20b37608411 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 15 Feb 2025 15:22:49 -0800 Subject: [PATCH 03/19] user [nfc]: Refer to "user store" rather than "store.users" in text At the end of this series, there won't be a `users` member on the store, so this code-like reference will no longer make sense. The underlying meaning in each of these contexts will still be apt, though: the data structure where the known users are tracked. So update them to make that reference more generically. --- test/model/autocomplete_test.dart | 2 +- test/widgets/recent_dm_conversations_test.dart | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index da05030493..e916151426 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -369,7 +369,7 @@ void main() { } }); - test('MentionAutocompleteView mutating store.users while in progress does not ' + test('MentionAutocompleteView mutating user store while in progress does not ' 'prevent query from finishing', () async { const narrow = ChannelNarrow(1); final store = eg.store(); diff --git a/test/widgets/recent_dm_conversations_test.dart b/test/widgets/recent_dm_conversations_test.dart index 0976ca1bfd..0d440accec 100644 --- a/test/widgets/recent_dm_conversations_test.dart +++ b/test/widgets/recent_dm_conversations_test.dart @@ -215,7 +215,7 @@ void main() { checkTitle(tester, user.fullName); }); - testWidgets('no error when user somehow missing from store.users', (tester) async { + testWidgets('no error when user somehow missing from user store', (tester) async { final user = eg.user(userId: 1); final message = eg.dmMessage(from: eg.selfUser, to: [user]); await setupPage(tester, @@ -271,7 +271,7 @@ void main() { checkTitle(tester, '${user0.fullName}, ${user1.fullName}'); }); - testWidgets('no error when one user somehow missing from store.users', (tester) async { + testWidgets('no error when one user somehow missing from user store', (tester) async { final users = usersList(2); final user0 = users[0]; final user1 = users[1]; From 7168c4869ae83ffb8bd559e0ecbbdba2f6ce39de Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 4 Feb 2025 20:35:22 -0800 Subject: [PATCH 04/19] user [nfc]: Document users map, especially its incompleteness --- lib/model/user.dart | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/lib/model/user.dart b/lib/model/user.dart index 2e829132b0..04a46e071f 100644 --- a/lib/model/user.dart +++ b/lib/model/user.dart @@ -4,6 +4,24 @@ import '../api/model/model.dart'; /// The portion of [PerAccountStore] describing the users in the realm. mixin UserStore { + /// All known users in the realm, by [User.userId]. + /// + /// There may be other users not found in this map, for multiple reasons: + /// + /// * The self-user may not have permission to see all the users in the + /// realm, for example because the self-user is a guest. + /// + /// * Because of the fetch/event race, any data that the client fetched + /// outside of the event system may reflect an earlier or later time + /// than this data, which is maintained by the event system. + /// This includes messages fetched for a message list, and notifications. + /// Those may therefore refer to users for which we have yet to see the + /// [RealmUserAddEvent], or have already handled a [RealmUserRemoveEvent]. + /// + /// Code that looks up a user in this map should therefore always handle + /// the possibility that the user is not found (except + /// where there is a specific reason to know the user should be found). + /// Consider using [ZulipLocalizations.unknownUserName]. Map get users; } From a7ecf5b87be2f6ddf933b0c6d52c1094d1b1272b Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 4 Feb 2025 20:48:25 -0800 Subject: [PATCH 05/19] user [nfc]: Factor out a userDisplayName --- lib/model/user.dart | 12 +++++++++++- lib/widgets/emoji_reaction.dart | 2 +- lib/widgets/inbox.dart | 8 ++------ lib/widgets/message_list.dart | 11 +++++------ lib/widgets/poll.dart | 3 +-- lib/widgets/profile.dart | 8 ++++---- lib/widgets/recent_dm_conversations.dart | 10 +++------- test/model/user_test.dart | 15 +++++++++++++++ 8 files changed, 42 insertions(+), 27 deletions(-) diff --git a/lib/model/user.dart b/lib/model/user.dart index 04a46e071f..583c3b9d62 100644 --- a/lib/model/user.dart +++ b/lib/model/user.dart @@ -1,6 +1,7 @@ import '../api/model/events.dart'; import '../api/model/initial_snapshot.dart'; import '../api/model/model.dart'; +import 'localizations.dart'; /// The portion of [PerAccountStore] describing the users in the realm. mixin UserStore { @@ -21,8 +22,17 @@ mixin UserStore { /// Code that looks up a user in this map should therefore always handle /// the possibility that the user is not found (except /// where there is a specific reason to know the user should be found). - /// Consider using [ZulipLocalizations.unknownUserName]. + /// Consider using [userDisplayName]. Map get users; + + /// The name to show the given user as in the UI, even for unknown users. + /// + /// This is the user's [User.fullName] if the user is known, + /// and otherwise a translation of "(unknown user)". + String userDisplayName(int userId) { + return users[userId]?.fullName + ?? GlobalLocalizations.zulipLocalizations.unknownUserName; + } } /// The implementation of [UserStore] that does the work. diff --git a/lib/widgets/emoji_reaction.dart b/lib/widgets/emoji_reaction.dart index de3e2baa26..c418337e62 100644 --- a/lib/widgets/emoji_reaction.dart +++ b/lib/widgets/emoji_reaction.dart @@ -162,7 +162,7 @@ class ReactionChip extends StatelessWidget { ? userIds.map((id) { return id == store.selfUserId ? zulipLocalizations.reactedEmojiSelfUser - : store.users[id]?.fullName ?? zulipLocalizations.unknownUserName; + : store.userDisplayName(id); }).join(', ') : userIds.length.toString(); diff --git a/lib/widgets/inbox.dart b/lib/widgets/inbox.dart index 799f763f1c..bb38f0002c 100644 --- a/lib/widgets/inbox.dart +++ b/lib/widgets/inbox.dart @@ -384,20 +384,16 @@ class _DmItem extends StatelessWidget { final store = PerAccountStoreWidget.of(context); final selfUser = store.users[store.selfUserId]!; - final zulipLocalizations = ZulipLocalizations.of(context); final designVariables = DesignVariables.of(context); final title = switch (narrow.otherRecipientIds) { // TODO dedupe with [RecentDmConversationsItem] [] => selfUser.fullName, - [var otherUserId] => - store.users[otherUserId]?.fullName ?? zulipLocalizations.unknownUserName, + [var otherUserId] => store.userDisplayName(otherUserId), // TODO(i18n): List formatting, like you can do in JavaScript: // new Intl.ListFormat('ja').format(['Chris', 'Greg', 'Alya', 'Shu']) // // 'Chris、Greg、Alya、Shu' - _ => narrow.otherRecipientIds.map( - (id) => store.users[id]?.fullName ?? zulipLocalizations.unknownUserName - ).join(', '), + _ => narrow.otherRecipientIds.map(store.userDisplayName).join(', '), }; return Material( diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 127bd65c61..b87d983e86 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -431,8 +431,7 @@ class MessageListAppBarTitle extends StatelessWidget { if (otherRecipientIds.isEmpty) { return Text(zulipLocalizations.dmsWithYourselfPageTitle); } else { - final names = otherRecipientIds.map( - (id) => store.users[id]?.fullName ?? zulipLocalizations.unknownUserName); + final names = otherRecipientIds.map(store.userDisplayName); // TODO show avatars return Text( zulipLocalizations.dmsWithOthersPageTitle(names.join(', '))); @@ -774,10 +773,10 @@ class _TypingStatusWidgetState extends State with PerAccount if (typistIds.isEmpty) return const SizedBox(); final text = switch (typistIds.length) { 1 => localizations.onePersonTyping( - store.users[typistIds.first]?.fullName ?? localizations.unknownUserName), + store.userDisplayName(typistIds.first)), 2 => localizations.twoPeopleTyping( - store.users[typistIds.first]?.fullName ?? localizations.unknownUserName, - store.users[typistIds.last]?.fullName ?? localizations.unknownUserName), + store.userDisplayName(typistIds.first), + store.userDisplayName(typistIds.last)), _ => localizations.manyPeopleTyping, }; @@ -1179,7 +1178,7 @@ class DmRecipientHeader extends StatelessWidget { if (message.allRecipientIds.length > 1) { title = zulipLocalizations.messageListGroupYouAndOthers(message.allRecipientIds .where((id) => id != store.selfUserId) - .map((id) => store.users[id]?.fullName ?? zulipLocalizations.unknownUserName) + .map(store.userDisplayName) .sorted() .join(", ")); } else { diff --git a/lib/widgets/poll.dart b/lib/widgets/poll.dart index dc340d08b8..b851c55525 100644 --- a/lib/widgets/poll.dart +++ b/lib/widgets/poll.dart @@ -80,8 +80,7 @@ class _PollWidgetState extends State { // new Intl.ListFormat('ja').format(['Chris', 'Greg', 'Alya', 'Zixuan']) // // 'Chris、Greg、Alya、Zixuan' final voterNames = option.voters - .map((userId) => - store.users[userId]?.fullName ?? zulipLocalizations.unknownUserName) + .map(store.userDisplayName) .join(', '); return Row( diff --git a/lib/widgets/profile.dart b/lib/widgets/profile.dart index 327910f6c0..8bef274a99 100644 --- a/lib/widgets/profile.dart +++ b/lib/widgets/profile.dart @@ -291,9 +291,6 @@ class _UserWidget extends StatelessWidget { @override Widget build(BuildContext context) { final store = PerAccountStoreWidget.of(context); - final zulipLocalizations = ZulipLocalizations.of(context); - final user = store.users[userId]; - final fullName = user?.fullName ?? zulipLocalizations.unknownUserName; return InkWell( onTap: () => Navigator.push(context, ProfilePage.buildRoute(context: context, @@ -301,9 +298,12 @@ class _UserWidget extends StatelessWidget { child: Padding( padding: const EdgeInsets.all(8), child: Row(children: [ + // TODO(#196) render active status Avatar(userId: userId, size: 32, borderRadius: 32 / 8), const SizedBox(width: 8), - Expanded(child: Text(fullName, style: _TextStyles.customProfileFieldText)), // TODO(#196) render active status + Expanded( + child: Text(store.userDisplayName(userId), + style: _TextStyles.customProfileFieldText)), ]))); } } diff --git a/lib/widgets/recent_dm_conversations.dart b/lib/widgets/recent_dm_conversations.dart index ddc32861a3..cd47345d62 100644 --- a/lib/widgets/recent_dm_conversations.dart +++ b/lib/widgets/recent_dm_conversations.dart @@ -1,6 +1,5 @@ import 'package:flutter/material.dart'; -import '../generated/l10n/zulip_localizations.dart'; import '../model/narrow.dart'; import '../model/recent_dm_conversations.dart'; import '../model/unreads.dart'; @@ -82,7 +81,6 @@ class RecentDmConversationsItem extends StatelessWidget { final store = PerAccountStoreWidget.of(context); final selfUser = store.users[store.selfUserId]!; - final zulipLocalizations = ZulipLocalizations.of(context); final designVariables = DesignVariables.of(context); final String title; @@ -95,16 +93,14 @@ class RecentDmConversationsItem extends StatelessWidget { // TODO(#296) actually don't show this row if the user is muted? // (should we offer a "spam folder" style summary screen of recent // 1:1 DM conversations from muted users?) - final otherUser = store.users[otherUserId]; - title = otherUser?.fullName ?? zulipLocalizations.unknownUserName; + title = store.userDisplayName(otherUserId); avatar = AvatarImage(userId: otherUserId, size: _avatarSize); default: // TODO(i18n): List formatting, like you can do in JavaScript: // new Intl.ListFormat('ja').format(['Chris', 'Greg', 'Alya']) // // 'Chris、Greg、Alya' - title = narrow.otherRecipientIds.map( - (id) => store.users[id]?.fullName ?? zulipLocalizations.unknownUserName - ).join(', '); + title = narrow.otherRecipientIds.map(store.userDisplayName) + .join(', '); avatar = ColoredBox(color: designVariables.groupDmConversationIconBg, child: Center( child: Icon(color: designVariables.groupDmConversationIcon, diff --git a/test/model/user_test.dart b/test/model/user_test.dart index b4e3065a19..2087abfdd7 100644 --- a/test/model/user_test.dart +++ b/test/model/user_test.dart @@ -5,8 +5,23 @@ import 'package:zulip/api/model/model.dart'; import '../api/model/model_checks.dart'; import '../example_data.dart' as eg; +import 'test_store.dart'; void main() { + group('userDisplayName', () { + test('on a known user', () async { + final user = eg.user(fullName: 'Some User'); + final store = eg.store(); + await store.addUser(user); + check(store.userDisplayName(user.userId)).equals('Some User'); + }); + + test('on an unknown user', () { + final store = eg.store(); + check(store.userDisplayName(eg.user().userId)).equals('(unknown user)'); + }); + }); + group('RealmUserUpdateEvent', () { // TODO write more tests for handling RealmUserUpdateEvent From cffe2e47ae01b7918478e50eceb04e8ae78cea62 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 4 Feb 2025 20:59:58 -0800 Subject: [PATCH 06/19] inbox [nfc]: Inline and unhoist a self-user lookup This way we don't actually do the lookup unless we need it. --- lib/widgets/inbox.dart | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/widgets/inbox.dart b/lib/widgets/inbox.dart index bb38f0002c..17718c5a96 100644 --- a/lib/widgets/inbox.dart +++ b/lib/widgets/inbox.dart @@ -382,12 +382,10 @@ class _DmItem extends StatelessWidget { @override Widget build(BuildContext context) { final store = PerAccountStoreWidget.of(context); - final selfUser = store.users[store.selfUserId]!; - final designVariables = DesignVariables.of(context); final title = switch (narrow.otherRecipientIds) { // TODO dedupe with [RecentDmConversationsItem] - [] => selfUser.fullName, + [] => store.users[store.selfUserId]!.fullName, [var otherUserId] => store.userDisplayName(otherUserId), // TODO(i18n): List formatting, like you can do in JavaScript: From c077a4589d70a2b134fb401fad3a9a18abc0834d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 4 Feb 2025 21:03:23 -0800 Subject: [PATCH 07/19] recent dms [nfc]: Unhoist a self-user lookup --- lib/widgets/recent_dm_conversations.dart | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/widgets/recent_dm_conversations.dart b/lib/widgets/recent_dm_conversations.dart index cd47345d62..8170d3b231 100644 --- a/lib/widgets/recent_dm_conversations.dart +++ b/lib/widgets/recent_dm_conversations.dart @@ -79,14 +79,13 @@ class RecentDmConversationsItem extends StatelessWidget { @override Widget build(BuildContext context) { final store = PerAccountStoreWidget.of(context); - final selfUser = store.users[store.selfUserId]!; - final designVariables = DesignVariables.of(context); final String title; final Widget avatar; switch (narrow.otherRecipientIds) { // TODO dedupe with DM items in [InboxPage] case []: + final selfUser = store.users[store.selfUserId]!; title = selfUser.fullName; avatar = AvatarImage(userId: selfUser.userId, size: _avatarSize); case [var otherUserId]: From 581e3778245b011b80f12cb2a7e89c2f07bbab2e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 4 Feb 2025 21:04:17 -0800 Subject: [PATCH 08/19] compose [nfc]: Unhoist a self-user lookup Also add some blank lines to make the structure of this switch a bit easier to see. --- lib/widgets/compose_box.dart | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 0a060a6bcf..5a4473d818 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1419,16 +1419,17 @@ class _ComposeBoxState extends State with PerAccountStoreAwareStateM Widget? _errorBanner(BuildContext context) { final store = PerAccountStoreWidget.of(context); - final selfUser = store.users[store.selfUserId]!; switch (widget.narrow) { case ChannelNarrow(:final streamId): case TopicNarrow(:final streamId): final channel = store.streams[streamId]; + final selfUser = store.users[store.selfUserId]!; if (channel == null || !store.hasPostingPermission(inChannel: channel, user: selfUser, byDate: DateTime.now())) { return _ErrorBanner(label: ZulipLocalizations.of(context).errorBannerCannotPostInChannelLabel); } + case DmNarrow(:final otherRecipientIds): final hasDeactivatedUser = otherRecipientIds.any((id) => !(store.users[id]?.isActive ?? true)); @@ -1436,6 +1437,7 @@ class _ComposeBoxState extends State with PerAccountStoreAwareStateM return _ErrorBanner(label: ZulipLocalizations.of(context).errorBannerDeactivatedDmLabel); } + case CombinedFeedNarrow(): case MentionsNarrow(): case StarredMessagesNarrow(): From ea468257709fd303b1a40812d1616630a23a858c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 4 Feb 2025 21:26:32 -0800 Subject: [PATCH 09/19] store [nfc]: Add a zulipFeatureLevel getter This is more convenient than going through store.connection, and should reduce the temptation to go through store.account. --- lib/model/autocomplete.dart | 4 ++-- lib/model/compose.dart | 4 ++-- lib/model/internal_link.dart | 2 +- lib/model/store.dart | 4 ++++ lib/widgets/action_sheet.dart | 6 +++--- lib/widgets/actions.dart | 6 ++---- lib/widgets/autocomplete.dart | 2 +- lib/widgets/profile.dart | 2 +- 8 files changed, 16 insertions(+), 14 deletions(-) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index dffe44d6fe..4b734f936c 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -622,13 +622,13 @@ class MentionAutocompleteView extends AutocompleteView= 247; // TODO(server-9) + final isChannelWildcardAvailable = store.zulipFeatureLevel >= 247; // TODO(server-9) if (isChannelWildcardAvailable && tryOption(WildcardMentionOption.channel)) break all; if (tryOption(WildcardMentionOption.stream)) break all; } } - final isTopicWildcardAvailable = store.account.zulipFeatureLevel >= 224; // TODO(server-8) + final isTopicWildcardAvailable = store.zulipFeatureLevel >= 224; // TODO(server-8) if (isComposingChannelMessage && isTopicWildcardAvailable) { tryOption(WildcardMentionOption.topic); } diff --git a/lib/model/compose.dart b/lib/model/compose.dart index 54c7f6ce00..094f639398 100644 --- a/lib/model/compose.dart +++ b/lib/model/compose.dart @@ -140,8 +140,8 @@ String userMention(User user, {bool silent = false, Map? users}) { String wildcardMention(WildcardMentionOption wildcardOption, { required PerAccountStore store, }) { - final isChannelWildcardAvailable = store.account.zulipFeatureLevel >= 247; // TODO(server-9) - final isTopicWildcardAvailable = store.account.zulipFeatureLevel >= 224; // TODO(server-8) + final isChannelWildcardAvailable = store.zulipFeatureLevel >= 247; // TODO(server-9) + final isTopicWildcardAvailable = store.zulipFeatureLevel >= 224; // TODO(server-8) String name = wildcardOption.canonicalString; switch (wildcardOption) { diff --git a/lib/model/internal_link.dart b/lib/model/internal_link.dart index 1290259033..749f60698c 100644 --- a/lib/model/internal_link.dart +++ b/lib/model/internal_link.dart @@ -60,7 +60,7 @@ String? decodeHashComponent(String str) { Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) { // TODO(server-7) final apiNarrow = resolveApiNarrowForServer( - narrow.apiEncode(), store.connection.zulipFeatureLevel!); + narrow.apiEncode(), store.zulipFeatureLevel); final fragment = StringBuffer('narrow'); for (ApiNarrowElement element in apiNarrow) { fragment.write('/'); diff --git a/lib/model/store.dart b/lib/model/store.dart index 65c7f0af0a..1b64acbc5d 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -405,6 +405,10 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel /// 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!; + String get zulipVersion => account.zulipVersion; final RealmWildcardMentionPolicy realmWildcardMentionPolicy; // TODO(#668): update this realm setting final bool realmMandatoryTopics; // TODO(#668): update this realm setting diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart index e943d7eb36..aa81461421 100644 --- a/lib/widgets/action_sheet.dart +++ b/lib/widgets/action_sheet.dart @@ -182,9 +182,9 @@ void showTopicActionSheet(BuildContext context, { final optionButtons = []; // TODO(server-7): simplify this condition away - final supportsUnmutingTopics = store.connection.zulipFeatureLevel! >= 170; + final supportsUnmutingTopics = store.zulipFeatureLevel >= 170; // TODO(server-8): simplify this condition away - final supportsFollowingTopics = store.connection.zulipFeatureLevel! >= 219; + final supportsFollowingTopics = store.zulipFeatureLevel >= 219; final visibilityOptions = []; final visibilityPolicy = store.topicVisibilityPolicy(channelId, topic); @@ -477,7 +477,7 @@ void showMessageActionSheet({required BuildContext context, required Message mes final isComposeBoxOffered = messageListPage.composeBoxController != null; final isMessageRead = message.flags.contains(MessageFlag.read); - final markAsUnreadSupported = store.connection.zulipFeatureLevel! >= 155; // TODO(server-6) + final markAsUnreadSupported = store.zulipFeatureLevel >= 155; // TODO(server-6) final showMarkAsUnreadButton = markAsUnreadSupported && isMessageRead; final optionButtons = [ diff --git a/lib/widgets/actions.dart b/lib/widgets/actions.dart index 814b56d009..d49ce690d0 100644 --- a/lib/widgets/actions.dart +++ b/lib/widgets/actions.dart @@ -24,9 +24,8 @@ abstract final class ZulipAction { /// for details on the UI feedback, see there. static Future markNarrowAsRead(BuildContext context, Narrow narrow) async { final store = PerAccountStoreWidget.of(context); - final connection = store.connection; final zulipLocalizations = ZulipLocalizations.of(context); - final useLegacy = connection.zulipFeatureLevel! < 155; // TODO(server-6) + final useLegacy = store.zulipFeatureLevel < 155; // TODO(server-6) if (useLegacy) { try { await _legacyMarkNarrowAsRead(context, narrow); @@ -78,8 +77,7 @@ abstract final class ZulipAction { Message message, Narrow narrow, ) async { - final connection = PerAccountStoreWidget.of(context).connection; - assert(connection.zulipFeatureLevel! >= 155); // TODO(server-6) + assert(PerAccountStoreWidget.of(context).zulipFeatureLevel >= 155); // TODO(server-6) final zulipLocalizations = ZulipLocalizations.of(context); await updateMessageFlagsStartingFromAnchor( context: context, diff --git a/lib/widgets/autocomplete.dart b/lib/widgets/autocomplete.dart index 40d1f2bf16..96f2192edd 100644 --- a/lib/widgets/autocomplete.dart +++ b/lib/widgets/autocomplete.dart @@ -241,7 +241,7 @@ class _MentionAutocompleteItem extends StatelessWidget { required PerAccountStore store, }) { final isDmNarrow = narrow is DmNarrow; - final isChannelWildcardAvailable = store.account.zulipFeatureLevel >= 247; // TODO(server-9) + final isChannelWildcardAvailable = store.zulipFeatureLevel >= 247; // TODO(server-9) final localizations = ZulipLocalizations.of(context); final description = switch (wildcardOption) { WildcardMentionOption.all || WildcardMentionOption.everyone => isDmNarrow diff --git a/lib/widgets/profile.dart b/lib/widgets/profile.dart index 8bef274a99..a10f17df2e 100644 --- a/lib/widgets/profile.dart +++ b/lib/widgets/profile.dart @@ -40,7 +40,7 @@ class ProfilePage extends StatelessWidget { /// /// Returns null if self-user isn't able to see [user]'s real email address. String? _getDisplayEmailFor(User user, {required PerAccountStore store}) { - if (store.account.zulipFeatureLevel >= 163) { // TODO(server-7) + if (store.zulipFeatureLevel >= 163) { // TODO(server-7) // A non-null value means self-user has access to [user]'s real email, // while a null value means it doesn't have access to the email. // Search for "delivery_email" in https://zulip.com/api/register-queue. From 1fd0c3f6d54b37b9e50075d6db6ced2ac853f58f Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 4 Feb 2025 21:33:00 -0800 Subject: [PATCH 10/19] user [nfc]: Move selfUserId to UserStore --- lib/model/store.dart | 15 +++++++-------- lib/model/user.dart | 15 +++++++++++++-- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index 1b64acbc5d..265fd785cd 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -308,7 +308,6 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel emoji: EmojiStoreImpl( realmUrl: realmUrl, allRealmEmoji: initialSnapshot.realmEmoji), accountId: accountId, - selfUserId: account.userId, userSettings: initialSnapshot.userSettings, typingNotifier: TypingNotifier( connection: connection, @@ -317,7 +316,9 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel typingStartedWaitPeriod: Duration( milliseconds: initialSnapshot.serverTypingStartedWaitPeriodMilliseconds), ), - users: UserStoreImpl(initialSnapshot: initialSnapshot), + users: UserStoreImpl( + selfUserId: account.userId, + initialSnapshot: initialSnapshot), typingStatus: TypingStatus( selfUserId: account.userId, typingStartedExpiryPeriod: Duration(milliseconds: initialSnapshot.serverTypingStartedExpiryPeriodMilliseconds), @@ -348,7 +349,6 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel required this.emailAddressVisibility, required EmojiStoreImpl emoji, required this.accountId, - required this.selfUserId, required this.userSettings, required this.typingNotifier, required UserStoreImpl users, @@ -358,8 +358,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel required this.unreads, required this.recentDmConversationsView, required this.recentSenders, - }) : assert(selfUserId == globalStore.getAccount(accountId)!.userId), - assert(realmUrl == globalStore.getAccount(accountId)!.realmUrl), + }) : assert(realmUrl == globalStore.getAccount(accountId)!.realmUrl), assert(realmUrl == connection.realmUrl), assert(emoji.realmUrl == realmUrl), _globalStore = globalStore, @@ -457,9 +456,6 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel /// Will throw if called after [dispose] has been called. Account get account => _globalStore.getAccount(accountId)!; - /// Always equal to `account.userId`. - final int selfUserId; - final UserSettings? userSettings; // TODO(server-5) final TypingNotifier typingNotifier; @@ -467,6 +463,9 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel //////////////////////////////// // Users and data about them. + @override + int get selfUserId => _users.selfUserId; + @override Map get users => _users.users; diff --git a/lib/model/user.dart b/lib/model/user.dart index 583c3b9d62..764f51fd72 100644 --- a/lib/model/user.dart +++ b/lib/model/user.dart @@ -5,6 +5,12 @@ import 'localizations.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]. + int get selfUserId; + /// All known users in the realm, by [User.userId]. /// /// There may be other users not found in this map, for multiple reasons: @@ -41,13 +47,18 @@ mixin UserStore { /// itself. Other code accesses this functionality through [PerAccountStore], /// or through the mixin [UserStore] which describes its interface. class UserStoreImpl with UserStore { - UserStoreImpl({required InitialSnapshot initialSnapshot}) - : users = Map.fromEntries( + UserStoreImpl({ + required this.selfUserId, + required InitialSnapshot initialSnapshot, + }) : users = Map.fromEntries( initialSnapshot.realmUsers .followedBy(initialSnapshot.realmNonActiveUsers) .followedBy(initialSnapshot.crossRealmBots) .map((user) => MapEntry(user.userId, user))); + @override + final int selfUserId; + @override final Map users; From 626d47362c604273fde208b7df00bfe638b115b0 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 4 Feb 2025 21:37:08 -0800 Subject: [PATCH 11/19] user [nfc]: Add a selfUser getter --- lib/model/user.dart | 8 ++++++++ lib/widgets/compose_box.dart | 3 +-- lib/widgets/inbox.dart | 2 +- lib/widgets/recent_dm_conversations.dart | 5 ++--- 4 files changed, 12 insertions(+), 6 deletions(-) diff --git a/lib/model/user.dart b/lib/model/user.dart index 764f51fd72..b0cf9e1309 100644 --- a/lib/model/user.dart +++ b/lib/model/user.dart @@ -9,6 +9,8 @@ mixin UserStore { /// 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; /// All known users in the realm, by [User.userId]. @@ -31,6 +33,12 @@ mixin UserStore { /// Consider using [userDisplayName]. Map get users; + /// The [User] object for the "self-user", + /// i.e. the account the person using this app is logged into. + /// + /// When only the user ID is needed, see [selfUserId]. + User get selfUser => users[selfUserId]!; + /// The name to show the given user as in the UI, even for unknown users. /// /// This is the user's [User.fullName] if the user is known, diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 5a4473d818..62a3e6b945 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1423,9 +1423,8 @@ class _ComposeBoxState extends State with PerAccountStoreAwareStateM case ChannelNarrow(:final streamId): case TopicNarrow(:final streamId): final channel = store.streams[streamId]; - final selfUser = store.users[store.selfUserId]!; if (channel == null || !store.hasPostingPermission(inChannel: channel, - user: selfUser, byDate: DateTime.now())) { + user: store.selfUser, byDate: DateTime.now())) { return _ErrorBanner(label: ZulipLocalizations.of(context).errorBannerCannotPostInChannelLabel); } diff --git a/lib/widgets/inbox.dart b/lib/widgets/inbox.dart index 17718c5a96..480437c912 100644 --- a/lib/widgets/inbox.dart +++ b/lib/widgets/inbox.dart @@ -385,7 +385,7 @@ class _DmItem extends StatelessWidget { final designVariables = DesignVariables.of(context); final title = switch (narrow.otherRecipientIds) { // TODO dedupe with [RecentDmConversationsItem] - [] => store.users[store.selfUserId]!.fullName, + [] => store.selfUser.fullName, [var otherUserId] => store.userDisplayName(otherUserId), // TODO(i18n): List formatting, like you can do in JavaScript: diff --git a/lib/widgets/recent_dm_conversations.dart b/lib/widgets/recent_dm_conversations.dart index 8170d3b231..982dde4f08 100644 --- a/lib/widgets/recent_dm_conversations.dart +++ b/lib/widgets/recent_dm_conversations.dart @@ -85,9 +85,8 @@ class RecentDmConversationsItem extends StatelessWidget { final Widget avatar; switch (narrow.otherRecipientIds) { // TODO dedupe with DM items in [InboxPage] case []: - final selfUser = store.users[store.selfUserId]!; - title = selfUser.fullName; - avatar = AvatarImage(userId: selfUser.userId, size: _avatarSize); + title = store.selfUser.fullName; + avatar = AvatarImage(userId: store.selfUserId, size: _avatarSize); case [var otherUserId]: // TODO(#296) actually don't show this row if the user is muted? // (should we offer a "spam folder" style summary screen of recent From de982003b1ec9549c2682c51607d171ade609eab Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 4 Feb 2025 21:44:39 -0800 Subject: [PATCH 12/19] compose [nfc]: Take UserStore in userMention, rather than bare Map Now that we have a UserStore type, we can give this a bit more abstraction. --- lib/model/compose.dart | 8 +++++--- lib/widgets/autocomplete.dart | 2 +- test/model/compose_test.dart | 6 +++--- test/widgets/autocomplete_test.dart | 2 +- 4 files changed, 10 insertions(+), 8 deletions(-) diff --git a/lib/model/compose.dart b/lib/model/compose.dart index 094f639398..1216b02459 100644 --- a/lib/model/compose.dart +++ b/lib/model/compose.dart @@ -5,6 +5,7 @@ import '../generated/l10n/zulip_localizations.dart'; import 'internal_link.dart'; import 'narrow.dart'; import 'store.dart'; +import 'user.dart'; /// The available user wildcard mention options, /// known to the server as [canonicalString]. @@ -127,11 +128,12 @@ String wrapWithBacktickFence({required String content, String? infoString}) { /// An @-mention of an individual user, like @**Chris Bobbe|13313**. /// /// To omit the user ID part ("|13313") whenever the name part is unambiguous, -/// pass a Map of all users we know about. This means accepting a linear scan +/// pass the full UserStore. This means accepting a linear scan /// through all users; avoid it in performance-sensitive codepaths. -String userMention(User user, {bool silent = false, Map? users}) { +String userMention(User user, {bool silent = false, UserStore? users}) { bool includeUserId = users == null - || users.values.where((u) => u.fullName == user.fullName).take(2).length == 2; + || users.users.values.where((u) => u.fullName == user.fullName) + .take(2).length == 2; return '@${silent ? '_' : ''}**${user.fullName}${includeUserId ? '|${user.userId}' : ''}**'; } diff --git a/lib/widgets/autocomplete.dart b/lib/widgets/autocomplete.dart index 96f2192edd..7a806039a4 100644 --- a/lib/widgets/autocomplete.dart +++ b/lib/widgets/autocomplete.dart @@ -202,7 +202,7 @@ class ComposeAutocomplete extends AutocompleteField(composeInputFinder).controller!.text) - .contains(userMention(user3, users: store.users)); + .contains(userMention(user3, users: store)); checkUserShown(user1, store, expected: false); checkUserShown(user2, store, expected: false); checkUserShown(user3, store, expected: false); From 8b172baf456fe95fc59e01f87d8a1370ff71308c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 4 Feb 2025 22:08:10 -0800 Subject: [PATCH 13/19] user [nfc]: Introduce senderDisplayName --- lib/model/user.dart | 16 ++++++++++++++++ test/model/user_test.dart | 30 ++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/lib/model/user.dart b/lib/model/user.dart index b0cf9e1309..234d5c2cd2 100644 --- a/lib/model/user.dart +++ b/lib/model/user.dart @@ -43,10 +43,26 @@ mixin UserStore { /// /// This is the user's [User.fullName] if the user is known, /// and otherwise a translation of "(unknown user)". + /// + /// When a [Message] is available which the user sent, + /// use [senderDisplayName] instead for a better-informed fallback. String userDisplayName(int userId) { return users[userId]?.fullName ?? GlobalLocalizations.zulipLocalizations.unknownUserName; } + + /// The name to show for the given message's sender in the UI. + /// + /// If the user is known (see [users]), this is their current [User.fullName]. + /// If unknown, this uses the fallback value conveniently provided on the + /// [Message] object itself, namely [Message.senderFullName]. + /// + /// For a user who isn't the sender of some known message, + /// see [userDisplayName]. + String senderDisplayName(Message message) { + return users[message.senderId]?.fullName + ?? message.senderFullName; + } } /// The implementation of [UserStore] that does the work. diff --git a/test/model/user_test.dart b/test/model/user_test.dart index 2087abfdd7..cdbe94953f 100644 --- a/test/model/user_test.dart +++ b/test/model/user_test.dart @@ -22,6 +22,36 @@ void main() { }); }); + group('senderDisplayName', () { + test('on a known user', () async { + final store = eg.store(); + final user = eg.user(fullName: 'Old Name'); + await store.addUser(user); + final message = eg.streamMessage(sender: user); + await store.addMessage(message); + check(store.senderDisplayName(message)).equals('Old Name'); + + // If the user's name changes, `store.senderDisplayName` should update... + await store.handleEvent(RealmUserUpdateEvent(id: 1, + userId: user.userId, fullName: 'New Name')); + check(store.senderDisplayName(message)).equals('New Name'); + // ... even though the Message object itself still has the old name. + check(store.messages[message.id]!).senderFullName.equals('Old Name'); + }); + + test('on an unknown user', () async { + final store = eg.store(); + final message = eg.streamMessage(sender: eg.user(fullName: 'Some User')); + await store.addMessage(message); + // If the user is unknown, `store.senderDisplayName` should fall back + // to the name in the message... + check(store.senderDisplayName(message)).equals('Some User'); + // ... even though `store.userDisplayName` (with no message available + // for fallback) only has a generic fallback name. + check(store.userDisplayName(message.senderId)).equals('(unknown user)'); + }); + }); + group('RealmUserUpdateEvent', () { // TODO write more tests for handling RealmUserUpdateEvent From 4adf1fc7248a5ee1a8ec3004438d35093e435c13 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 4 Feb 2025 22:15:56 -0800 Subject: [PATCH 14/19] user [nfc]: Note unknown-user crashes where senderDisplayName can help --- lib/model/compose.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/model/compose.dart b/lib/model/compose.dart index 1216b02459..b4c880dc8b 100644 --- a/lib/model/compose.dart +++ b/lib/model/compose.dart @@ -191,7 +191,7 @@ String quoteAndReplyPlaceholder( required Message message, }) { final sender = store.users[message.senderId]; - assert(sender != null); + assert(sender != null); // TODO(#716): should use `store.senderDisplayName` final url = narrowLink(store, SendableNarrow.ofMessage(message, selfUserId: store.selfUserId), nearMessageId: message.id); @@ -213,7 +213,7 @@ String quoteAndReply(PerAccountStore store, { required String rawContent, }) { final sender = store.users[message.senderId]; - assert(sender != null); + assert(sender != null); // TODO(#716): should use `store.senderDisplayName` final url = narrowLink(store, SendableNarrow.ofMessage(message, selfUserId: store.selfUserId), nearMessageId: message.id); From b61f1354d2267a62300a10e2d48bee303b2d41c6 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 4 Feb 2025 22:20:23 -0800 Subject: [PATCH 15/19] user [nfc]: Note places lacking live-update where senderDisplayName helps --- lib/widgets/lightbox.dart | 2 +- lib/widgets/message_list.dart | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/widgets/lightbox.dart b/lib/widgets/lightbox.dart index 65d013a4b6..dd2aca5b82 100644 --- a/lib/widgets/lightbox.dart +++ b/lib/widgets/lightbox.dart @@ -178,7 +178,7 @@ class _LightboxPageLayoutState extends State<_LightboxPageLayout> { child: RichText( text: TextSpan(children: [ TextSpan( - text: '${widget.message.senderFullName}\n', + text: '${widget.message.senderFullName}\n', // TODO(#716): use `store.senderDisplayName` // Restate default style: themeData.textTheme.titleLarge!.copyWith(color: appBarForegroundColor)), diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index b87d983e86..d36fff6c50 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -1352,7 +1352,7 @@ class MessageWithPossibleSender extends StatelessWidget { userId: message.senderId), const SizedBox(width: 8), Flexible( - child: Text(message.senderFullName, // TODO get from user data + child: Text(message.senderFullName, // TODO(#716): use `store.senderDisplayName` style: TextStyle( fontSize: 18, height: (22 / 18), From 6bbe20990d5672c0daedbd966010c4bec492d293 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 4 Feb 2025 22:28:08 -0800 Subject: [PATCH 16/19] autocomplete [nfc]: Make explicit why two user lookups have null-assertions --- lib/widgets/autocomplete.dart | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/widgets/autocomplete.dart b/lib/widgets/autocomplete.dart index 7a806039a4..9517da4e1f 100644 --- a/lib/widgets/autocomplete.dart +++ b/lib/widgets/autocomplete.dart @@ -200,9 +200,10 @@ class ComposeAutocomplete extends AutocompleteField Date: Tue, 4 Feb 2025 22:26:01 -0800 Subject: [PATCH 17/19] user [nfc]: Factor out a getUser method --- lib/model/compose.dart | 4 ++-- lib/model/user.dart | 13 ++++++++++--- lib/widgets/autocomplete.dart | 4 ++-- lib/widgets/compose_box.dart | 4 ++-- lib/widgets/content.dart | 2 +- lib/widgets/message_list.dart | 2 +- lib/widgets/profile.dart | 2 +- test/model/user_test.dart | 2 +- 8 files changed, 20 insertions(+), 13 deletions(-) diff --git a/lib/model/compose.dart b/lib/model/compose.dart index b4c880dc8b..8670e17e4a 100644 --- a/lib/model/compose.dart +++ b/lib/model/compose.dart @@ -190,7 +190,7 @@ String quoteAndReplyPlaceholder( PerAccountStore store, { required Message message, }) { - final sender = store.users[message.senderId]; + final sender = store.getUser(message.senderId); assert(sender != null); // TODO(#716): should use `store.senderDisplayName` final url = narrowLink(store, SendableNarrow.ofMessage(message, selfUserId: store.selfUserId), @@ -212,7 +212,7 @@ String quoteAndReply(PerAccountStore store, { required Message message, required String rawContent, }) { - final sender = store.users[message.senderId]; + final sender = store.getUser(message.senderId); assert(sender != null); // TODO(#716): should use `store.senderDisplayName` final url = narrowLink(store, SendableNarrow.ofMessage(message, selfUserId: store.selfUserId), diff --git a/lib/model/user.dart b/lib/model/user.dart index 234d5c2cd2..7c4523b80f 100644 --- a/lib/model/user.dart +++ b/lib/model/user.dart @@ -37,7 +37,14 @@ mixin UserStore { /// i.e. the account the person using this app is logged into. /// /// When only the user ID is needed, see [selfUserId]. - User get selfUser => users[selfUserId]!; + User get selfUser => getUser(selfUserId)!; + + /// The user with the given ID, if that user is known. + /// + /// There may be perfectly real users that are not known, + /// so callers must handle that possibility. + /// For details, see [users]. + User? getUser(int userId) => users[userId]; /// The name to show the given user as in the UI, even for unknown users. /// @@ -47,7 +54,7 @@ mixin UserStore { /// When a [Message] is available which the user sent, /// use [senderDisplayName] instead for a better-informed fallback. String userDisplayName(int userId) { - return users[userId]?.fullName + return getUser(userId)?.fullName ?? GlobalLocalizations.zulipLocalizations.unknownUserName; } @@ -60,7 +67,7 @@ mixin UserStore { /// For a user who isn't the sender of some known message, /// see [userDisplayName]. String senderDisplayName(Message message) { - return users[message.senderId]?.fullName + return getUser(message.senderId)?.fullName ?? message.senderFullName; } } diff --git a/lib/widgets/autocomplete.dart b/lib/widgets/autocomplete.dart index 9517da4e1f..5da053c329 100644 --- a/lib/widgets/autocomplete.dart +++ b/lib/widgets/autocomplete.dart @@ -200,7 +200,7 @@ class ComposeAutocomplete extends AutocompleteField with PerAccountStoreAwareStateM case DmNarrow(:final otherRecipientIds): final hasDeactivatedUser = otherRecipientIds.any((id) => - !(store.users[id]?.isActive ?? true)); + !(store.getUser(id)?.isActive ?? true)); if (hasDeactivatedUser) { return _ErrorBanner(label: ZulipLocalizations.of(context).errorBannerDeactivatedDmLabel); diff --git a/lib/widgets/content.dart b/lib/widgets/content.dart index 8799e8d192..728ee880f6 100644 --- a/lib/widgets/content.dart +++ b/lib/widgets/content.dart @@ -1509,7 +1509,7 @@ class AvatarImage extends StatelessWidget { @override Widget build(BuildContext context) { final store = PerAccountStoreWidget.of(context); - final user = store.users[userId]; + final user = store.getUser(userId); if (user == null) { // TODO(log) return const SizedBox.shrink(); diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index d36fff6c50..c4ca22bce3 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -1330,7 +1330,7 @@ class MessageWithPossibleSender extends StatelessWidget { final designVariables = DesignVariables.of(context); final message = item.message; - final sender = store.users[message.senderId]; + final sender = store.getUser(message.senderId); Widget? senderRow; if (item.showSender) { diff --git a/lib/widgets/profile.dart b/lib/widgets/profile.dart index a10f17df2e..00620cb82d 100644 --- a/lib/widgets/profile.dart +++ b/lib/widgets/profile.dart @@ -67,7 +67,7 @@ class ProfilePage extends StatelessWidget { Widget build(BuildContext context) { final zulipLocalizations = ZulipLocalizations.of(context); final store = PerAccountStoreWidget.of(context); - final user = store.users[userId]; + final user = store.getUser(userId); if (user == null) { return const _ProfileErrorPage(); } diff --git a/test/model/user_test.dart b/test/model/user_test.dart index cdbe94953f..63ac1589c7 100644 --- a/test/model/user_test.dart +++ b/test/model/user_test.dart @@ -60,7 +60,7 @@ void main() { final store = eg.store(initialSnapshot: eg.initialSnapshot( realmUsers: [eg.selfUser, user])); - User getUser() => store.users[user.userId]!; + User getUser() => store.getUser(user.userId)!; await store.handleEvent(RealmUserUpdateEvent(id: 1, userId: user.userId, deliveryEmail: null)); From dadf9de20ab86742865878134b1dc79a31e099d1 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 4 Feb 2025 22:35:19 -0800 Subject: [PATCH 18/19] user [nfc]: Factor out an allUsers iterable --- lib/model/autocomplete.dart | 2 +- lib/model/compose.dart | 2 +- lib/model/user.dart | 9 +++++++++ test/model/store_test.dart | 4 ++-- 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 4b734f936c..cf3b484564 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -449,7 +449,7 @@ class MentionAutocompleteView extends AutocompleteView u.fullName == user.fullName) + || users.allUsers.where((u) => u.fullName == user.fullName) .take(2).length == 2; return '@${silent ? '_' : ''}**${user.fullName}${includeUserId ? '|${user.userId}' : ''}**'; diff --git a/lib/model/user.dart b/lib/model/user.dart index 7c4523b80f..40deb908a6 100644 --- a/lib/model/user.dart +++ b/lib/model/user.dart @@ -46,6 +46,15 @@ mixin UserStore { /// For details, see [users]. User? getUser(int userId) => users[userId]; + /// All known users in the realm. + /// + /// This may have a large number of elements, like tens of thousands. + /// Consider [getUser] or other alternatives to iterating through this. + /// + /// There may be perfectly real users which are not known + /// and so are not found here. For details, see [users]. + Iterable get allUsers => users.values; + /// The name to show the given user as in the UI, even for unknown users. /// /// This is the user's [User.fullName] if the user is known, diff --git a/test/model/store_test.dart b/test/model/store_test.dart index 5adc97a39b..eb6444309d 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -433,7 +433,7 @@ void main() { // clobber the recorded registerQueue request so we can't check it. // checkLastRequest(); - check(updateMachine.store.users.values).unorderedMatches( + check(updateMachine.store.allUsers).unorderedMatches( users.map((expected) => (it) => it.fullName.equals(expected.fullName))); })); @@ -490,7 +490,7 @@ void main() { updateMachine.debugPauseLoop(); check(complete).isTrue(); // checkLastRequest(); TODO UpdateMachine.debugPauseLoop was too late; see comment above - check(updateMachine.store.users.values).unorderedMatches( + check(updateMachine.store.allUsers).unorderedMatches( users.map((expected) => (it) => it.fullName.equals(expected.fullName))); })); From 30c64a0d15e41ce693a3334b14ca6289e8be778a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 4 Feb 2025 22:44:42 -0800 Subject: [PATCH 19/19] user [nfc]: Make the actual users Map private This gives somewhat better encapsulation -- other code isn't expected to add or remove items from this map. --- lib/model/store.dart | 5 +++- lib/model/user.dart | 49 ++++++++++++++++++------------------ test/model/store_checks.dart | 1 - 3 files changed, 28 insertions(+), 27 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index 265fd785cd..94f6c56084 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -467,7 +467,10 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel int get selfUserId => _users.selfUserId; @override - Map get users => _users.users; + User? getUser(int userId) => _users.getUser(userId); + + @override + Iterable get allUsers => _users.allUsers; final UserStoreImpl _users; diff --git a/lib/model/user.dart b/lib/model/user.dart index 40deb908a6..1577e21048 100644 --- a/lib/model/user.dart +++ b/lib/model/user.dart @@ -13,9 +13,10 @@ mixin UserStore { /// For the corresponding [User] object, see [selfUser]. int get selfUserId; - /// All known users in the realm, by [User.userId]. + /// The user with the given ID, if that user is known. /// - /// There may be other users not found in this map, for multiple reasons: + /// There may be other users that are perfectly real but are + /// not known to the app, for multiple reasons: /// /// * The self-user may not have permission to see all the users in the /// realm, for example because the self-user is a guest. @@ -27,24 +28,11 @@ mixin UserStore { /// Those may therefore refer to users for which we have yet to see the /// [RealmUserAddEvent], or have already handled a [RealmUserRemoveEvent]. /// - /// Code that looks up a user in this map should therefore always handle + /// Code that looks up a user here should therefore always handle /// the possibility that the user is not found (except /// where there is a specific reason to know the user should be found). /// Consider using [userDisplayName]. - Map get users; - - /// The [User] object for the "self-user", - /// i.e. the account the person using this app is logged into. - /// - /// When only the user ID is needed, see [selfUserId]. - User get selfUser => getUser(selfUserId)!; - - /// The user with the given ID, if that user is known. - /// - /// There may be perfectly real users that are not known, - /// so callers must handle that possibility. - /// For details, see [users]. - User? getUser(int userId) => users[userId]; + User? getUser(int userId); /// All known users in the realm. /// @@ -52,8 +40,14 @@ mixin UserStore { /// Consider [getUser] or other alternatives to iterating through this. /// /// There may be perfectly real users which are not known - /// and so are not found here. For details, see [users]. - Iterable get allUsers => users.values; + /// and so are not found here. For details, see [getUser]. + Iterable get allUsers; + + /// The [User] object for the "self-user", + /// i.e. the account the person using this app is logged into. + /// + /// When only the user ID is needed, see [selfUserId]. + User get selfUser => getUser(selfUserId)!; /// The name to show the given user as in the UI, even for unknown users. /// @@ -69,7 +63,7 @@ mixin UserStore { /// The name to show for the given message's sender in the UI. /// - /// If the user is known (see [users]), this is their current [User.fullName]. + /// If the user is known (see [getUser]), this is their current [User.fullName]. /// If unknown, this uses the fallback value conveniently provided on the /// [Message] object itself, namely [Message.senderFullName]. /// @@ -90,7 +84,7 @@ class UserStoreImpl with UserStore { UserStoreImpl({ required this.selfUserId, required InitialSnapshot initialSnapshot, - }) : users = Map.fromEntries( + }) : _users = Map.fromEntries( initialSnapshot.realmUsers .followedBy(initialSnapshot.realmNonActiveUsers) .followedBy(initialSnapshot.crossRealmBots) @@ -99,19 +93,24 @@ class UserStoreImpl with UserStore { @override final int selfUserId; + final Map _users; + + @override + User? getUser(int userId) => _users[userId]; + @override - final Map users; + Iterable get allUsers => _users.values; void handleRealmUserEvent(RealmUserEvent event) { switch (event) { case RealmUserAddEvent(): - users[event.person.userId] = event.person; + _users[event.person.userId] = event.person; case RealmUserRemoveEvent(): - users.remove(event.userId); + _users.remove(event.userId); case RealmUserUpdateEvent(): - final user = users[event.userId]; + final user = _users[event.userId]; if (user == null) { return; // TODO log } diff --git a/test/model/store_checks.dart b/test/model/store_checks.dart index f2ef63cd4b..00ada1eea5 100644 --- a/test/model/store_checks.dart +++ b/test/model/store_checks.dart @@ -39,7 +39,6 @@ extension PerAccountStoreChecks on Subject { Subject get account => has((x) => x.account, 'account'); Subject get selfUserId => has((x) => x.selfUserId, 'selfUserId'); Subject get userSettings => has((x) => x.userSettings, 'userSettings'); - Subject> get users => has((x) => x.users, 'users'); Subject> get streams => has((x) => x.streams, 'streams'); Subject> get streamsByName => has((x) => x.streamsByName, 'streamsByName'); Subject> get subscriptions => has((x) => x.subscriptions, 'subscriptions');