Skip to content

msglist: Make mark-as-read button listen to the Unreads model #2020

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chrisbobbe wants to merge 1 commit into
base: main
Choose a base branch
from
Open

msglist: Make mark-as-read button listen to the Unreads model #2020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import '../model/message_list.dart';
import '../model/narrow.dart';
import '../model/store.dart';
import '../model/typing_status.dart';
import '../model/unreads.dart';
import 'action_sheet.dart';
import 'actions.dart';
import 'app_bar.dart';
Expand Down Expand Up @@ -1416,9 +1417,30 @@ class MarkAsReadWidget extends StatefulWidget {
State<MarkAsReadWidget> createState() => _MarkAsReadWidgetState();
}

class _MarkAsReadWidgetState extends State<MarkAsReadWidget> {
class _MarkAsReadWidgetState extends State<MarkAsReadWidget> with PerAccountStoreAwareStateMixin {
Unreads? unreadsModel;

bool _loading = false;

void _unreadsModelChanged() {
setState(() {
// The actual state lives in [unreadsModel].
});
}

@override
void onNewStore() {
final newStore = PerAccountStoreWidget.of(context);
unreadsModel?.removeListener(_unreadsModelChanged);
unreadsModel = newStore.unreads..addListener(_unreadsModelChanged);
}

@override
void dispose() {
unreadsModel?.removeListener(_unreadsModelChanged);
super.dispose();
}

void _handlePress(BuildContext context) async {
if (!context.mounted) return;
setState(() => _loading = true);
Expand All @@ -1429,8 +1451,7 @@ class _MarkAsReadWidgetState extends State<MarkAsReadWidget> {
@override
Widget build(BuildContext context) {
final zulipLocalizations = ZulipLocalizations.of(context);
final store = PerAccountStoreWidget.of(context);
final unreadCount = store.unreads.countInNarrow(widget.narrow);
final unreadCount = unreadsModel!.countInNarrow(widget.narrow);
final shouldHide = unreadCount == 0;

final messageListTheme = MessageListTheme.of(context);
Expand Down
37 changes: 37 additions & 0 deletions test/widgets/message_list_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1001,6 +1001,43 @@ void main() {
check(isMarkAsReadButtonVisible(tester)).isFalse();
});

testWidgets('listens to Unreads model, not just PerAccountStore and MessageListView', (tester) async {
// Regression test for an edge case where the button wouldn't disappear
// when the narrow's unreads were cleared, because the button would only
// respond to notifications from PerAccountStore and MessageListView,
// not Unreads. To test this, we simulate an event that causes Unreads
// but not PerAccountStore and MessageListView to notify listeners,
// and check that the button responds.

final message = eg.streamMessage(flags: [MessageFlag.mentioned]);
Copy link
Member

@gnprice gnprice Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the premise of this test scenario is that this message is older than all the messages that get fetched, it'd be good to give it a smaller message ID too in order to be consistent with that.

That or leave all the message IDs out ­— they're really only needed when the messages get constructed in the test out of order from when they were supposed to be sent.

final unreadMsgs = eg.unreadMsgs(
channels: [
UnreadChannelSnapshot(
topic: message.topic,
streamId: message.streamId,
unreadMessageIds: [message.id],
),
],
mentions: [message.id],
);
await setupMessageListPage(tester,
narrow: MentionsNarrow(),
unreadMsgs: unreadMsgs,
// omit `message`; if present, MessageListView would notify listeners
messages: List.generate(300, (i) =>
eg.streamMessage(id: 950 + i, sender: eg.selfUser,
flags: [MessageFlag.read, MessageFlag.mentioned])),
Comment on lines +1026 to +1029
Copy link
Member

@gnprice gnprice Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. IIUC, the point is that the message had been in the narrow, and an event removes it from the narrow; but it hadn't been one of the messages fetched, because it was too old. Is that right? I think that high-level strategy would be good to make explicit in the test's main comment above.

It looks like the same thing would happen with a channel or topic narrow if the message got moved out of the channel/topic, too.

foundOldest: false);
check(isMarkAsReadButtonVisible(tester)).isTrue();

// The message no longer has an @-mention.
// It was the only unread message with an @-mention,
// so the button should disappear.
await store.handleEvent(eg.updateMessageEditEvent(message, flags: []));
await tester.pumpAndSettle();
check(isMarkAsReadButtonVisible(tester)).isFalse();
});

testWidgets("messages don't shift position", (tester) async {
final message = eg.streamMessage(flags: []);
final unreadMsgs = eg.unreadMsgs(channels:[
Expand Down