463273 - Error: Failed to read 'repeatDetailsOrdinal0' from chrome://calendar/locale/calendar-event-dialog.properties

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Stefan Sitter [:ssitter]]

Attachment: Testcase from Bug 356207

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081105 Calendar/1.0pre (BuildID: 20081105110610)

Steps to Reproduce:
1. Start Sunbird with clean profile
2. Import the attached testcase that was taken from Bug 356207
3. Open the event for editing, choose "Edit all occurrences" when prompted

Actual Results:
Recurrence summary in the Edit Event dialog shows
[[[
Occurs the Failed to read 'repeatDetailsOrdinal0' from chrome://calendar/locale/calendar-event-dialog.properties. Wednesday effective 9/1/2004 from 2:15 PM to 2:15 PM
]]]

Error Console shows:
[[[
Error: Failed to read 'repeatDetailsOrdinal0' from chrome://calendar/locale/calendar-event-dialog.properties. Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.GetStringFromName]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://calendar/content/calUtils.js :: calGetString :: line 658"  data: no]
Source file: chrome://calendar/content/calUtils.js
Line: 662
]]]

Expected Results:
No error. I think the correct recurrence summary should be "Occurs every Wednesday the 1st effective ...". If this is not possible display nothing.

Comment 1

[Philipp Kewisch [:Fallen][☁️📆]]

Attachment: Fix - v1

This patch takes care, catching all edge cases and at least displaying a message to click for details. It also adds plural forms for all strings added and adds a test calendar to be used for l10n with events that each cause a different locale string and plural form to be shown.

I've added events only for plural forms 1-3, if more is needed please file a bug.

Comment 2

[Philipp Kewisch [:Fallen][☁️📆]]

Attachment: [checked in] Fix - v1

Forgot to qref the ics file.

Comment 3

[Berend Cornelius]

Comment on attachment 357870 [details] [diff] [review]
[checked in] Fix - v1

patch works fine with the testcase and greatly improves the readability of the source code. I wouldn't mind defining "getRString" and "getDString" globally probably with a more meaningfull name, so that we can use them in all the other places, too.

Comment 4

[Philipp Kewisch [:Fallen][☁️📆]]

I'd rather keep them local.

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/b9cd225d077e>

-> FIXED

Comment 5

[Philipp Kewisch [:Fallen][☁️📆]]

Since defining the function is very simple, I'd like to keep them only in functions that really need it. I guess I could imagine renaming getRString and making it a top-level function, but getDString is quite specific.

Comment 6

[Hubert Gajewski]

There is:
yearlyEveryNthOfNth=every %1$S of %2$S;every 3 years on every %1$S of %2$S

Should be:
yearlyEveryNthOfNth=every %1$S of %2$S;every #4 years on every %1$S of %2$S

-> REOPEN

BTW - # XXX Please file a bug if this makes it hard to localize the list.

Without keys for weekdays and months it is impossible to localize correctly in Polish. Do you need a separate bug?

Comment 7

[Hubert Gajewski]

repeatDetailsOrdinalN (where N=[1-5]) and repeatDetailsOrdinal-1 are unused and should be removed

Comment 8

[Hubert Gajewski]

