566149 - Last instance of repeated event does not show in calendar display

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Erich Lieth]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 ( .NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.9) Gecko/20100317 Lightning/1.0b1 Thunderbird/3.0.4

When creating a repeated event, the last instance does not appear in the calendar if the start time of the event is before the current time of day at the time of creation.

Reproducible: Always

Steps to Reproduce:
1. Double-click a date on the calendar - New Event dialog opens
2. Move the start time back one hr, e.g. if it defaults to 2pm then set it to 1pm
3. In the Repeat option select 'custom'
4. Select the Repeat Until radio dial and click the down arrow to open the calendar
5. Click on a date further in the future, e.g. one week later. Note that the preview pane indicates the event repeating until and including the date selected.
6. Select 'ok' and then 'save and close'
7. The month view of the calendar shows the event ending on the last instance before the date that was selected in the dialog.
8. Double-click the event to view it and select edit all occurrences.
9. The event correctly indicates the end date originally selected, demonstrating that this is a bug in the display of the calendar, not in the dialog for specifying the event.
Actual Results:  
see above

Expected Results:  
The calendar view should display the last date of the event occurring on the end date selected in the dialog

Comment 1

[Decathlon]

I can confirm this issue. I'm not sure if this is a duplicated because the bug occurs on Lightning 0.9 too.
The event created according to steps to reproduce, seems to have a recurrence rule with an untilDate one hour earlier and this cuts off the last occurrence.

If you open the dialog to create a new event (e.g. double click on month view), the date proposed could be e.g.
2010/07/06 10:00
then you change the time one hour earlier:
2010/07/06 09:00
and set the until date e.g. a week later (2010/06/13), then gStartTime and untilDate that you can find inside "onSave" function
http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog-recurrence.js#362
are:

gStartTime: 2010/07/06 09:00:00 Europe/Berlin isDate=0
untilDate: 2010/06/13 09:00:00 Europe/Berlin isDate=0

and when reading the recRule.untilDate" (line 369) the value is

recRule.untilDate: 2010/06/13 07:00:00 UTC isDate=0

that are all correct, therefore the preview shows the right number of recurrences. 

