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

app: change date/time format in log widget #800

Merged
merged 1 commit into from Apr 14, 2018

Conversation

Projects
None yet
3 participants
@alex-gulyas
Contributor

alex-gulyas commented Apr 12, 2018

The previous format displayed a weird date/time when the active locale
didn't have AM/PM (%p): 10:45 57s 2018-04-12.

app: change date/time format in log widget
The previous format displayed a weird date/time when the active locale
didn't have AM/PM (%p): `10:45  57s 2018-04-12`.

Signed-off-by: Alex Gulyás <gulyas.alex@gmail.com>
@davvid

I dunno, this changes the format string for folks that do have AM/PM, so I can't merge this as-is.

Maybe we can use a strategy similar to what they do here in the last answer:

https://stackoverflow.com/questions/41864935/how-can-i-check-if-the-current-locale-uses-am-pm-or-24-hour-time

Test strftime('%p') before using the alternate format string.

That said, I don't really see what's so weird about the format string you pasted. The logic behind it is that we want to show the most volatile parts first -- the time and seconds. The date is less volatile, so there's no sense in showing it first.

This new format string loses that property. Maybe a simple compromise would be to reverse it to '%X %x: ' instead?

@alex-gulyas

This comment has been minimized.

Contributor

alex-gulyas commented Apr 13, 2018

I dunno, this changes the format string for folks that do have AM/PM, so I can't merge this as-is.

This is not correct, if your locale uses AM/PM then so will this format:

  • LC_TIME=en_US.UTF-8: 04/13/2018 04:15:05 PM
  • LC_TIME=hu_HU.UTF-8: 2018-04-13 16:18:01

That said, I don't really see what's so weird about the format string you pasted.

By weird I actually meant this (I should've explained it more in the PR, sorry):

  • Because %p is empty for me, there will be two spaces between the minutes and seconds (markdown ate the 2nd space in my PR message)
  • Even though %p is empty, the time is still showed as 12 hour based, so 16:20 is confusingly printed as 04:20

Sample with hu_HU locale at 16:20:38

04:20  38s 2018-04-13
     ^^

The logic behind it is that we want to show the most volatile parts first -- the time and seconds. The date is less volatile, so there's no sense in showing it first.

Actually I didn't mean to swap the order, I did it instinctively. I can live with the reverse order, but I have to say, I cannot remember ever seeing the time before the date in any kind of log messages.

@davvid

This comment has been minimized.

Member

davvid commented Apr 14, 2018

Okay, you've convinced me. It's true that the format string we're using is peculiar from the global perspective. Thanks!

davvid added a commit to davvid/git-cola that referenced this pull request Apr 14, 2018

Merge pull request git-cola#800 from alex-gulyas/master
* alex-gulyas/master:
  app: change date/time format in log widget

Signed-off-by: David Aguilar <davvid@gmail.com>

@davvid davvid merged commit 7c73ec2 into git-cola:master Apr 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kakra

This comment has been minimized.

Contributor

kakra commented Apr 14, 2018

The format string could be passed through I18n/gettext... That way, it's possible to show the most volatile first correctly in every locale...

davvid added a commit to davvid/git-cola that referenced this pull request Apr 15, 2018

Revert "app: change date/time format in log widget"
This reverts commit 7c73ec2.

Related-to: git-cola#800
Suggested-by: Kai Krakow <kai@kaishome.de>
Signed-off-by: David Aguilar <davvid@gmail.com>

davvid added a commit to davvid/git-cola that referenced this pull request Apr 15, 2018

log: pass the format string through gettext
Related-to: git-cola#800
Suggested-by: Kai Krakow <kai@kaishome.de>
Signed-off-by: David Aguilar <davvid@gmail.com>

davvid added a commit to davvid/git-cola that referenced this pull request Apr 15, 2018

i18n: use a better format string for Hungarian locales
Related-to: git-cola#800
Suggested-by: Kai Krakow <kai@kaishome.de>
Signed-off-by: David Aguilar <davvid@gmail.com>
@alex-gulyas

This comment has been minimized.

Contributor

alex-gulyas commented Apr 15, 2018

I don't think passing the time format through gettext is the correct thing to do, because then the time format will depend on the current language, and not the current locale. Locale and language settings are not the same. %x and %X are formats which correctly use the current locales date and time format, which for example might be different for US and UK locales, event though they both use English.

Locale settings control how numbers, dates, and times display for your region – which may be a country, or a portion of country or may not even honor country boundaries. They are independent of your language. [...] A language, on the other hand, is what we speak, read, and write. Language settings control in what language text appears independently of the locale settings.

-- The Differences between Locales and Languages (blogs.msdn.microsoft.com)

davvid added a commit to davvid/git-cola that referenced this pull request Apr 17, 2018

Revert "i18n: use a better format string for Hungarian locales"
This reverts commit 78376a5.

Related-to: git-cola#800
Suggested-by: Alex Gulyás <gulyas.alex@gmail.com>
Signed-off-by: David Aguilar <davvid@gmail.com>

davvid added a commit to davvid/git-cola that referenced this pull request Apr 17, 2018

Revert "log: pass the format string through gettext"
This reverts commit efa7a99.

Related-to: git-cola#800
Suggested-by: Alex Gulyás <gulyas.alex@gmail.com>
Signed-off-by: David Aguilar <davvid@gmail.com>

davvid added a commit to davvid/git-cola that referenced this pull request Apr 17, 2018

Merge branch 'log-widget'
After all the back-and-forth, Alex's version seemed like the best
compromise.

* log-widget:
  app: change date/time format in log widget
  Revert "log: pass the format string through gettext"
  Revert "i18n: use a better format string for Hungarian locales"

Resolves git-cola#800
Helped-by: Alex Gulyás <gulyas.alex@gmail.com>
Signed-off-by: David Aguilar <davvid@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment