Skip to content

Commit b1ea5e0

Browse files
committed
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 #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 9296fb3 commit b1ea5e0

File tree

2 files changed

+61
-3
lines changed

2 files changed

+61
-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';
@@ -1416,9 +1417,30 @@ class MarkAsReadWidget extends StatefulWidget {
14161417
State<MarkAsReadWidget> createState() => _MarkAsReadWidgetState();
14171418
}
14181419

1419-
class _MarkAsReadWidgetState extends State<MarkAsReadWidget> {
1420+
class _MarkAsReadWidgetState extends State<MarkAsReadWidget> with PerAccountStoreAwareStateMixin {
1421+
Unreads? unreadsModel;
1422+
14201423
bool _loading = false;
14211424

1425+
void _unreadsModelChanged() {
1426+
setState(() {
1427+
// The actual state lives in [unreadsModel].
1428+
});
1429+
}
1430+
1431+
@override
1432+
void onNewStore() {
1433+
final newStore = PerAccountStoreWidget.of(context);
1434+
unreadsModel?.removeListener(_unreadsModelChanged);
1435+
unreadsModel = newStore.unreads..addListener(_unreadsModelChanged);
1436+
}
1437+
1438+
@override
1439+
void dispose() {
1440+
unreadsModel?.removeListener(_unreadsModelChanged);
1441+
super.dispose();
1442+
}
1443+
14221444
void _handlePress(BuildContext context) async {
14231445
if (!context.mounted) return;
14241446
setState(() => _loading = true);
@@ -1429,8 +1451,7 @@ class _MarkAsReadWidgetState extends State<MarkAsReadWidget> {
14291451
@override
14301452
Widget build(BuildContext context) {
14311453
final zulipLocalizations = ZulipLocalizations.of(context);
1432-
final store = PerAccountStoreWidget.of(context);
1433-
final unreadCount = store.unreads.countInNarrow(widget.narrow);
1454+
final unreadCount = unreadsModel!.countInNarrow(widget.narrow);
14341455
final shouldHide = unreadCount == 0;
14351456

14361457
final messageListTheme = MessageListTheme.of(context);

test/widgets/message_list_test.dart

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1001,6 +1001,43 @@ void main() {
10011001
check(isMarkAsReadButtonVisible(tester)).isFalse();
10021002
});
10031003

1004+
testWidgets('listens to Unreads model, not just PerAccountStore and MessageListView', (tester) async {
1005+
// Regression test for an edge case where the button wouldn't disappear
1006+
// when the narrow's unreads were cleared, because the button would only
1007+
// respond to notifications from PerAccountStore and MessageListView,
1008+
// not Unreads. To test this, we simulate an event that causes Unreads
1009+
// but not PerAccountStore and MessageListView to notify listeners,
1010+
// and check that the button responds.
1011+
1012+
final message = eg.streamMessage(flags: [MessageFlag.mentioned]);
1013+
final unreadMsgs = eg.unreadMsgs(
1014+
channels: [
1015+
UnreadChannelSnapshot(
1016+
topic: message.topic,
1017+
streamId: message.streamId,
1018+
unreadMessageIds: [message.id],
1019+
),
1020+
],
1021+
mentions: [message.id],
1022+
);
1023+
await setupMessageListPage(tester,
1024+
narrow: MentionsNarrow(),
1025+
unreadMsgs: unreadMsgs,
1026+
// omit `message`; if present, MessageListView would notify listeners
1027+
messages: List.generate(300, (i) =>
1028+
eg.streamMessage(id: 950 + i, sender: eg.selfUser,
1029+
flags: [MessageFlag.read, MessageFlag.mentioned])),
1030+
foundOldest: false);
1031+
check(isMarkAsReadButtonVisible(tester)).isTrue();
1032+
1033+
// The message no longer has an @-mention.
1034+
// It was the only unread message with an @-mention,
1035+
// so the button should disappear.
1036+
await store.handleEvent(eg.updateMessageEditEvent(message, flags: []));
1037+
await tester.pumpAndSettle();
1038+
check(isMarkAsReadButtonVisible(tester)).isFalse();
1039+
});
1040+
10041041
testWidgets("messages don't shift position", (tester) async {
10051042
final message = eg.streamMessage(flags: []);
10061043
final unreadMsgs = eg.unreadMsgs(channels:[

0 commit comments

Comments
 (0)