502435 - Recurrence summary shows only the first weekday of a monthly rule with BYDAY tag and more weekdays

Status: RESOLVED FIXED

(Calendar :: Dialogs, defect)

Description

[Decathlon]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; it-IT; rv:1.9.0.11) Gecko/2009060215 Firefox/3.0.11
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.1pre) Gecko/20090702 Lightning/1.0pre Shredder/3.0b3pre

A calendar with monthly rule and BYDAY tag with more than one weekday like this:

BEGIN:VCALENDAR
PRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN
VERSION:2.0
BEGIN:VEVENT
CREATED:20090529T202927Z
LAST-MODIFIED:20090529T203001Z
DTSTAMP:20090529T203001Z
UID:fbacc989-a182-4def-8c76-15219d89689c
SUMMARY:TEST
RRULE:FREQ=MONTHLY;BYDAY=MO,TU,WE
DTSTART;VALUE=DATE:20090601
DTEND;VALUE=DATE:20090602
TRANSP:TRANSPARENT
END:VEVENT
END:VCALENDAR

is fully handled by Lightnig/Sunbird and correctly showed in calendar views
with three event for week on Monday, Tuesday and Wednesey, but editing the events, the recurrence summary shows a wrong rule with only the first weekday. 


Reproducible: Always

Steps to Reproduce:
1. create a new calendar;
2. import the ics calendar showed above;
3. edit a calendar's events;

Actual Results:  
the recurrence summary shows the rule:
"Occurs every Monday of every month
 effective 01/06/2009"


Expected Results:  
a rule like this:
"Occurs every Monday, Tuesday and Wednesday of every month
 effective 01/06/2009"
or a notice which informs the user that the rule can't be showed correctly.

Comment 1

[Decathlon]

Attachment: patch-"rule too complex"

Lightning/Sunbird's UI doesn't allow to set a monthly rule with more than one weekday, so I think recurrence summary should reflect this limitation and should treat a monthly rule with more than one BYDAY element like a "too complex rule" and show the sentence "click here for details" instead of a wrong monthly rule. This patch change recurrence summary in this way.

It's also possible to change the summary to match the rule, but wouldn't be coherent with the user interface.

Comment 2

[Martin Schröder [:mschroeder]]

Is editing such a rule possible? Maybe anyone knows if there already exists a bug report.

Comment 3

[Decathlon]

reply to comment #2)
> Is editing such a rule possible?
No it's impossible because controls on recurrence dialog doesn't allow to set more than one weekday for monthly rule.
Opening the edit recurrence dialog and closing it with OK button without modification transforms the rule:

from  RRULE:FREQ=MONTHLY;BYDAY=MO,TU,WE
to    RRULE:FREQ=MONTHLY;BYDAY=MO

because controls are set for this rule (only one day, the first day of the rule).
Maybe something should be done for this fact too.

 
> Maybe anyone knows if there already exists a bug report.

I've searched with single keyword BYDAY, recurrence summary, weekday, but I haven't found a similar description.

Comment 4

[Decathlon]

Attachment: patch that handle the rule

Previous patch probably is a bit 'drastic'. This patch, instead, generates a recurrence summary for monthly rule with BYDAY tag and more weekdays with and without a positive or negative (only -1) prefix e.g.

RRULE:FREQ=MONTHLY;BYDAY=MO,TU,WE
-> "every Monday, Tuesday and Wednesday of every month";

RRULE:FREQ=MONTHLY;BYDAY=MO,TU,-1WE,2FR,SA
-> "every Monday, Tuesday, Saturday, the last Wednesday and the second Friday of every month";

I add also a control to avoid writing in the summary a single weekday with a prefix number if it already exists without prefix, but I don't know if this is useless precaution:

RRULE:FREQ=MONTHLY;BYDAY=MO,2MO
-> "every Monday ..." instead of "every Monday and the second Monday ..."

The patch would need the change of the string 'monthlyNthOfEveryNounclass' in  
'calendar-event-dialog.properties' file because it requires only a parameter 
%1$S instead of two:

now ordinal and weekday are separated %1$S %2$S -> 'the first' 'Monday'
with the patch a single parameter is used for the whole (composite) string
%1$S -> 'the first Monday, the last Sunday and the second Friday'
(i.e. ordinal and weekday, one or more, are already composite before using the rule string).

For now I resolved without changing the string (Calendar is in string freeze) and using a null string for the second parameter but I have a doubt if %1$S and %2$S, ordinal and weekdays noun, need to be separated for localization reasons, maybe someone could give me a hint.

Comment 5

[Decathlon]

Attachment: [after beta] unbitrotted patch that handle the rule

updated patch after changes in bug 443752.

Comment 6

[Stefan Sitter [:ssitter]]

I'll probably not review this patches anytime soon. I would be delighted if someone takes over the reviews.

Comment 7

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

Comment on attachment 397609 [details] [diff] [review]
[after beta] unbitrotted patch that handle the rule

This bug has a patch with strings, so nothing will happen until the beta anyway. I'm sure we can find a reviewer afterwards though.

Comment 8

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

Comment on attachment 386871 [details] [diff] [review]
patch-"rule too complex"

Decathlon, could you update this patch so it applies again? 

I'll be happy to review this patch afterwards.

Comment 9

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

Comment on attachment 397609 [details] [diff] [review]
[after beta] unbitrotted patch that handle the rule

Same for this one, please update.

Comment 10

[Decathlon]

Attachment: patch "rule too complex" - v2

Comment 11

[Decathlon]

Attachment: patch that handles the rule - v2

Updated both patches.
About the second patch, as I said in comment #4, my doubt is that I've put together the ordinal string and the weekday noun in the code and I've removed an arguments from the string monthlyNthOfEvery:

the original string

monthlyNthOfEveryNounclass1=%1$S %2$S of every month;%1$S %2$S of every #3 months
(%1$S = ordinal string; %2$S = weekdays)
 
becomes

monthlyNthOfEveryNounclass1=%1$S of every month;%1$S of every #3 months
where %1$S is the composite string ordinal+weekdays (in the code: ordinalString + " " + dayString).
I'm not sure whether ordinal and weekdays must be separated for localization reasons.

I've changed a string and I know I should also change the string name, but since the modify is only for an argument, could it be possible to avoid to change the string name?

Comment 12

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

Comment on attachment 432792 [details] [diff] [review]
patch that handles the rule - v2

>+                            if (byday.some(function(element, index, array) {
>+                                               if (day_position(element) == 0) {
>+                                                    return (day_of_week(byday[i]) == day_of_week(element));
>+                                               }
>+                                           })
This function doesn't always return a value. You should explicitly return in case the day_position is not zero.


>-monthlyNthOfEveryNounclass1=%1$S %2$S of every month;%1$S %2$S of every #3 months
>-monthlyNthOfEveryNounclass2=%1$S %2$S of every month;%1$S %2$S of every #3 months
>+monthlyNthOfEveryNounclass1=%1$S of every month;%1$S of every #2 months
>+monthlyNthOfEveryNounclass2=%1$S of every month;%1$S of every #2 months
Localizers will need to update the string, even though you are just removing an argument. Otherwise there will be an extra space. I think its ok to just rename the string.



r=philipp, but please upload a new patch for checkin.

Comment 13

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

Comment on attachment 432791 [details] [diff] [review]
 patch "rule too complex" - v2

Lets handle the rule instead of ignoring it.

Comment 14

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

> About the second patch, as I said in comment #4, my doubt is that I've put
> together the ordinal string and the weekday noun in the code and I've removed
> an arguments from the string monthlyNthOfEvery:
> 
> I'm not sure whether ordinal and weekdays must be separated for localization
> reasons.
I'm not really good on this either. I guess thats up to the localizers to find out?

Comment 15

[Emerson Prado]

I'm a pt-BR localizer.
Does getting the two arguments in a single string means there will be OxW combinations (O = number of possible ordinals, W = ... weekdays)? The only inconvenience, IMHO, is the harassing number of strings.
But there's an advantage too: in Portuguese, there are different genders among weekdays, and this gender changes the ordinal string: "primeiro domingo", "primeira segunda" (first Sunday, first Monday). Integrating arguments lets the localizer readily see each need without having to have more than one string for each ordinal.

Comment 16

[Decathlon]

Thanks for your comment Emerson.

(In reply to comment #15)
> I'm a pt-BR localizer.
> Does getting the two arguments in a single string means there will be OxW
> combinations (O = number of possible ordinals, W = ... weekdays)? The only
> inconvenience, IMHO, is the harassing number of strings.

No, any OxW combinations. With this patch nothing changes compared to the actual situation, you wouldn't have anymore the control over the two elements 'ordinal' + 'weekday' individually (only for the string monthlyNthOfEveryNounclass...), because you will get a unique string with ordinal and weekday together and this isn't a problem for many languages. 

Now you have:
%1$S %2$S of every month -> 'PRIMERA' 'SEGUNDA' de cada mês
                         -> 'PRIMERO' 'DOMINGO' de cada mês

With the patch you would have:
%1$S of every month -> 'PRIMERA SEGUNDA' de cada mês
                    -> 'PRIMERO DOMINGO' de cada mês

where the string 'PRIMERA SEGUNDA' is provided by the code as a unique string (%1$S) but, obviously, it is constructed in the same way with the already known rules about gender (noun class) and ordinal (with article) that every localizer has already written with the others strings in calendar-event-dialog.properties file. No further string has to be added.

For what I've seen for pt-BR but e.g. Italian, Polish, English as well, nothing changes, localizer have only to _delete a parameter (%2$S) and a space_ from two strings inside calendar-event-dialog.properties file.

The problem is that you can't, for example, invert ordinal and weekday order (AFAIK not a problem for pt-BR, it, pl, en):

%1$S %2$S of every month ->  %2$S %1$S .......

or adding a word (e.g. xxx) or a symbol between them (AFAIK again not a problem for pt-BR, it, pl, en):

%1$S %2$S of every month ->   %1$S xxx %2$S .......

or any other thing that might need an ordinal *separated* from weekday for a certain language.

If there is at least a language that needs a such feature for the string 
monthlyNthOfEveryNounclass1 (only for this), I'll have to change the patch.
Do you know if there are languages with this necessity?

I've read the above string for some languages inside l10n-mozilla-1.9.1 and seems that %1$S %2$S always have that order, and there isn't any word in the middle but 
1) I haven't checked every local (for obvious reasons :-));
2) I don't know if every file is updated (because it seems to me that e.g. French local is wrong).

Comment 17

[Decathlon]

PRIMERA -> *PRIMEIRA* 

Sorry Emerson :-(

Comment 18

[Decathlon]

Attachment: patch that handles the rule - v3

I've done a search for the string "monthlyNthOfEvery" on l10n-mozilla1.9.1 and, after, I've searched for all the strings that don't have the sequence "%1$S %2$S" (I had to do it before, sorry).
I've found that four locales need to keep separated the elements "ordinal" and "weekday" for the monthlyNthOfEvery string. Two of them need to invert ordinal with weekday and the others two need to insert some words or characters between: 
  Romanian  http://tinyurl.com/ykq36ye,
 Slovenian  http://tinyurl.com/yf3sn8p,
    Hebrew  http://tinyurl.com/ykzu3la
Vietnamese  http://tinyurl.com/yfln9bn.

I think these files on l10n-mozilla1.9.1 are updated and without errors (for what concerns this patch), therefore the patch must be modified to allow writing the correct sentence in these languages.
Unfortunately, I don't see another way than adding a further string (ordinalWeekdayOrder) which allows to handle the couple "ordinal" + "weekday" before passing the result to the string "monthlyNthOfEvery".

Philipp, what's your opinion? Could it be done in a different manner?
I've also changed the string's name monthlyNthOfEvery, but must it change inside the file calendar/test/calendars/l10n-rrule-details.ics too? 
It would seem useless because in that file it's been used only as a simple text.

Comment 19

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

Comment on attachment 436296 [details] [diff] [review]
patch that handles the rule - v3

>-monthlyNthOfEveryNounclass1=%1$S %2$S of every month;%1$S %2$S of every #3 months
>-monthlyNthOfEveryNounclass2=%1$S %2$S of every month;%1$S %2$S of every #3 months
>+monthlyNThOfEveryNounclass1=%1$S of every month;%1$S of every #2 months
>+monthlyNThOfEveryNounclass2=%1$S of every month;%1$S of every #2 months
Not quite sure if compare-locales will catch case changes. Asking the experts now, will change this before checkin if needed.

I think this patch is fine, I see no better solution right now.

r=philipp

Comment 20

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

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

Comment 21

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

I had to change the string name, Pike told me its best not to change just the case, since localizers will easily overlook that fact.