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

Merged
merged 10 commits into from
Jul 9, 2025

Conversation

chrisbobbe
Copy link
Collaborator

@chrisbobbe chrisbobbe commented Jun 24, 2025

Fixes #217.

We've had some complaints about not being able to see the timestamps of messages in a run of messages from the same sender, e.g.:

#mobile > Sent time not visible @ 💬

There is a "message sent" time shown in an upper right hand corner of the message. But when the messages are sent by bot, the time is only shown for the first message of the day. The old android app at least shown the time when tapped on the message, but I haven't found a way how to display the time here.

We have #457 for how to offer that in a really quick interaction. But this PR lets you get that information, and it aligns with a different part of the Figma, namely #217. As discussed: #mobile > Sent time not visible @ 💬

(edit: all but one of these screenshots are outdated in the timestamp format; the one with the full date and time is current)

Light Dark
image image
image image
image image

This revision leaves the message completely interactable; you can open images in the lightbox, open and close spoilers, etc.; that seemed most straightforward to implement. It does not implement text selection of the content; that's requested in the Figma, and #10 is for that, and I've left a TODO for it.

@chrisbobbe chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Jun 24, 2025
@chrisbobbe
Copy link
Collaborator Author

This also mitigates #1142 because it gives a way to see one message by itself.

Copy link
Member

@rajveermalviya rajveermalviya left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @chrisbobbe!

This looks and works great for me, one small comment below.

SizedBox(height: 8),
if (header != null)
Flexible(
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.]

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.

@chrisbobbe chrisbobbe force-pushed the pr-message-content-in-action-sheet branch from 8bc6031 to 6a2649a Compare June 26, 2025 07:17
@chrisbobbe
Copy link
Collaborator Author

Thanks! I've added TODOs for all those points.

@rajveermalviya rajveermalviya added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Jun 30, 2025
@rajveermalviya rajveermalviya requested a review from gnprice June 30, 2025 16:14
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks for building this! Generally it all looks good; comments below.

Comment on lines 106 to 111
await tester.longPress(find.byType(MessageContent), warnIfMissed: false);
await tester.longPress(
find.descendant(
of: find.byType(MessageList),
matching: find.byType(MessageContent)),
warnIfMissed: false);
Copy link
Member

Choose a reason for hiding this comment

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

action_sheet test: Narrow a Finder (long-press for message action sheet)

We're about to put a MessageContent in the action sheet itself,
for #217.

This change seems fine but I'm a bit surprised it's needed. There's no action sheet in the tree at this point, right? This is the step that will cause one to appear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I think this isn't actually needed here; I should be able to simplify by dropping this commit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh wait, not the whole commit, just this one change.

@@ -1887,6 +1887,12 @@ class _ZulipContentParser {
}
}

ZulipMessageContent parseMessageContent(Message message) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: put at end of file, matching the specific-to-general ordering of the other parsing code here

}) {
// Could omit this if we need _showActionSheet outside a per-account context.
final store = PerAccountStoreWidget.of(pageContext);
Copy link
Member

Choose a reason for hiding this comment

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

Let's use accountIdOf instead, to make clear that this doesn't use anything else from the store.

(If it kept this store around and then used much of anything else from it in the build callback below, that'd be a bug because the store might change; in general we don't want to be keeping references to the store around beyond the lifetime of a build method or a gesture callback, and instead should look the store up anew from a context.)

Comment on lines 78 to 80
child: ColoredBox(
color: headerBackgroundColor ?? Colors.transparent,
child: header))))
Copy link
Member

Choose a reason for hiding this comment

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

This color is currently always transparent, right? So can simplify this out, if that's the value we want.

Comment on lines +641 to +645
_showActionSheet(pageContext,
optionButtons: optionButtons,
header: _MessageActionSheetHeader(message: message));
Copy link
Member

Choose a reason for hiding this comment

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

