diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index cf5c6b86e0..37d5db44db 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -6,7 +6,6 @@ import 'package:unorm_dart/unorm_dart.dart' as unorm; import '../api/model/events.dart'; import '../api/model/model.dart'; -import '../api/route/channels.dart'; import '../generated/l10n/zulip_localizations.dart'; import '../widgets/compose_box.dart'; import 'algorithms.dart'; @@ -1175,10 +1174,8 @@ class TopicAutocompleteView extends AutocompleteView _fetch() async { assert(!_isFetching); _isFetching = true; - final result = await getStreamTopics(store.connection, streamId: streamId, - allowEmptyTopicName: true, - ); - _topics = result.topics.map((e) => e.name); + await store.fetchTopics(streamId); + _topics = store.getChannelTopics(streamId)!.map((e) => e.name); _isFetching = false; return _startSearch(); } diff --git a/lib/model/channel.dart b/lib/model/channel.dart index 10e9596eed..a3759f01dd 100644 --- a/lib/model/channel.dart +++ b/lib/model/channel.dart @@ -1,14 +1,19 @@ +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 'realm.dart'; import 'store.dart'; import 'user.dart'; +final _apiGetChannelTopics = 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 @@ -78,6 +83,27 @@ mixin ChannelStore on UserStore { }; } + /// Fetch topics in a channel from the server, only if they're not fetched yet. + /// + /// The results from the last successful fetch + /// can be retrieved with [getChannelTopics]. + Future fetchTopics(int channelId); + + /// Pairs of the known topics and its latest message ID, in the given channel. + /// + /// 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] + /// descending, 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 [getChannelTopics] to determine which topic the message + /// is in. Instead, refer to [PerAccountStore.messages]. + /// See [handleUpdateMessageEvent] on how this could happen. + List? getChannelTopics(int channelId); + /// The visibility policy that the self-user has for the given topic. /// /// This does not incorporate the user's channel-level policy, @@ -288,6 +314,13 @@ mixin ProxyChannelStore on ChannelStore { @override Map get channelFolders => channelStore.channelFolders; + @override + Future fetchTopics(int channelId) => channelStore.fetchTopics(channelId); + + @override + List? getChannelTopics(int channelId) => + channelStore.getChannelTopics(channelId); + @override UserTopicVisibilityPolicy topicVisibilityPolicy(int streamId, TopicName topic) => channelStore.topicVisibilityPolicy(streamId, topic); @@ -368,6 +401,37 @@ class ChannelStoreImpl extends HasUserStore with ChannelStore { @override final Map channelFolders; + /// Maps indexed by channel IDs, of the known latest message IDs in each topic. + /// + /// For example: `_latestMessageIdsByChannelTopic[channel.streamId][topic] = maxId` + /// + /// In some cases, the same message IDs, when affected by message moves, can + /// be present for mutliple channel-topic keys. + /// See [handleUpdateMessageEvent] on how this could happen. + final Map> _latestMessageIdsByChannelTopic = {}; + + @override + Future fetchTopics(int channelId) async { + if (_latestMessageIdsByChannelTopic[channelId] != null) return; + + final result = await _apiGetChannelTopics(connection, streamId: channelId, + allowEmptyTopicName: true); + _latestMessageIdsByChannelTopic[channelId] = { + for (final GetStreamTopicsEntry(:name, :maxId) in result.topics) + name: maxId, + }; + } + + @override + List? getChannelTopics(int channelId) { + final latestMessageIdsByTopic = _latestMessageIdsByChannelTopic[channelId]; + if (latestMessageIdsByTopic == null) return null; + return [ + for (final MapEntry(:key, :value) in latestMessageIdsByTopic.entries) + GetStreamTopicsEntry(maxId: value, name: key), + ].sortedBy((value) => -value.maxId); + } + @override Map> get debugTopicVisibility => topicVisibility; @@ -572,6 +636,65 @@ 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 = _latestMessageIdsByChannelTopic[streamId]; + // If we don't already know about the list of topics of the channel this + // message belongs to, we don't want to proceed and put one entry about the + // topic of this message, otherwise [fetchTopics] and the callers of + // [getChannelTopics] would think that the channel only has this one topic + // and would never fetch the complete list of topics for that matter. + if (latestMessageIdsByTopic == null) return false; + + // If this message is already the latest message in the topic because it was + // received through fetch in fetch/event race, or it is a message sent even + // before the latest message of the fetch, we don't do the update. + final currentLatestMessageId = latestMessageIdsByTopic[topic]; + if (currentLatestMessageId != null && currentLatestMessageId >= event.message.id) { + return false; + } + latestMessageIdsByTopic[topic] = event.message.id; + return true; + } + + /// Handle an [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 = _latestMessageIdsByChannelTopic[origStreamId]; + // We only handle the case where all the messages of [origTopic] are + // moved to [newTopic]; in that case we can remove [origTopic] safely. + // But if only one messsage is moved (`PropagateMode.changeOne`) or a few + // messages are moved (`PropagateMode.changeLater`), we cannot do anything + // about [origTopic] here as we cannot determine the new `maxId` for it. + // (This is the case where there could be multiple channel-topic keys with + // the same `maxId`) + if (propagateMode == PropagateMode.changeAll + && origLatestMessageIdsByTopics != null) { + shouldNotify = origLatestMessageIdsByTopics.remove(origTopic) != null; + } + + final newLatestMessageIdsByTopics = _latestMessageIdsByChannelTopic[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 88ca778100..9012384442 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -856,6 +856,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. @@ -863,6 +864,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 16111fc678..86d0aaacc1 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'; @@ -124,16 +125,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 @@ -148,23 +146,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 actual state lives in the PerAccountStore. }); } @override Widget build(BuildContext context) { - if (lastFetchedTopics == null) { + final store = PerAccountStoreWidget.of(context); + final channelTopics = store.getChannelTopics(widget.streamId); + if (channelTopics == null) { return const Center(child: CircularProgressIndicator()); } @@ -172,7 +169,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 channelTopics) { final unreadMessageIds = unreadsModel!.streams[widget.streamId]?[topic] ?? []; final countInTopic = unreadMessageIds.length; @@ -182,17 +179,9 @@ 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, )); } - topicItems.sort((a, b) { - final aMaxId = a.maxId; - final bMaxId = b.maxId; - return bMaxId.compareTo(aMaxId); - }); return SafeArea( // Don't pad the bottom here; we want the list content to do that. @@ -234,6 +223,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.getChannelTopics] + 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; @@ -262,7 +259,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 daa4628efd..2e9825a790 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/realm.dart'; import 'package:zulip/api/route/saved_snippets.dart'; @@ -15,4 +17,9 @@ extension GetServerSettingsResultChecks on Subject { Subject get realmUrl => has((e) => e.realmUrl, 'realmUrl'); } +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 6c2de3e83c..21c1e195d2 100644 --- a/test/model/channel_test.dart +++ b/test/model/channel_test.dart @@ -1,16 +1,21 @@ import 'package:checks/checks.dart'; import 'package:collection/collection.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 '../stdlib_checks.dart'; import 'binding.dart'; +import 'store_checks.dart'; import 'test_store.dart'; void main() { @@ -221,6 +226,363 @@ void main() { }); }); + group('topics data', () { + final channel = eg.stream(); + final otherChannel = 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 isStreamTopicsEntry(String topic, int maxId) { + return (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([channel, otherChannel]); + await store.addSubscriptions([ + eg.subscription(channel), + eg.subscription(otherChannel), + ]); + await store.addMessages(messages ?? []); + store.addListener(() { + notifiedCount++; + }); + + connection = store.connection as FakeApiConnection; + connection.prepare(json: GetStreamTopicsResult(topics: topics ?? []).toJson()); + await store.fetchTopics(channel.streamId); + check(connection.takeRequests()).single.isA() + ..method.equals('GET') + ..url.path.equals('/api/v1/users/me/${channel.streamId}/topics'); + } + + test('fetch topics -> update data, try fetching again -> no request sent', () async { + await prepare(topics: [ + eg.getStreamTopicsEntry(name: 'foo', maxId: 100), + eg.getStreamTopicsEntry(name: 'bar', maxId: 200), + eg.getStreamTopicsEntry(name: 'baz', maxId: 300), + ]); + check(store).getStreamTopics(channel.streamId).isNotNull().deepEquals([ + isStreamTopicsEntry('baz', 300), + isStreamTopicsEntry('bar', 200), + isStreamTopicsEntry('foo', 100), + ]); + + connection.prepare(json: GetStreamTopicsResult(topics: [ + eg.getStreamTopicsEntry(name: 'foo', maxId: 100), + eg.getStreamTopicsEntry(name: 'bar', maxId: 200), + eg.getStreamTopicsEntry(name: 'baz', maxId: 300), + ]).toJson()); + await store.fetchTopics(channel.streamId); + check(connection.takeRequests()).isEmpty(); + }); + + test('getChannelTopics sorts 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(channel.streamId).isNotNull().deepEquals([ + isStreamTopicsEntry('baz', 300), + isStreamTopicsEntry('bar', 200), + isStreamTopicsEntry('foo', 100), + ]); + + await store.addMessage(eg.streamMessage( + id: 301, stream: channel, topic: 'foo')); + check(store).getStreamTopics(channel.streamId).isNotNull().deepEquals([ + isStreamTopicsEntry('foo', 301), + isStreamTopicsEntry('baz', 300), + isStreamTopicsEntry('bar', 200), + ]); + }); + + group('handleMessageEvent', () { + test('new message in fetched channel', () async { + await prepare(topics: [ + eg.getStreamTopicsEntry(name: 'old topic', maxId: 1), + ]); + + await store.addMessage( + eg.streamMessage(id: 2, stream: channel, topic: 'new topic')); + check(store).getStreamTopics(channel.streamId).isNotNull().deepEquals([ + isStreamTopicsEntry('new topic', 2), + isStreamTopicsEntry('old topic', 1), + ]); + checkNotifiedOnce(); + + await store.addMessage( + eg.streamMessage(id: 3, stream: channel, topic: 'old topic')); + check(store).getStreamTopics(channel.streamId).isNotNull().deepEquals([ + isStreamTopicsEntry('old topic', 3), + isStreamTopicsEntry('new topic', 2), + ]); + checkNotifiedOnce(); + }); + + test('new message in channel not fetched before', () async { + await prepare(); + check(store).getStreamTopics(otherChannel.streamId).isNull(); + + await store.addMessage( + eg.streamMessage(id: 2, stream: otherChannel, topic: 'new topic')); + check(store).getStreamTopics(otherChannel.streamId).isNull(); + checkNotNotified(); + }); + + test('new message with equal or lower message ID', () async { + await prepare(topics: [ + eg.getStreamTopicsEntry(name: 'topic', maxId: 2), + ]); + + await store.addMessage( + eg.streamMessage(id: 2, stream: channel, topic: 'topic')); + check(store).getStreamTopics(channel.streamId).isNotNull().single + ..name.equals(eg.t('topic')) + ..maxId.equals(2); + checkNotNotified(); + + await store.addMessage( + eg.streamMessage(id: 1, stream: channel, topic: 'topic')); + check(store).getStreamTopics(channel.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 == channel.streamId)); + await store.addMessages(messages); + check(store).getStreamTopics(channel.streamId).isNotNull().deepEquals(expectedTopics); + checkNotified(count: messages.length); + } + + test('ignore content only update', () async { + final message = eg.streamMessage(id: 123, stream: channel, topic: 'foo'); + await prepareWithMessages([message], expectedTopics: [ + isStreamTopicsEntry('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: channel, topic: 'foo')); + await prepareWithMessages(messagesToMove, expectedTopics: [ + isStreamTopicsEntry('foo', 109), + ]); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: messagesToMove, + newStreamId: otherChannel.streamId, + propagateMode: PropagateMode.changeAll)); + check(store).getStreamTopics(channel.streamId).isNotNull().isEmpty(); + check(store).getStreamTopics(otherChannel.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: channel, topic: 'foo')); + await prepareWithMessages(messagesToMove, expectedTopics: [ + isStreamTopicsEntry('foo', 109), + ]); + + // Make sure that topics in otherChannel has been fetched. + connection.prepare(json: GetStreamTopicsResult(topics: [ + eg.getStreamTopicsEntry(name: 'foo', maxId: 1), + ]).toJson()); + await store.fetchTopics(otherChannel.streamId); + check(store).getStreamTopics(otherChannel.streamId).isNotNull().single + ..name.equals(eg.t('foo')) + ..maxId.equals(1); + checkNotNotified(); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: messagesToMove, + newStreamId: otherChannel.streamId, + newTopicStr: 'bar', + propagateMode: PropagateMode.changeAll)); + check(store).getStreamTopics(channel.streamId).isNotNull().isEmpty(); + check(store).getStreamTopics(otherChannel.streamId).isNotNull().deepEquals([ + isStreamTopicsEntry('bar', 109), + isStreamTopicsEntry('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: channel, topic: 'foo')); + await prepareWithMessages(messagesToMove, expectedTopics: [ + isStreamTopicsEntry('foo', 109), + ]); + + // Make sure that topics in otherChannel has been fetched. + connection.prepare(json: GetStreamTopicsResult(topics: [ + eg.getStreamTopicsEntry(name: 'foo', maxId: 1), + ]).toJson()); + await store.fetchTopics(otherChannel.streamId); + check(store).getStreamTopics(otherChannel.streamId).isNotNull().single + ..name.equals(eg.t('foo')) + ..maxId.equals(1); + checkNotNotified(); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: messagesToMove, + newStreamId: otherChannel.streamId, + propagateMode: PropagateMode.changeAll)); + check(store).getStreamTopics(channel.streamId).isNotNull().isEmpty(); + check(store).getStreamTopics(otherChannel.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: channel, topic: 'foo')); + await prepareWithMessages(messagesToMove, expectedTopics: [ + isStreamTopicsEntry('foo', 109), + ]); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: messagesToMove, + newTopicStr: 'bar', + propagateMode: PropagateMode.changeAll)); + check(store).getStreamTopics(channel.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: channel, topic: 'foo')); + await prepareWithMessages([ + ...messagesToMove, + eg.streamMessage(id: 1, stream: channel, topic: 'bar'), + ], expectedTopics: [ + isStreamTopicsEntry('foo', 109), + isStreamTopicsEntry('bar', 1), + ]); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: messagesToMove, + newTopicStr: 'bar', + propagateMode: PropagateMode.changeAll)); + check(store).getStreamTopics(channel.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: channel, topic: 'foo'); + await prepareWithMessages([ + messageToMove, + ], expectedTopics: [ + isStreamTopicsEntry('foo', 101), + ]); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: [messageToMove], + newTopicStr: 'bar', + propagateMode: PropagateMode.changeOne)); + check(store).getStreamTopics(channel.streamId).isNotNull().deepEquals([ + isStreamTopicsEntry('foo', 101), + isStreamTopicsEntry('bar', 101), + ]); + checkNotifiedOnce(); + }); + + test('message moved to known topic; moved message ID < maxId', () async { + final messageToMove = + eg.streamMessage(id: 100, stream: channel, topic: 'foo'); + await prepareWithMessages([ + messageToMove, + eg.streamMessage(id: 999, stream: channel, topic: 'bar'), + ], expectedTopics: [ + isStreamTopicsEntry('bar', 999), + isStreamTopicsEntry('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(channel.streamId).isNotNull().deepEquals([ + isStreamTopicsEntry('bar', 999), + isStreamTopicsEntry('foo', 100), + ]); + checkNotNotified(); + }); + + test('message moved to known topic; moved message ID > maxId', () async { + final messageToMove = + eg.streamMessage(id: 999, stream: channel, topic: 'foo'); + await prepareWithMessages([ + messageToMove, + eg.streamMessage(id: 100, stream: channel, topic: 'bar'), + ], expectedTopics: [ + isStreamTopicsEntry('foo', 999), + isStreamTopicsEntry('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(channel.streamId).isNotNull().deepEquals([ + isStreamTopicsEntry('foo', 999), + isStreamTopicsEntry('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 dac078a434..579246b89e 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'; @@ -67,6 +68,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.getChannelTopics(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/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index 621f759f28..ffc4d58205 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -221,12 +221,7 @@ void main() { channel ??= someChannel; connection.prepare(json: eg.newestGetMessagesResult( - foundOldest: true, messages: []).toJson()); - if (narrow case ChannelNarrow()) { - // We auto-focus the topic input when there are no messages; - // this is for topic autocomplete. - connection.prepare(json: GetStreamTopicsResult(topics: []).toJson()); - } + foundOldest: true, messages: [eg.streamMessage(stream: channel)]).toJson()); await tester.pumpWidget(TestZulipApp( accountId: eg.selfAccount.id, child: MessageListPage( diff --git a/test/widgets/topic_list_test.dart b/test/widgets/topic_list_test.dart index 1cd9c4bb26..f0be2f671f 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'; @@ -124,7 +126,7 @@ void main() { check(find.byType(CircularProgressIndicator)).findsNothing(); }); - testWidgets('fetch again when navigating away and back', (tester) async { + testWidgets("dont't fetch again when navigating away and back", (tester) async { addTearDown(testBinding.reset); await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); final store = await testBinding.globalStore.perAccount(eg.selfAccount.id); @@ -145,20 +147,28 @@ void main() { await tester.tap(find.byIcon(ZulipIcons.topics)); await tester.pump(); await tester.pump(Duration.zero); + check(connection.takeRequests()) + ..length.equals(2) // one for the messages request, another for the topics + ..last.which((last) => last + .isA() + ..method.equals('GET') + ..url.path.equals('/api/v1/users/me/${channel.streamId}/topics')); check(find.text('topic A')).findsOne(); // … go back to the message list page… await tester.pageBack(); await tester.pump(); - // … then back to the topic-list page, expecting to fetch again. + // … then back to the topic-list page, expecting not to fetch again but + // use existing data which is kept up-to-date anyways. connection.prepare(json: GetStreamTopicsResult( topics: [eg.getStreamTopicsEntry(name: 'topic B')]).toJson()); await tester.tap(find.byIcon(ZulipIcons.topics)); await tester.pump(); await tester.pump(Duration.zero); - check(find.text('topic A')).findsNothing(); - check(find.text('topic B')).findsOne(); + check(connection.takeRequests()).isEmpty(); + check(find.text('topic B')).findsNothing(); + check(find.text('topic A')).findsOne(); }); Finder topicItemFinder = find.descendant( @@ -169,6 +179,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 +212,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 {