GNOME Bugzilla – Bug 699196
ugly date/time format in commit list
Last modified: 2014-07-10 12:31:26 UTC
In the list of commits, gitg displays most dates/times like this: 04/15/2013 10:44:15 PM +0200 In my opinion this is ugly. For commits more than a day old, I think gitg should display dates/times like Nautilus does: Apr 15 or possibly include both the date and time: Apr 15 10:44 PM If the user wants to know more, they can always click on the commit to see the detailed date/time information.
Created attachment 242815 [details] [review] Fix date format in commit history view. The time format now reads: Shortmonth Date, Hours:Minutes AM/PM Timezone (offset by UTC) Example: Apr 15 10:44 +200 If you are not happy with display of timezone information, then we have the option of displaying timezone only when timestamps have different timezone information. If timestamps have same timezone information in the history view, we do not display the timezone.
Review of attachment 242815 [details] [review]: Sindhu, thanks for the patch! 1. GNOME has a setting that lets the user choose whether to display 12-hour time (e.g. "1:34 PM") or 24-hour time ("13:34"). This is stored in GSettings at /org/gnome/desktop/interface/clock-format. We need to respect that. 2. We should format in a way that's sensitive to the user's locale if possible. I'm not sure that %b %d will work in all locales - there might be places where the format is "15 Apr" rather than "Apr 15", for example. I think you should look at the Nautilus code to see how it does its formatting. If it's easily possible to use that same code path in gitg, that would be great. Otherwise we should probably just do what Nautilus does. 3. I personally think it's overkill to display the time zone here - if the user want to see it they can always click on the commit and look at the detailed information below. Looking forward to your next patch iteration!
Hi, Adam! Thank you for your reviews, you are have a sharp eye for detail! Could you please elaborate on these for me? 1. How can I incorporate GSettings clock-format into Gitg? 2. It would be great if you can link me to the specific Nautilus code you are referring to. Am cloning nautilus right now and will look for it myself in the meantime. 3. OK, agreed on this point. I'll work on my next patch now :) Thank you so much!
Sindhu, the code that formats dates/times in Nautilus is in nautilus_file_get_date_as_string() in nautilus-file.c. You can see that it queries the clock-format setting and uses that to decide how to display the date. But I'd be reluctant to copy and paste that code into gitg - copying and pasting code is almost always a bad idea. Note that there's already an outstanding bug (bug 698363) to unify the date formats in Nautilus and GtkFileChooser. I think the right way to solve this is probably to move this date formatting code into GLib and call it from GTK, Nautilus, and gitg. That will involve coordination between a few different teams and shuffling code in C, so if you're still a beginning C or GNOME programmer I'd suggest picking something else to work on, honestly - there are many easier bugs here.
Created attachment 242836 [details] [review] Fix date format to show only Time in commit history view. I understand but till the most elegant solution is in place, I think we can come up with an OK solution for DateTime formatting in Gitg that would be optimal if not perfect :) In your previous comment, you said: "We should format in a way that's sensitive to the user's locale if possible. I'm not sure that %b %d will work in all locales - there might be places where the format is "15 Apr" rather than "Apr 15", for example" What do you mean by "there might be places where the format is…"? What other places/scenarios or settings exist which could affect how Gitg formats time? I have attached a new patch with only a minor variation. Single digit dates are now displayed without a preceding zero, making it more close to human readable time. Example: April 1 (and not April 01).
When I said "places", I meant geographic places such as countries. Each country has different conventions for displaying dates and times, and these may differ even among countries that speak the same language. So I meant that even though in the US it's conventional to write "Apr 15", there may be other English-speaking countries where the convention is "15 Apr". If someone runs gitg in one of those places, the environment variables LANG and/or LC_TIME will indicate that, and in response we should display "15 Apr". The way Nautilus solves this problem is through translation. In the file nautilus-file.c I mentioned before there are a bunch of translatable strings such as TODAY_TIME_FORMAT. Translators are expected to translate those into strings which are appropriate for each language. I'm not convinced that even Nautilus has this completely right, since from glancing at that it code seems the time format might be determined only by LANG and not react correctly to LC_TIME. I'll leave it to the primary gitg developers (I am not one) to decide whether to accept a completely locale-insensitive patch as an incremental step as you suggest.
Sinhu, I agree here with Adam, although it is also true that right now we do not really support translation in gitg correctly due to all our libs magic. I couldn't get it right last time that I tried to make gitg translatable. So let's put a FIXME comment pointing to this bug for when we have this ready and do as Adam say.
So, these are the steps from my side? Please confirm. 1. Edit the code and add a comment that says "FIXME: Bug 699196". 2. Which of the above patches is OK to push to master (with addition of comment)? Or 3 Should I discard them all and add the comment mentioned above?
Let's push it anyway but with the comment pointing to the bug.
You mean no patch above must be used and only a comment must be added and pushed? Sorry, I didn't understand so I am asking for confirmation to push to master.
It should be pointed out that by pushing this change, we're going from a format which is localized but ugly to a prettier format which is correct only for some locales (e.g. the US). I live in the US and don't mind seeing this format so I won't complain much; others might care more.
Created attachment 243089 [details] [review] Fix date format to show only Time in commit history view and add comment on editing code to make Gitg honour user locales. We went from using the format %x %X %z that respects user locale to using %h %e, %I:%M %p in this patch. This patch also includes a comment on editing code to make Gitg more translatable and honour user set locales in the future with a link to this bug that can be closed once code complies with requirements discussed. Please review and confirm if the patch can be pushed to master. Thanks.
Review of attachment 243089 [details] [review]: Sindhu, The first line of your commit message, which summarizes the commit, says "Add comment on fixing code to make Gitg honour user locales". But the commit does much more than this: it actually changes the format used for displaying times/dates. The change in behavior is more important than the comment, and needs to be mentioned in the first line.
Created attachment 243092 [details] [review] Fix date format to show only Time in commit history view and add comment on editing code to make Gitg honour user locales. Fixed commit message. Please review, again :) Thanks.
Review of attachment 243092 [details] [review]: Sindhu, note that in your commit message there should be a blank line between the summary line and the longer explanation. Please read these pages for background: https://live.gnome.org/Git/CommitMessages http://git-scm.com/book/en/Distributed-Git-Contributing-to-a-Project (section "Commit Guidelines") Also, it's best if you link to this bug in your commit message. I think the best way to do that is to include the full URL of this bug after a blank line at the end of your message. One more iteration, please! :)
Created attachment 243093 [details] [review] Fix date format to show only Time in commit history view and add comment on editing code to make Gitg honour user locales. Thank you for being so patient with me, Adam! It's good to learn this way :) > note that in your commit message there should be a blank line between the > summary line and the longer explanation. Please read these pages for > background: So far as I can see, I *have* given a blank line after the commit summary (shown as 'Subject' by Bugzilla. Is there anywhere else I need to give a blank line? > Also, it's best if you link to this bug in your commit message. I think the > best way to do that is to include the full URL of this bug after a blank line > at the end of your message. > > One more iteration, please! :) Attached patch, please review. :) Thanks.
Review of attachment 243093 [details] [review]: First, my apologies - yes, you did have a blank line after the commit summary. Sorry I missed that. I've realized that there's a more fundamental problem with this patch that I should have noticed on previous reviews. With your patch, gitg will show dates with a month and day but no year, even for commits from previous years. We definitely want to use this abbreviated format only for commits from this calendar year.
> With your patch, gitg will show dates > with a month and day but no year, even for commits from previous years. Yes, it was initially suggested to bring the dates to the format: Apr 15 10:44 PM only, which lead to year being overlooked. > We definitely want to use this abbreviated format only for commits from this > calendar year. I have a few questions as your comment is not clear: 1. You want year information to appear for those commits that are *not* in the current calendar year right? As it doesn't make sense to have year information for commits that *are* in the current calendar year. 2. How would like the year information to be displayed? %y (without century) or %Y (with century)? I am leaning towards %Y as it's more accurate. Please confirm. Thanks.
(In reply to comment #18) > > With your patch, gitg will show dates > > with a month and day but no year, even for commits from previous years. > > Yes, it was initially suggested to bring the dates to the format: Apr 15 10:44 > PM only, which lead to year being overlooked. > > > We definitely want to use this abbreviated format only for commits from this > > calendar year. > > I have a few questions as your comment is not clear: > > 1. You want year information to appear for those commits that are *not* in the > current calendar year right? As it doesn't make sense to have year information > for commits that *are* in the current calendar year. Right. > 2. How would like the year information to be displayed? %y (without century) or > %Y (with century)? I am leaning towards %Y as it's more accurate. Certainly with the century. It's true that's more accurate, and also we want to be consistent with Nautilus, which displays a full year with century.
Created attachment 243148 [details] [review] Fix date format in commit history view. Patch now compares year in committer's DateTime and Gitg user's system DateTime. If commits fall within current calendar year, then does *not* display any year information. If commits falls outside of the Gitg user's system's current calendar year, then year information is displayed. Comment about fixing code for honouring user set system locales from previous patches, retained. Please review. Thanks.
Review of attachment 243148 [details] [review]: Sindhu, thanks for your latest patch. 1. Your patch displays dates/times in the format "Dec 22, 11:47 AM 2012". I think it looks strange to have a time in between two parts of a date. A format such as "Dec 22 2012, 11:47 AM" would be better. 2. In general we'd like to be consistent with Nautilus. Nautilus displays "am"/"pm", not "AM"/"PM", so let's use lowercase. 3. You're testing whether years match by comparing strings. You should just compare integers instead, which is easier and faster. 4. You've added comments in the code describing how it has changed ("Changed time formatting from previous localized format...") In general, the git log is the best place for that. It's better if code comments talk about how things are now and/or how they could change in the future, e.g. "TODO: localize this date format".
Created attachment 243166 [details] [review] Fix date format in commit history view. Made changes as suggested in previous review. In addition, commits as old as an hour now read 'An hour ago' and commits older than that, read 'N hours ago'. For bug: http://bugzilla.gnome.org/show_bug.cgi?id=699514 Please review, thank you :)
Created attachment 243167 [details] [review] Fix date format in commit history view. In case you are not OK with previous patch resolving another bug, this patch has all changes related to *this* bug only. Thanks.
Review of attachment 243167 [details] [review]: Sindhu, it's best to use one patch per bug, so I'm reviewing the second patch you attached. Please attach a separate patch for the "1 hours" bug to bug 699514 - thanks. This change now looks reasonable to me, but please don't push it unless you get an OK from one of the gitg developers (Ignacio or Jesse). Thanks!
Comment on attachment 243167 [details] [review] Fix date format in commit history view. looks good to commit
Thank you. Fixed in commit 13795da99481dab060fe30654b83ed3567866cd8 https://git.gnome.org/browse/gitg/commit/?id=13795da99481dab060fe30654b83ed3567866cd8 Closing bug now, thanks.