405222 - Time format not respected in prefs window

Status: RESOLVED FIXED

(Calendar :: Preferences, defect)

Description

[Christian Schmidt]

In the preferences window on the Views tab, there are two dropdown menus for selecting the start and end of the day:
Day starts at: [ 5:00 AM ]
Day ends at:   [ 9:00 PM ]

The labels in the dropdown menu are hardcoded to times in AM/PM format and do not respect my system setting (LC_TIME) that specifies a 24 hour clock.

Comment 1

[Christian Schmidt]

Attachment: Use same format as in day view and multi-day view

See also bug 405199 for another i18n change to the same preference tab.

Comment 2

[Simon Paquet [:sipaq]]

Comment on attachment 290017 [details] [diff] [review]
Use same format as in day view and multi-day view

Matt is not active at the moment. Please request reviews from the active reviewers listed on http://wiki.mozilla.org/Calendar:Module_Ownership

Comment 3

[Christian Schmidt]

Attachment: Use same format as in day view and multi-day view, v2

Comment 4

[Stefan Sitter [:ssitter]]

Comment on attachment 290026 [details] [diff] [review]
Use same format as in day view and multi-day view, v2

This patch incorrectly changes the values for dayendhour from 1..24 to 0..23.

Comment 5

[Christian Schmidt]

Attachment: Use same format as in day view and multi-day view, v3

(In reply to comment #4)
> This patch incorrectly changes the values for dayendhour from 1..24 to 0..23.
Sorry, I missed that. It is fixed now.

I considered using "24:00" rather than "0:00" in dayendhour. This is often used, at least in some 24-hour locales, and it is even permitted in ISO 8601 dates. However, this convention isn't used elsewhere in the application (AFAICS it isn't even supported by the date formatter), so I decided not to use it here either.

Comment 6

[Michael Büttner (no reviews TFN)]

Comment on attachment 290443 [details] [diff] [review]
Use same format as in day view and multi-day view, v3

The menulist elements 'daystarthour' and 'dayendhour' won't remember their selection with this patch applied. At least this is the behavior under Window XP/SP2. I didn't drill into the details why this happens, but a possible workaround could be to manually lookup the current preference value  (e.g. getPrefSafe) and set the selection. Apart from the the patch looks good, but I need to minus it until the selection works reliable with this patch in place.

Comment 7

[Christian Schmidt]

That's weird. I just did a fresh MingW build of Sunbird on WinXP/SP2. And it also works on Linux in Sunbird as well as Lightning. It sounds like a bug somewhere else. I don't mind making a workaround, but it would be nice to know a bit more about the bug. Are the prefs not saved (i.e. does changing the selection actually change the day view), or are the menulists just not properly initialized? If you close the preferences dialog and open it again, are the selections then forgotten, or is it only when you close the app?

Comment 8

[Christian Schmidt]

Is anybody else experiencing the issue described in comment 6? I cannot reproduce it myself, so it is difficult for me to make a workaround.

According to <http://developer.mozilla.org/en/docs/XUL:menuitem>, the label attribute of <menuitem> is not mandatory, so if it breaks because of the missing label, this should be filed as a separate bug.

Comment 10

[Robin Edrenius]

Attachment: Slightly different approach; Keeping midnight & noon as the localized strings

Posting a slightly revised version of my patch from Bug 450601 here.

This patch keeps the localized labels for "Midnight" & "Noon", I don't know whether that's the way it's preferred or not, but seeing as the en-US version uses words for those two rather than the numbers I'm assuming it is.

Christian, if you intend to continue working on this bug, please feel free to obsolete my patch. Also feel free to obsolete it in case posting patches to bugs assigned to someone else is frowned upon.

Comment 11

[Christian Schmidt]

> Christian, if you intend to continue working on this bug, please feel free to
> obsolete my patch.
Please, go ahead :-)

> I don't know whether that's the way it's preferred or not, but seeing as the
> en-US version uses words for those two rather than the numbers I'm assuming
> it is.
In a 24-hour locale, I assume people would prefer to use the labels "0" (or "00") and "12", but "Midnight" and "Noon" are probably preferable in AM/PM locales in order to avoid confusion.

Comment 12

