Skip to content

Show message content, sender, and timestamp in message action sheet #1624

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.

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
wants to merge 8 commits into
base: main
Choose a base branch
from
6 changes: 6 additions & 0 deletions lib/model/content.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1887,6 +1887,12 @@ class _ZulipContentParser {
}
}

ZulipMessageContent parseMessageContent(Message message) {
final poll = message.poll;
if (poll != null) return PollContent(poll);
return parseContent(message.content);
}

/// Parse a complete piece of Zulip HTML content,
/// such as an entire value of [Message.content].
ZulipContent parseContent(String html) {
Expand Down
14 changes: 4 additions & 10 deletions lib/model/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -244,16 +244,10 @@ mixin _MessageSequence {
}
}

ZulipMessageContent _parseMessageContent(Message message) {
final poll = message.poll;
if (poll != null) return PollContent(poll);
return parseContent(message.content);
}

/// Update data derived from the content of the index-th message.
void _reparseContent(int index) {
final message = messages[index];
final content = _parseMessageContent(message);
final content = parseMessageContent(message);
contents[index] = content;

final itemIndex = findItemWithMessageId(message.id);
Expand All @@ -270,7 +264,7 @@ mixin _MessageSequence {
void _addMessage(Message message) {
assert(contents.length == messages.length);
messages.add(message);
contents.add(_parseMessageContent(message));
contents.add(parseMessageContent(message));
assert(contents.length == messages.length);
_processMessage(messages.length - 1);
}
Expand Down Expand Up @@ -349,7 +343,7 @@ mixin _MessageSequence {
assert(contents.length == messages.length);
messages.insertAll(index, toInsert);
contents.insertAll(index, toInsert.map(
(message) => _parseMessageContent(message)));
(message) => parseMessageContent(message)));
assert(contents.length == messages.length);
if (index <= middleMessage) {
middleMessage += messages.length - oldLength;
Expand Down Expand Up @@ -412,7 +406,7 @@ mixin _MessageSequence {
void _recompute() {
assert(contents.length == messages.length);
contents.clear();
contents.addAll(messages.map((message) => _parseMessageContent(message)));
contents.addAll(messages.map((message) => parseMessageContent(message)));
assert(contents.length == messages.length);
_reprocessAll();
}
Expand Down
111 changes: 90 additions & 21 deletions lib/widgets/action_sheet.dart
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,14 @@ import '../api/route/channels.dart';
import '../api/route/messages.dart';
import '../generated/l10n/zulip_localizations.dart';
import '../model/binding.dart';
import '../model/content.dart';
import '../model/emoji.dart';
import '../model/internal_link.dart';
import '../model/narrow.dart';
import 'actions.dart';
import 'color.dart';
import 'compose_box.dart';
import 'content.dart';
import 'dialog.dart';
import 'emoji.dart';
import 'emoji_reaction.dart';
Expand All @@ -32,39 +34,71 @@ import 'theme.dart';
import 'topic_list.dart';

void _showActionSheet(
BuildContext context, {
BuildContext pageContext, {
required List<Widget> optionButtons,
Widget? header,
Color? headerBackgroundColor,
}) {
// Could omit this if we need _showActionSheet outside a per-account context.
final store = PerAccountStoreWidget.of(pageContext);

showModalBottomSheet<void>(
context: context,
context: pageContext,
// Clip.hardEdge looks bad; Clip.antiAliasWithSaveLayer looks pixel-perfect
// on my iPhone 13 Pro but is marked as "much slower":
// https://api.flutter.dev/flutter/dart-ui/Clip.html
clipBehavior: Clip.antiAlias,
useSafeArea: true,
isScrollControlled: true,
builder: (BuildContext _) {
return Semantics(
role: SemanticsRole.menu,
child: SafeArea(
minimum: const EdgeInsets.only(bottom: 16),
child: Padding(
padding: const EdgeInsets.fromLTRB(16, 0, 16, 0),
final designVariables = DesignVariables.of(pageContext);
return PerAccountStoreWidget(
accountId: store.accountId,
child: Semantics(
role: SemanticsRole.menu,
child: SafeArea(
minimum: const EdgeInsets.only(bottom: 16),
child: Column(
crossAxisAlignment: CrossAxisAlignment.stretch,
mainAxisSize: MainAxisSize.min,
children: [
// TODO(#217): show message text
Flexible(child: InsetShadowBox(
top: 8, bottom: 8,
color: DesignVariables.of(context).bgContextMenu,
child: SingleChildScrollView(
padding: const EdgeInsets.only(top: 16, bottom: 8),
child: ClipRRect(
borderRadius: BorderRadius.circular(7),
child: Column(spacing: 1,
children: optionButtons))))),
const ActionSheetCancelButton(),
if (header != null)
Flexible(
// TODO(upstream) Enforce a flex ratio (e.g. 1:3)
// only when the header height plus the buttons' height
// exceeds available space. Otherwise let one or the other
// grow to fill available space even if it breaks the ratio.
// Needs support for separate properties like `flex-grow`
// and `flex-shrink`.
flex: 1,
Copy link
Member

Choose a reason for hiding this comment

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

The figma design says:

Menu should grow until it fits the whole screen

But currently it doesn't, so maybe let's have a TODO for it.

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; there's a note on #1077 that's relevant:

(TODO upstream issue)
Richer Flex (aka Row and Column), with more CSS flexbox features like flex-basis vs flex-grow vs flex-shrink. (This is one of the few things we miss from React Native: its Yoga layout engine has those features.)
We probably need something like this for: [etc.]

child: InsetShadowBox(
top: 8, bottom: 8,
color: designVariables.bgContextMenu,
child: SingleChildScrollView(
padding: EdgeInsets.symmetric(vertical: 8),
child: ColoredBox(
color: headerBackgroundColor ?? Colors.transparent,
child: header))))
else
SizedBox(height: 8),
Flexible(
flex: 3,
child: Padding(
padding: const EdgeInsets.fromLTRB(16, 0, 16, 0),
child: Column(
crossAxisAlignment: CrossAxisAlignment.stretch,
mainAxisSize: MainAxisSize.min,
children: [
Flexible(child: InsetShadowBox(
top: 8, bottom: 8,
color: designVariables.bgContextMenu,
child: SingleChildScrollView(
padding: const EdgeInsets.symmetric(vertical: 8),
child: ClipRRect(
borderRadius: BorderRadius.circular(7),
child: Column(spacing: 1,
children: optionButtons))))),
const ActionSheetCancelButton(),
]))),
]))));
});
}
Expand Down Expand Up @@ -604,7 +638,42 @@ void showMessageActionSheet({required BuildContext context, required Message mes
EditButton(message: message, pageContext: pageContext),
];

_showActionSheet(pageContext, optionButtons: optionButtons);
_showActionSheet(pageContext,
optionButtons: optionButtons,
header: _MessageActionSheetHeader(message: message));
}

class _MessageActionSheetHeader extends StatelessWidget {
const _MessageActionSheetHeader({required this.message});

final Message message;

@override
Widget build(BuildContext context) {
final designVariables = DesignVariables.of(context);

// TODO this seems to lose the hero animation when opening an image;
// investigate.
// TODO should we close the sheet before opening a narrow link?
// On popping the pushed narrow route, the sheet is still open.

return Container(
// TODO(#647) use different color for highlighted messages
// TODO(#681) use different color for DM messages
color: designVariables.bgMessageRegular,
padding: EdgeInsets.symmetric(vertical: 4),
child: Column(
mainAxisSize: MainAxisSize.max,
spacing: 4,
children: [
SenderRow(message: message, showTimestamp: true),
Padding(
padding: const EdgeInsets.symmetric(horizontal: 16),
// TODO(#10) offer text selection; the Figma asks for it here:
// https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3483-30210&m=dev
child: MessageContent(message: message, content: parseMessageContent(message))),
Copy link
Member

Choose a reason for hiding this comment

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

Couple of things I noticed with interactions in the message content:

  • There was no hero animation when opening an image.
  • Clicking on a narrow link, would open the narrow, but wouldn't close the action sheet (which we do for some for some cases).

Neither seem blocker for this feature though.

]));
}
}

abstract class MessageActionSheetMenuItemButton extends ActionSheetMenuItemButton {
Expand Down
8 changes: 4 additions & 4 deletions lib/widgets/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1675,8 +1675,8 @@ String formatHeaderDate(
// TODO(i18n): web seems to ignore locale in formatting time, but we could do better
final _kMessageTimestampFormat = DateFormat('h:mm aa', 'en_US');

class _SenderRow extends StatelessWidget {
const _SenderRow({required this.message, required this.showTimestamp});
class SenderRow extends StatelessWidget {
const SenderRow({super.key, required this.message, required this.showTimestamp});

final MessageBase message;
final bool showTimestamp;
Expand Down Expand Up @@ -1805,7 +1805,7 @@ class MessageWithPossibleSender extends StatelessWidget {
padding: const EdgeInsets.only(top: 4),
child: Column(children: [
if (item.showSender)
_SenderRow(message: message, showTimestamp: true),
SenderRow(message: message, showTimestamp: true),
Row(
crossAxisAlignment: CrossAxisAlignment.baseline,
textBaseline: localizedTextBaseline(context),
Expand Down Expand Up @@ -1956,7 +1956,7 @@ class OutboxMessageWithPossibleSender extends StatelessWidget {
padding: const EdgeInsets.only(top: 4),
child: Column(children: [
if (item.showSender)
_SenderRow(message: message, showTimestamp: false),
SenderRow(message: message, showTimestamp: false),
Padding(
padding: const EdgeInsets.symmetric(horizontal: 16),
child: Column(crossAxisAlignment: CrossAxisAlignment.stretch,
Expand Down
51 changes: 49 additions & 2 deletions test/widgets/action_sheet_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import '../api/fake_api.dart';
import '../example_data.dart' as eg;
import '../flutter_checks.dart';
import '../model/binding.dart';
import '../model/content_test.dart';
import '../model/test_store.dart';
import '../stdlib_checks.dart';
import '../test_clipboard.dart';
Expand Down Expand Up @@ -103,7 +104,11 @@ Future<void> setupToMessageActionSheet(WidgetTester tester, {
// because its render box effectively has HitTestBehavior.deferToChild, and
// the long-press might land where no child hit-tests as true,
// like if it's in padding around a Paragraph.
await tester.longPress(find.byType(MessageContent), warnIfMissed: false);
await tester.longPress(
find.descendant(
of: find.byType(MessageList),
matching: find.byType(MessageContent)),
warnIfMissed: false);
// sheet appears onscreen; default duration of bottom-sheet enter animation
await tester.pump(const Duration(milliseconds: 250));
// Check the action sheet did in fact open, so we don't defeat any tests that
Expand Down Expand Up @@ -850,6 +855,44 @@ void main() {
});

group('message action sheet', () {
group('header', () {
testWidgets('message sender and content shown', (tester) async {
final message = eg.streamMessage(content: ContentExample.userMentionPlain.html);
await setupToMessageActionSheet(tester,
message: message,
narrow: TopicNarrow.ofMessage(message));
check(find.descendant(
of: find.byType(BottomSheet),
matching: find.byWidgetPredicate(
(widget) => widget is Avatar && widget.userId == message.senderId))
).findsOne();
check(find.descendant(
of: find.byType(BottomSheet),
matching: find.byType(UserMention))
).findsOne();
});

testWidgets('poll is rendered', (tester) async {
final submessageContent = eg.pollWidgetData(
question: 'poll', options: ['First option', 'Second option']);
final message = eg.streamMessage(
sender: eg.selfUser,
submessages: [eg.submessage(content: submessageContent)]);
await setupToMessageActionSheet(tester,
message: message,
narrow: TopicNarrow.ofMessage(message));
check(find.descendant(
of: find.byType(BottomSheet),
matching: find.byWidgetPredicate(
(widget) => widget is Avatar && widget.userId == message.senderId))
).findsOne();
check(find.descendant(
of: find.byType(BottomSheet),
matching: find.text('First option'))
).findsOne();
});
});

group('ReactionButtons', () {
testWidgets('absent if ServerEmojiData not loaded', (tester) async {
final message = eg.streamMessage();
Expand Down Expand Up @@ -1592,7 +1635,11 @@ void main() {
}

// See comment in setupToMessageActionSheet about warnIfMissed: false
await tester.longPress(find.byType(MessageContent), warnIfMissed: false);
await tester.longPress(
find.descendant(
of: find.byType(MessageList),
matching: find.byType(MessageContent)),
warnIfMissed: false);
// sheet appears onscreen; default duration of bottom-sheet enter animation
await tester.pump(const Duration(milliseconds: 250));
check(find.byType(BottomSheet)).findsOne();
Expand Down