After 10pm local time, a new "All Day Event" covers two days

RESOLVED FIXED in 1.0b3

Status

Calendar
Dialogs
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: benc, Assigned: Decathlon)

Tracking

Sunbird 0.9
1.0b3

Details

Attachments

(1 attachment, 1 obsolete attachment)

3.38 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
STEP: 

I noticed that when you create a new event, it sets the default times to create an event in the next hour, local time.

This means that at 10pm and later, it creates

Start: (today) 23:00
End: (tomorrow) 00:00

When you click on the "All day Event" checkbox, it sets the times to "0:00", but doesn't modify the End date in this edge case. So, you get an "All day Event" that covers two days.
OS: Mac OS X → All
Priority: P4 → --
Hardware: x86 → All

Comment 1

8 years ago
This is also true for the Lightning Add on.

The Reminder Fox addon handles this boundary condition.
(Assignee)

Comment 2

8 years ago
Created attachment 451371 [details] [diff] [review]
patch - v1

I think the same issue occurs when the user wants to transform any event that ends at 0:00 into an all-day event. The code simply changes the "isDate" part of the end date into a true value, in this way the last day becomes part of the all-day event but actually it shouldn't because end at 0:00 is as midnight in the day before, therefore the last but one day should be considered as the new end day when the event becomes all-day (and vice versa when the event turn back to a non all-day event).
This patch subtracts a day to the end date during the conversion to all-day and adds a day in the opposite case when the end time is 0:00.
Assignee: nobody → bv1578
Status: NEW → ASSIGNED
Attachment #451371 - Flags: review?(Mozilla)
Comment on attachment 451371 [details] [diff] [review]
patch - v1

Patch works fine.

Just some nits I'd like to see fixed:
>gOldStartTime.isDate = 0;
please change these lines to "... = False;"
It is like this everywhere else and it is also more readable.

>-                // checkbox "all day" has been unchecked after a date change
>-                let startTimeHour =  gOldStartTime.hour;
>+                // Checkbox "all day" has been unchecked after a date change
Add a period at the end of the sentence

>+                // When we restore 0:00 as end time, we need to add one day to
>+                // the end date in order to include the last day until midnight
same as above.

with this r=markus
Attachment #451371 - Flags: review?(Mozilla) → review+
(Assignee)

Comment 4

8 years ago
Created attachment 457155 [details] [diff] [review]
patch - v2

Patch with nits fixed.
Attachment #451371 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/9a7547ab6ee1>
-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0b3
Keywords: checkin-needed
Attachment #457155 - Flags: review+
You need to log in before you can comment on or make changes to this bug.