395883 - Readonly dialog says: "Repeat: Occurs ???" on yearly and monthly recurring items.

Status: VERIFIED FIXED

(Calendar :: General, defect)

Description

[Ingo Müller]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
Build Identifier: Lightning Nightly Build from 2007-09-11

The recurrence information is not displayed correctly in read only calendars (with their typical read only dialog) for items that reccure on a monthly or yearly basis. Other intervals seem to work fine and say for example "Repeat: Occurs every week".

Reproducible: Always

Steps to Reproduce:
1. Create a new event in a calendar, choose to repeat it monthly or yearly.
2. Set this calendar to read-only.
3. Double-click on the event, then choose "All occurences" or "This occurrence only".
Actual Results:  
The dialog says: "Repeat: Occurs ???"

Expected Results:  
The dialog should say: "Repeat: Occurs yearly" or "Repeat: Occurs monthly"

Comment 1

[Martin Schröder [:mschroeder]]

Confirmed with Lightning 0.7pre (2007091106) on WinXP.

Comment 2

[Ingo Müller]

I did a bit of research of the problem of this bug. I think it's the commonUpdateRepeatDetails function at http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-dialog-utils.js#180. When the type of the recurrence rule is set to MONTHLY or DAILY, it expects the components BYMONTHDAY or BYDAY, or BYMONTH and BYMONTHDAY or BYDAY respectively to be set, too. If these components are not set as expected, the string set to "???" at the beginning is not modified.

I think, simple recurrence rules don't need to have these components, as this information can be computed with the start date. One possible solution is to compute these components, if they are not present, before commonUpdateRepeatDetails checks them. I'll submit a patch soon which does this.

I guess, that the function also returns "???", if the recurrence type is set to null/"", "SECONDLY", "MINUTELY" or "HOURLY". Maybe it's worth spending some thoughts about this, too.

Comment 3

[Ingo Müller]

Attachment: Sets BYMONTHDAY and BYMONTH, if not set, according to the start date.

This is a patch against http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-dialog-utils.js?raw=1 of 2007-09-17 or the nightly build from that date. This is my second patch. I used "diff -pu8" this time, as I was told. I hope that's the right flags. I also requested review (I think) as I was told. Just tell me, if I did something wrong...

Comment 4

[Martin Schröder [:mschroeder]]

Comment on attachment 281177 [details] [diff] [review]
Sets BYMONTHDAY and BYMONTH, if not set, according to the start date.

Ingo, you have to ask a specific developer for review (see http://wiki.mozilla.org/Calendar:Module_Ownership). I did that for you, and I think no ui-review is needed.

Comment 5

[Michael Büttner (no reviews TFN)]

Comment on attachment 281177 [details] [diff] [review]
Sets BYMONTHDAY and BYMONTH, if not set, according to the start date.

>+                if (!checkRecurrenceRule(rule, ['BYDAY','BYMONTHDAY'])) {
>+                    rule.setComponent('BYMONTHDAY', 1, [startDate.day]);
>+                }
>                 if (checkRecurrenceRule(rule, ['BYDAY'])) {
You introduce a 'BYMONTHDAY' rule if it's not available just to unconditionally enter the immediately following evaluation function. The problem is that the separate evaluations are intended to each have their very own possibility to create the resulting string. By injecting recurrence rules on the fly you cut off the possibility to create a string like "Occurs yearly" instead of "Occurs every September 18...". It strikes me as if the 'catch all' branch for these 'if'-statements is missing. In other words, I would prefer to add an 'else' branch to the evaluation rules in order to produce the missing strings. This would allow to later easily add the much shorter forms such as "Occurs yearly" after 0.7 is out of the door.

Comment 6

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

This patch adds the missing 'catch all' branch to the evaluation functions as suggested in my previous comment. This still doesn't take care of all possible recurrence rules, but at least it doesn't bail out on the most basic forms.

Comment 7

[Ingo Müller]

Great! Your patch is a lot cleaner than mine. For me, it solves the issue. The other BYxxx ("MINUTELY" , "HOURLY", ...) don't need a catch-all branch as the according xxxLY ("MINUTELY", "HOURLY", ...) are not yet supported, right?

Comment 8

[Martin Schröder [:mschroeder]]

Comment on attachment 281311 [details] [diff] [review]
patch v1

Tested. Works as advertised.
r=mschroeder

Comment 9

[Michael Büttner (no reviews TFN)]

(In reply to comment #7)
> Great! Your patch is a lot cleaner than mine. For me, it solves the issue. The
> other BYxxx ("MINUTELY" , "HOURLY", ...) don't need a catch-all branch as the
> according xxxLY ("MINUTELY", "HOURLY", ...) are not yet supported, right?
Right, at least for now. We definitely need to revise this whole function in terms of handling any kind of exotic rule that might gets thrown at it (from some foreign ics file) and in terms of localization until we reach 1.0. But that's a story for some other day. Thanks for confirming that the patch works (of course, this applies to martin, as well).

Comment 10

[Michael Büttner (no reviews TFN)]

patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED

Comment 11

[Omar Bajraszewski]

Verified with lightning 2007092403