Created attachment 261424 [details]
changed 24 hour time format to 12 hour time format

This is my first patch.
Please review.

Created attachment 261428 [details] [review]
0001-timestamps-don-t-follow-time-format-settings-label


This is my first patch.
Please review.

Not a maintainer, but I wonder why
-            format = "%H:%M";
+            format = "%l:%M %p";
is not marked for translation. Looks like another bug to fix.

Review of attachment 261428 [details] [review]:

Using 12-hour format unconditionally is just as wrong as using 24-hour format unconditionally. We'll have to either use the 'clock-format' setting from org.gnome.desktop.interface, or piggy-back on GnomeWallClock (from gnome-desktop). The former is more work for translators, the latter is possibly more confusing; still, GnomeWallClock covers cases where 12-hour format is requested but the locale does not have AM/PM, which would be a bit tedious to replicate ...

Created attachment 261478 [details] [review]
chatView: Use WallClock for timestamps

For what it's worth, here's what I suggested earlier.

(In reply to comment #5)
> Created an attachment (id=261478) [details] [review]
> chatView: Use WallClock for timestamps
> 
> For what it's worth, here's what I suggested earlier.

I forgot to add a comment here that I let the bug to be made for Javed and didn't "cherry-picked" from gnome-shell for that =(

Sorry Javed and thanks for the work!

(In reply to comment #3)
> Not a maintainer, but I wonder why
> -            format = "%H:%M";
> +            format = "%l:%M %p";
> is not marked for translation. Looks like another bug to fix.

When I did that patch for gnome-shell I was told it doesn't need to be translated since it is something like "14:20 AM". Is it ok? (if not I hve to fix it also for gnome-shell)

Review of attachment 261478 [details] [review]:

lgtm!

::: src/chatView.js
@@ +530,2 @@
         let format;
         // Show only the hour if date is on today

Just a nitpick that I fixed in gnome-shell, not sure it worth to change it. The comment is more understandable if it is said "time" instead of "hour", I am wrong?

Comment on attachment 261478 [details] [review]
chatView: Use WallClock for timestamps

The patch is actually based on a stupid thinko - WallClock always represents the current time, so the approach is complete bogus.
Carlos, fancy doing a patch based on your corresponding gnome-shell one? Or maybe you want to give it another go Javed?

Created attachment 270569 [details] [review]
chatView: Use locale format for timestamps

Until now the timestamps were using 24h format.
Check gsetting clock-format to know when
the user is using 12h format or 24h format and
make the timestamp acordingly.

(In reply to comment #10)
> Created an attachment (id=270569) [details] [review]
> chatView: Use locale format for timestamps
> 
> Until now the timestamps were using 24h format.
> Check gsetting clock-format to know when
> the user is using 12h format or 24h format and
> make the timestamp acordingly.

It's a copy paste from https://git.gnome.org/browse/gnome-shell/commit/?id=58191ea66bc381a5e2c94ff195e1979d8eb29669 except for the date.getYear that it's date.get_year, since in polari we are using Glib.DateTime and in gnome-shell we are using Date class from javascript.

Review of attachment 270569 [details] [review]:

typo: accordingly.

No extra points for ignoring the no-%p-in-LC_TIME case gnome-desktop covers, but that's probably fine - Setting only exposes the setting if the locale supports it.

Some more style nits below, otherwise fine

::: src/chatView.js
@@ +539,3 @@
         let format;
+        let desktopSettings = new Gio.Settings({ schema: 'org.gnome.desktop.interface' });
+        let clockFormat = desktopSettings.get_string(CLOCK_FORMAT_KEY);

Nit: Either use constants or string literals, this mix doesn't make any sense at all.

@@ +549,3 @@
+                }
+                // Show the word "Yesterday" and time if date is on yesterday
+                else if(daysAgo <2){

Mmh, maybe a good opportunity to change to a less peculiar indentation?

(I know this is due to the comments, but they could be a lot shorter if they stopped duplicating the translator comments:

if (daysAgo < 1) { // today
} else if (daysAgo < 2) { // yesterday
} else if (daysAgo < 7) { //this week
} else if (...) { // this year
} else { // oh my ...


Not in the scope of this bug, but eventually I'd like to revisit some of those timestamps. Basically when I connect at 10am, *yesterday* noon is considered *today*, which feels fairly odd. Not that we have a way to figure out "before I went to bed" and "after I got up", but I do wonder if just making the cut at midnight turned out to be more predictable ...

Oh, and one more thing - we are in string announcement period, so a quick mail to gnome-i18n@gnome.org would be appreciated. Also pointing to the identical translations in gnome-shell is a good idea :-)

Created attachment 270978 [details] [review]
chatView: Use locale format for timestamps

Until now the timestamps were using 24h format.
Check gsetting clock-format to know when
the user is using 12h format or 24h format and
make the timestamp accordingly.

(In reply to comment #12)
> Review of attachment 270569 [details] [review]:
> 
> typo: accordingly.
> 
> No extra points for ignoring the no-%p-in-LC_TIME case gnome-desktop covers,
> but that's probably fine - Setting only exposes the setting if the locale
> supports it.
> 
sorry, don't get it =( you mean there's locales that can force to not use AM/PM in 12h format?
> Some more style nits below, otherwise fine
> 
> ::: src/chatView.js
> @@ +539,3 @@
>          let format;
> +        let desktopSettings = new Gio.Settings({ schema:
> 'org.gnome.desktop.interface' });
> +        let clockFormat = desktopSettings.get_string(CLOCK_FORMAT_KEY);
> 
> Nit: Either use constants or string literals, this mix doesn't make any sense
> at all.
> 
rigth, was copy pasted from gnome-shell
> @@ +549,3 @@
> +                }
> +                // Show the word "Yesterday" and time if date is on yesterday
> +                else if(daysAgo <2){
> 
> Mmh, maybe a good opportunity to change to a less peculiar indentation?
> 
> (I know this is due to the comments, but they could be a lot shorter if they
> stopped duplicating the translator comments:
> 
> if (daysAgo < 1) { // today
> } else if (daysAgo < 2) { // yesterday
> } else if (daysAgo < 7) { //this week
> } else if (...) { // this year
> } else { // oh my ...
> 
> 
I also fixed some previous-patch style nit like a space between ){
> Not in the scope of this bug, but eventually I'd like to revisit some of those
> timestamps. Basically when I connect at 10am, *yesterday* noon is considered
> *today*, which feels fairly odd. Not that we have a way to figure out "before I
> went to bed" and "after I got up", but I do wonder if just making the cut at
> midnight turned out to be more predictable ...
rigth

(In reply to comment #13)
> Oh, and one more thing - we are in string announcement period, so a quick mail
> to gnome-i18n@gnome.org would be appreciated. Also pointing to the identical
> translations in gnome-shell is a good idea :-)

Before pushing and I need acceptance like when I send to release-team?

(In reply to comment #15)
> (In reply to comment #12)
> > No extra points for ignoring the no-%p-in-LC_TIME case gnome-desktop covers,
> > but that's probably fine - Setting only exposes the setting if the locale
> > supports it.
> > 
> sorry, don't get it =( you mean there's locales that can force to not use AM/PM
> in 12h format?

There are locales that don't have the concept of AM/PM - to pick a "random" example, how would you translate "AM" into Spanish? Both gnome-desktop and gnome-control-center take this into account by ignoring/hiding the setting in those cases - just compare the UI of running
  LC_TIME=es_ES.utf8 gnome-control-center datetime
to the one when running
  LC_TIME=en_US.utf8 gnome-control-center datetime

But as mentioned, I doubt this will become a common issue (e.g. requires manual fiddling with gsettings), and if it somehow does, it can easily be addressed then.


(In reply to comment #16)
> Before pushing and I need acceptance like when I send to release-team?

No, string announcement means just that - you just notify translators of a string change without having to wait for a reply/approval. The latter will be required in string freeze, which uhm ... has started this week (though the corresponding tarball is still pending, so I'd say it's ok to ignore)

Comment on attachment 270978 [details] [review]
chatView: Use locale format for timestamps

LGTM

Attachment 270978 [details] pushed as e1516ea - chatView: Use locale format for timestamps