Skip to content

msglist: Tapping a message in Starred or Mentions opens anchored msglist #1657

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

Merged
merged 2 commits into from
Jul 3, 2025
Merged

msglist: Tapping a message in Starred or Mentions opens anchored msglist #1657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0cf88f3
msglist test: Add missing test for tapping channel in recipient header
chrisbobbe Jun 26, 2025
a922f56
msglist: Tapping a message in Starred or Mentions opens anchored msglist
chrisbobbe Jul 2, 2025
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
37 changes: 34 additions & 3 deletions lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import 'dart:async';

import 'package:collection/collection.dart';
import 'package:flutter/material.dart';
import 'package:flutter_color_models/flutter_color_models.dart';
Expand Down Expand Up @@ -989,11 +991,15 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
final header = RecipientHeader(message: data.message, narrow: widget.narrow);
return MessageItem(
key: ValueKey(data.message.id),
narrow: widget.narrow,
header: header,
item: data);
case MessageListOutboxMessageItem():
final header = RecipientHeader(message: data.message, narrow: widget.narrow);
return MessageItem(header: header, item: data);
return MessageItem(
narrow: widget.narrow,
header: header,
item: data);
}
}
}
Expand Down Expand Up @@ -1315,10 +1321,12 @@ class DateSeparator extends StatelessWidget {
class MessageItem extends StatelessWidget {
const MessageItem({
super.key,
required this.narrow,
required this.item,
required this.header,
});

final Narrow narrow;
final MessageListMessageBaseItem item;
final Widget header;

Expand All @@ -1331,7 +1339,9 @@ class MessageItem extends StatelessWidget {
color: designVariables.bgMessageRegular,
child: Column(children: [
switch (item) {
MessageListMessageItem() => MessageWithPossibleSender(item: item),
MessageListMessageItem() => MessageWithPossibleSender(
narrow: narrow,
item: item),
MessageListOutboxMessageItem() => OutboxMessageWithPossibleSender(item: item),
},
// TODO refine this padding; discussion:
Expand Down Expand Up @@ -1748,8 +1758,13 @@ class _SenderRow extends StatelessWidget {
// - https://github.com/zulip/zulip-mobile/issues/5511
// - https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=538%3A20849&mode=dev
class MessageWithPossibleSender extends StatelessWidget {
const MessageWithPossibleSender({super.key, required this.item});
const MessageWithPossibleSender({
super.key,
required this.narrow,
required this.item,
});

final Narrow narrow;
final MessageListMessageItem item;

@override
Expand Down Expand Up @@ -1798,8 +1813,24 @@ class MessageWithPossibleSender extends StatelessWidget {
}
}

final tapOpensConversation = switch (narrow) {
CombinedFeedNarrow()
|| ChannelNarrow()
|| TopicNarrow()
|| DmNarrow() => false,
MentionsNarrow()
|| StarredMessagesNarrow() => true,
};

return GestureDetector(
behavior: HitTestBehavior.translucent,
onTap: tapOpensConversation
? () => unawaited(Navigator.push(context,
MessageListPage.buildRoute(context: context,
narrow: SendableNarrow.ofMessage(message, selfUserId: store.selfUserId),
// TODO(#1655) "this view does not mark messages as read on scroll"
initAnchorMessageId: message.id)))
: null,
onLongPress: () => showMessageActionSheet(context: context, message: message),
child: Padding(
padding: const EdgeInsets.only(top: 4),
Expand Down
1 change: 1 addition & 0 deletions test/widgets/message_list_checks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@ import 'package:zulip/widgets/message_list.dart';

extension MessageListPageChecks on Subject<MessageListPage> {
Subject<Narrow> get initNarrow => has((x) => x.initNarrow, 'initNarrow');
Subject<int?> get initAnchorMessageId => has((x) => x.initAnchorMessageId, 'initAnchorMessageId');
}
103 changes: 103 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 @@ -1342,6 +1342,33 @@ void main() {
tester.widget(find.text('new stream name'));
});

testWidgets('navigates to ChannelNarrow on tapping channel in CombinedFeedNarrow', (tester) async {
final pushedRoutes = <Route<void>>[];
final navObserver = TestNavigatorObserver()
..onPushed = (route, prevRoute) => pushedRoutes.add(route);
final channel = eg.stream();
final subscription = eg.subscription(channel);
final message = eg.streamMessage(stream: channel, topic: 'topic name');
await setupMessageListPage(tester,
narrow: CombinedFeedNarrow(),
subscriptions: [subscription],
messages: [message],
navObservers: [navObserver]);

assert(pushedRoutes.length == 1);
pushedRoutes.clear();

connection.prepare(json: eg.newestGetMessagesResult(
foundOldest: true, messages: [message]).toJson());
await tester.tap(find.descendant(
of: find.byType(StreamMessageRecipientHeader),
matching: find.text(channel.name)));
await tester.pump();
check(pushedRoutes).single.isA<WidgetRoute>().page.isA<MessageListPage>()
.initNarrow.equals(ChannelNarrow(channel.streamId));
await tester.pumpAndSettle();
});

testWidgets('navigates to TopicNarrow on tapping topic in ChannelNarrow', (tester) async {
final pushedRoutes = <Route<void>>[];
final navObserver = TestNavigatorObserver()
Expand Down Expand Up @@ -1626,6 +1653,82 @@ void main() {

debugNetworkImageHttpClientProvider = null;
});

group('Opens conversation on tap?', () {
// (copied from test/widgets/content_test.dart)
Future<void> tapText(WidgetTester tester, Finder textFinder) async {
final height = tester.getSize(textFinder).height;
final target = tester.getTopLeft(textFinder)
.translate(height/4, height/2); // aim for middle of first letter
await tester.tapAt(target);
}

final subscription = eg.subscription(eg.stream(streamId: eg.defaultStreamMessageStreamId));
final topic = 'some topic';

void doTest(Narrow narrow, {
required bool expected,
required Message Function() mkMessage,
}) {
testWidgets('${expected ? 'yes' : 'no'}, if in $narrow', (tester) async {
final message = mkMessage();

Route<dynamic>? lastPushedRoute;
final navObserver = TestNavigatorObserver()
..onPushed = ((route, prevRoute) => lastPushedRoute = route);

await setupMessageListPage(
tester,
narrow: narrow,
messages: [message],
subscriptions: [subscription],
navObservers: [navObserver]
);
lastPushedRoute = null;

// Tapping interactive content still works.
await store.handleEvent(eg.updateMessageEditEvent(message,
renderedContent: '<p><a href="https://example/">link</a></p>'));
await tester.pump();
await tapText(tester, find.text('link'));
await tester.pump(Duration.zero);
check(lastPushedRoute).isNull();
final launchUrlCalls = testBinding.takeLaunchUrlCalls();
check(launchUrlCalls.single.url).equals(Uri.parse('https://example/'));

// Tapping non-interactive content opens the conversation (if expected).
await store.handleEvent(eg.updateMessageEditEvent(message,
renderedContent: '<p>plain content</p>'));
await tester.pump();
await tapText(tester, find.text('plain content'));
if (expected) {
final expectedNarrow = SendableNarrow.ofMessage(message, selfUserId: store.selfUserId);

check(lastPushedRoute).isNotNull().isA<MaterialAccountWidgetRoute>()
.page.isA<MessageListPage>()
..initNarrow.equals(expectedNarrow)
..initAnchorMessageId.equals(message.id);
} else {
check(lastPushedRoute).isNull();
}

// TODO test tapping whitespace in message
Copy link
Member

Choose a reason for hiding this comment

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

I guess this TODO test is the one that would fail if the HitTestBehavior.translucent were changed to deferToChild.

Leaving a TODO seems fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah; I timed out trying to simulate tapping the back button to clear the state from the previous part of the test (find.backButton() wasn't working)

});
}

doTest(expected: false, CombinedFeedNarrow(),
mkMessage: () => eg.streamMessage());
doTest(expected: false, ChannelNarrow(subscription.streamId),
mkMessage: () => eg.streamMessage(stream: subscription));
doTest(expected: false, TopicNarrow(subscription.streamId, eg.t(topic)),
mkMessage: () => eg.streamMessage(stream: subscription));
doTest(expected: false, DmNarrow.withUsers([], selfUserId: eg.selfUser.userId),
mkMessage: () => eg.streamMessage(stream: subscription, topic: topic));
doTest(expected: true, StarredMessagesNarrow(),
mkMessage: () => eg.streamMessage(flags: [MessageFlag.starred]));
doTest(expected: true, MentionsNarrow(),
mkMessage: () => eg.streamMessage(flags: [MessageFlag.mentioned]));
});
});

group('OutboxMessageWithPossibleSender', () {
Expand Down