From b7080527578a867d1b6d201cc8af7d48b2d62704 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 8 May 2025 15:25:25 -0400 Subject: [PATCH] channel: Keep get-stream-topics data up-to-date Fixes: #1499 --- lib/model/channel.dart | 101 +++++++++ lib/model/store.dart | 2 + lib/widgets/topic_list.dart | 34 +-- test/api/route/route_checks.dart | 7 + test/model/channel_test.dart | 360 ++++++++++++++++++++++++++++++ test/model/store_checks.dart | 2 + test/widgets/topic_list_test.dart | 88 +++++++- 7 files changed, 569 insertions(+), 25 deletions(-) diff --git a/lib/model/channel.dart b/lib/model/channel.dart index 3a214e2032..23feab666b 100644 --- a/lib/model/channel.dart +++ b/lib/model/channel.dart @@ -1,13 +1,18 @@ +import 'dart:async'; import 'dart:collection'; +import 'package:collection/collection.dart'; import 'package:flutter/foundation.dart'; import '../api/model/events.dart'; import '../api/model/initial_snapshot.dart'; import '../api/model/model.dart'; +import '../api/route/channels.dart'; import 'store.dart'; import 'user.dart'; +final _apiGetStreamTopics = getStreamTopics; // similar to _apiSendMessage in lib/model/message.dart + /// The portion of [PerAccountStore] for channels, topics, and stuff about them. /// /// This type is useful for expressing the needs of other parts of the @@ -38,6 +43,26 @@ mixin ChannelStore on UserStore { /// and [streamsByName]. Map get subscriptions; + /// Fetch topics in a stream from the server. + /// + /// The results from the last successful fetch + /// can be retrieved with [getStreamTopics]. + Future fetchTopics(int streamId); + + /// Pairs of the known topics and its latest message ID, in the given stream. + /// + /// Returns null if the data has never been fetched yet. + /// To fetch it from the server, use [fetchTopics]. + /// + /// The result is guaranteed to be sorted by [GetStreamTopicsEntry.maxId], and the + /// topics are guaranteed to be distinct. + /// + /// In some cases, the same maxId affected by message moves can be present in + /// multiple [GetStreamTopicsEntry] entries. For this reason, the caller + /// should not rely on [getStreamTopics] to determine which topic the message + /// is in. Instead, refer to [PerAccountStore.messages]. + List? getStreamTopics(int streamId); + /// The visibility policy that the self-user has for the given topic. /// /// This does not incorporate the user's channel-level policy, @@ -199,6 +224,13 @@ mixin ProxyChannelStore on ChannelStore { @override Map get subscriptions => channelStore.subscriptions; + @override + Future fetchTopics(int streamId) => channelStore.fetchTopics(streamId); + + @override + List? getStreamTopics(int streamId) => + channelStore.getStreamTopics(streamId); + @override UserTopicVisibilityPolicy topicVisibilityPolicy(int streamId, TopicName topic) => channelStore.topicVisibilityPolicy(streamId, topic); @@ -260,6 +292,34 @@ class ChannelStoreImpl extends HasUserStore with ChannelStore { @override final Map subscriptions; + /// Maps indexed by stream IDs, of the known latest message IDs in each topic. + /// + /// For example: `_latestMessageIdsByStreamTopic[stream.id][topic] = maxId` + /// + /// In some cases, the same message IDs, when affected by message moves, can + /// be present for mutliple stream-topic keys. + final Map> _latestMessageIdsByStreamTopic = {}; + + @override + Future fetchTopics(int streamId) async { + final result = await _apiGetStreamTopics(connection, streamId: streamId, + allowEmptyTopicName: true); + _latestMessageIdsByStreamTopic[streamId] = { + for (final GetStreamTopicsEntry(:name, :maxId) in result.topics) + name: maxId, + }; + } + + @override + List? getStreamTopics(int streamId) { + final latestMessageIdsInStream = _latestMessageIdsByStreamTopic[streamId]; + if (latestMessageIdsInStream == null) return null; + return [ + for (final MapEntry(:key, :value) in latestMessageIdsInStream.entries) + GetStreamTopicsEntry(maxId: value, name: key), + ].sortedBy((value) => -value.maxId); + } + @override Map> get debugTopicVisibility => topicVisibility; @@ -425,6 +485,47 @@ class ChannelStoreImpl extends HasUserStore with ChannelStore { forStream[event.topicName] = visibilityPolicy; } } + + /// Handle a [MessageEvent], returning whether listeners should be notified. + bool handleMessageEvent(MessageEvent event) { + if (event.message is! StreamMessage) return false; + final StreamMessage(:streamId, :topic) = event.message as StreamMessage; + + final latestMessageIdsByTopic = _latestMessageIdsByStreamTopic[streamId]; + if (latestMessageIdsByTopic == null) return false; + assert(!latestMessageIdsByTopic.containsKey(topic) + || latestMessageIdsByTopic[topic]! < event.message.id); + latestMessageIdsByTopic[topic] = event.message.id; + return true; + } + + /// Handle a [UpdateMessageEvent], returning whether listeners should be + /// notified. + bool handleUpdateMessageEvent(UpdateMessageEvent event) { + if (event.moveData == null) return false; + final UpdateMessageMoveData( + :origStreamId, :origTopic, :newStreamId, :newTopic, :propagateMode, + ) = event.moveData!; + bool shouldNotify = false; + + final origLatestMessageIdsByTopics = _latestMessageIdsByStreamTopic[origStreamId]; + if (propagateMode == PropagateMode.changeAll + && origLatestMessageIdsByTopics != null) { + shouldNotify = origLatestMessageIdsByTopics.remove(origTopic) != null; + } + + final newLatestMessageIdsByTopics = _latestMessageIdsByStreamTopic[newStreamId]; + if (newLatestMessageIdsByTopics != null) { + final movedMaxId = event.messageIds.max; + if (!newLatestMessageIdsByTopics.containsKey(newTopic) + || newLatestMessageIdsByTopics[newTopic]! < movedMaxId) { + newLatestMessageIdsByTopics[newTopic] = movedMaxId; + shouldNotify = true; + } + } + + return shouldNotify; + } } /// A [Map] with [TopicName] keys and [V] values. diff --git a/lib/model/store.dart b/lib/model/store.dart index 03ff13022a..c3f755b17a 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -781,6 +781,7 @@ class PerAccountStore extends PerAccountStoreBase with unreads.handleMessageEvent(event); recentDmConversationsView.handleMessageEvent(event); recentSenders.handleMessage(event.message); // TODO(#824) + if (_channels.handleMessageEvent(event)) notifyListeners(); // When adding anything here (to handle [MessageEvent]), // it probably belongs in [reconcileMessages] too. @@ -788,6 +789,7 @@ class PerAccountStore extends PerAccountStoreBase with assert(debugLog("server event: update_message ${event.messageId}")); _messages.handleUpdateMessageEvent(event); unreads.handleUpdateMessageEvent(event); + if (_channels.handleUpdateMessageEvent(event)) notifyListeners(); case DeleteMessageEvent(): assert(debugLog("server event: delete_message ${event.messageIds}")); diff --git a/lib/widgets/topic_list.dart b/lib/widgets/topic_list.dart index 61e16df6ec..a80aeb8d4d 100644 --- a/lib/widgets/topic_list.dart +++ b/lib/widgets/topic_list.dart @@ -4,6 +4,7 @@ import '../api/model/model.dart'; import '../api/route/channels.dart'; import '../generated/l10n/zulip_localizations.dart'; import '../model/narrow.dart'; +import '../model/store.dart'; import '../model/unreads.dart'; import 'action_sheet.dart'; import 'app_bar.dart'; @@ -121,16 +122,13 @@ class _TopicList extends StatefulWidget { class _TopicListState extends State<_TopicList> with PerAccountStoreAwareStateMixin { Unreads? unreadsModel; - // TODO(#1499): store the results on [ChannelStore], and keep them - // up-to-date by handling events - List? lastFetchedTopics; @override void onNewStore() { unreadsModel?.removeListener(_modelChanged); final store = PerAccountStoreWidget.of(context); unreadsModel = store.unreads..addListener(_modelChanged); - _fetchTopics(); + _fetchTopics(store); } @override @@ -145,23 +143,22 @@ class _TopicListState extends State<_TopicList> with PerAccountStoreAwareStateMi }); } - void _fetchTopics() async { + void _fetchTopics(PerAccountStore store) async { // Do nothing when the fetch fails; the topic-list will stay on // the loading screen, until the user navigates away and back. // TODO(design) show a nice error message on screen when this fails - final store = PerAccountStoreWidget.of(context); - final result = await getStreamTopics(store.connection, - streamId: widget.streamId, - allowEmptyTopicName: true); + await store.fetchTopics(widget.streamId); if (!mounted) return; setState(() { - lastFetchedTopics = result.topics; + // The actuall state lives in the PerAccountStore }); } @override Widget build(BuildContext context) { - if (lastFetchedTopics == null) { + final store = PerAccountStoreWidget.of(context); + final streamTopics = store.getStreamTopics(widget.streamId); + if (streamTopics == null) { return const Center(child: CircularProgressIndicator()); } @@ -169,7 +166,7 @@ class _TopicListState extends State<_TopicList> with PerAccountStoreAwareStateMi // This is adapted from parts of the build method on [_InboxPageState]. final topicItems = <_TopicItemData>[]; - for (final GetStreamTopicsEntry(:maxId, name: topic) in lastFetchedTopics!) { + for (final GetStreamTopicsEntry(:maxId, name: topic) in streamTopics) { final unreadMessageIds = unreadsModel!.streams[widget.streamId]?[topic] ?? []; final countInTopic = unreadMessageIds.length; @@ -179,9 +176,6 @@ class _TopicListState extends State<_TopicList> with PerAccountStoreAwareStateMi topic: topic, unreadCount: countInTopic, hasMention: hasMention, - // `lastFetchedTopics.maxId` can become outdated when a new message - // arrives or when there are message moves, until we re-fetch. - // TODO(#1499): track changes to this maxId: maxId, )); } @@ -231,6 +225,14 @@ class _TopicItem extends StatelessWidget { final store = PerAccountStoreWidget.of(context); final designVariables = DesignVariables.of(context); + // The message with `maxId` might not remain in `topic` since we last fetch + // the list of topics. Make sure we check for that before passing `maxId` + // to the topic action sheet. + // See also: [ChannelStore.getStreamTopics] + final message = store.messages[maxId]; + final isMaxIdInTopic = message is StreamMessage + && message.streamId == streamId + && message.topic.isSameAs(topic); final visibilityPolicy = store.topicVisibilityPolicy(streamId, topic); final double opacity; @@ -259,7 +261,7 @@ class _TopicItem extends StatelessWidget { onLongPress: () => showTopicActionSheet(context, channelId: streamId, topic: topic, - someMessageIdInTopic: maxId), + someMessageIdInTopic: isMaxIdInTopic ? maxId : null), splashFactory: NoSplash.splashFactory, child: Padding(padding: EdgeInsetsDirectional.fromSTEB(6, 8, 12, 8), child: Row( diff --git a/test/api/route/route_checks.dart b/test/api/route/route_checks.dart index 1ecd90e9c8..f69c912287 100644 --- a/test/api/route/route_checks.dart +++ b/test/api/route/route_checks.dart @@ -1,4 +1,6 @@ import 'package:checks/checks.dart'; +import 'package:zulip/api/model/model.dart'; +import 'package:zulip/api/route/channels.dart'; import 'package:zulip/api/route/messages.dart'; import 'package:zulip/api/route/saved_snippets.dart'; @@ -9,4 +11,9 @@ extension CreateSavedSnippetResultChecks on Subject { Subject get savedSnippetId => has((e) => e.savedSnippetId, 'savedSnippetId'); } +extension GetStreamTopicEntryChecks on Subject { + Subject get maxId => has((e) => e.maxId, 'maxId'); + Subject get name => has((e) => e.name, 'name'); +} + // TODO add similar extensions for other classes in api/route/*.dart diff --git a/test/model/channel_test.dart b/test/model/channel_test.dart index 07e542e6a1..8e9eed448c 100644 --- a/test/model/channel_test.dart +++ b/test/model/channel_test.dart @@ -1,14 +1,20 @@ import 'package:checks/checks.dart'; +import 'package:http/http.dart' as http; import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; +import 'package:zulip/api/route/channels.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/model/channel.dart'; +import '../api/fake_api.dart'; import '../api/model/model_checks.dart'; +import '../api/route/route_checks.dart'; import '../example_data.dart' as eg; +import '../fake_async.dart'; import '../stdlib_checks.dart'; +import 'store_checks.dart'; import 'test_store.dart'; void main() { @@ -134,6 +140,360 @@ void main() { }); }); + group('topics data', () { + final ZulipStream stream = eg.stream(); + final ZulipStream otherStream = eg.stream(); + late PerAccountStore store; + late FakeApiConnection connection; + late int notifiedCount; + + void checkNotified({required int count}) { + check(notifiedCount).equals(count); + notifiedCount = 0; + } + void checkNotNotified() { + checkNotified(count: 0); + } + void checkNotifiedOnce() => checkNotified(count: 1); + + Condition conditionGetStreamTopicsEntry(String topic, int maxId) => + (it) => it.isA()..maxId.equals(maxId) + ..name.equals(eg.t(topic)); + + Future prepare({ + List? topics, + List? messages, + }) async { + notifiedCount = 0; + store = eg.store(); + await store.addStreams([stream, otherStream]); + await store.addSubscriptions([ + eg.subscription(stream), + eg.subscription(otherStream), + ]); + await store.addMessages(messages ?? []); + store.addListener(() { + notifiedCount++; + }); + + connection = store.connection as FakeApiConnection; + connection.prepare(json: + GetStreamTopicsResult(topics: topics ?? []).toJson()); + await store.fetchTopics(stream.streamId); + check(connection.takeRequests()).single.isA() + ..method.equals('GET') + ..url.path.equals('/api/v1/users/me/${stream.streamId}/topics'); + } + + test('last successful fetchTopics overrides existing data', () => awaitFakeAsync((async) async { + await prepare(topics: []); + + connection.prepare(json: GetStreamTopicsResult(topics: [ + eg.getStreamTopicsEntry(name: 'foo'), + ]).toJson(), delay: Duration(seconds: 2)); + final fetchFuture1 = store.fetchTopics(stream.streamId); + + connection.prepare(json: GetStreamTopicsResult(topics: [ + eg.getStreamTopicsEntry(name: 'bar'), + ]).toJson(), delay: Duration(seconds: 1)); + final fetchFuture2 = store.fetchTopics(stream.streamId); + + async.elapse(Duration(seconds: 1)); + await fetchFuture2; + check(store).getStreamTopics(stream.streamId).isNotNull().single + .name.equals(eg.t('bar')); + + async.elapse(Duration(seconds: 1)); + await fetchFuture1; + check(store).getStreamTopics(stream.streamId).isNotNull().single + .name.equals(eg.t('foo')); + })); + + test('getStreamTopics sort descending by maxId', () async { + await prepare(topics: [ + eg.getStreamTopicsEntry(name: 'bar', maxId: 200), + eg.getStreamTopicsEntry(name: 'foo', maxId: 100), + eg.getStreamTopicsEntry(name: 'baz', maxId: 300), + ]); + check(store).getStreamTopics(stream.streamId).isNotNull().deepEquals([ + conditionGetStreamTopicsEntry('baz', 300), + conditionGetStreamTopicsEntry('bar', 200), + conditionGetStreamTopicsEntry('foo', 100), + ]); + + await store.addMessage(eg.streamMessage( + id: 301, stream: stream, topic: 'foo')); + check(store).getStreamTopics(stream.streamId).isNotNull().deepEquals([ + conditionGetStreamTopicsEntry('foo', 301), + conditionGetStreamTopicsEntry('baz', 300), + conditionGetStreamTopicsEntry('bar', 200), + ]); + }); + + group('handleMessageEvent', () { + test('new message in fetched stream', () async { + await prepare(topics: [ + eg.getStreamTopicsEntry(name: 'old topic', maxId: 1), + ]); + + await store.addMessage( + eg.streamMessage(id: 2, stream: stream, topic: 'new topic')); + check(store).getStreamTopics(stream.streamId).isNotNull().deepEquals([ + conditionGetStreamTopicsEntry('new topic', 2), + conditionGetStreamTopicsEntry('old topic', 1), + ]); + checkNotifiedOnce(); + + await store.addMessage( + eg.streamMessage(id: 3, stream: stream, topic: 'old topic')); + check(store).getStreamTopics(stream.streamId).isNotNull().deepEquals([ + conditionGetStreamTopicsEntry('old topic', 3), + conditionGetStreamTopicsEntry('new topic', 2), + ]); + checkNotifiedOnce(); + }); + + test('new message in stream not fetched before', () async { + await prepare(); + check(store).getStreamTopics(otherStream.streamId).isNull(); + + await store.addMessage( + eg.streamMessage(id: 2, stream: otherStream, topic: 'new topic')); + check(store).getStreamTopics(otherStream.streamId).isNull(); + checkNotNotified(); + }); + + test('new message with lower message ID', () async { + await prepare(topics: [ + eg.getStreamTopicsEntry(name: 'topic', maxId: 2), + ]); + + await check(store.addMessage( + eg.streamMessage(id: 1, stream: stream, topic: 'topic'))).throws(); + check(store).getStreamTopics(stream.streamId).isNotNull().single + ..name.equals(eg.t('topic')) + ..maxId.equals(2); + checkNotNotified(); + }); + + test('ignore DM messages', () async { + await prepare(); + await store.addUsers([eg.selfUser, eg.otherUser]); + checkNotified(count: 2); + + await store.addMessage(eg.dmMessage(from: eg.selfUser, to: [eg.otherUser])); + checkNotNotified(); + }); + }); + + group('handleUpdateMessageEvent', () { + Future prepareWithMessages(List messages, { + required List> expectedTopics, + }) async { + await prepare(); + assert(messages.isNotEmpty); + assert(messages.every((m) => m.streamId == stream.streamId)); + await store.addMessages(messages); + check(store).getStreamTopics(stream.streamId).isNotNull().deepEquals(expectedTopics); + checkNotified(count: messages.length); + } + + test('ignore content only update', () async { + final message = eg.streamMessage(id: 123, stream: stream, topic: 'foo'); + await prepareWithMessages([message], expectedTopics: [ + conditionGetStreamTopicsEntry('foo', 123), + ]); + + await store.handleEvent(eg.updateMessageEditEvent(message)); + checkNotNotified(); + }); + + group('PropagateMode.changedAll', () { + test('topic moved to another stream with no previously fetched topics', () async { + final messagesToMove = List.generate(10, (i) => + eg.streamMessage(id: 100 + i, stream: stream, topic: 'foo')); + await prepareWithMessages(messagesToMove, expectedTopics: [ + conditionGetStreamTopicsEntry('foo', 109), + ]); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: messagesToMove, + newStreamId: otherStream.streamId, + propagateMode: PropagateMode.changeAll)); + check(store).getStreamTopics(stream.streamId).isNotNull().isEmpty(); + check(store).getStreamTopics(otherStream.streamId).isNull(); + checkNotifiedOnce(); + }); + + test('topic moved to new topic in another stream', () async { + final messagesToMove = List.generate(10, (i) => + eg.streamMessage(id: 100 + i, stream: stream, topic: 'foo')); + await prepareWithMessages(messagesToMove, expectedTopics: [ + conditionGetStreamTopicsEntry('foo', 109), + ]); + + // Make sure that topics in othterStream has been fetched. + connection.prepare(json: GetStreamTopicsResult(topics: [ + eg.getStreamTopicsEntry(name: 'foo', maxId: 1), + ]).toJson()); + await store.fetchTopics(otherStream.streamId); + check(store).getStreamTopics(otherStream.streamId).isNotNull().single + ..name.equals(eg.t('foo')) + ..maxId.equals(1); + checkNotNotified(); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: messagesToMove, + newStreamId: otherStream.streamId, + newTopicStr: 'bar', + propagateMode: PropagateMode.changeAll)); + check(store).getStreamTopics(stream.streamId).isNotNull().isEmpty(); + check(store).getStreamTopics(otherStream.streamId).isNotNull().deepEquals([ + conditionGetStreamTopicsEntry('bar', 109), + conditionGetStreamTopicsEntry('foo', 1), + ]); + checkNotifiedOnce(); + }); + + test('topic moved to known topic in another stream', () async { + final messagesToMove = List.generate(10, (i) => + eg.streamMessage(id: 100 + i, stream: stream, topic: 'foo')); + await prepareWithMessages(messagesToMove, expectedTopics: [ + conditionGetStreamTopicsEntry('foo', 109), + ]); + + // Make sure that topics in othterStream has been fetched. + connection.prepare(json: GetStreamTopicsResult(topics: [ + eg.getStreamTopicsEntry(name: 'foo', maxId: 1), + ]).toJson()); + await store.fetchTopics(otherStream.streamId); + check(store).getStreamTopics(otherStream.streamId).isNotNull().single + ..name.equals(eg.t('foo')) + ..maxId.equals(1); + checkNotNotified(); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: messagesToMove, + newStreamId: otherStream.streamId, + propagateMode: PropagateMode.changeAll)); + check(store).getStreamTopics(stream.streamId).isNotNull().isEmpty(); + check(store).getStreamTopics(otherStream.streamId).isNotNull().single + ..name.equals(eg.t('foo')) + ..maxId.equals(109); + checkNotifiedOnce(); + }); + + test('topic moved to new topic in the same stream', () async { + final messagesToMove = List.generate(10, (i) => + eg.streamMessage(id: 100 + i, stream: stream, topic: 'foo')); + await prepareWithMessages(messagesToMove, expectedTopics: [ + conditionGetStreamTopicsEntry('foo', 109), + ]); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: messagesToMove, + newTopicStr: 'bar', + propagateMode: PropagateMode.changeAll)); + check(store).getStreamTopics(stream.streamId).isNotNull().single + ..name.equals(eg.t('bar')) + ..maxId.equals(109); + checkNotifiedOnce(); + }); + + test('topic moved to known topic in the same stream', () async { + final messagesToMove = List.generate(10, (i) => + eg.streamMessage(id: 100 + i, stream: stream, topic: 'foo')); + await prepareWithMessages([ + ...messagesToMove, + eg.streamMessage(id: 1, stream: stream, topic: 'bar'), + ], expectedTopics: [ + conditionGetStreamTopicsEntry('foo', 109), + conditionGetStreamTopicsEntry('bar', 1), + ]); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: messagesToMove, + newTopicStr: 'bar', + propagateMode: PropagateMode.changeAll)); + check(store).getStreamTopics(stream.streamId).isNotNull().single + ..name.equals(eg.t('bar')) + ..maxId.equals(109); + checkNotifiedOnce(); + }); + }); + + test('message moved to new topic', () async { + final messageToMove = + eg.streamMessage(id: 101, stream: stream, topic: 'foo'); + await prepareWithMessages([ + messageToMove, + ], expectedTopics: [ + conditionGetStreamTopicsEntry('foo', 101), + ]); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: [messageToMove], + newTopicStr: 'bar', + propagateMode: PropagateMode.changeOne)); + check(store).getStreamTopics(stream.streamId).isNotNull().deepEquals([ + conditionGetStreamTopicsEntry('foo', 101), + conditionGetStreamTopicsEntry('bar', 101), + ]); + checkNotifiedOnce(); + }); + + test('message moved to known topic; moved message ID < maxId', () async { + final messageToMove = + eg.streamMessage(id: 100, stream: stream, topic: 'foo'); + await prepareWithMessages([ + messageToMove, + eg.streamMessage(id: 999, stream: stream, topic: 'bar'), + ], expectedTopics: [ + conditionGetStreamTopicsEntry('bar', 999), + conditionGetStreamTopicsEntry('foo', 100), + ]); + + // Message with ID 100 moved from "foo" to "bar", whose maxId was 999. + // We expect no updates to "bar"'s maxId, since the moved message + // has a lower ID. + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: [messageToMove], + newTopicStr: 'bar', + propagateMode: PropagateMode.changeOne)); + check(store).getStreamTopics(stream.streamId).isNotNull().deepEquals([ + conditionGetStreamTopicsEntry('bar', 999), + conditionGetStreamTopicsEntry('foo', 100), + ]); + checkNotNotified(); + }); + + test('message moved to known topic; moved message ID > maxId', () async { + final messageToMove = + eg.streamMessage(id: 999, stream: stream, topic: 'foo'); + await prepareWithMessages([ + messageToMove, + eg.streamMessage(id: 100, stream: stream, topic: 'bar'), + ], expectedTopics: [ + conditionGetStreamTopicsEntry('foo', 999), + conditionGetStreamTopicsEntry('bar', 100), + ]); + + // Message with ID 999 moved from "foo" to "bar", whose maxId was 100. + // We expect an update to "bar"'s maxId, since the moved message has + // a higher ID. + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: [messageToMove], + newTopicStr: 'bar', + propagateMode: PropagateMode.changeOne)); + check(store).getStreamTopics(stream.streamId).isNotNull().deepEquals([ + conditionGetStreamTopicsEntry('foo', 999), + conditionGetStreamTopicsEntry('bar', 999), + ]); + checkNotifiedOnce(); + }); + }); + }); + group('topic visibility', () { final stream1 = eg.stream(); final stream2 = eg.stream(); diff --git a/test/model/store_checks.dart b/test/model/store_checks.dart index 6321fa057e..b14898e593 100644 --- a/test/model/store_checks.dart +++ b/test/model/store_checks.dart @@ -2,6 +2,7 @@ import 'package:checks/checks.dart'; import 'package:zulip/api/core.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; +import 'package:zulip/api/route/channels.dart'; import 'package:zulip/model/autocomplete.dart'; import 'package:zulip/model/binding.dart'; import 'package:zulip/model/database.dart'; @@ -62,6 +63,7 @@ extension PerAccountStoreChecks on Subject { Subject> get streams => has((x) => x.streams, 'streams'); Subject> get streamsByName => has((x) => x.streamsByName, 'streamsByName'); Subject> get subscriptions => has((x) => x.subscriptions, 'subscriptions'); + Subject?> getStreamTopics(int streamId) => has((x) => x.getStreamTopics(streamId), 'getStreamTopics'); Subject> get messages => has((x) => x.messages, 'messages'); Subject get unreads => has((x) => x.unreads, 'unreads'); Subject get recentDmConversationsView => has((x) => x.recentDmConversationsView, 'recentDmConversationsView'); diff --git a/test/widgets/topic_list_test.dart b/test/widgets/topic_list_test.dart index 1cd9c4bb26..3d5bed8c9c 100644 --- a/test/widgets/topic_list_test.dart +++ b/test/widgets/topic_list_test.dart @@ -14,8 +14,10 @@ import 'package:zulip/widgets/message_list.dart'; import 'package:zulip/widgets/topic_list.dart'; import '../api/fake_api.dart'; +import '../api/route/route_checks.dart'; import '../example_data.dart' as eg; import '../model/binding.dart'; +import '../model/store_checks.dart'; import '../model/test_store.dart'; import '../stdlib_checks.dart'; import 'test_app.dart'; @@ -169,6 +171,18 @@ void main() { of: topicItemFinder.at(index), matching: finder); + testWidgets('sort topics by maxId', (tester) async { + await prepare(tester, topics: [ + eg.getStreamTopicsEntry(name: 'A', maxId: 3), + eg.getStreamTopicsEntry(name: 'B', maxId: 2), + eg.getStreamTopicsEntry(name: 'C', maxId: 4), + ]); + + check(findInTopicItemAt(0, find.text('C'))).findsOne(); + check(findInTopicItemAt(1, find.text('A'))).findsOne(); + check(findInTopicItemAt(2, find.text('B'))).findsOne(); + }); + testWidgets('show topic action sheet', (tester) async { final channel = eg.stream(); await prepare(tester, channel: channel, @@ -190,16 +204,72 @@ void main() { }); }); - testWidgets('sort topics by maxId', (tester) async { - await prepare(tester, topics: [ - eg.getStreamTopicsEntry(name: 'A', maxId: 3), - eg.getStreamTopicsEntry(name: 'B', maxId: 2), - eg.getStreamTopicsEntry(name: 'C', maxId: 4), - ]); + testWidgets('show topic action sheet before and after moves', (tester) async { + final channel = eg.stream(); + final message = eg.streamMessage(id: 123, stream: channel, topic: 'foo'); + await prepare(tester, channel: channel, + topics: [eg.getStreamTopicsEntry(name: 'foo', maxId: 123)], + messages: [message]); + check(store).getStreamTopics(channel.streamId).isNotNull().single + .maxId.equals(123); + check(topicItemFinder).findsOne(); + + // Before the move, "foo"'s maxId is known to be accurate. This makes + // topic actions that require `someMessageIdInTopic` available. + await tester.longPress(find.text('foo')); + await tester.pump(Duration(milliseconds: 150)); // bottom-sheet animation + check(find.text('Mark as resolved')).findsOne(); + await tester.tap(find.text('Cancel')); - check(findInTopicItemAt(0, find.text('C'))).findsOne(); - check(findInTopicItemAt(1, find.text('A'))).findsOne(); - check(findInTopicItemAt(2, find.text('B'))).findsOne(); + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: [message], + newTopicStr: 'bar')); + await tester.pump(); + check(topicItemFinder).findsExactly(2); + + // After the move, the message with maxId moved away from "foo". The topic + // actions that require `someMessageIdInTopic` is no longer available. + await tester.longPress(find.text('foo')); + await tester.pump(Duration(milliseconds: 150)); // bottom-sheet animation + check(find.text('Mark as resolved')).findsNothing(); + await tester.tap(find.text('Cancel')); + await tester.pump(); + + // Topic actions that require `someMessageIdInTopic` is available + // for "bar", the new topic that the message moved to. + await tester.longPress(find.text('bar')); + await tester.pump(Duration(milliseconds: 150)); // bottom-sheet animation + check(find.text('Mark as resolved')).findsOne(); + }); + + // event handling is more thoroughly tested in test/model/channel_test.dart + testWidgets('smoke resolve topic from topic action sheet', (tester) async { + final channel = eg.stream(); + final messages = List.generate(10, (i) => + eg.streamMessage(id: 100+i, stream: channel, topic: 'foo')); + + await prepare(tester, channel: channel, + topics: [eg.getStreamTopicsEntry(maxId: messages.last.id, name: 'foo')], + messages: messages); + await tester.longPress(topicItemFinder); + await tester.pump(Duration(milliseconds: 150)); // bottom-sheet animation + check(findInTopicItemAt(0, find.byIcon(ZulipIcons.check))).findsNothing(); + check(find.descendant(of: topicItemFinder, + matching: find.text('foo'))).findsOne(); + + connection.prepare(json: {}); + await tester.tap(find.text('Mark as resolved')); + await tester.pump(); + await tester.pump(Duration.zero); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: messages, + newTopic: eg.t('foo').resolve(), + propagateMode: PropagateMode.changeAll)); + await tester.pump(); + check(findInTopicItemAt(0, find.byIcon(ZulipIcons.check))).findsOne(); + check(find.descendant(of: topicItemFinder, + matching: find.text('foo'))).findsOne(); }); testWidgets('resolved and unresolved topics', (tester) async {