8.15 KB, patch
|Details | Diff | Splinter Review|
927 bytes, text/plain
3.47 KB, patch
Sebastian Schwieger: review+
Michael Büttner (no reviews TFN): review+
|Details | Diff | Splinter Review|
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:18.104.22.168) Gecko/20060909 Firefox/22.214.171.124 Build Identifier: Lightning BuildID 2006092220 Thunderbird version 3 alpha 1 20060919 (personal build) There are two basic bugs here. One is detailed by the bug 349788. And the resulting fix made in bug 353797 should be rolled back once this bug is fixed. The other bug that must be addressed here is that because the second icaltime_compare (line 477) does not take into account the fact that the dtend may actually be after the recur until date, you might get extra occurrences in some special cases. (Usually negative offset timezones). The steps for this are below. Reproducible: Always Steps to Reproduce: 1. Set your machine and your Lightning/Sunbird timeozne to America/Chicago. (any negative offset timezone should do). 2. Create a recurring all day event, set it to repeat weekly on Wednesday, Thursday, and Saturday. 3. Set the Repeat Until date to be a *FRIDAY* about three weeks away. 4. Allow the event to be created and check in the view Actual Results: There will be an ocurrence of the event on the Saturday following the "Repeat Until" date you selected. Expected Results: You would not have any occurrences of the event after the Repeat Until date. Note that this fix should also address the failures of bug 349788 and bug 353797. The internal components, specifically calRecurrenceRule::GetOccurrences does not truly handle the case of all-day events versus non-all day events when calculating occurrences for recurring events. The issue is the icaltime_compare done at line 472. If either one of the parameters into this comparison have the is_date attribute set, then the timezone offset will not be applied to that item during the comparison, and an incorrect comparison may result. The items being compared should either BOTH have is_date set to 1 or they should BOTH have is_date set to 0. Then, the comparison will work properly.
Created attachment 239738 [details] [diff] [review] Test patch that shows some of the changes that we will have to make for this change This patch shows some of the changes that dmose and I discussed making in the chat. I still don't have a good grasp on how we would set such an attribute. The places that calRecurrenceItem::isNegative is set are as follows: (grep'd from lxr), RecurrenceInfo.js 537 calrecurrenceDate.cpp 59 calRecurrenceRule.cpp 61 calStorage Calendar 1497 and 1819 WCAPCalendarItems 152 Also, there might be code that needs to be tweaked in calRecurrenceRule::GetNextOccurrence for these changes as well.
It's not clear to me how bad this bug is. I don't know how likely people are to be affected by it, and I am not clear on what all the failure modes are anymore. I'm going to mark [qa discussion needed] to see if we can get some feedback on these two things before nominating it to block 0.5.
(In reply to comment #2) Bug is only reproducible in the case where you have recurring, all-day events AND an RECURUNTIL date that is <= 24 hours from an occurrence of the event. For example, an all day event that occurs on Mon, Wed, Sat from May 1 2007 to Friday May 25 2007 will show an invalid instance on Saturday May 26 2007. However, if you change the Recur Until date to be Thursday May 31, 2007, you will not see any extra events. This is because Thursday May 31, 2007 is more than 24 hours away from the next instance of this occurrence (which would be Saturday June 2, 2007 if the RECUR UNTIL date was not set). Should be rel-noted. The probability is pretty high that people will see this if they use all-day weekly occurring events. If they only have all-day monthly events, then the probability that they will see this is very low.
FWIW, this shows up for me for daily repeating events. I use all day events as a lazy shortcut to track when people are on vacation. I enter an all-day event for the first day someone is out, and then repeat for the rest of the days. Except now everyone shows as being out one extra day! (This is my long winded way of saying that, at least for me, this bug is pretty bad!). Thanks for listening.
Should be resolved for 0.8.
All-day handling (with RRULE) has been fixed with bug 333363; could this one resolved, too?
Clint, could you please comment if this one has been fixed with bug 333363?
Created attachment 294234 [details] test calendar Unfortunately, this bug still exists. I used the attached ics calendar and switched system timezone and Lightning timezone to America/Chicago.
The UNTIL value is actually 20070526T045959Z. That was created using Lightning with these timezones set. If I change that to 20070525T235959Z everything works fine (works also in Lightning v0.7).
Created attachment 295080 [details] [diff] [review] fix This patch - feeds UNTIL datepicker control with floating jsDates - enforces same value type as DTSTART - sets DTSTART's hour/minute/second (unless DTSTART is a date)
Comment on attachment 295080 [details] [diff] [review] fix >- // Convert the datetime from UTC to localtime. >- endDate = endDate.getInTimezone(calendarDefaultTimezone()); >+ endDate = endDate.getInTimezone(gStartTime.timezone); // calIRecurrenceRule::endDate is always UTC or floating ... >+ var endDate = jsDateToDateTime(getElementValue("repeat-until-date"), gStartTime.timezone); Further comment: The datepicker for UNTIL follows DTSTART in the very same timezone, not using calendarDefaultTimezone(). IMO this is ok since UNTIL is typically specified with regards to DTSTART (and it's timezone).
Comment on attachment 295080 [details] [diff] [review] fix [snip] >+ var endDate = jsDateToDateTime(getElementValue("repeat-until-date"), gStartTime.timezone); Shouldn't it be "floating" here?
(In reply to comment #12) > > Shouldn't it be "floating" here? No, it shouldn't. endDate will be converted to UTC in calRecurrenceRule::SetEndDate. r=sebo
Comment on attachment 295080 [details] [diff] [review] fix r=mickey.
Checked in on HEAD and MOZILLA_1_8_BRANCH.
Verified in latest nightly build 20080114 -> task is fixed.