From cb4e0e131be65897dae006ddfda7cca03d526030 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 21 Nov 2023 22:24:43 -0800 Subject: [PATCH] unread: Apply stream and topic muting to unread counts --- lib/model/unreads.dart | 46 ++++++++++++++++++---- lib/widgets/subscription_list.dart | 3 +- test/model/unreads_test.dart | 26 ++++++++++-- test/widgets/message_list_test.dart | 50 +++++++++++++++--------- test/widgets/subscription_list_test.dart | 23 +++++++++++ 5 files changed, 116 insertions(+), 32 deletions(-) diff --git a/lib/model/unreads.dart b/lib/model/unreads.dart index cd68acc6689..952158d2fe0 100644 --- a/lib/model/unreads.dart +++ b/lib/model/unreads.dart @@ -132,34 +132,64 @@ class Unreads extends ChangeNotifier { final int selfUserId; - // TODO(#346): account for muted topics and streams // TODO(#370): maintain this count incrementally, rather than recomputing from scratch int countInAllMessagesNarrow() { int c = 0; for (final messageIds in dms.values) { c = c + messageIds.length; } - for (final topics in streams.values) { - for (final messageIds in topics.values) { - c = c + messageIds.length; + for (final MapEntry(key: streamId, value: topics) in streams.entries) { + for (final MapEntry(key: topic, value: messageIds) in topics.entries) { + if (streamStore.isTopicVisibleAsUnread(streamId, topic)) { + c = c + messageIds.length; + } + } + } + return c; + } + + /// The "strict" unread count for this stream, + /// using [StreamStore.isTopicVisibleAsUnread]. + /// + /// If the stream is muted, this will count only topics that are + /// actively unmuted. + /// + /// For a count that's appropriate in UI contexts that are focused + /// specifically on this stream, see [countInStreamNarrow]. + // TODO(#370): maintain this count incrementally, rather than recomputing from scratch + int countInStream(int streamId) { + final topics = streams[streamId]; + if (topics == null) return 0; + int c = 0; + for (final entry in topics.entries) { + if (streamStore.isTopicVisibleAsUnread(streamId, entry.key)) { + c = c + entry.value.length; } } return c; } - // TODO(#346): account for muted topics and streams + /// The "broad" unread count for this stream, + /// using [StreamStore.isTopicVisibleInStream]. + /// + /// This includes topics that have no visibility policy of their own, + /// even if the stream itself is muted. + /// + /// For a count that's appropriate in UI contexts that are not already + /// focused on this stream, see [countInStream]. // TODO(#370): maintain this count incrementally, rather than recomputing from scratch int countInStreamNarrow(int streamId) { final topics = streams[streamId]; if (topics == null) return 0; int c = 0; - for (final messageIds in topics.values) { - c = c + messageIds.length; + for (final entry in topics.entries) { + if (streamStore.isTopicVisibleInStream(streamId, entry.key)) { + c = c + entry.value.length; + } } return c; } - // TODO(#346): account for muted topics and streams int countInTopicNarrow(int streamId, String topic) { final topics = streams[streamId]; return topics?[topic]?.length ?? 0; diff --git a/lib/widgets/subscription_list.dart b/lib/widgets/subscription_list.dart index e055c31f12e..935da572803 100644 --- a/lib/widgets/subscription_list.dart +++ b/lib/widgets/subscription_list.dart @@ -176,7 +176,8 @@ class _SubscriptionList extends StatelessWidget { itemCount: subscriptions.length, itemBuilder: (BuildContext context, int index) { final subscription = subscriptions[index]; - final unreadCount = unreadsModel!.countInStreamNarrow(subscription.streamId); + final unreadCount = unreadsModel!.countInStream(subscription.streamId); + // TODO(#346): if stream muted, show a dot for unreads return SubscriptionItem(subscription: subscription, unreadCount: unreadCount); }); } diff --git a/test/model/unreads_test.dart b/test/model/unreads_test.dart index 555235185ec..af019d6d057 100644 --- a/test/model/unreads_test.dart +++ b/test/model/unreads_test.dart @@ -9,6 +9,7 @@ import 'package:zulip/model/store.dart'; import 'package:zulip/model/unreads.dart'; import '../example_data.dart' as eg; +import 'test_store.dart'; import 'unreads_checks.dart'; void main() { @@ -152,30 +153,47 @@ void main() { group('count helpers', () { test('countInAllMessagesNarrow', () { - final stream1 = eg.stream(); - final stream2 = eg.stream(); + final stream1 = eg.stream(streamId: 1, name: 'stream 1'); + final stream2 = eg.stream(streamId: 2, name: 'stream 2'); + final stream3 = eg.stream(streamId: 3, name: 'stream 3'); prepare(); + streamStore.addStreams([stream1, stream2, stream3]); + streamStore.addSubscription(eg.subscription(stream1)); + streamStore.addSubscription(eg.subscription(stream3, isMuted: true)); + streamStore.addUserTopic(stream1, 'a', UserTopicVisibilityPolicy.muted); fillWithMessages([ eg.streamMessage(stream: stream1, topic: 'a', flags: []), eg.streamMessage(stream: stream1, topic: 'b', flags: []), eg.streamMessage(stream: stream1, topic: 'b', flags: []), eg.streamMessage(stream: stream2, topic: 'c', flags: []), + eg.streamMessage(stream: stream3, topic: 'd', flags: []), eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], flags: []), eg.dmMessage(from: eg.thirdUser, to: [eg.selfUser], flags: []), ]); - check(model.countInAllMessagesNarrow()).equals(6); + check(model.countInAllMessagesNarrow()).equals(4); }); - test('countInStreamNarrow', () { + test('countInStream/Narrow', () { final stream = eg.stream(); prepare(); + streamStore.addStream(stream); + streamStore.addSubscription(eg.subscription(stream)); + streamStore.addUserTopic(stream, 'a', UserTopicVisibilityPolicy.unmuted); + streamStore.addUserTopic(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: 'b', flags: []), eg.streamMessage(stream: stream, topic: 'b', flags: []), + eg.streamMessage(stream: stream, topic: 'c', flags: []), ]); + check(model.countInStream (stream.streamId)).equals(5); + check(model.countInStreamNarrow(stream.streamId)).equals(5); + + streamStore.handleEvent(SubscriptionUpdateEvent(id: 1, streamId: stream.streamId, + property: SubscriptionProperty.isMuted, value: true)); + check(model.countInStream (stream.streamId)).equals(2); check(model.countInStreamNarrow(stream.streamId)).equals(5); }); diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index c8555e2b74a..a9f6c77a6c9 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -9,6 +9,7 @@ import 'package:http/http.dart' as http; 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/model/narrow.dart'; import 'package:zulip/api/route/messages.dart'; import 'package:zulip/model/localizations.dart'; import 'package:zulip/model/narrow.dart'; @@ -432,7 +433,8 @@ void main() { } testWidgets('from read to unread', (WidgetTester tester) async { - final message = eg.streamMessage(flags: [MessageFlag.read]); + final message = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], + flags: [MessageFlag.read]); await setupMessageListPage(tester, messages: [message]); check(isMarkAsReadButtonVisible(tester)).isFalse(); @@ -443,9 +445,10 @@ void main() { }); testWidgets('from unread to read', (WidgetTester tester) async { - final message = eg.streamMessage(flags: []); - final unreadMsgs = eg.unreadMsgs(streams:[ - UnreadStreamSnapshot(topic: message.subject, streamId: message.streamId, unreadMessageIds: [message.id]) + final message = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]); + final unreadMsgs = eg.unreadMsgs(dms: [ + UnreadDmSnapshot(otherUserId: eg.otherUser.userId, + unreadMessageIds: [message.id]), ]); await setupMessageListPage(tester, messages: [message], unreadMsgs: unreadMsgs); check(isMarkAsReadButtonVisible(tester)).isTrue(); @@ -461,14 +464,14 @@ void main() { }); group('onPressed behavior', () { - final message = eg.streamMessage(flags: []); - final unreadMsgs = eg.unreadMsgs(streams: [ - UnreadStreamSnapshot(streamId: message.streamId, topic: message.subject, + final message = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]); + final unreadMsgs = eg.unreadMsgs(dms: [ + UnreadDmSnapshot(otherUserId: eg.otherUser.userId, unreadMessageIds: [message.id]), ]); testWidgets('smoke test on modern server', (WidgetTester tester) async { - final narrow = TopicNarrow.ofMessage(message); + final narrow = DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId); await setupMessageListPage(tester, narrow: narrow, messages: [message], unreadMsgs: unreadMsgs); check(isMarkAsReadButtonVisible(tester)).isTrue(); @@ -487,7 +490,8 @@ void main() { 'include_anchor': 'false', 'num_before': '0', 'num_after': '1000', - 'narrow': jsonEncode(narrow.apiEncode()), + 'narrow': jsonEncode(resolveDmElements(narrow.apiEncode(), + eg.recentZulipFeatureLevel)), 'op': 'add', 'flag': 'read', }); @@ -526,7 +530,7 @@ void main() { testWidgets('markNarrowAsRead pagination', (WidgetTester tester) async { // Check that `lastProcessedId` returned from an initial // response is used as `anchorId` for the subsequent request. - final narrow = TopicNarrow.ofMessage(message); + final narrow = DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId); await setupMessageListPage(tester, narrow: narrow, messages: [message], unreadMsgs: unreadMsgs); check(isMarkAsReadButtonVisible(tester)).isTrue(); @@ -545,7 +549,8 @@ void main() { 'include_anchor': 'false', 'num_before': '0', 'num_after': '1000', - 'narrow': jsonEncode(narrow.apiEncode()), + 'narrow': jsonEncode(resolveDmElements(narrow.apiEncode(), + eg.recentZulipFeatureLevel)), 'op': 'add', 'flag': 'read', }); @@ -564,7 +569,8 @@ void main() { 'include_anchor': 'false', 'num_before': '0', 'num_after': '1000', - 'narrow': jsonEncode(narrow.apiEncode()), + 'narrow': jsonEncode(resolveDmElements(narrow.apiEncode(), + eg.recentZulipFeatureLevel)), 'op': 'add', 'flag': 'read', }); @@ -572,7 +578,7 @@ void main() { testWidgets('markNarrowAsRead on invalid response', (WidgetTester tester) async { final zulipLocalizations = GlobalLocalizations.zulipLocalizations; - final narrow = TopicNarrow.ofMessage(message); + final narrow = DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId); await setupMessageListPage(tester, narrow: narrow, messages: [message], unreadMsgs: unreadMsgs); check(isMarkAsReadButtonVisible(tester)).isTrue(); @@ -591,7 +597,8 @@ void main() { 'include_anchor': 'false', 'num_before': '0', 'num_after': '1000', - 'narrow': jsonEncode(narrow.apiEncode()), + 'narrow': jsonEncode(resolveDmElements(narrow.apiEncode(), + eg.recentZulipFeatureLevel)), 'op': 'add', 'flag': 'read', }); @@ -621,7 +628,12 @@ void main() { }); testWidgets('StreamNarrow on legacy server', (WidgetTester tester) async { + final message = eg.streamMessage(); final narrow = StreamNarrow(message.streamId); + final unreadMsgs = eg.unreadMsgs(streams: [ + UnreadStreamSnapshot(streamId: message.streamId, topic: message.subject, + unreadMessageIds: [message.id]), + ]); await setupMessageListPage(tester, narrow: narrow, messages: [message], unreadMsgs: unreadMsgs); check(isMarkAsReadButtonVisible(tester)).isTrue(); @@ -641,7 +653,12 @@ void main() { }); testWidgets('TopicNarrow on legacy server', (WidgetTester tester) async { + final message = eg.streamMessage(); final narrow = TopicNarrow.ofMessage(message); + final unreadMsgs = eg.unreadMsgs(streams: [ + UnreadStreamSnapshot(streamId: message.streamId, topic: message.subject, + unreadMessageIds: [message.id]), + ]); await setupMessageListPage(tester, narrow: narrow, messages: [message], unreadMsgs: unreadMsgs); check(isMarkAsReadButtonVisible(tester)).isTrue(); @@ -662,12 +679,7 @@ void main() { }); testWidgets('DmNarrow on legacy server', (WidgetTester tester) async { - final message = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]); final narrow = DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId); - final unreadMsgs = eg.unreadMsgs(dms: [ - UnreadDmSnapshot(otherUserId: eg.otherUser.userId, - unreadMessageIds: [message.id]), - ]); await setupMessageListPage(tester, narrow: narrow, messages: [message], unreadMsgs: unreadMsgs); check(isMarkAsReadButtonVisible(tester)).isTrue(); diff --git a/test/widgets/subscription_list_test.dart b/test/widgets/subscription_list_test.dart index 1d596a04977..1259bca4a4c 100644 --- a/test/widgets/subscription_list_test.dart +++ b/test/widgets/subscription_list_test.dart @@ -7,6 +7,7 @@ import 'package:zulip/widgets/store.dart'; import 'package:zulip/widgets/subscription_list.dart'; import 'package:zulip/widgets/unread_count_badge.dart'; +import '../flutter_checks.dart'; import '../model/binding.dart'; import '../example_data.dart' as eg; @@ -15,12 +16,14 @@ void main() { Future setupStreamListPage(WidgetTester tester, { required List subscriptions, + List userTopics = const [], UnreadMessagesSnapshot? unreadMsgs, }) async { addTearDown(testBinding.reset); final initialSnapshot = eg.initialSnapshot( subscriptions: subscriptions, streams: subscriptions.toList(), + userTopics: userTopics, unreadMsgs: unreadMsgs, ); await testBinding.globalStore.add(eg.selfAccount, initialSnapshot); @@ -111,6 +114,26 @@ void main() { check(find.byType(UnreadCountBadge).evaluate()).length.equals(1); }); + testWidgets('unread badge counts unmuted only', (tester) async { + final stream = eg.stream(); + final unreadMsgs = eg.unreadMsgs(streams: [ + UnreadStreamSnapshot(streamId: stream.streamId, topic: 'a', unreadMessageIds: [1, 2]), + UnreadStreamSnapshot(streamId: stream.streamId, topic: 'b', unreadMessageIds: [3]), + ]); + await setupStreamListPage(tester, + subscriptions: [eg.subscription(stream, isMuted: true)], + userTopics: [UserTopicItem( + streamId: stream.streamId, + topicName: 'b', + lastUpdated: 1234567890, + visibilityPolicy: UserTopicVisibilityPolicy.unmuted, + )], + unreadMsgs: unreadMsgs); + check(tester.widget(find.descendant( + of: find.byType(UnreadCountBadge), matching: find.byType(Text)))) + .data.equals('1'); + }); + testWidgets('unread badge does not show with no unreads', (tester) async { final stream = eg.stream(); final unreadMsgs = eg.unreadMsgs(streams: []);