-
Notifications
You must be signed in to change notification settings - Fork 310
Show dates in recipient headers, and DM interlocutors' names #283
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! LGTM, with one query below.
if (message.allRecipientIds.length > 1) { | ||
final otherNames = message.allRecipientIds | ||
.where((id) => id != store.account.userId) | ||
.map((id) => store.users[id]?.fullName ?? '(unknown user)') | ||
.sorted() | ||
.join(", "); | ||
title = 'You and $otherNames'; | ||
} else { | ||
title = 'You with yourself'; // TODO pick string; web has glitchy "You and $yourname" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using a filtered message.allRecipientIds
, we could put DmNarrow.ofMessage(message, …)
in a variable (we already do one of those below, although that's in an onTap
callback), and get .otherRecipientIds
from that. If we think it's cleaner and not too much overhead for a build method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. It wouldn't be much if at all shorter that way — the .where
only adds one line — and I think this logic is fairly self-explanatory to read.
Conversely, there would be some distinct extra overhead in the DmNarrow approach. The DmNarrow.ofMessage
constructor would allocate a List to hold all the recipient IDs, then otherRecipientIds
would allocate another, and then the sorted
would allocate a third. In this version, I believe the only allocation is for sorted
.
I think ideally we would have a single DmNarrow object per distinct DM narrow (and similarly perhaps TopicNarrow and StreamNarrow), and an efficient way to look up that single object from a message that's in it. Then those allocations would be done just once for a given narrow; and it'd give us a handy place to go further and memoize the sorted list of names, too (though that one is a bit trickier in that it needs to get invalidated when names change), and/or the string we get from those with join
. At some point when we have a stronger need for it I may try to arrange something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
Thanks for the review! Merging. |
Otherwise it can look fairly glitchy when it functions as a sticky header, overlaying part of the message content or the gutter between its message and the next message.
This size is in *physical* pixels, not logical -- so with the tests' default device pixel ratio of 3, it corresponds to 200w267h in logical pixels, which is a very small screen. (It's a bit over 2 inches (53mm) diagonal when converted at 160px/in, which is the nominal value for a logical pixel.) Better to use the default size, which is somewhat more realistic. In particular, we have a known issue (zulip#282) that the recipient headers suffer overflow if a stream name is long. This tiny screen causes that issue to appear even when a name is not very long at all; and when we add dates to the recipient headers (zulip#172), it would start happening in the tests as they stand. A handful of tests need adjustments to keep working with the new size, mostly just to fill the message list with more messages.
Fixes: #172