Skip to content

Commit d75373b

Browse files
chrisbobbegnprice
authored andcommitted
msglist: Make mark-as-read button listen to the Unreads model
Instead of just listening to PerAccountStore and (via an ancestor widget) a MessageListView. Issue zulip#370 is relevant here -- "Maintain total unread counts efficiently". The button causes linear scans through Unreads each time it rebuilds, and ideally we'd avoid that, especially since now it'll rebuild in more cases than it was before. I think the additional cases aren't very many, though: I've thought of just one kind of Zulip event that would cause Unreads but not PerAccountStore and MessageListView to notify listeners, and I added a test exercising that.
1 parent ab1bb45 commit d75373b

File tree

2 files changed

+66
-3
lines changed

2 files changed

+66
-3
lines changed

lib/widgets/message_list.dart

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import '../model/message_list.dart';
1515
import '../model/narrow.dart';
1616
import '../model/store.dart';
1717
import '../model/typing_status.dart';
18+
import '../model/unreads.dart';
1819
import 'action_sheet.dart';
1920
import 'actions.dart';
2021
import 'app_bar.dart';
@@ -1499,9 +1500,30 @@ class MarkAsReadWidget extends StatefulWidget {
14991500
State<MarkAsReadWidget> createState() => _MarkAsReadWidgetState();
15001501
}
15011502

1502-
class _MarkAsReadWidgetState extends State<MarkAsReadWidget> {
1503+
class _MarkAsReadWidgetState extends State<MarkAsReadWidget> with PerAccountStoreAwareStateMixin {
1504+
Unreads? unreadsModel;
1505+
15031506
bool _loading = false;
15041507

1508+
void _unreadsModelChanged() {
1509+
setState(() {
1510+
// The actual state lives in [unreadsModel].
1511+
});
1512+
}
1513+
1514+
@override
1515+
void onNewStore() {
1516+
final newStore = PerAccountStoreWidget.of(context);
1517+
unreadsModel?.removeListener(_unreadsModelChanged);
1518+
unreadsModel = newStore.unreads..addListener(_unreadsModelChanged);
1519+
}
1520+
1521+
@override
1522+
void dispose() {
1523+
unreadsModel?.removeListener(_unreadsModelChanged);
1524+
super.dispose();
1525+
}
1526+
15051527
void _handlePress(BuildContext context) async {
15061528
if (!context.mounted) return;
15071529
setState(() => _loading = true);
@@ -1512,8 +1534,7 @@ class _MarkAsReadWidgetState extends State<MarkAsReadWidget> {
15121534
@override
15131535
Widget build(BuildContext context) {
15141536
final zulipLocalizations = ZulipLocalizations.of(context);
1515-
final store = PerAccountStoreWidget.of(context);
1516-
final unreadCount = store.unreads.countInNarrow(widget.narrow);
1537+
final unreadCount = unreadsModel!.countInNarrow(widget.narrow);
15171538
final shouldHide = unreadCount == 0;
15181539

15191540
final messageListTheme = MessageListTheme.of(context);

test/widgets/message_list_test.dart

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1211,6 +1211,48 @@ void main() {
12111211
check(isMarkAsReadButtonVisible(tester)).isFalse();
12121212
});
12131213

1214+
testWidgets('listens to Unreads model, not just PerAccountStore and MessageListView', (tester) async {
1215+
// Regression test for an edge case where the button wouldn't disappear
1216+
// when the narrow's unreads were cleared, because the button would only
1217+
// respond to notifications from PerAccountStore and MessageListView,
1218+
// not Unreads.
1219+
//
1220+
// For this test, there's one unread message in the narrow,
1221+
// and it's old enough that it hasn't been fetched yet.
1222+
// We simulate an event that removes the message from the narrow,
1223+
// thus causing Unreads but not MessageListView or PerAccountStore
1224+
// to notify listeners.
1225+
// And we check that the button responds by disappearing.
1226+
1227+
final message = eg.streamMessage(id: 100, flags: [MessageFlag.mentioned]);
1228+
final unreadMsgs = eg.unreadMsgs(
1229+
channels: [
1230+
UnreadChannelSnapshot(
1231+
topic: message.topic,
1232+
streamId: message.streamId,
1233+
unreadMessageIds: [message.id],
1234+
),
1235+
],
1236+
mentions: [message.id],
1237+
);
1238+
await setupMessageListPage(tester,
1239+
narrow: MentionsNarrow(),
1240+
unreadMsgs: unreadMsgs,
1241+
// omit `message`; if present, MessageListView would notify listeners
1242+
messages: List.generate(300, (i) =>
1243+
eg.streamMessage(id: 950 + i, sender: eg.selfUser,
1244+
flags: [MessageFlag.read, MessageFlag.mentioned])),
1245+
foundOldest: false);
1246+
check(isMarkAsReadButtonVisible(tester)).isTrue();
1247+
1248+
// The message no longer has an @-mention.
1249+
// It was the only unread message with an @-mention,
1250+
// so the button should disappear.
1251+
await store.handleEvent(eg.updateMessageEditEvent(message, flags: []));
1252+
await tester.pumpAndSettle();
1253+
check(isMarkAsReadButtonVisible(tester)).isFalse();
1254+
});
1255+
12141256
testWidgets("messages don't shift position", (tester) async {
12151257
final message = eg.streamMessage(flags: []);
12161258
final unreadMsgs = eg.unreadMsgs(channels:[

0 commit comments

Comments
 (0)