nit: put header before option buttons; it appears before them physically, and logically they feel like the payload and this more like some decoration, so those two ways of ordering agree with each other

color: designVariables.bgMessageRegular,
padding: EdgeInsets.symmetric(vertical: 4),
child: Column(
mainAxisSize: MainAxisSize.max,
Copy link
Member

Choose a reason for hiding this comment

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

Hmm interesting. What's the effect of this parameter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, I think this isn't wanted; will remove. (I'm not noticing it doing anything, and can't think of why we'd want it.)

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.

At first I'd thought this wouldn't be possible without more work
toward issue #488:

- #488 "content: Support Zulip content outside messages (even
  outside per-account contexts)".

But it turns out that our content widgets don't assume a MessageList
ancestor; it's sufficient that we provide a per-account context, via
PerAccountStoreWidget, and a per-message context, via
InheritedMessage (created by MessageContent).

How does this interact with muted users, #1560? That's one place where some related code, at least, relies on state that lives at MessageList.

(This is probably an instance where we're paying some cost for the technical debt of not yet having merged those muted-users PRs.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, this calls for some work to allow SenderRow to appear where there isn't an ancestor MessageListPage. Will fix.

@@ -850,6 +855,44 @@ void main() {
});

group('message action sheet', () {
group('header', () {
testWidgets('message sender and content shown', (tester) async {
Copy link
Member

Choose a reason for hiding this comment

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

Is it easy to also add a check that the timestamp is shown? That's the motivating use case that prompted us to actually go and implement this feature now, so it'd be good to test it too 🙂

(But I know controlling times and time zones is an annoying area in tests still, so if it's not easy then we can skip it in favor of shipping, with I guess a TODO comment here in the tests.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I started writing a test for this just now, then realized we don't show the full date :) is that okay, do you think? You could be looking at a message from a different day and see "1:54 PM" here, and I wonder if that might confuse people into thinking it was sent today.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, good catch!

That does sound potentially confusing, so I think it'd be better to avoid it. Perhaps show the date (as if in a date separator or recipient header) and then the time? Or perhaps a refinement: skip the date if it's today.

@chrisbobbe chrisbobbe force-pushed the pr-message-content-in-action-sheet branch from 6a2649a to c98481d Compare July 3, 2025 21:09
@chrisbobbe
Copy link
Collaborator Author

Thanks for the review! Revision pushed.

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks for the revision! Comments below.

Comment on lines 1677 to 1756
await tester.longPress(find.byType(MessageContent), warnIfMissed: false);
await tester.longPress(
find.descendant(
of: find.byType(MessageList),
matching: find.byType(MessageContent)),
Copy link
Member

Choose a reason for hiding this comment

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

Same question as at #1624 (comment) , I think. In this one, there was an action sheet at a previous step of the test, but we already hit a button on it and then even used the compose box to save a message edit. So it seems like the story of the test calls for the action sheet to not be here now — which is why we need this step, after all, to bring it back.

It looks like nothing above waits for the action sheet to actually disappear after the button is hit, though; instead there's just await tester.pump(Duration.zero);. Maybe have that wait longer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch; yes, thanks for spotting this.

@@ -850,6 +855,44 @@ void main() {
});

group('message action sheet', () {
group('header', () {
testWidgets('message sender and content shown', (tester) async {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, good catch!

That does sound potentially confusing, so I think it'd be better to avoid it. Perhaps show the date (as if in a date separator or recipient header) and then the time? Or perhaps a refinement: skip the date if it's today.

).findsOne();
});

testWidgets('muted sender', (tester) async {
Copy link
Member

Choose a reason for hiding this comment

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

nit: clarify the expected outcome (as it's the opposite of what you might expect if you just look at the test name):

Suggested change
testWidgets('muted sender', (tester) async {
testWidgets('muted sender also shown', (tester) async {

Comment on lines 1782 to 1783
&& (MessageListPage.maybeRevealedMutedMessagesOf(context)
?.isMutedMessageRevealed((message as Message).id) == false);
Copy link
Member

Choose a reason for hiding this comment

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

This feels a bit hard to follow; and the behavior when there is no MessageListPage gets kind of buried here, whereas I feel like it should be more explicit and get a brief comment.

I'll send a quick PR to pull this whole condition out as a private method, which you can rebase atop to add this piece.

Copy link
Member

Choose a reason for hiding this comment

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

Sent as #1664.

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Jul 4, 2025
…ting

Thanks Greg for noticing that we were missing this part in the story
of the test:
  zulip#1624 (comment)

We're about to put a MessageContent in the action sheet itself,
for zulip#217, and without this change, we'd get a failure later in the
test when two MessageContents were found when one was expected (one
in the message list and one in the action sheet).
@chrisbobbe chrisbobbe force-pushed the pr-message-content-in-action-sheet branch from c98481d to 5e56d4c Compare July 4, 2025 01:45
@chrisbobbe
Copy link
Collaborator Author

Thanks! Revision pushed, and I've put some thoughts about the timestamp format in a dartdoc.

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks for the revision! Would you update one of the screenshots to show the new date/time format? (Fine to leave the rest un-updated, I think — this doesn't really interact with the variables that vary between them.)

@@ -1970,6 +1971,38 @@ class _SenderRow extends StatelessWidget {
}
}

// TODO centralize on this for wherever we show message timestamps
enum MessageTimestampDisplay {
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, I like this way of organizing it.

@@ -2223,7 +2257,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, timestampDisplay: null),
Copy link
Member

Choose a reason for hiding this comment

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

How about:

Suggested change
SenderRow(message: message, timestampDisplay: null),
SenderRow(message: message, timestampStyle: MessageTimestampStyle.none),

(which in the near future will look like:

          SenderRow(message: message, timestampStyle: .none),

)

I think that'd be a bit more explicit than using null for this parameter.

Then the format method can return null for that style, so that the timestamp local in the build method is still null, which I think is helpful in that logic.

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Jul 8, 2025
…ting

Thanks Greg for noticing that we were missing this part in the story
of the test:
  zulip#1624 (comment)

We're about to put a MessageContent in the action sheet itself,
for zulip#217, and without this change, we'd get a failure later in the
test when two MessageContents were found when one was expected (one
in the message list and one in the action sheet).
@chrisbobbe chrisbobbe force-pushed the pr-message-content-in-action-sheet branch from 5e56d4c to 1022fb8 Compare July 8, 2025 19:02
@chrisbobbe
Copy link
Collaborator Author

Thanks! Update a screenshot and revision pushed, PTAL.

@gnprice
Copy link
Member

gnprice commented Jul 8, 2025

Thanks! LGTM.

@alya, do you have any feedback on the screenshots? (The one at the bottom left is current; the others have a previous version of the timestamp display but I believe are otherwise the same as the current revision.)

@alya
Copy link
Collaborator

alya commented Jul 8, 2025

Generally looks good! Is it an illusion, or do we have different letter spacing in "Jul" vs. "AM" (which seems weird)?

Screenshot 2025-07-08 at 13 51 35

@alya
Copy link
Collaborator

alya commented Jul 8, 2025

Do we use "Today at" and "Yesterday at" as appropriate? Not essential, but seems nice.

@alya
Copy link
Collaborator

alya commented Jul 8, 2025

Do we need to handle long names running into the date/time, or is max sender name length short enough for that not to be an issue?

@gnprice
Copy link
Member

gnprice commented Jul 8, 2025

I think the letter spacing is just the letters A and M being wider, and so the font letting them get a bit closer together.

It's one string displayed with a single text style (font and all the other parameters).

@chrisbobbe
Copy link
Collaborator Author

Do we use "Today at" and "Yesterday at" as appropriate? Not essential, but seems nice.

Good question; I wrote down my thinking in a code comment (see full):

// TODO centralize on this for wherever we show message timestamps
enum MessageTimestampStyle {
  none,
  timeOnly,

  /// The longest format, with full date and time as numbers, not "Today"/etc.
  ///
  /// For UI contexts focused just on the one message,
  /// or as a tooltip on a shorter-formatted timestamp.
  ///
  /// The detail won't always be needed, but this format makes mental timezone
  /// conversions easier, which is helpful when the user is thinking about
  /// business hours on a different continent,
  /// or traveling and they know their device timezone setting is wrong, etc.
  full,
  ;

@alya
Copy link
Collaborator

alya commented Jul 8, 2025

I dunno, to me the date is harder to think about than "Today" in the vast majority of cases when there's nothing weird about my time zone. I don't always know today's date.

@chrisbobbe
Copy link
Collaborator Author

Do we need to handle long names running into the date/time, or is max sender name length short enough for that not to be an issue?

A long name will just be truncated with "…" if it runs into the timestamp. We could allow it to wrap on a second line, if helpful?

@alya
Copy link
Collaborator

alya commented Jul 8, 2025

Truncating seems fine. Just as long as it looks reasonable. :)

@chrisbobbe
Copy link
Collaborator Author

chrisbobbe commented Jul 8, 2025

when there's nothing weird about my time zone

(Or the sender's time zone 🙂)

I can add a TODO for localized "Today"/"Yesterday"-style timestamps here. We have a plan for that in the lightbox (see #45) but it'll take investigating and possibly upstream work. Should the TODO say to follow the #45 format exactly, so with "21 minutes ago" etc.?

@gnprice
Copy link
Member

gnprice commented Jul 9, 2025

I think the TODO doesn't have to be precise about that; and it can link to this discussion 🙂

@gnprice
Copy link
Member

gnprice commented Jul 9, 2025

I'm planning to include this PR in a v30.0.261 release tomorrow. It'd be great to have it merged first.

chrisbobbe added 10 commits July 8, 2025 22:08
…ting

Thanks Greg for noticing that we were missing this part in the story
of the test:
  zulip#1624 (comment)

We're about to put a MessageContent in the action sheet itself,
for zulip#217, and without this change, we'd get a failure later in the
test when two MessageContents were found when one was expected (one
in the message list and one in the action sheet).
We'd like to make SenderRow public, for use where there isn't a
MessageListPage ancestor and we don't want to show the "unrevealed"
state.
We're about to give _showActionSheet a `Widget? header` param, which
we'll place as a child of this new Column.
For explicitness. All three callers are already passing a "page
context" i.e. the result of PageRoot.contextOf.
See Figma:
  https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3483-29966&m=dev

See also a different header variant in Figma:
  https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3481-26993&m=dev

At first I'd thought this wouldn't be possible without more work
toward issue zulip#488:

- zulip#488 "content: Support Zulip content outside messages (even
  outside per-account contexts)".

But it turns out that our content widgets don't assume a MessageList
ancestor; it's sufficient that we provide a per-account context, via
PerAccountStoreWidget, and a per-message context, via
InheritedMessage (created by MessageContent).

Fixes: zulip#217
@chrisbobbe
Copy link
Collaborator Author

Here's a long name getting truncated:

image

@chrisbobbe chrisbobbe force-pushed the pr-message-content-in-action-sheet branch from 1022fb8 to bf3fe42 Compare July 9, 2025 05:11
@chrisbobbe
Copy link
Collaborator Author

Thanks both! Revision pushed, and @alya see screenshot in my previous comment here.

@gnprice gnprice merged commit bf3fe42 into zulip:main Jul 9, 2025
1 check passed
@gnprice
Copy link
Member

gnprice commented Jul 9, 2025

Thanks! Looks good; merging.

(If we find further design tweaks we want to make, we can always do those as follow-ups.)

@chrisbobbe chrisbobbe deleted the pr-message-content-in-action-sheet branch July 9, 2025 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show message text in bottom sheet
4 participants