(In reply to comment #7)
> repeatDetailsOrdinalN (where N=[1-5]) and repeatDetailsOrdinal-1 are unused and
> should be removed

Nevermind, my mistake. I am sorry for the spam.

Comment 9

[Hubert Gajewski]

Attachment: Patch (few fixes)

Patch resolves a problem described in comment 6, add two elements to xul file (now 2 keys in properties file are unable to see in the Edit Reccurence window) and does few other fixes.

Comment 10

[Hubert Gajewski]

Attachment: Edit Recurrence window (patch applied)

Comment 11

[Philipp Kewisch [:Fallen][☁️📆]]

For english at least the correct form would be "Every Saturday of April" (i.e "The" needs to be collapsed when "Every" is selected.

Comment 12

[Hubert Gajewski]

(In reply to comment #11)
> For english at least the correct form would be "Every Saturday of April" (i.e
> "The" needs to be collapsed when "Every" is selected.

On the other hand - I am affraid that described behavior can be incorrect in few languages...

Maybe we should leave event.recurrence.repeat.every.label and event.recurrence.pattern.yearly.every.weekday.label empty?

Monthly:
Every _____ Sunday
Every First Sunday
Every Last  Sunday

Yearly:
Every _____ Sunday of January
Every First Sunday of January
Every Last  Sunday of January


Also I should change "monthly-ordinal-first-every" to "monthly-ordinal-every-label"

Comment 13

[Berend Cornelius]

Comment on attachment 358713 [details] [diff] [review]
Patch (few fixes)

looks good; r=berend

Comment 14

[Hubert Gajewski]

(In reply to comment #13)
> (From update of attachment 358713 [details] [diff] [review])
> looks good; r=berend

What about with Philipp's comment?

Comment 15

[Takeshi Nishimura]

Just one notification from reviewing source code.

http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-dialog-utils.js#183
> let monthlyString = getRString("monthlyDayOfNth", [startDate.day]);
> ruleString = PluralForm.get(rule.interval, monthlyString)
>                        .replace("#1", rule.interval);

Is it correct? #2 instead of #1?

http://mxr.mozilla.org/comm-central/source/calendar/locales/en-US/chrome/calendar/calendar-event-dialog.properties#87
> monthlyDayOfNth=day %1$S of every month;day %1$S of every #2 months

Comment 16

[Martin Schröder [:mschroeder]]

Comment on attachment 358713 [details] [diff] [review]
Patch (few fixes)

>                     <menupopup id="montly-ordinal-menupopup">
>+                      <menuitem id="monthly-ordinal-first-every"
>+                                label="&event.recurrence.repeat.every.label;"
>+                                value="0"/>
>                       <menuitem id="monthly-ordinal-first-label"
>                                 label="&event.recurrence.repeat.first.label;"
>                                 value="1"/>
>                       <menuitem id="mnthly-ordinal-second-label"
>                                 label="&event.recurrence.repeat.second.label;"
>                                 value="2"/>

Please, fix the id of the menuitem element "mnthly-ordinal-second-label".

Comment 17

[Rimas Kudelis [use needinfo to ping]]

From calendar-event-dialog.properties:

# LOCALIZATION NOTE (repeatDetailsRuleDaily3):
# Edit recurrence window -> Recurrence pattern -> Daily repeat rules
# #1 - number
# e.g. "every 4 days"
dailyEveryNth=every day;every #1 days
repeatDetailsRuleDaily4=every weekday

I'd like to mention that not all locales have "1" as a separate rule. For
example, in Lithuanian 1, 21, 31, ..., 91, 101, 121 etc. match the first rule,
and if I leave the first string as "every day", this wouldn't be correct.

OTOH, I don't want to see "every 1 day" here either.

Could we please have separate recurrence strings to translate for cases where
X=1 ?

Comment 18

[Hubert Gajewski]

Comment on attachment 358713 [details] [diff] [review]
Patch (few fixes)

I will prepare a new patch (but I wait for decision regarding "the every").

Comment 19

[Stefan Sitter [:ssitter]]

Did this fix Bug 351671 too?

Comment 20

[Rimas Kudelis [use needinfo to ping]]

I'm also wondering why repeatDetailsMonthX and repeatDetailsDayX were removed? Where will month and day names be taken from now?

Comment 21

[Hubert Gajewski]

(In reply to comment #20)
> I'm also wondering why repeatDetailsMonthX and repeatDetailsDayX were removed?

Don't care, it will back :)

Comment 22

[Rimas Kudelis [use needinfo to ping]]

Hubert, would you also take care to update reminderCustomUnit* to support plurals?.. :)

Comment 23

[Hubert Gajewski]

(In reply to comment #22)
> Hubert, would you also take care to update reminderCustomUnit* to support
> plurals?.. :)

Please submit a new bug for this.

Comment 24

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #23)
> (In reply to comment #22)
> > Hubert, would you also take care to update reminderCustomUnit* to support
> > plurals?.. :)
> 
> Please submit a new bug for this.

OK, it's now in bug 475278.

Comment 26

[Merike Sell (:merike)]

The example .ics file has three UIDs used twice, monthlyLastDayOfNth-[123] and therefore Sunbird fails to import three events. Some events also have end date earlier than start date, but that doesn't hurt much as you want to use that file only to view occurrence wording not to change any event.

Also, perhaps calendar-event-dialog.properties/dtd files should have a comment about where you can find example calendar so that future localizers would know that it exists.

Comment 27

[Philipp Kewisch [:Fallen][☁️📆]]

(In reply to comment #20)
> I'm also wondering why repeatDetailsMonthX and repeatDetailsDayX were removed?
> Where will month and day names be taken from now?

This is now taken from dateFormat.properties. I hope this is ok.

Comment 28

[Hubert Gajewski]

(In reply to comment #27)
> (In reply to comment #20)
> > I'm also wondering why repeatDetailsMonthX and repeatDetailsDayX were removed?
> > Where will month and day names be taken from now?
> 
> This is now taken from dateFormat.properties. I hope this is ok.

Unfortunately not. Some languages (like Polish) cannot use forms from dateFormat.properties here.

English / Polish
the first / pierwszy
January / styczeń
The first of January / Pierwszy stycznia

Comment 29

[Merike Sell (:merike)]

It also limits Estonian traslation at the moment.

Could we use recurrence.pattern.monthly* and recurrence.pattern.yearly* values from caledar-event-dialog.dtd the same way they're used in dropdowns when you pick recurrence rule? With these I was able to get it right for 0.9.

Comment 30

[Philipp Kewisch [:Fallen][☁️📆]]

dateFormat: I can just revert to using extra strings in the recurrence dialog.

Comment 31

[Hubert Gajewski]

(In reply to comment #30)
> dateFormat: I can just revert to using extra strings in the recurrence dialog.

I will add these changes to the new patch.

Comment 32

[Hubert Gajewski]

Attachment: Patch 4 (see comments: 11, 12, 15, 28)

New patch after comments: 11, 12, 15, 19.

Empty options in dropdown lists are not the best resolution however I don't have better idea.

Comment 33

[Hubert Gajewski]

Attachment: Edit Recurrence window (patch 4 applied)

Comment 34

[Philipp Kewisch [:Fallen][☁️📆]]

(In reply to comment #32)
> Empty options in dropdown lists are not the best resolution however I don't
> have better idea.

Two possible solutions
======================

* Make the dropdown contain "The", i.e:

  ( )  [ The First \/]  [ Wednesday \/]
  ( )  [   Every   \/]  [ Wednesday \/]

* Collapse (visbility:hidden shouldn't change the width) the "The" when Every
  is selected:

  ( ) The [ First \/]  [ Wednesday \/]
  ( )     [ Every \/]  [ Wednesday \/]

Comment 35

[Hubert Gajewski]

Attachment: Patch 5 (see comment 34)

(In reply to comment #34)
> Two possible solutions
> ======================
> 
> * Make the dropdown contain "The", i.e:
> 
>   ( )  [ The First \/]  [ Wednesday \/]
>   ( )  [   Every   \/]  [ Wednesday \/]

OK.

> * Collapse (visbility:hidden shouldn't change the width) the "The" when Every
>   is selected:
> 
>   ( ) The [ First \/]  [ Wednesday \/]
>   ( )     [ Every \/]  [ Wednesday \/]

It is risky - see my comment 12 regarding localization.

Comment 36

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 359488 [details] [diff] [review]
Patch 5 (see comment 34)

>-<!ENTITY event.recurrence.repeat.first.label            "First">
>-<!ENTITY event.recurrence.repeat.second.label           "Second">
>-<!ENTITY event.recurrence.repeat.third.label            "Third">
>-<!ENTITY event.recurrence.repeat.fourth.label           "Fourth">
>-<!ENTITY event.recurrence.repeat.fifth.label            "Fifth">
>-<!ENTITY event.recurrence.repeat.last.label             "Last">

>+<!ENTITY event.recurrence.repeat.first.label            "The First">
>+<!ENTITY event.recurrence.repeat.second.label           "The Second">
>+<!ENTITY event.recurrence.repeat.third.label            "The Third">
>+<!ENTITY event.recurrence.repeat.fourth.label           "The Fourth">
>+<!ENTITY event.recurrence.repeat.fifth.label            "The Fifth">
>+<!ENTITY event.recurrence.repeat.last.label             "The Last">
...
>-<!ENTITY event.recurrence.pattern.yearly.first.label         "First">
>-<!ENTITY event.recurrence.pattern.yearly.second.label        "Second">
>-<!ENTITY event.recurrence.pattern.yearly.third.label         "Third">
>-<!ENTITY event.recurrence.pattern.yearly.fourth.label        "Fourth">
>-<!ENTITY event.recurrence.pattern.yearly.fifth.label         "Fifth">
>-<!ENTITY event.recurrence.pattern.yearly.last.label          "Last">

>+<!ENTITY event.recurrence.pattern.yearly.first.label         "The First">
>+<!ENTITY event.recurrence.pattern.yearly.second.label        "The Second">
>+<!ENTITY event.recurrence.pattern.yearly.third.label         "The Third">
>+<!ENTITY event.recurrence.pattern.yearly.fourth.label        "The Fourth">
>+<!ENTITY event.recurrence.pattern.yearly.fifth.label         "The Fifth">
>+<!ENTITY event.recurrence.pattern.yearly.last.label          "The Last">
These labels need renaming since the string changed.

r=philipp with the above changes, please upload a new patch and set r+ and checkin-needed

Comment 37

[Hubert Gajewski]

Attachment: [checked in] Patch 6 (see comment 36)

I've also changed key name yearlyEveryNthOfNth to yearlyOnEveryNthOfNth (many localizers did the same mistake as we have in en-US - "3 years" instead of "#3 years")

Comment 38

[Philipp Kewisch [:Fallen][☁️📆]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/3d71892bdb83>

-> FIXED

Comment 39

[Iacopo Benesperi [:iacchi]]

I'd like to say one last thing on this matter. Sorry for not having replied sooner, but I had no time during this week. I've already spoken with Axel Hecht about what I'm going to say and he told me that we can't do anything now and that we should wait for Mozilla2. I'm writing just to let you all know this and (hope) remember when it'll be the right time.

Here in italy we have gender accordance of articles, adjectives and so on. The problem is that monday, ..., saturday are masculine nouns, but sunday is feminine. So for example:

The First Monday -> Il primo lunedì
The First Sunday -> La prima domenica

So now I translate The First and so on with the masculine accordance because it happens more often; but if a user want to set up a reminder on a sunday event he will get

Il primo domenica

which is like using "he" for "Sarah" or "Michelle".

Comment 40

[Philipp Kewisch [:Fallen][☁️📆]]

I guess we could add even more strings and do something like


ordinalFirstSunday=The first
  -> ordinalFirstSunday=La prima

ordinalFirstMonday=The first
  -> ordinalFirstMonday=Il primo

This will result in a lot of strings though and we'll have to add code to switch the names based on the selected weekday. If you think this is worth the effort, do you think you could whip up a patch to take care? If so, please file a new bug.

Comment 41

[Hubert Gajewski]

(In reply to comment #39)

> The First Monday -> Il primo lunedì
> The First Sunday -> La prima domenica

In Polish we don't use articles so it easier for me, however I have similar problem:

> The First Monday -> Pierwszy poniedziałek
> The First Sunday -> Pierwsza niedziela

so I use:

1. poniedziałek
1. niedziela

Comment 42

[Iacopo Benesperi [:iacchi]]

(In reply to comment #40)
> I guess we could add even more strings and do something like
> This will result in a lot of strings though and we'll have to add code to
> switch the names based on the selected weekday.

I know it's a big work, and there are other strings affected by this problem. As I said, I can wait (I don't know the others, but I can) Mozilla2. I just wanted to report the problem now to advertize it. I wrote some more examples here the last July: https://wiki.mozilla.org/Talk:L20n:Examples as requested by Axel, and I'll write some more in the future. So, just remember this (I'll remind it to you, anyway) when you'll be able to solve this problem easily.

Comment 43

[Iacopo Benesperi [:iacchi]]

(In reply to comment #41)
> so I use:
> 
> 1. poniedziałek
> 1. niedziela

This could be a good workaround. I'll look into it, thanks.

Comment 44

[Philipp Kewisch [:Fallen][☁️📆]]

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".