[Martin Schröder [:mschroeder]]

Comment on attachment 333904 [details] [diff] [review]
Slightly different approach; Keeping midnight & noon as the localized strings

Changing reviewer to Philipp.

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 333904 [details] [diff] [review]
Slightly different approach; Keeping midnight & noon as the localized strings

>+        for(var theHour = 1; theHour <= 23; theHour++) {
Please see our Style Guide, add a space between "for" and the "(". The same goes for if() and other block constructs.

>+            var time = Components.classes["@mozilla.org/intl/scriptabledateformat;1"]
>+                                 .getService(Components.interfaces.nsIScriptableDateFormat)
>+                                 .FormatTime("", Components.interfaces.nsIScriptableDateFormat
>+                                 .timeFormatNoSeconds, theHour, 0, 0);
Instead of getting the service in every loop cycle, I'd prefer getting it once outside the loop and then calling FormatTime inside the loop.

>+            labelIdStart = "timeStart"+theHour;
Spaces before and after operators here and elsewhere.

>+        // Deselect and reselect to update visible item title (Taken from line 56 in general.js)
I'd drop the line number, since it might change in the future, which would make this comment confusing. You might also consider making this a helper function, if its used in various places.


>-<!ENTITY time.1 "1:00 AM" >
>-<!ENTITY time.2 "2:00 AM" >
...
>-<!ENTITY time.23 "11:00 PM" >
Please file a followup bug to remove these entities after the release since we are already in string freeze. In that bug, we can maybe also make displaying the Noon/Midnight strings dependant on the locale, i.e AM/PM locales use Noon/Midnight while 24 hour locales use 0 and 12.

r=philipp at least codewise, I'll give this a short test later on. Please post a new patch with above comments fixed.

Comment 14

[Robin Edrenius]

Attachment: Updated patch

(In reply to comment #13)
> (From update of attachment 333904 [details] [diff] [review])
> >+        for(var theHour = 1; theHour <= 23; theHour++) {
> Please see our Style Guide, add a space between "for" and the "(". The same
> goes for if() and other block constructs.
> 

Adjusted those throughout the patch.

> >+            var time = Components.classes["@mozilla.org/intl/scriptabledateformat;1"]
> >+                                 .getService(Components.interfaces.nsIScriptableDateFormat)
> >+                                 .FormatTime("", Components.interfaces.nsIScriptableDateFormat
> >+                                 .timeFormatNoSeconds, theHour, 0, 0);
> Instead of getting the service in every loop cycle, I'd prefer getting it once
> outside the loop and then calling FormatTime inside the loop.
> 

Fixed!

> >+            labelIdStart = "timeStart"+theHour;
> Spaces before and after operators here and elsewhere.
> 

Fixed!

> >+        // Deselect and reselect to update visible item title (Taken from line 56 in general.js)
> I'd drop the line number, since it might change in the future, which would make
> this comment confusing. You might also consider making this a helper function,
> if its used in various places.
> 

Made this into a helper function (I hope calUtils.js is the right place to put it), also made general.js use the helper function.
Should these be done in another bug (and/or a split up patch)?

> 
> >-<!ENTITY time.1 "1:00 AM" >
> >-<!ENTITY time.2 "2:00 AM" >
> ...
> >-<!ENTITY time.23 "11:00 PM" >
> Please file a followup bug to remove these entities after the release since we
> are already in string freeze. In that bug, we can maybe also make displaying
> the Noon/Midnight strings dependant on the locale, i.e AM/PM locales use
> Noon/Midnight while 24 hour locales use 0 and 12.
> 

Filed Bug 451035 for this

Comment 15

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 334265 [details] [diff] [review]
Updated patch

>+
>+function updateLabel(aElementId) {
>+    var selectedIndex = aElementId.selectedIndex;
>+    aElementId.selectedIndex = -1;
>+    aElementId.selectedIndex = selectedIndex;
>+}
Yes, looks nice. I'd prefer some documentation and rather calling it updateSelectedLabel(). I'll do that myself before checkin.

r=philipp

Comment 16

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I rather moved the function to calendar-ui-utils.js and also updated the parameter name to aElement, and used similar logic as in setElementValue() to ensure the passed element is a xul element.

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED