Closed Bug 502435 Opened 10 years ago Closed 10 years ago

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

Categories

(Calendar :: Dialogs, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bv1578, Assigned: bv1578)

Details

Attachments

(1 file, 5 obsolete files)

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.
Attached patch patch-"rule too complex" (obsolete) — Splinter Review
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.
Attachment #386871 - Flags: review?(ssitter)
OS: Windows XP → All
Is editing such a rule possible? Maybe anyone knows if there already exists a bug report.
Assignee: nobody → bv1578
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Hardware: x86 → All
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.
Attached patch patch that handle the rule (obsolete) — Splinter Review
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.
Attachment #388475 - Flags: review?(ssitter)
Summary: Recurrence summary shows only part of a monthly rule with BYDAY tag and more weekdays → Recurrence summary shows only the first weekday of a monthly rule with BYDAY tag and more weekdays
updated patch after changes in bug 443752.
Attachment #388475 - Attachment is obsolete: true
Attachment #397609 - Flags: review?(ssitter)
Attachment #388475 - Flags: review?(ssitter)
I'll probably not review this patches anytime soon. I would be delighted if someone takes over the reviews.
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.
Attachment #397609 - Attachment description: unbitrotted patch that handle the rule → [after beta] unbitrotted patch that handle the rule
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.
Attachment #386871 - Flags: review?(ssitter) → review-
Comment on attachment 397609 [details] [diff] [review]
[after beta] unbitrotted patch that handle the rule

Same for this one, please update.
Attachment #397609 - Flags: review?(ssitter) → review-
Attached patch patch "rule too complex" - v2 (obsolete) — Splinter Review
Attachment #386871 - Attachment is obsolete: true
Attachment #397609 - Attachment is obsolete: true
Attachment #432791 - Flags: review?(philipp)
Attached patch patch that handles the rule - v2 (obsolete) — Splinter Review
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?
Attachment #432792 - Flags: review?(philipp)
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.
Attachment #432792 - Flags: review?(philipp) → review+
Comment on attachment 432791 [details] [diff] [review]
 patch "rule too complex" - v2

Lets handle the rule instead of ignoring it.
Attachment #432791 - Attachment is obsolete: true
Attachment #432791 - Flags: review?(philipp)
> 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?
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.
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).
PRIMERA -> *PRIMEIRA* 

Sorry Emerson :-(
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.
Attachment #432792 - Attachment is obsolete: true
Attachment #436296 - Flags: review?(philipp)
Component: General → Dialogs
QA Contact: general → dialogs
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
Attachment #436296 - Flags: review?(philipp) → review+
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/fb85020c2128>
-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0b2
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.
You need to log in before you can comment on or make changes to this bug.