diff --git a/lib/model/channel.dart b/lib/model/channel.dart index a13e19cc18..4dbc61756e 100644 --- a/lib/model/channel.dart +++ b/lib/model/channel.dart @@ -1,3 +1,5 @@ +import 'dart:collection'; + import 'package:flutter/foundation.dart'; import '../api/model/events.dart'; @@ -41,6 +43,8 @@ mixin ChannelStore { /// /// For policies directly applicable in the UI, see /// [isTopicVisibleInStream] and [isTopicVisible]. + /// + /// Topics are treated case-insensitively; see [TopicName.isSameAs]. UserTopicVisibilityPolicy topicVisibilityPolicy(int streamId, TopicName topic); /// The raw data structure underlying [topicVisibilityPolicy]. @@ -171,13 +175,13 @@ class ChannelStoreImpl with ChannelStore { streams.putIfAbsent(stream.streamId, () => stream); } - final topicVisibility = >{}; + final topicVisibility = >{}; for (final item in initialSnapshot.userTopics ?? const []) { if (_warnInvalidVisibilityPolicy(item.visibilityPolicy)) { // Not a value we expect. Keep it out of our data structures. // TODO(log) continue; } - final forStream = topicVisibility.putIfAbsent(item.streamId, () => {}); + final forStream = topicVisibility.putIfAbsent(item.streamId, () => makeTopicKeyedMap()); forStream[item.topicName] = item.visibilityPolicy; } @@ -204,9 +208,9 @@ class ChannelStoreImpl with ChannelStore { final Map subscriptions; @override - Map> get debugTopicVisibility => topicVisibility; + Map> get debugTopicVisibility => topicVisibility; - final Map> topicVisibility; + final Map> topicVisibility; @override UserTopicVisibilityPolicy topicVisibilityPolicy(int streamId, TopicName topic) { @@ -364,8 +368,26 @@ class ChannelStoreImpl with ChannelStore { topicVisibility.remove(event.streamId); } } else { - final forStream = topicVisibility.putIfAbsent(event.streamId, () => {}); + final forStream = topicVisibility.putIfAbsent(event.streamId, () => makeTopicKeyedMap()); forStream[event.topicName] = visibilityPolicy; } } } + +/// A [Map] with [TopicName] keys and [V] values. +/// +/// When one of these is created by [makeTopicKeyedMap], +/// key equality is done case-insensitively; see there. +/// +/// This type should only be used for maps created by [makeTopicKeyedMap]. +/// It would be nice to enforce that. +typedef TopicKeyedMap = Map; + +/// Make a case-insensitive, case-preserving [TopicName]-keyed [LinkedHashMap]. +/// +/// The equality function is [TopicName.isSameAs], +/// and the hash code is [String.hashCode] of [TopicName.canonicalize]. +TopicKeyedMap makeTopicKeyedMap() => LinkedHashMap( + equals: (a, b) => a.isSameAs(b), + hashCode: (k) => k.canonicalize().hashCode, +); diff --git a/lib/model/recent_senders.dart b/lib/model/recent_senders.dart index a5c4bca778..f8b046641d 100644 --- a/lib/model/recent_senders.dart +++ b/lib/model/recent_senders.dart @@ -4,6 +4,7 @@ import 'package:flutter/foundation.dart'; import '../api/model/events.dart'; import '../api/model/model.dart'; import 'algorithms.dart'; +import 'channel.dart'; /// Tracks the latest messages sent by each user, in each stream and topic. /// @@ -16,7 +17,7 @@ class RecentSenders { // topicSenders[streamId][topic][senderId] = MessageIdTracker @visibleForTesting - final Map>> topicSenders = {}; + final Map>> topicSenders = {}; /// The latest message the given user sent to the given stream, /// or null if no such message is known. @@ -27,6 +28,8 @@ class RecentSenders { /// The latest message the given user sent to the given topic, /// or null if no such message is known. + /// + /// Topics are treated case-insensitively; see [TopicName.isSameAs]. int? latestMessageIdOfSenderInTopic({ required int streamId, required TopicName topic, @@ -53,7 +56,7 @@ class RecentSenders { } for (final entry in messagesByUserInTopic.entries) { final (streamId, topic, senderId) = entry.key; - (((topicSenders[streamId] ??= {})[topic] ??= {}) + (((topicSenders[streamId] ??= makeTopicKeyedMap())[topic] ??= {}) [senderId] ??= MessageIdTracker()).addAll(entry.value); } } @@ -64,7 +67,7 @@ class RecentSenders { final StreamMessage(:streamId, :topic, :senderId, id: int messageId) = message; ((streamSenders[streamId] ??= {}) [senderId] ??= MessageIdTracker()).add(messageId); - (((topicSenders[streamId] ??= {})[topic] ??= {}) + (((topicSenders[streamId] ??= makeTopicKeyedMap())[topic] ??= {}) [senderId] ??= MessageIdTracker()).add(messageId); } diff --git a/lib/model/unreads.dart b/lib/model/unreads.dart index ad6b7b4b8d..e65197a980 100644 --- a/lib/model/unreads.dart +++ b/lib/model/unreads.dart @@ -41,14 +41,22 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier { required CorePerAccountStore core, required ChannelStore channelStore, }) { - final streams = >>{}; + final streams = >>{}; final dms = >{}; final mentions = Set.of(initial.mentions); for (final unreadChannelSnapshot in initial.channels) { final streamId = unreadChannelSnapshot.streamId; final topic = unreadChannelSnapshot.topic; - (streams[streamId] ??= {})[topic] = QueueList.from(unreadChannelSnapshot.unreadMessageIds); + final topics = (streams[streamId] ??= makeTopicKeyedMap()); + topics.update(topic, + // Older servers differentiate topics case-sensitively, but shouldn't: + // https://github.com/zulip/zulip/pull/31869 + // Our topic-keyed map is case-insensitive. When we've seen this + // topic before, modulo case, aggregate instead of clobbering. + // TODO(server-10) simplify away + (value) => setUnion(value, unreadChannelSnapshot.unreadMessageIds), + ifAbsent: () => QueueList.from(unreadChannelSnapshot.unreadMessageIds)); } for (final unreadDmSnapshot in initial.dms) { @@ -88,7 +96,10 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier { // int count; /// Unread stream messages, as: stream ID → topic → message IDs (sorted). - final Map>> streams; + /// + /// The topic-keyed map is case-insensitive and case-preserving; + /// it comes from [makeTopicKeyedMap]. + final Map>> streams; /// Unread DM messages, as: DM narrow → message IDs (sorted). final Map> dms; @@ -405,7 +416,7 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier { _slowRemoveAllInDms(messageIdsSet); } case UpdateMessageFlagsRemoveEvent(): - final newlyUnreadInStreams = >>{}; + final newlyUnreadInStreams = >>{}; final newlyUnreadInDms = >{}; for (final messageId in event.messages) { final detail = event.messageDetails![messageId]; @@ -420,7 +431,7 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier { } switch (detail.type) { case MessageType.stream: - final topics = (newlyUnreadInStreams[detail.streamId!] ??= {}); + final topics = (newlyUnreadInStreams[detail.streamId!] ??= makeTopicKeyedMap()); final messageIds = (topics[detail.topic!] ??= QueueList()); messageIds.add(messageId); case MessageType.direct: @@ -488,14 +499,15 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier { } void _addLastInStreamTopic(int messageId, int streamId, TopicName topic) { - ((streams[streamId] ??= {})[topic] ??= QueueList()).addLast(messageId); + ((streams[streamId] ??= makeTopicKeyedMap())[topic] ??= QueueList()) + .addLast(messageId); } // [messageIds] must be sorted ascending and without duplicates. void _addAllInStreamTopic(QueueList messageIds, int streamId, TopicName topic) { assert(messageIds.isNotEmpty); assert(isSortedWithoutDuplicates(messageIds)); - final topics = streams[streamId] ??= {}; + final topics = streams[streamId] ??= makeTopicKeyedMap(); topics.update(topic, ifAbsent: () => messageIds, // setUnion dedupes existing and incoming unread IDs, diff --git a/test/model/channel_test.dart b/test/model/channel_test.dart index 5c00aca814..0fd83f64f9 100644 --- a/test/model/channel_test.dart +++ b/test/model/channel_test.dart @@ -1,4 +1,3 @@ - import 'package:checks/checks.dart'; import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/events.dart'; @@ -7,7 +6,9 @@ import 'package:zulip/api/model/model.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/model/channel.dart'; +import '../api/model/model_checks.dart'; import '../example_data.dart' as eg; +import '../stdlib_checks.dart'; import 'test_store.dart'; void main() { @@ -146,7 +147,7 @@ void main() { test('with nothing for topic', () async { final store = eg.store(); - await store.addUserTopic(stream1, 'other topic', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream1, 'other topic', UserTopicVisibilityPolicy.muted); check(store.topicVisibilityPolicy(stream1.streamId, eg.t('topic'))) .equals(UserTopicVisibilityPolicy.none); }); @@ -158,9 +159,13 @@ void main() { UserTopicVisibilityPolicy.unmuted, UserTopicVisibilityPolicy.followed, ]) { - await store.addUserTopic(stream1, 'topic', policy); + await store.setUserTopic(stream1, 'topic', policy); check(store.topicVisibilityPolicy(stream1.streamId, eg.t('topic'))) .equals(policy); + + // Case-insensitive + check(store.topicVisibilityPolicy(stream1.streamId, eg.t('ToPiC'))) + .equals(policy); } }); }); @@ -193,27 +198,39 @@ void main() { final store = eg.store(); await store.addStream(stream1); await store.addSubscription(eg.subscription(stream1)); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); check(store.isTopicVisibleInStream(stream1.streamId, eg.t('topic'))).isFalse(); check(store.isTopicVisible (stream1.streamId, eg.t('topic'))).isFalse(); + + // Case-insensitive + check(store.isTopicVisibleInStream(stream1.streamId, eg.t('ToPiC'))).isFalse(); + check(store.isTopicVisible (stream1.streamId, eg.t('ToPiC'))).isFalse(); }); test('with policy unmuted', () async { final store = eg.store(); await store.addStream(stream1); await store.addSubscription(eg.subscription(stream1, isMuted: true)); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.unmuted); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.unmuted); check(store.isTopicVisibleInStream(stream1.streamId, eg.t('topic'))).isTrue(); check(store.isTopicVisible (stream1.streamId, eg.t('topic'))).isTrue(); + + // Case-insensitive + check(store.isTopicVisibleInStream(stream1.streamId, eg.t('tOpIc'))).isTrue(); + check(store.isTopicVisible (stream1.streamId, eg.t('tOpIc'))).isTrue(); }); test('with policy followed', () async { final store = eg.store(); await store.addStream(stream1); await store.addSubscription(eg.subscription(stream1, isMuted: true)); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.followed); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.followed); check(store.isTopicVisibleInStream(stream1.streamId, eg.t('topic'))).isTrue(); check(store.isTopicVisible (stream1.streamId, eg.t('topic'))).isTrue(); + + // Case-insensitive + check(store.isTopicVisibleInStream(stream1.streamId, eg.t('TOPIC'))).isTrue(); + check(store.isTopicVisible (stream1.streamId, eg.t('TOPIC'))).isTrue(); }); }); @@ -221,6 +238,18 @@ void main() { UserTopicEvent mkEvent(UserTopicVisibilityPolicy policy) => eg.userTopicEvent(stream1.streamId, 'topic', policy); + // For testing case-insensitivity + UserTopicEvent mkEventDifferentlyCased(UserTopicVisibilityPolicy policy) => + eg.userTopicEvent(stream1.streamId, 'ToPiC', policy); + + assert(() { + // (sanity check on mkEvent and mkEventDifferentlyCased) + final event1 = mkEvent(UserTopicVisibilityPolicy.followed); + final event2 = mkEventDifferentlyCased(UserTopicVisibilityPolicy.followed); + return event1.topicName.isSameAs(event2.topicName) + && event1.topicName.apiName != event2.topicName.apiName; + }()); + void checkChanges(PerAccountStore store, UserTopicVisibilityPolicy newPolicy, UserTopicVisibilityEffect expectedInStream, @@ -228,6 +257,10 @@ void main() { final event = mkEvent(newPolicy); check(store.willChangeIfTopicVisibleInStream(event)).equals(expectedInStream); check(store.willChangeIfTopicVisible (event)).equals(expectedOverall); + + final event2 = mkEventDifferentlyCased(newPolicy); + check(store.willChangeIfTopicVisibleInStream(event2)).equals(expectedInStream); + check(store.willChangeIfTopicVisible (event2)).equals(expectedOverall); } test('stream not muted, policy none -> followed, no change', () async { @@ -341,7 +374,7 @@ void main() { group('events', () { test('add with new stream', () async { final store = eg.store(); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); compareTopicVisibility(store, [ eg.userTopicItem(stream1, 'topic', UserTopicVisibilityPolicy.muted), ]); @@ -349,8 +382,8 @@ void main() { test('add in existing stream', () async { final store = eg.store(); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); - await store.addUserTopic(stream1, 'other topic', UserTopicVisibilityPolicy.unmuted); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream1, 'other topic', UserTopicVisibilityPolicy.unmuted); compareTopicVisibility(store, [ eg.userTopicItem(stream1, 'topic', UserTopicVisibilityPolicy.muted), eg.userTopicItem(stream1, 'other topic', UserTopicVisibilityPolicy.unmuted), @@ -359,18 +392,24 @@ void main() { test('update existing policy', () async { final store = eg.store(); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.unmuted); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.unmuted); compareTopicVisibility(store, [ eg.userTopicItem(stream1, 'topic', UserTopicVisibilityPolicy.unmuted), ]); + + // case-insensitivity + await store.setUserTopic(stream1, 'ToPiC', UserTopicVisibilityPolicy.followed); + compareTopicVisibility(store, [ + eg.userTopicItem(stream1, 'topic', UserTopicVisibilityPolicy.followed), + ]); }); test('remove, with others in stream', () async { final store = eg.store(); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); - await store.addUserTopic(stream1, 'other topic', UserTopicVisibilityPolicy.unmuted); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.none); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream1, 'other topic', UserTopicVisibilityPolicy.unmuted); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.none); compareTopicVisibility(store, [ eg.userTopicItem(stream1, 'other topic', UserTopicVisibilityPolicy.unmuted), ]); @@ -378,16 +417,18 @@ void main() { test('remove, as last in stream', () async { final store = eg.store(); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.none); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); + // case-insensitivity + await store.setUserTopic(stream1, 'ToPiC', UserTopicVisibilityPolicy.none); compareTopicVisibility(store, [ ]); }); test('treat unknown enum value as removing', () async { final store = eg.store(); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.unknown); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); + // case-insensitivity + await store.setUserTopic(stream1, 'ToPiC', UserTopicVisibilityPolicy.unknown); compareTopicVisibility(store, [ ]); }); @@ -404,7 +445,8 @@ void main() { ])); check(store.topicVisibilityPolicy(stream.streamId, eg.t('topic 1'))) .equals(UserTopicVisibilityPolicy.muted); - check(store.topicVisibilityPolicy(stream.streamId, eg.t('topic 2'))) + // case-insensitivity + check(store.topicVisibilityPolicy(stream.streamId, eg.t('ToPiC 2'))) .equals(UserTopicVisibilityPolicy.unmuted); check(store.topicVisibilityPolicy(stream.streamId, eg.t('topic 3'))) .equals(UserTopicVisibilityPolicy.followed); @@ -412,4 +454,30 @@ void main() { .equals(UserTopicVisibilityPolicy.none); }); }); + + group('makeTopicKeyedMap', () { + test('"a" equals "A"', () { + final map = makeTopicKeyedMap() + ..[eg.t('a')] = 1 + ..[eg.t('A')] = 2; + check(map) + ..[eg.t('a')].equals(2) + ..[eg.t('A')].equals(2) + ..entries.which((it) => it.single + ..key.apiName.equals('a') + ..value.equals(2)); + }); + + test('"A" equals "a"', () { + final map = makeTopicKeyedMap() + ..[eg.t('A')] = 1 + ..[eg.t('a')] = 2; + check(map) + ..[eg.t('A')].equals(2) + ..[eg.t('a')].equals(2) + ..entries.which((it) => it.single + ..key.apiName.equals('A') + ..value.equals(2)); + }); + }); } diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 0779af52a1..dbe2e6d8eb 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -359,7 +359,7 @@ void main() { final stream = eg.stream(); final otherStream = eg.stream(); await prepare(narrow: ChannelNarrow(stream.streamId)); - await store.addUserTopic(stream, 'muted', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream, 'muted', UserTopicVisibilityPolicy.muted); await prepareOutboxMessagesTo([ StreamDestination(stream.streamId, eg.t('topic')), StreamDestination(stream.streamId, eg.t('muted')), @@ -1124,7 +1124,7 @@ void main() { eg.streamMessage(id: 2, stream: stream, topic: topic), ]; await prepareMessages(foundOldest: true, messages: messages); - await store.addUserTopic(stream, 'muted', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream, 'muted', UserTopicVisibilityPolicy.muted); await prepareOutboxMessagesTo([ StreamDestination(stream.streamId, eg.t(topic)), StreamDestination(stream.streamId, eg.t('muted')), @@ -2218,9 +2218,9 @@ void main() { await prepare(narrow: const CombinedFeedNarrow()); await store.addStreams([stream1, stream2]); await store.addSubscription(eg.subscription(stream1)); - await store.addUserTopic(stream1, 'B', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream1, 'B', UserTopicVisibilityPolicy.muted); await store.addSubscription(eg.subscription(stream2, isMuted: true)); - await store.addUserTopic(stream2, 'C', UserTopicVisibilityPolicy.unmuted); + await store.setUserTopic(stream2, 'C', UserTopicVisibilityPolicy.unmuted); // Check filtering on fetchInitial… await prepareMessages(foundOldest: false, messages: [ @@ -2278,8 +2278,8 @@ void main() { await prepare(narrow: ChannelNarrow(stream.streamId)); await store.addStream(stream); await store.addSubscription(eg.subscription(stream, isMuted: true)); - await store.addUserTopic(stream, 'A', UserTopicVisibilityPolicy.unmuted); - await store.addUserTopic(stream, 'C', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream, 'A', UserTopicVisibilityPolicy.unmuted); + await store.setUserTopic(stream, 'C', UserTopicVisibilityPolicy.muted); // Check filtering on fetchInitial… await prepareMessages(foundOldest: false, messages: [ @@ -2323,7 +2323,7 @@ void main() { await prepare(narrow: ChannelNarrow(stream.streamId)); await store.addStream(stream); await store.addSubscription(eg.subscription(stream)); - await store.addUserTopic(stream, 'muted', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream, 'muted', UserTopicVisibilityPolicy.muted); await prepareMessages(foundOldest: true, messages: []); // Check filtering on sent messages… @@ -2356,7 +2356,7 @@ void main() { await prepare(narrow: eg.topicNarrow(stream.streamId, 'A')); await store.addStream(stream); await store.addSubscription(eg.subscription(stream, isMuted: true)); - await store.addUserTopic(stream, 'A', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream, 'A', UserTopicVisibilityPolicy.muted); // Check filtering on fetchInitial… await prepareMessages(foundOldest: false, messages: [ @@ -2386,7 +2386,7 @@ void main() { const mutedTopic = 'muted'; await prepare(narrow: const MentionsNarrow()); await store.addStream(stream); - await store.addUserTopic(stream, mutedTopic, UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream, mutedTopic, UserTopicVisibilityPolicy.muted); await store.addSubscription(eg.subscription(stream, isMuted: true)); List getMessages(int startingId) => [ @@ -2424,7 +2424,7 @@ void main() { const mutedTopic = 'muted'; await prepare(narrow: const StarredMessagesNarrow()); await store.addStream(stream); - await store.addUserTopic(stream, mutedTopic, UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream, mutedTopic, UserTopicVisibilityPolicy.muted); await store.addSubscription(eg.subscription(stream, isMuted: true)); List getMessages(int startingId) => [ diff --git a/test/model/recent_senders_test.dart b/test/model/recent_senders_test.dart index 602362cbcc..990811d216 100644 --- a/test/model/recent_senders_test.dart +++ b/test/model/recent_senders_test.dart @@ -1,13 +1,16 @@ import 'package:checks/checks.dart'; +import 'package:collection/collection.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/model.dart'; +import 'package:zulip/model/channel.dart'; import 'package:zulip/model/recent_senders.dart'; import '../example_data.dart' as eg; /// [messages] should be sorted by [id] ascending. void checkMatchesMessages(RecentSenders model, List messages) { final Map>> messagesByUserInStream = {}; - final Map>>> messagesByUserInTopic = {}; + final Map>>> messagesByUserInTopic = {}; for (final message in messages) { if (message is! StreamMessage) { throw UnsupportedError('Message of type ${message.runtimeType} is not expected.'); @@ -17,7 +20,7 @@ void checkMatchesMessages(RecentSenders model, List messages) { ((messagesByUserInStream[streamId] ??= {}) [senderId] ??= {}).add(messageId); - (((messagesByUserInTopic[streamId] ??= {})[topic] ??= {}) + (((messagesByUserInTopic[streamId] ??= makeTopicKeyedMap())[topic] ??= {}) [senderId] ??= {}).add(messageId); } @@ -125,6 +128,16 @@ void main() { [eg.streamMessage(stream: streamA, topic: 'other', sender: userX)]); }); + test('case-insensitive topics', () { + checkHandleMessages( + [eg.streamMessage(stream: streamA, topic: 'thing', sender: userX)], + [eg.streamMessage(stream: streamA, topic: 'ThInG', sender: userX)]); + check(model.topicSenders).values.single.deepEquals( + {eg.t('thing'): + {userX.userId: (Subject it) => + it.isA().ids.length.equals(2)}}); + }); + test('add new stream', () { checkHandleMessages( [eg.streamMessage(stream: streamA, topic: 'thing', sender: userX)], @@ -161,6 +174,16 @@ void main() { Map.fromEntries(messages.map((msg) => MapEntry(msg.id, msg)))); checkMatchesMessages(model, [messages[1]]); + + // check case-insensitivity + model.handleDeleteMessageEvent(DeleteMessageEvent( + id: 0, + messageIds: [messages[1].id], + messageType: MessageType.stream, + streamId: stream.streamId, + topic: eg.t('oThEr'), + ), {messages[1].id: messages[1]}); + checkMatchesMessages(model, []); }); test('RecentSenders.latestMessageIdOfSenderInStream', () { @@ -200,6 +223,9 @@ void main() { check(model.latestMessageIdOfSenderInTopic(streamId: 1, topic: eg.t('a'), senderId: 10)).equals(300); + // case-insensitivity + check(model.latestMessageIdOfSenderInTopic(streamId: 1, + topic: eg.t('A'), senderId: 10)).equals(300); // No message of user 20 in topic "a". check(model.latestMessageIdOfSenderInTopic(streamId: 1, topic: eg.t('a'), senderId: 20)).equals(null); @@ -211,3 +237,7 @@ void main() { topic: eg.t('a'), senderId: 10)).equals(null); }); } + +extension MessageIdTrackerChecks on Subject { + Subject> get ids => has((x) => x.ids, 'ids'); +} diff --git a/test/model/test_store.dart b/test/model/test_store.dart index d979e737f9..1d086d4290 100644 --- a/test/model/test_store.dart +++ b/test/model/test_store.dart @@ -287,7 +287,7 @@ extension PerAccountStoreTestExtension on PerAccountStore { await handleEvent(SubscriptionAddEvent(id: 1, subscriptions: subscriptions)); } - Future addUserTopic(ZulipStream stream, String topic, UserTopicVisibilityPolicy visibilityPolicy) async { + Future setUserTopic(ZulipStream stream, String topic, UserTopicVisibilityPolicy visibilityPolicy) async { await handleEvent(eg.userTopicEvent(stream.streamId, topic, visibilityPolicy)); } diff --git a/test/model/unreads_test.dart b/test/model/unreads_test.dart index de554b220c..e1c5ca6986 100644 --- a/test/model/unreads_test.dart +++ b/test/model/unreads_test.dart @@ -4,14 +4,29 @@ import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; +import 'package:zulip/model/algorithms.dart'; +import 'package:zulip/model/channel.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/model/unreads.dart'; import '../example_data.dart' as eg; +import '../stdlib_checks.dart'; import 'test_store.dart'; import 'unreads_checks.dart'; +void checkInvariants(Unreads model) { + for (final MapEntry(value: topics) in model.streams.entries) { + for (final MapEntry(value: messageIds) in topics.entries) { + check(isSortedWithoutDuplicates(messageIds)).isTrue(); + } + } + + for (final MapEntry(value: messageIds) in model.dms.entries) { + check(isSortedWithoutDuplicates(messageIds)).isTrue(); + } +} + void main() { // These variables are the common state operated on by each test. // Each test case calls [prepare] to initialize them. @@ -23,7 +38,10 @@ void main() { check(notifiedCount).equals(count); notifiedCount = 0; } - void checkNotNotified() => checkNotified(count: 0); + void checkNotNotified() { + checkInvariants(model); + checkNotified(count: 0); + } void checkNotifiedOnce() => checkNotified(count: 1); /// Initialize [model] and the rest of the test state. @@ -38,15 +56,18 @@ void main() { ), }) { store = eg.store(initialSnapshot: eg.initialSnapshot(unreadMsgs: initial)); + checkInvariants(store.unreads); notifiedCount = 0; model = store.unreads ..addListener(() { + checkInvariants(model); notifiedCount++; }); checkNotNotified(); } - void fillWithMessages(Iterable messages) { + void fillWithMessages(List messages) { + check(isSortedWithoutDuplicates(messages.map((m) => m.id).toList())).isTrue(); for (final message in messages) { model.handleMessageEvent(eg.messageEvent(message)); } @@ -57,7 +78,7 @@ void main() { assert(Set.of(messages.map((m) => m.id)).length == messages.length, 'checkMatchesMessages: duplicate messages in test input'); - final Map>> expectedStreams = {}; + final Map>> expectedStreams = {}; final Map> expectedDms = {}; final Set expectedMentions = {}; for (final message in messages) { @@ -66,7 +87,7 @@ void main() { } switch (message) { case StreamMessage(): - final perTopic = expectedStreams[message.streamId] ??= {}; + final perTopic = expectedStreams[message.streamId] ??= makeTopicKeyedMap(); final messageIds = perTopic[message.topic] ??= QueueList(); messageIds.add(message.id); case DmMessage(): @@ -117,16 +138,19 @@ void main() { eg.unreadChannelMsgs(streamId: stream1.streamId, topic: 'b', unreadMessageIds: [3, 4]), eg.unreadChannelMsgs(streamId: stream2.streamId, topic: 'b', unreadMessageIds: [5, 6]), eg.unreadChannelMsgs(streamId: stream2.streamId, topic: 'c', unreadMessageIds: [7, 8]), + + // TODO(server-10) drop this (see implementation) + eg.unreadChannelMsgs(streamId: stream2.streamId, topic: 'C', unreadMessageIds: [9, 10]), ], dms: [ - UnreadDmSnapshot(otherUserId: 1, unreadMessageIds: [9, 10]), - UnreadDmSnapshot(otherUserId: 2, unreadMessageIds: [11, 12]), + UnreadDmSnapshot(otherUserId: 1, unreadMessageIds: [11, 12]), + UnreadDmSnapshot(otherUserId: 2, unreadMessageIds: [13, 14]), ], huddles: [ - UnreadHuddleSnapshot(userIdsString: '1,2,${eg.selfUser.userId}', unreadMessageIds: [13, 14]), - UnreadHuddleSnapshot(userIdsString: '2,3,${eg.selfUser.userId}', unreadMessageIds: [15, 16]), + UnreadHuddleSnapshot(userIdsString: '1,2,${eg.selfUser.userId}', unreadMessageIds: [15, 16]), + UnreadHuddleSnapshot(userIdsString: '2,3,${eg.selfUser.userId}', unreadMessageIds: [17, 18]), ], - mentions: [6, 12, 16], + mentions: [6, 14, 18], oldUnreadsMissing: false, )); checkMatchesMessages([ @@ -138,14 +162,16 @@ void main() { eg.streamMessage(id: 6, stream: stream2, topic: 'b', flags: [MessageFlag.mentioned]), eg.streamMessage(id: 7, stream: stream2, topic: 'c', flags: []), eg.streamMessage(id: 8, stream: stream2, topic: 'c', flags: []), - eg.dmMessage(id: 9, from: user1, to: [eg.selfUser], flags: []), - eg.dmMessage(id: 10, from: user1, to: [eg.selfUser], flags: []), - eg.dmMessage(id: 11, from: user2, to: [eg.selfUser], flags: []), - eg.dmMessage(id: 12, from: user2, to: [eg.selfUser], flags: [MessageFlag.mentioned]), - eg.dmMessage(id: 13, from: user1, to: [user2, eg.selfUser], flags: []), - eg.dmMessage(id: 14, from: user1, to: [user2, eg.selfUser], flags: []), - eg.dmMessage(id: 15, from: user2, to: [user3, eg.selfUser], flags: []), - eg.dmMessage(id: 16, from: user2, to: [user3, eg.selfUser], flags: [MessageFlag.wildcardMentioned]), + eg.streamMessage(id: 9, stream: stream2, topic: 'C', flags: []), + eg.streamMessage(id: 10, stream: stream2, topic: 'C', flags: []), + eg.dmMessage(id: 11, from: user1, to: [eg.selfUser], flags: []), + eg.dmMessage(id: 12, from: user1, to: [eg.selfUser], flags: []), + eg.dmMessage(id: 13, from: user2, to: [eg.selfUser], flags: []), + eg.dmMessage(id: 14, from: user2, to: [eg.selfUser], flags: [MessageFlag.mentioned]), + eg.dmMessage(id: 15, from: user1, to: [user2, eg.selfUser], flags: []), + eg.dmMessage(id: 16, from: user1, to: [user2, eg.selfUser], flags: []), + eg.dmMessage(id: 17, from: user2, to: [user3, eg.selfUser], flags: []), + eg.dmMessage(id: 18, from: user2, to: [user3, eg.selfUser], flags: [MessageFlag.wildcardMentioned]), ]); }); }); @@ -160,7 +186,7 @@ void main() { await store.addSubscription(eg.subscription(stream1)); await store.addSubscription(eg.subscription(stream2)); await store.addSubscription(eg.subscription(stream3, isMuted: true)); - await store.addUserTopic(stream1, 'a', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream1, 'a', UserTopicVisibilityPolicy.muted); fillWithMessages([ eg.streamMessage(stream: stream1, topic: 'a', flags: []), eg.streamMessage(stream: stream1, topic: 'b', flags: []), @@ -178,14 +204,14 @@ void main() { prepare(); await store.addStream(stream); await store.addSubscription(eg.subscription(stream)); - await store.addUserTopic(stream, 'a', UserTopicVisibilityPolicy.unmuted); - await store.addUserTopic(stream, 'c', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream, 'a', UserTopicVisibilityPolicy.unmuted); + await store.setUserTopic(stream, 'c', UserTopicVisibilityPolicy.muted); fillWithMessages([ eg.streamMessage(stream: stream, topic: 'a', flags: []), - eg.streamMessage(stream: stream, topic: 'a', flags: []), - eg.streamMessage(stream: stream, topic: 'b', flags: []), + eg.streamMessage(stream: stream, topic: 'A', flags: []), eg.streamMessage(stream: stream, topic: 'b', flags: []), eg.streamMessage(stream: stream, topic: 'b', flags: []), + eg.streamMessage(stream: stream, topic: 'B', flags: []), eg.streamMessage(stream: stream, topic: 'c', flags: []), ]); check(model.countInChannel (stream.streamId)).equals(5); @@ -201,9 +227,13 @@ void main() { test('countInTopicNarrow', () { final stream = eg.stream(); prepare(); - fillWithMessages(List.generate(7, (i) => eg.streamMessage( - stream: stream, topic: 'a', flags: []))); - check(model.countInTopicNarrow(stream.streamId, eg.t('a'))).equals(7); + final messages = [ + ...List.generate(7, (i) => eg.streamMessage(stream: stream, topic: 'a', flags: [])), + ...List.generate(2, (i) => eg.streamMessage(stream: stream, topic: 'A', flags: [])), + ]; + fillWithMessages(messages); + check(model.countInTopicNarrow(stream.streamId, eg.t('a'))).equals(9); + check(model.countInTopicNarrow(stream.streamId, eg.t('A'))).equals(9); }); test('countInDmNarrow', () { @@ -242,9 +272,9 @@ void main() { group('isUnread', () { final unreadDmMessage = eg.dmMessage( from: eg.otherUser, to: [eg.selfUser], flags: []); + final unreadChannelMessage = eg.streamMessage(flags: []); final readDmMessage = eg.dmMessage( from: eg.otherUser, to: [eg.selfUser], flags: [MessageFlag.read]); - final unreadChannelMessage = eg.streamMessage(flags: []); final readChannelMessage = eg.streamMessage(flags: [MessageFlag.read]); final allMessages = [ @@ -351,6 +381,24 @@ void main() { }); } }); + + test('topics case-insensitive but case-preserving', () { + final stream = eg.stream(); + final message1 = eg.streamMessage(stream: stream, topic: 'aaa'); + final message2 = eg.streamMessage(stream: stream, topic: 'AaA'); + final message3 = eg.streamMessage(stream: stream, topic: 'aAa'); + prepare(); + fillWithMessages([message1]); + model.handleMessageEvent(eg.messageEvent(message2)); + model.handleMessageEvent(eg.messageEvent(message3)); + checkNotified(count: 2); + checkMatchesMessages([message1, message2, message3]); + // Redundant with checkMatchesMessages, but for explicitness here: + check(model).streams.values.single + .entries.single + ..key.equals(eg.t('aaa')) + ..value.length.equals(3); + }); }); group('DM messages', () { @@ -481,11 +529,11 @@ void main() { prepare(); await store.addStream(origChannel); await store.addSubscription(eg.subscription(origChannel)); + unreadMessages = List.generate(10, + (_) => eg.streamMessage(stream: origChannel, topic: origTopic)); readMessages = List.generate(10, (_) => eg.streamMessage(stream: origChannel, topic: origTopic, flags: [MessageFlag.read])); - unreadMessages = List.generate(10, - (_) => eg.streamMessage(stream: origChannel, topic: origTopic)); } List copyMessagesWith(Iterable messages, { @@ -600,17 +648,49 @@ void main() { test('tolerates unsorted messages', () async { await prepareStore(); final unreadMessages = List.generate(10, (i) => - eg.streamMessage( - id: 1000 - i, stream: origChannel, topic: origTopic)); + eg.streamMessage(stream: origChannel, topic: origTopic)); fillWithMessages(unreadMessages); model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom( - origMessages: unreadMessages, + origMessages: unreadMessages.reversed.toList(), newTopicStr: newTopic)); checkNotifiedOnce(); checkMatchesMessages(copyMessagesWith(unreadMessages, newTopic: newTopic)); }); + test('topics case-insensitive but case-preserving', () async { + final message1 = eg.streamMessage(stream: origChannel, topic: 'aaa', flags: []); + final message2 = eg.streamMessage(stream: origChannel, topic: 'aaa', flags: []); + final messages = [message1, message2]; + await prepareStore(); + fillWithMessages(messages); + + model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom( + // 'AAA' finds the key 'aaa' + origMessages: copyMessagesWith([message1], newTopic: 'AAA'), + newTopicStr: 'bbb')); + checkNotifiedOnce(); + checkMatchesMessages([ + ...copyMessagesWith([message1], newTopic: 'bbb'), + message2, + ]); + + model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom( + origMessages: [message2], + // 'BBB' finds the key 'bbb' + newTopicStr: 'BBB')); + checkNotifiedOnce(); + checkMatchesMessages([ + ...copyMessagesWith([message1], newTopic: 'bbb'), + ...copyMessagesWith([message2], newTopic: 'BBB'), + ]); + // Redundant with checkMatchesMessages, but for explicitness here: + check(model).streams.values.single + .entries.single + ..key.equals(eg.t('bbb')) + ..value.length.equals(2); + }); + test('tolerates unreads unknown to the model', () async { await prepareStore(); fillWithMessages(unreadMessages); @@ -672,6 +752,7 @@ void main() { fillWithMessages(messages); final expectedRemainingMessages = Set.of(messages); + assert(messages.any((m) => m.id == 14)); for (final message in messages) { final event = switch (message) { StreamMessage() => DeleteMessageEvent( @@ -679,7 +760,12 @@ void main() { messageType: MessageType.stream, messageIds: [message.id], streamId: message.streamId, - topic: message.topic, + topic: () { + if (message.id != 14) return message.topic; + final uppercase = message.topic.apiName.toUpperCase(); + assert(message.topic.apiName != uppercase); + return eg.t(uppercase); // exercise case-insensitivity of topics + }(), ), DmMessage() => DeleteMessageEvent( id: 0, diff --git a/test/stdlib_checks.dart b/test/stdlib_checks.dart index 8bfaea54fd..28b5f5e5f2 100644 --- a/test/stdlib_checks.dart +++ b/test/stdlib_checks.dart @@ -16,6 +16,11 @@ extension ListChecks on Subject> { Subject operator [](int index) => has((l) => l[index], '[$index]'); } +extension MapEntryChecks on Subject> { + Subject get key => has((e) => e.key, 'key'); + Subject get value => has((e) => e.value, 'value'); +} + extension NullableMapChecks on Subject?> { void deepEquals(Map? expected) { if (expected == null) { diff --git a/test/widgets/inbox_test.dart b/test/widgets/inbox_test.dart index 4d24e5d831..fe96b427ab 100644 --- a/test/widgets/inbox_test.dart +++ b/test/widgets/inbox_test.dart @@ -9,6 +9,7 @@ import 'package:zulip/widgets/color.dart'; import 'package:zulip/widgets/home.dart'; import 'package:zulip/widgets/icons.dart'; import 'package:zulip/widgets/channel_colors.dart'; +import 'package:zulip/widgets/unread_count_badge.dart'; import '../example_data.dart' as eg; import '../flutter_checks.dart'; @@ -58,6 +59,7 @@ void main() { List? subscriptions, List? users, required List unreadMessages, + List? otherMessages, NavigatorObserver? navigatorObserver, }) async { addTearDown(testBinding.reset); @@ -227,7 +229,7 @@ void main() { streams: [stream], subscriptions: [subscription], unreadMessages: [eg.streamMessage(stream: stream, topic: 'lunch')]); - await store.addUserTopic(stream, 'lunch', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream, 'lunch', UserTopicVisibilityPolicy.muted); await tester.pump(); check(tester.widgetList(find.text('lunch'))).length.equals(0); }); @@ -249,7 +251,7 @@ void main() { streams: [stream], subscriptions: [subscription], unreadMessages: [eg.streamMessage(stream: stream, topic: 'lunch')]); - await store.addUserTopic(stream, 'lunch', UserTopicVisibilityPolicy.unmuted); + await store.setUserTopic(stream, 'lunch', UserTopicVisibilityPolicy.unmuted); await tester.pump(); check(tester.widgetList(find.text('lunch'))).length.equals(1); }); @@ -327,7 +329,7 @@ void main() { streams: [channel], subscriptions: [eg.subscription(channel)], unreadMessages: [message]); - await store.addUserTopic(channel, topic, UserTopicVisibilityPolicy.followed); + await store.setUserTopic(channel, topic, UserTopicVisibilityPolicy.followed); await tester.pump(); check(hasIcon(tester, parent: findRowByLabel(tester, topic), @@ -340,7 +342,7 @@ void main() { subscriptions: [eg.subscription(channel)], unreadMessages: [eg.streamMessage(stream: channel, topic: topic, flags: [MessageFlag.mentioned])]); - await store.addUserTopic(channel, topic, UserTopicVisibilityPolicy.followed); + await store.setUserTopic(channel, topic, UserTopicVisibilityPolicy.followed); await tester.pump(); check(hasIcon(tester, parent: findRowByLabel(tester, topic), @@ -356,12 +358,45 @@ void main() { streams: [channel], subscriptions: [eg.subscription(channel, isMuted: true)], unreadMessages: [message]); - await store.addUserTopic(channel, topic, UserTopicVisibilityPolicy.unmuted); + await store.setUserTopic(channel, topic, UserTopicVisibilityPolicy.unmuted); await tester.pump(); check(hasIcon(tester, parent: findRowByLabel(tester, topic), icon: ZulipIcons.unmute)).isTrue(); }); + + testWidgets('unmuted (topics treated case-insensitively)', (tester) async { + // Case-insensitivity of both topic-visibility and unreads data + // TODO(#1065) this belongs in test/model/ once the inbox page has + // its own view-model + + final message1 = eg.streamMessage(stream: channel, topic: 'aaa'); + final message2 = eg.streamMessage(stream: channel, topic: 'AaA', flags: [MessageFlag.read]); + final message3 = eg.streamMessage(stream: channel, topic: 'aAa', flags: [MessageFlag.read]); + await setupPage(tester, + users: [eg.selfUser, eg.otherUser], + streams: [channel], + subscriptions: [eg.subscription(channel, isMuted: true)], + unreadMessages: [message1]); + await store.setUserTopic(channel, 'aaa', UserTopicVisibilityPolicy.unmuted); + await tester.pump(); + + check(find.descendant( + of: find.byWidget(findRowByLabel(tester, 'aaa')!), + matching: find.widgetWithText(UnreadCountBadge, '1'))).findsOne(); + + await store.handleEvent(eg.updateMessageFlagsRemoveEvent(MessageFlag.read, [message2])); + await tester.pump(); + check(find.descendant( + of: find.byWidget(findRowByLabel(tester, 'aaa')!), + matching: find.widgetWithText(UnreadCountBadge, '2'))).findsOne(); + + await store.handleEvent(eg.updateMessageFlagsRemoveEvent(MessageFlag.read, [message3])); + await tester.pump(); + check(find.descendant( + of: find.byWidget(findRowByLabel(tester, 'aaa')!), + matching: find.widgetWithText(UnreadCountBadge, '3'))).findsOne(); + }); }); group('collapsing', () { diff --git a/test/widgets/topic_list_test.dart b/test/widgets/topic_list_test.dart index cf76ff3917..f87ddaf1ee 100644 --- a/test/widgets/topic_list_test.dart +++ b/test/widgets/topic_list_test.dart @@ -42,7 +42,7 @@ void main() { await store.addStream(channel); await store.addSubscription(eg.subscription(channel)); for (final userTopic in userTopics) { - await store.addUserTopic( + await store.setUserTopic( channel, userTopic.topicName.apiName, userTopic.visibilityPolicy); } topics ??= [eg.getStreamTopicsEntry()];