-
Notifications
You must be signed in to change notification settings - Fork 320
Follow user's time format setting (12 or 24 hour) #1730
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
base: main
Are you sure you want to change the base?
Conversation
Conveniently for making screenshots, I could fit three things on the screen at once:
|
Before | After |
---|---|
![]() |
![]() |
![]() |
![]() |
user_settings.twenty_four_hour_time: true
Before | After |
---|---|
![]() |
![]() |
![]() |
![]() |
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.
Thanks @chrisbobbe! This works great, and LGTM.
Thanks for the review! |
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.
Thanks for building this! Comments below.
final messageListTheme = MessageListTheme.of(context); | ||
final zulipLocalizations = ZulipLocalizations.of(context); | ||
final formattedTimestamp = MessageTimestampStyle.dateOnlyRelative.format( | ||
timestamp, | ||
now: ZulipBinding.instance.utcNow().toLocal(), |
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.
Using the binding is a good idea. How about having the callee invoke it? Then it doesn't need to be a parameter that each caller passes.
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.
(Potentially that happens after the other commits — it might be simpler to do that change after this one:
26d5ef5 msglist [nfc]: Finish centralizing on MessageTimestampStyle
than vice versa.)
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.
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.
Hmm, refreshing relative times is an interesting point. I guess let's leave it this way for now, and we'll see what we come up with to organize the code when we go to handle that.
final now = DateTime.parse("2023-01-10 12:00"); | ||
final testCases = [ | ||
("2023-01-10 12:00", zulipLocalizations.today), | ||
("2023-01-10 00:00", zulipLocalizations.today), | ||
("2023-01-10 23:59", zulipLocalizations.today), | ||
("2023-01-09 23:59", zulipLocalizations.yesterday), | ||
("2023-01-09 00:00", zulipLocalizations.yesterday), | ||
("2023-01-08 00:00", "Jan 8"), | ||
("2022-12-31 00:00", "Dec 31, 2022"), | ||
// Future times | ||
("2023-01-10 19:00", zulipLocalizations.today), | ||
("2023-01-11 00:00", "Jan 11, 2023"), |
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.
For these test cases of the "relative date" logic, it's quite useful that the different times are all right next to each other and next to the value of "now". When you look at "now" and then read through the different cases, they tell a legible story about how the intended logic works and what edge cases are successfully covered.
test/widgets/message_list_test.dart
Outdated
doTests("2023-01-10 23:59", (style, expectTwelveHour) => switch (style) { | ||
MessageTimestampStyle.none => null, | ||
MessageTimestampStyle.dateOnlyRelative => zulipLocalizations.today, | ||
MessageTimestampStyle.timeOnly => | ||
expectTwelveHour ? '11:59 PM' : '23:59', | ||
MessageTimestampStyle.lightbox => | ||
expectTwelveHour ? 'Jan 10, 2023 11:59:00 PM' : 'Jan 10, 2023 23:59:00', | ||
MessageTimestampStyle.full => | ||
expectTwelveHour ? 'Jan 10, 2023 11:59 PM' : 'Jan 10, 2023 23:59', |
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.
Meanwhile, all these non-relative formats don't really benefit from most of the different test values that are there for the relative-date format. After seeing how timeOnly
comes out on one timestamp that's at 11:59pm, there's nothing new learned from seeing another such timestamp.
test/widgets/message_list_test.dart
Outdated
withClock(Clock.fixed(DateTime.parse("2023-01-10 12:00")), () { | ||
// TODO take the message timestamp as an int, like [Message.timestamp]? | ||
final timestamp = DateTime.parse(messageTimestampStr).millisecondsSinceEpoch ~/ 1000; |
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.
No, it's useful that the test source code has it as a readable date/time string. That's what makes it possible for a human reading the tests to judge whether a given expected answer is right, and also to think about what edge cases there might be and then look to see if they're covered by the given examples.
lib/api/model/initial_snapshot.dart
Outdated
@JsonKey( | ||
readValue: JsonNullable.readFromJsonAssertingPresent<bool>, | ||
fromJson: TwentyFourHourTimeMode.fromApiValue, | ||
toJson: TwentyFourHourTimeMode.staticToJson, | ||
) | ||
TwentyFourHourTimeMode twentyFourHourTime; |
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.
How about without the readValue?
That'd be simpler in the code. I believe the only difference in behavior is that if a server didn't send the value at all, this would treat it the same as null, whereas the PR's current version would treat it as malformed.
I don't think we gain anything by rejecting a case where the server doesn't send the value. In general across our API binding we treat that the same as sending null; the only exceptions are in a small number of places where the API's semantics actually give different meanings to absent and null. (In fact it looks like just one so far: deliveryEmail
on RealmUserUpdateEvent
.) But it's not doing that here; if it were, "asserting present" would be wrong.
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.
(And then can we also eliminate the other JsonKey arguments? What if we rename or alias TwentyFourHourTimeMode.fromApiValue
as fromJson
?)
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.
(And then can we also eliminate the other JsonKey arguments? What if we rename or alias
TwentyFourHourTimeMode.fromApiValue
asfromJson
?)
I think when the .g.dart files are generated, an enum's fromJson
or toJson
methods (when present) are never actually used, unless you ask for it with those JsonKey arguments.
I'd normally just do @JsonEnum(valueField: "apiValue")
, but when I do that, dart run build_runner build
gives an error:
Failed to build with build_runner in 18s; wrote 381 outputs.
log output for json_serializable on lib/api/model/initial_snapshot.dart
E `JsonEnum.valueField` was set to "apiValue", but that field does not have a type of
String, int, or null.
package:zulip/api/model/model.dart:303:6
╷
303 │ enum TwentyFourHourTimeMode {
│ ^^^^^^^^^^^^^^^^^^^^^^
╵
(The apiValue
field is typed bool?
.)
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.
Hmm, I see. OK, then.
test/widgets/content_test.dart
Outdated
@@ -139,9 +140,16 @@ void main() { | |||
Future<void> prepareContent(WidgetTester tester, Widget child, { | |||
List<NavigatorObserver> navObservers = const [], | |||
bool wrapWithPerAccountStoreWidget = false, | |||
TwentyFourHourTimeMode? twentyFourHourTimeMode |
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.
nit:
TwentyFourHourTimeMode? twentyFourHourTimeMode | |
TwentyFourHourTimeMode? twentyFourHourTimeMode, |
test/widgets/content_test.dart
Outdated
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); | ||
final initialSnapshot = eg.initialSnapshot(); | ||
if (twentyFourHourTimeMode != null) { | ||
initialSnapshot.userSettings.twentyFourHourTime = twentyFourHourTimeMode; | ||
} | ||
await testBinding.globalStore.add(eg.selfAccount, initialSnapshot); |
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.
How about taking an optional initialSnapshot
parameter instead?
That way it's up to the caller to deal with whatever details it wants to add, and this shared prep function doesn't need to grow for each feature that only a small number of test cases will interact with.
test/widgets/content_test.dart
Outdated
tester.widget(find.textContaining(renderedTextRegexp)); | ||
|
||
// TODO(#1727) test with different locales | ||
tester.widget(find.textContaining(renderedTextRegexpTwelveHour)); |
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.
tester.widget(find.textContaining(renderedTextRegexp)); | |
// TODO(#1727) test with different locales | |
tester.widget(find.textContaining(renderedTextRegexpTwelveHour)); | |
// TODO(#1727) test with different locales | |
tester.widget(find.textContaining(renderedTextRegexpTwelveHour)); |
This is equivalent, right?
test/widgets/content_test.dart
Outdated
twentyFourHourTimeMode: TwentyFourHourTimeMode.localeDefault, | ||
plainContent('<p>$timeSpanHtml</p>')); | ||
tester.widget(find.textContaining(renderedTextRegexp)); | ||
|
||
// TODO(#1727) test with different locales | ||
check(find.textContaining(renderedTextRegexpTwelveHour)).findsOne(); |
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.
It'd be good to have a short comment saying why this expectation is right — it looks counterintuitive.
test/widgets/content_test.dart
Outdated
testWidgets('smoke', (tester) async { | ||
await prepareContent(tester, plainContent('<p>$timeSpanHtml</p>')); | ||
await prepareContent(tester, | ||
wrapWithPerAccountStoreWidget: false, |
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.
This test case is now doing something very unusual (indeed something that doesn't yet happen in the live app) — so it should definitely get a more specific name than this :-)
Thanks Komyyy for this idea, which I took from PR zulip#1363. Co-authored-by: Komyyy <[email protected]>
Servers can't yet start sending null without breaking clients. Releasing this will start lowering the number of client installs that would break, and eventually there will be few enough that the breakage is acceptable; see discussion (same link as in comment): https://chat.zulip.org/#narrow/channel/378-api-design/topic/.60user_settings.2Etwenty_four_hour_time.60/near/2220696
See the doc: https://pub.dev/documentation/intl/latest/intl/DateFormat-class.html > Formatting dates in the default 'en_US' format does not require > any initialization. (And we haven't been doing the described initialization for 'en_US' or any other locale; it's asynchronous, and we have a better plan for international formatting described in zulip#45, using ffi.)
We don't use these yet; coming up.
…tamps Fixes-partly: zulip#1015
f5c7a54
to
5152d41
Compare
Thanks for the review! Revision pushed. The last commit isn't needed for this feature— 5152d41 json: Implement JsonNullable.toJson and readFromJsonAssertingPresent —but I left it in so we have the option to merge it in case it might be helpful in the future. |
Thanks for the revision! Let's skip that last commit — I think for the same reasons as at #1730 (comment) , "readFromJsonAssertingPresent" doesn't really make sense for the situations that JsonNullable is for. Other than that, all looks good to me; please go ahead and merge. |
5152d41
to
0755c52
Compare
Mm, |
Fixes #1015.
cc @alya for screenshots below.