443722 - Add possiblity to change elements order in Edit Recurrence window

Status: VERIFIED FIXED

(Calendar :: General, defect)

Description

[Hubert Gajewski]

There are languages (bug 352513 comment c20) which require different order for dropdown lists.

An example: Recurrence pattern -> Repeat annually in English:
Every [ Fourth v] [ Friday v]
   of [ June               v]

In Lithuanian they need something like this:
Every [ June v] [ Fourth v] 
      [ Friday           v]

I'll try to fix it.

Comment 1

[Hubert Gajewski]

Attachment: Patch

Comment 2

[Hubert Gajewski]

Attachment: Patch 0.2

Some small changes.

Comment 3

[Stefan Sitter [:ssitter]]

Comment on attachment 328265 [details] [diff] [review]
Patch 0.2

mvl, mickey and ctalbert are currently not actively doing reviews. For the dialog topics you probably want to ask philipp or berend, see <http://wiki.mozilla.org/Calendar:Module_Ownership>

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: patch 0.3

>+function monthlyWidgetsOrder() {
>+function yearlyWidgetsOrder1() {
>+function yearlyWidgetsOrder2() {
These functions are all quite similar, it would be nice to consolidate them into one function.

r=me with functions consolidated

Since our string freeze is coming up, I've created the patch for you. If you don't agree, please feel free to correct me and I'll check in a correction.

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED

Comment 6

[Rimas Kudelis [use needinfo to ping]]

Is there any reason why the string "of" is included as %3$S, and not just "of" in yearlyOrder2 ?

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

Yes. These strings are not real strings to translate, they just determine the order of elements which is changed when the dialog is loaded. The way this was implemented, it doesn't make sense to use hardcoded strings there.

Comment 8

[Atsushi Sakai]

> # Edit recurrence window -> Recurrence pattern -> Repeat montly
> # %1$S - ordinal, %2$S - month
> monthlyOrder=%1$S %2$S

%2$S is not month, but weekday?

Comment 9

[Atsushi Sakai]

>            // This will place the node at the end
>            element.parentNode.appendChild(element);

As for yearlyOrder2, this does not work correctly
because the parent of "yearly-weekday" and the parent of "yearly-month-rule" are not same.

Comment 10

[Hubert Gajewski]

(In reply to comment #8)
> > # Edit recurrence window -> Recurrence pattern -> Repeat montly
> > # %1$S - ordinal, %2$S - month
> > monthlyOrder=%1$S %2$S
> 
> %2$S is not month, but weekday?

Yes, you are right. My fault.

+# Edit recurrence window -> Recurrence pattern -> Repeat yearly
+# %1$S - ordinal, %2$S - month
+yearlyOrder=%1$S %2$S

And here we have rather "day of month", not ordinal.

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #9)
> >            // This will place the node at the end
> >            element.parentNode.appendChild(element);
> 
> As for yearlyOrder2, this does not work correctly
> because the parent of "yearly-weekday" and the parent of "yearly-month-rule"
> are not same.
> 

I now understand what that part of Hubert's code was doing. Sorry for bashing it. I'll post a fix later today.

Comment 12

[Hubert Gajewski]

(In reply to comment #11)
> 
> I'll post a fix later today.

BTW - please fix my wrong localization notes (described in comment 9 and 10). 

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: patch 0.3.1

This patch should do it.

Comment 14

[Hubert Gajewski]

Comment on attachment 329671 [details] [diff] [review]
patch 0.3.1

r=hubert

Comment 15

[Atsushi Sakai]

(In reply to comment #13)
> This patch should do it. 

If yearlyOrder2=%4$S %2$S %3$S %1$S,

Every [ month ] [ weekday ]
   of [ ordinal           ]

is expected, but actually

Every [ weekday ] [ month ]
   of [ ordinal           ]

Comment 16

[Hubert Gajewski]

(In reply to comment #15)
> (In reply to comment #13)
> > This patch should do it. 
> 
> If yearlyOrder2=%4$S %2$S %3$S %1$S,
> 
> Every [ month ] [ weekday ]
>    of [ ordinal           ]
> 
> is expected, but actually
> 
> Every [ weekday ] [ month ]
>    of [ ordinal           ]
>

I was checking this configuration and it was OK. Let me check again. 

Comment 17

[Hubert Gajewski]

Comment on attachment 329671 [details] [diff] [review]
patch 0.3.1

Thanks Atsushi, I was wrong.

Comment 18

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: patch 0.3.2

Sorry, didn't notice. Taking out the check to see if the element is already at the right place seems to do it. I've tested the following combinations:


1 2 3 4
2 4 3 1
2 4 1 3
4 2 3 1
4 3 2 1

Comment 19

[Hubert Gajewski]

Comment on attachment 329797 [details] [diff] [review]
patch 0.3.2

r=hubert

I've checked also 4 different configurations. Looks OK.

Now I remember - I had similar problem with yearlyOrder2 - this is why I was removing all elements.

Comment 20

[Rimas Kudelis [use needinfo to ping]]

1 4 3 2 (used in Lithuanian) doesn't seem to work either.

Comment 21

[Hubert Gajewski]

Attachment: Edit recurrence window in Lithuanian

Rimas: WFM - please look at the screenshot.

Comment 22

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #21)
> Created an attachment (id=329828) [details]
> Edit recurrence window in Lithuanian
> 
> Rimas: WFM - please look at the screenshot.

Yes, this is correct.

Possibly another bug, but the captions of the minimonths below in the same dialog should also be localizable. This also applies to the calendar widget in the Date tab in the left pane of the main window. There's a monthInYear entity defined in calendar.properties that could be used for this, but not necessarily.

Comment 23

[Philipp Kewisch [:Fallen][📱📆]]

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED

Comment 24

[Rimas Kudelis [use needinfo to ping]]

Attachment: Edit recurrence window in Lithuanian (latest branch build)

Hubert,
check this out please. I just downloaded the latest Tinderbox build from ftp://ftp.mozilla.org/pub/mozilla.org/calendar/sunbird/tinderbox-builds/latest-mozilla1.8-l10n/ , but it still manifests the ordering problem.

I even extracted sun-dialog-event-dialog.properties to make sure the order is correct in there, and it is correct actually.

Any ideas?

Comment 25

[Atsushi Sakai]

(In reply to comment #24)
> I just downloaded the latest Tinderbox build

Its build ID is 2008071520.
Please wait the build after 2008071612.

Comment 26

[Hubert Gajewski]

I've check latest Linux build. WFM.

Comment 27

[Rimas Kudelis [use needinfo to ping]]

Thanks to both of you. I'll check again later then. :)

Comment 28

[Rimas Kudelis [use needinfo to ping]]

Works for me too now (build 2008071620), thanks!

Comment 29

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 328263 [details] [diff] [review]
Patch

>Index: mozilla/calendar/locales/en-US/chrome/prototypes/sun-calendar-event-dialog.properties
>+# LOCALIZATION NOTE: You can change the order of below params
>+# Edit recurrence window -> Recurrence pattern -> Repeat montly
>+# %1$S - ordinal, %2$S - month
>+monthlyOrder=%1$S %2$S

(very late comment, sorry). UIntil now, we tries to use the OS formatting for dates and times. But this patch seems like a switch away from that. That seems inconsistent to me. Is this really what is wanted? Should everything else also be changed to use date format from l10n, instead of the OS?

Comment 30

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #29)
> (very late comment, sorry). UIntil now, we tries to use the OS formatting for
> dates and times. But this patch seems like a switch away from that. That seems
> inconsistent to me. Is this really what is wanted? Should everything else also
> be changed to use date format from l10n, instead of the OS?

I asked a similar question in some other bug. The answer is that not all Operating Systems are localized into all locales that Mozilla apps support. And even when they are, you can't always expect the user to use OS localized into his preferred language.