Closed Bug 388656 Opened 17 years ago Closed 16 years ago

DATE values must not specify a TZID

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: dbo, Assigned: dbo)

References

Details

Attachments

(2 files)

Section 3.8.3.1 (Time Zone Identifier) of <http://www.ietf.org/internet-drafts/draft-ietf-calsify-rfc2445bis-07.txt> specifies that TZID must only be specified on properties of value type DATE-TIME or TIME, but not DATE. Our products currently write a TZID for DATE values.
Flags: blocking-calendar0.7?
Depends on: 398724
Flags: blocking-calendar0.7? → wanted-calendar0.8+
Version: Sunbird 0.5 → unspecified
I am unsure whether we should remove a TZID param from existing DATE properties: strictly spoken this is dataloss. But we should omit writing a TZID when the user changes an event/task to be all-day in the event/task dialog.
Assignee: nobody → michael.buettner
Flags: wanted-calendar0.8+ → blocking-calendar0.8+
No problem with deleting the TZID from the DATE when changing events as we adhere to the spec and the behaviour stays the same. However, if you don't have a tzid on the all-day event (DATE) and the event gets changed to a non-allday-event I think the event might go floating while it should jump to the TZ from the preferences (or even th epreviously assigned TZID like it does now). Didn't try this yet though, this might change since with the foreign timezone fix. 
(In reply to comment #2)
> non-allday-event I think the event might go floating while it should jump to
> the TZ from the preferences (or even th epreviously assigned TZID like it does
> now). Didn't try this yet though, this might change since with the foreign
Yes, makes sense. Non-Allday events should get a sensible timezone by default.
What's the prospect for a patch on this little monster? Are we going tohave to change libical or are we going to enforce this at the CalDateTime layer?
I propose that we leave data "as is" unless the user changes an item. That said I think it's event/task dialog work (only).
Assignee: michael.buettner → daniel.boelzle
The "TZID" property parameter MUST NOT be applied to DATE properties, and DATE-TIME or TIME properties whose time values are specified in UTC.

is specifically stated in rfc2445-bis07 so I suggest dropping the TZID and adding the default whenever an event has a DATE-TIME or TIME without a corresponding timezone (we want to avoid floating events I guess)
Bas, you're right, but please think about calendar interop with applications that may still interpret the TZID in some way. Being defensive, I'd just strip the TZID from DATEs when actually edited.
Attached patch fixSplinter Review
Attachment #299189 - Flags: review?(michael.buettner)
Comment on attachment 299189 [details] [diff] [review]
fix

>+function onUpdateAllDay() {
updateAllDay() is the dedicated handler for the allday-checkbox, there's no need to introduce another function that calls updateAllDay(). I suggest to merge the body of this new function into updateAllDay().

>+    if (!isEvent(window.calendarItem)) {
>+        return;
>+    }
This check will be performed in updateAllDay(), no need to do that twice.

>+    var allDay = getElementValue("event-all-day", "checked");
Same here, already contained in updateAllDay().

>+    gStartTimezone = (allDay ? floating(): calendarDefaultTimezone());
>+    gEndTimezone = gStartTimezone;
>+    gStartTime.timezone = gStartTimezone;
>+    gEndTime.timezone = gEndTimezone;
These four lines should just be moved into updateAllDay(), that's it.

Apart from the above mentioned minor restructuring the patch looks just fine. r=mickey.
Attachment #299189 - Flags: review?(michael.buettner) → review+
Mickey, please rethink your review.
Initially I've added the code to updateAllday(), but it regressed other stuff. updateAllday() is called from lots of other update functions throughout the file, even recursively (controlled via that weird global gIgnoreUpdate variable, control flow is quite hard to predict IMHO).
So I moved it into that new function which is called on actual user action *only*.
Just because the call flow is probably not immediately obvious nor trivial this isn't a good reason for taking the easy road. Feel free to ignore my review, but I carefully thought about what I wrote in my previous comment.
Attached patch suggestionSplinter Review
Is this what you've proposed? I respect your input, but it doesn't WFM since a all events' timezones will be forced to calendarDefaultTimezone() in the event dialog.
I don't exactly understand your previous comment. Did you mean that with the proposed patch the timezone will always be set to calendarDefaultTimezone()? This shouldn't be the case and I can't find a location that unconditionally revert to the default timezone. Dumb question, but are we talking about the timezone option being enabled or disabled? If it's enabled all timezones should be taken from gStart/EndTimezone, otherwise we always default to the default timezone. At least the proposed patch is what I had in mind previously... :-)
(In reply to comment #13)
> I don't exactly understand your previous comment. Did you mean that with the
> proposed patch the timezone will always be set to calendarDefaultTimezone()?
Yes. It's displayed that way even if the event's timezone is different.

> This shouldn't be the case and I can't find a location that unconditionally
> revert to the default timezone. Dumb question, but are we talking about the
> timezone option being enabled or disabled? If it's enabled all timezones should
> be taken from gStart/EndTimezone, otherwise we always default to the default
> timezone. At least the proposed patch is what I had in mind previously... :-)
mabye it's triggered via updateCalendar()
Checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Checked in latest nightly build -> task is fixed and verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.