Recipient header date format

[sirpengi]

This PR is built on top of #426

Fixes #411

[gnprice]

Thanks @sirpengi! Generally this looks good. Comments below, mainly on the tests.

[gnprice]

Technically these terms are "deictic temporal pronouns" but
that term seemed too academic, so decided on `temporalNoun`
for labeling them in translations.

😄 I agree.

How about just today and yesterday, though? I'm not sure "temporal noun" adds any specificity here — if there is some language where there are two different ways one might translate "today", I'm not sure that saying "temporal noun" (or even "deictic temporal pronoun") would help a translator in choosing between them. Partly because it may be unclear what the phrase means; but also because since I don't have a concrete idea of what that distinction would be, I'm not sure it'd necessarily be the case that the desired translation was a deictic temporal pronoun and the less apt translation wasn't.

(For example, perhaps in some language you'd translate "today" differently in "Today is a good day." vs. "Today I sent this message." Probably the former would be that deictic temporal pronoun, while the latter might be an adverb that looks different from the pronoun. But the right translation in this UI would likely correspond to the latter — so that it'd be the one that wasn't a noun or a pronoun.)

[gnprice]

nit: generally omit arguments that are the same as the default:

Suggested change

	DateTime.fromMillisecondsSinceEpoch(message.timestamp * 1000,
	isUtc: false),
	DateTime.fromMillisecondsSinceEpoch(message.timestamp * 1000),

There's even a lint rule for this in the Dart linter:
https://dart.dev/tools/linter-rules/avoid_redundant_argument_values
which Flutter upstream uses. Possibly we should turn it on.

Aside from making the code a bit longer, the redundant argument value makes it look like that's probably not the default, which can confuse people about how the underlying API works.

The main exception, where I sometimes like to pass an argument even though it matches the default, is when calling a test helper like eg.user. The defaults there are arbitrary, designed for tests where the values are beside the point of the test, so it's good for tests to be explicit when the value is part of the point. But for just about any non-test API (and really the same would be true of a test API if it were used by a much larger codebase), the defaults are always as much a part of the API as anything else — plenty of code does depend on them, so changing them would be a breaking change — so we should just take them as part of the meaning of the function we're calling.

[gnprice]

Relatedly: because this call was there without isUtc: false and this commit adds it, it makes it look like that's part of what's changing — like it was UTC before and now isn't. (This is a version of "can confuse people about how the underlying API works".)

So even if we did want to make this change, it should be a separate commit, whose commit message can then clarify that it's NFC.

[sirpengi]

I was overgeneralizing the exception we have for tests, as here we need to ensure the times being passed in are both in local-time. I thought to be explicit here about that but I see the point about the defaults being part of the api. I'll remove the redundant default but add an assert containing a note to enforce this.

[gnprice]

This looks wrong — the logic we have in web will show dates in the future with an explicit year. (Because e.g. if it's Dec 1 and you see a message labeled "Dec 2", you might naturally assume it was from Dec 2 of last year; if it's actually from tomorrow because some clock is wrong, we should be explicit about that.) Was there a reason for the change?

… Oh I see. This actually matches the logic we currently have in web. What happened is that we accidentally altered that behavior in a commit that was described as just replacing a dependency, zulip/zulip@9896782 . This condition had matched the comment, but no longer does:

-    } else if (is_older_year) {
+    } else if (time.getFullYear() !== today.getFullYear()) {
         // For long running servers, searching backlog can get ambiguous
         // without a year stamp. Only show year if message is from an older year

We do correctly handle future times in format_time_modern, which is used in the "Recent conversations" view (and which does have a test for this case):

    if (time > today) {
        /* For timestamps in the future, we always show the year*/
        return get_localized_date_or_time_for_format(time, "dayofyear_year");
    } else if (hours < 24) {
        return stringify_time(time);
    } else if (days_old === 1) {
        return $t({defaultMessage: "Yesterday"});
    // …

Anyway. Let's handle this correctly in our code, matching how web used to work and how it does work on a different screen. And then fixing web's recipient headers for this case can be a separate issue.

[gnprice]

… Oh I see. This actually matches the logic we currently have in web. What happened is that we accidentally altered that behavior in a commit that was described as just replacing a dependency, zulip/zulip@9896782 . This condition had matched the comment, but no longer does:

-    } else if (is_older_year) {
+    } else if (time.getFullYear() !== today.getFullYear()) {
         // For long running servers, searching backlog can get ambiguous
         // without a year stamp. Only show year if message is from an older year

FYI @andersk as the author of that change. Though the root issue here is that we apparently never had a test for that case.

[sirpengi]

I dug into this a bit this morning. It looks like format_time_modern actually hasn't been used since it was reverted back in March 2022 in zulip/zulip@7bc0e70 . Running git grep format_time_modern in main shows it is only referenced in tests. See https://chat.zulip.org/#narrow/stream/137-feedback/topic/recent.20topics.20timestamps/near/1337670 for background.

Web is currently using relative_time_string_from_date for this label (the date for last message time in recent topics).

export function relative_time_string_from_date({
    date,
    current_date = new Date(),
}: {
    date: Date;
    current_date?: Date; 
}): string {
    const minutes = differenceInMinutes(current_date, date);
    if (minutes <= 2) {
        return $t({defaultMessage: "Just now"});
    }

and sadly it looks like this function will always return Just now for any times from the future. I can also confirm by changing my computer clock to the previous day and every time in my inbox shows Just now.

I think showing times in the future is weird. Even for format_time_modern it shows the date label if the future time even a minute ahead, despite being on the same day (so it could be Dec 1 and the label will say Dec 1, 2023 where I think it should continue to say Today). I think it's quite likely for devices to drift a few minutes and hit this condition.

Open to further discussion, but for now I'm making a change so that for future dates, dates that are today still show up as Today but dates further off will have the year attached.

[gnprice]

t looks like format_time_modern actually hasn't been used since it was reverted back in March 2022 in zulip/zulip@7bc0e70 .

Hmm, yeah, I remember that conversation now. Ah well.

and sadly it looks like this function will always return Just now for any times from the future.

Wow, yeah, that seems wrong.

for future dates, dates that are today still show up as Today but dates further off will have the year attached.

Sure. Given that we're only showing to calendar-day granularity anyway, that's the behavior I'd expect here.

[gnprice]

Instead of these dynamic casts, better to get testCases the appropriate type in the first place. The elements are naturally pairs, not lists:

Suggested change

	[DateTime(2023, 1, 11, 12), "Jan 11"],
	];
	for (final [dateTime as DateTime, expected as String] in testCases) {
	(DateTime(2023, 1, 11, 12), "Jan 11"),
	];
	for (final (dateTime, expected) in testCases) {

[gnprice]

redundant toString — interpolation does that itself (that's why e.g. you can interpolate a number):

Suggested change

	test('${dateTime.toString()} returns $expected', () {
	test('$dateTime returns $expected', () {

[gnprice]

These times can be made easier to read by writing them as strings:

Suggested change

	[DateTime(2023, 1, 10, 12), zulipLocalizations.temporalNounToday],
	["2023-01-10 12:00", zulipLocalizations.temporalNounToday],

and then using DateTime.parse.

[gnprice]

Can avoid the ? by making these always have the year. It's OK to assume the clock when running the tests is in the future relative to the time you're writing the code, so that just means putting them both in 2022 or earlier.

The first date is already good (and to see how I picked it, look at git log --reverse | head); you can make up a new value for the other one.

[gnprice]

I believe I've heard of a locale where this wouldn't work — IIRC in parts of Ethiopia, the days run from midnight to midnight but the hours are counted from noon to noon.

But I'm not certain how to write correct logic for that: partly because I'm not sure I remember the details right, partly because I'm not sure how other software handles it, including DateTime.copyWith, and partly because it's just complicated to think about. So it wouldn't make sense to try to handle that case without writing tests for it. Since we don't have a reasonable way to control the timezone (as commented on that existing test below), I think it's best to just ignore that case at present.

[sirpengi]

@gnprice this is ready for another round!

[gnprice]

Suggested change

	final now = DateTime(2023, 1, 10, 12);
	final testCases = [
	("2023-01-10 12:00", zulipLocalizations.today),
	final now = DateTime.parse("2023-01-10 12:00");
	final testCases = [
	("2023-01-10 12:00", zulipLocalizations.today),

[gnprice]

Suggested change

	assert(!dateTime.isUtc && !dateTime.isUtc,
	assert(!dateTime.isUtc && !now.isUtc,

:-)

[gnprice]

This worried me for a bit — what if local time happens to be UTC?

But it looks like it's fine: isUtc is true only if the explicit isUtc was set to true at construction. Here's a test program:

void main() {
  final t1 = DateTime.fromMillisecondsSinceEpoch(1701739446000);
  final t2 = DateTime.fromMillisecondsSinceEpoch(1701739446000, isUtc: true);
  print("$t1 timeZoneName: ${t1.timeZoneName} isUtc: ${t1.isUtc}");
  print("$t2 timeZoneName: ${t2.timeZoneName} isUtc: ${t2.isUtc}");
}

And here's output:

$ TZ=UTC dart /tmp/time.dart 
2023-12-05 01:24:06.000 timeZoneName: UTC isUtc: false
2023-12-05 01:24:06.000Z timeZoneName: UTC isUtc: true

[gnprice]

nit:

Suggested change

	'`dateTime` and `now` need to be in local time.');
	if (dateTime.year == now.year &&
	'`dateTime` and `now` need to be in local time.');
	if (dateTime.year == now.year &&

The assert is less closely related to the same-day conditional than the latter is to what's after it. So since there's a blank line separating afterward, there should be a blank line separating before.

[gnprice]

t looks like format_time_modern actually hasn't been used since it was reverted back in March 2022 in zulip/zulip@7bc0e70 .

Hmm, yeah, I remember that conversation now. Ah well.

and sadly it looks like this function will always return Just now for any times from the future.

Wow, yeah, that seems wrong.

for future dates, dates that are today still show up as Today but dates further off will have the year attached.

Sure. Given that we're only showing to calendar-day granularity anyway, that's the behavior I'd expect here.

[gnprice]

Thanks @sirpengi for the revision!

A few small comments above. In order to get this into a v0.0.7 today, and since it's already nighttime for you, I'll go ahead and fix those and merge.