Though, when you close and save the dialog, the function "onStartDateChange" is called and it changes the untilDate value because it subtracts the hour initially changed in the dialog (see http://mxr.mozilla.org/comm-central/source/calendar/base/src/calRecurrenceInfo.js#813)
so, it results:

ritem.untilDate: 2010/06/13 06:00:00 UTC isDate=0

that seems one hour too early to show the last occurrence too.

Comment 3

[Bernd Reimann]

I suffer from the same bug since at least 0.9 release. 
The problem does not occur, if the repeat function is set before setting the time of the event.

Comment 6

[Decathlon]

Attachment: wip patch (keep the recurrence duration)

This is a work in progress patch. It misses the tasks, I need to verify well if there are particular cases to take into account and if it doesn't break something, but substantially it seems working.
Before going on I ask for feedback because, other than possible flaws, I need to know if the behaviour about the until date in the Edit dialog is what we want .

The current behaviour is to maintain the recurrence duration when the user changes the start date of an event with an until date (by means of the function onStartDateChange), hence the issue is only to fix the bug and adding in the dialog the ability to show the change of the until date with the start date (as currently happens for the end date) that is totally missing. This last part is important otherwise the user doesn't realize that the until date has changed because no explicit action has been done.

The problem is that the exact opposite behaviour has been requested with bug 555645. It asks to maintain the until date when the start date changes. In this case, apart from fixing the current bug, it doesn't need to update the informations about the until date, but only to verify if the start date is earlier than the until date and restore somehow a valid date, that is a different patch compared to this one.
This behaviour may be more intuitive and for sure is preferable by the users that are used to set the until date before the start date.

Stefan, I'm interested in your opinion too, could you please give it a try?

Here there are the builds for all the platforms:
http://ftp.mozilla.org/pub/mozilla.org/calendar/lightning/try-builds/[e-mail]-0d88eb6e412c/

Comment 7

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

Comment on attachment 8632091 [details] [diff] [review]
wip patch (keep the recurrence duration)

Review of attachment 8632091 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Decathlon from comment #6)
> The problem is that the exact opposite behaviour has been requested with bug
> 555645. It asks to maintain the until date when the start date changes. [...]
> This behaviour may be more intuitive and for sure is preferable by the users
> that are used to set the until date before the start date.

Maybe we can go for a hybrid solution. I'm not sure how it would feel, but what if you definitely keep the "date" part of the UNTIL date even if the start date/time changes, but always change the "time" part of the UNTIL date to match the start time in the event dialog? This way, the date is left intact and the user in bug 555645 is happy, but there are no strange missing occurrences because the until date is an hour wrong.

Comment 8

[Decathlon]

(In reply to Philipp Kewisch [:Fallen] from comment #7)
 
> Maybe we can go for a hybrid solution. I'm not sure how it would feel, but
> what if you definitely keep the "date" part of the UNTIL date even if the
> start date/time changes, but always change the "time" part of the UNTIL date
> to match the start time in the event dialog? This way, the date is left
> intact and the user in bug 555645 is happy, but there are no strange missing
> occurrences because the until date is an hour wrong.

Yes, a correct time part was tacit in order to prevent the bug (that anyway occurs also when changing the date and not only the time), the problem is the different way to treat the until date when the start date changes.
If we want to do as requested in bug 555645 it should be a feedback- to this patch ;)
But I agree, it can be more intuitive because once the user sets the until date, this doesn't change by itself when changing others dates, only a warning is required if the start and until date are not in the right order.

Comment 9

[Decathlon]

Attachment: keep the until date - v1

This patch makes the until date entered in the datepicker (or in the recurrence dialog) a date that doesn't change when the user changes the start date, apart from its time part.

With this approach the user can run into a warning "The until date occurs before the start date" when he changes the *start* date. To restore valid dates after the warning dialog, I've changed the *until* date into a date equal to the start date.
The alternative would be to restore the old start date, but in this way the workflow would be annoying because it would require to set the until date before the start date.
The real solution for this problem (and also for the end date before the start date) is to get rid of the warning dialog. We need to decide about the behavior and the look of the UI in such cases though.

I've used only one message in the warning dialog for start/until date incompatibility. Let me know if you think it should be different when the incompatibility is caused by the until date or by the start date.

When testing please verify also dates with timezones and allday events which  before didn't generate always a correct Until date (e.g. when checking the "all day" checkbox after the until date).

Here the builds from the try-server (the missing css style in the timepickers doesn't depend on the patch)
http://ftp.mozilla.org/pub/calendar/lightning/try-builds/[e-mail]-f43938854999710c523c09138bcd4781081b3886/

Comment 10

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

Comment on attachment 8703452 [details] [diff] [review]
keep the until date - v1

Review of attachment 8703452 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the following changes.

::: calendar/base/content/dialogs/calendar-event-dialog.js
@@ +238,5 @@
>      window.calendarItem = item;
>      // store the initial date value for datepickers in New Task dialog
>      window.initialStartDateValue = args.initialStartDateValue;
> +    // Store the initial start date for possible untilDate correction.
> +    window.startDateStored = item[calGetStartDateProp(item)];

Please make sure this works even after clicking "save", as this line is not called when just the save button is pressed.

I'm not too happy about a new global, especially since it has a similar name as initialStartDateValue, but if there is no other way then it is ok.

@@ +1071,5 @@
>  function updateUntilControls(rule) {
>      let untilDate = "forever";
>      if (!rule.isByCount) {
> +        if (rule.isFinite) {
> +            gUntilDate = rule.untilDate.clone().getInTimezone(calendarDefaultTimezone());

cal.calendarDefaultTimezone()

@@ +2712,5 @@
> +        let rrules = splitRecurrenceRules(recurrenceInfo);
> +        recRule = rrules[0][0];
> +    }
> +    let defaultTimezone = cal.calendarDefaultTimezone();
> +    let repeatDeck = document.getElementById("repeat-deck").selectedIndex;

Using selectedItem and then an attribute with the name would avoid using indexes and ensure changing the order won't affect this code.

Comment 11

[Decathlon]

Attachment: keep the until date - v2

(In reply to Philipp Kewisch [:Fallen] from comment #10)
> > +    // Store the initial start date for possible untilDate correction.
> > +    window.startDateStored = item[calGetStartDateProp(item)];
> 
> Please make sure this works even after clicking "save", as this line is not
> called when just the save button is pressed.

Yes, it worked because the variable was set again in another place just after the start date was saved.
Anyway, with this new patch, it doesn't matter anymore.

> 
> I'm not too happy about a new global, especially since it has a similar name
> as initialStartDateValue, but if there is no other way then it is ok.

I've double checked the file and it seems that the start date in the item is set only inside the function saveDateTime(), so the item's start date that we find there is always the last saved one (or the initial date just after opening the dialog), hence there is no need to store an intermediate value with a global variable.


> > +    let defaultTimezone = cal.calendarDefaultTimezone();
> > +    let repeatDeck = document.getElementById("repeat-deck").selectedIndex;
> 
> Using selectedItem and then an attribute with the name would avoid using
> indexes and ensure changing the order won't affect this code.

Thanks. I've used the menulist item-repeat instead of the deck and it's much better now.

Comment 12

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

Comment on attachment 8706999 [details] [diff] [review]
keep the until date - v2

Review of attachment 8706999 [details] [diff] [review]:
-----------------------------------------------------------------

I like this patch very much, great work!

::: calendar/base/content/dialogs/calendar-event-dialog.js
@@ +1222,5 @@
>  
>  /**
> + * Changes the until date in the rule in order to compensate the automatic
> + * correction caused by the function onStartDateChange() when saving the
> + * item. 

A whitespace snuck in here.

Comment 13

[Decathlon]

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/9bd34a64dcf2

Comment 16

[[:MakeMyDay]]

Do you want this for 4.7, too? If so, it would need a quick approval, because it contains a string change. The other late l10n bugs already landed on aurora, but I haven't made an announcement on m.d.l10n yet.

Comment 17

[Decathlon]

(In reply to MakeMyDay from comment #16)
> Do you want this for 4.7, too?

Absolutely no.
This needs to be tested and verified as much as possible. It contains some little extra fix that I made just because I discovered something wrong while testing.