Closed Bug 298349 Opened 15 years ago Closed 14 years ago

Need to support events (not all-day) that span multiple days

Categories

(Calendar :: Lightning Only, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Lightning 0.1

People

(Reporter: shaver, Assigned: jminta)

References

Details

Attachments

(3 files, 1 obsolete file)

Right now we only show them on the first day, I think.
*** Bug 295388 has been marked as a duplicate of this bug. ***
Priority: -- → P2
Target Milestone: --- → Lightning 0.8
Attached patch patch v1Splinter Review
This ought to get things going for events spanning multiple days in both views.  There's also some minor cleanup in the month-view, because I couldn't do proper testing without having a working observer and proper delete calls.

Conflicts with Sunbird's new-views patch.
Assignee: vladimir → jminta
Status: NEW → ASSIGNED
Attachment #204036 - Flags: first-review?(dmose)
Blocks: 321164
Comment on attachment 204036 [details] [diff] [review]
patch v1

This needs to updated to dodge the new-views conflicts as discussed on IRC.
Attachment #204036 - Flags: first-review?(dmose)
I'm seeing a somewhat similar problem, with the new views, but for all-day events. I have separate all-day events, where "All day" *is* set, and yet the new multi-week view is only showing me the first day. Hovering over the event shows the correct span in the popup.

Is that a different bug I'm seeing (which I need to log) or part of this one?
apologies: I'm talking about Sunbird trunk, in case that wasn't obvious.

Should I log a new bug for this?
QA Contact: shaver → lightning
Attached patch patch v2Splinter Review
Updated patch after all the new-views landing.  Pretty much the same changes as before, though.  In reference to comment #4 and comment #5, this patch fixes all the views, including multiweek/month.
Attachment #207524 - Flags: first-review?(dmose)
right, but does it fix all-day events too? The Summary suggests otherwise, but I'm seeing this bug with all-day events.
(In reply to comment #7)
> right, but does it fix all-day events too? The Summary suggests otherwise, but
> I'm seeing this bug with all-day events.
> 
In the month/multiweek views all-day events are treated identically to normal-time events, so yes, it fixes this issue with regards to all-day events in those views as well.  We need Bug 295387 to fix all-day events in the day/week views, since they are handled differently than normal-time events there.
gotcha, thanks Joey.
Comment on attachment 207524 [details] [diff] [review]
patch v2

I'd like to see this split into two patches (maybe even two bugs?).  In particular, I'm not entirely sold that we want to add extra boxes in the month view, and I'd like to solicit more feedback about that before reviewing it.  As an example, if I go to a party from 10:00 PM that overflows until 2:00 AM the next morning, I don't particularly want to see the box in the next day.  Although perhaps we need to have some sort of "virtual day" preference that can overflow midnight for those of us who are night owls.  Perhaps beltzner has some thoughts here...

The multiday-view piece of the fix looks good, but needs a few minor tweaks.

>Index: mozilla/calendar/base/content/calendar-multiday-view.xml
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/base/content/calendar-multiday-view.xml,v
>retrieving revision 1.48
>diff -p -U8 -r1.48 calendar-multiday-view.xml
>--- mozilla/calendar/base/content/calendar-multiday-view.xml	26 Dec 2005 16:29:07 -0000	1.48
>+++ mozilla/calendar/base/content/calendar-multiday-view.xml	4 Jan 2006 19:06:33 -0000
>@@ -520,16 +520,26 @@
>            var enddate = aOccurrence.endDate;
> 
>            if (stdate.timezone != this.mTimezone)
>                stdate = stdate.getInTimezone (this.mTimezone);
> 
>            if (enddate.timezone != this.mTimezone)
>                enddate = enddate.getInTimezone (this.mTimezone);
> 
>+           // Handle cases where an event begins or ends on a day other than this
>+           if (stdate.compare(this.mDate) == -1) {
>+               stdate.hour = 0;
>+               stdate.minute = 0;
>+           }
>+           if (enddate.compare(this.mDate) == 1) {
>+               enddate.hour = 23;
>+               enddate.minute = 59;
>+           }
>+

On events, at least, start dates are inclusive and end dates are non-inclusive.  So at some level, maybe this wants to be 23:60 or 24:00.  I guess it really depends on how the various callers treat the returned values.

Also, can you add a doxygen comment to the top of this function explicitly stating that it has side-effects (ie may modify the dates on the occurence passed in)?

>-      <method name="findColumnForItem">
>+      <method name="findColumnsForItem">
>         <parameter name="aItem"/>
>         <body><![CDATA[
>-          var estart;
>-          if(aItem.startDate) 
>-              estart = aItem.startDate;
>-          //XXX This isn't really good yet.  It may need to be changed once
>-          //    tasks are fully implemented.
>-          if(aItem.entryDate)
>-              estart = aItem.entryDate;
>-          if(!estart)
>-              return null;
>+          // We don't support tasks here, yet
>+          if (!(aItem instanceof Components.interfaces.calIEvent))
>+              return;
>+
>+          var columns = new Array();
>+          var targetDate = aItem.startDate.clone();
>+          var finishDate = aItem.endDate;
>+
>+          while (targetDate.compare(finishDate) == -1) {

It looks to me as though the body of this loop won't execute for single day events where both the startDate and endDate items have isDate set.  Changing from |while| to |do ... while| might fix this.  I'd also suggest adding a comment pointing out that targetDate and finishDate may or may not in fact be dates (ie have isDate set); that confused me on first reading).

Does drag-n-drop behave reasonably on multi-column events?  If not please file another bug to track that when you land this.

r- on the full patch, conditional r+ on the multiday-view chunk with the issues mentioned above addressed.

Thanks!
Attachment #207524 - Flags: first-review?(dmose) → first-review-
(In reply to comment #10)
> (From update of attachment 207524 [details] [diff] [review] [edit])
> I'd like to see this split into two patches (maybe even two bugs?).  In
> particular, I'm not entirely sold that we want to add extra boxes in the month
> view, and I'd like to solicit more feedback about that before reviewing it.  As
> an example, if I go to a party from 10:00 PM that overflows until 2:00 AM the
> next morning, I don't particularly want to see the box in the next day. 
> Although perhaps we need to have some sort of "virtual day" preference that can
> overflow midnight for those of us who are night owls.  Perhaps beltzner has
> some thoughts here...

For 0.1 I'm thinking we can add a hidden pref with that ignored events overflowing from the previous day that end before a particular hour, ie:

calendar.overflow.hidehour = 6 means that events that overflow from the day before, but end before 6am will not be shown.  I'd also suggest that this be set to 0 by default, to avoid the semi-dataloss that comes with events not being shown.  The 0 behavior is what Sunbird users will expect, since it's effectively how Sunbird has been functioning since prior to 0.2.

> It looks to me as though the body of this loop won't execute for single day
> events where both the startDate and endDate items have isDate set.  Changing
> from |while| to |do ... while| might fix this.  
That's essentially an invalid (or at least a 0-time) event.  All-day events have exclusive endDates, meaning that the end date would be the next day, and hence the loop would iterate once.

> 
> Does drag-n-drop behave reasonably on multi-column events?  If not please file
> another bug to track that when you land this.
Will do.

> 
> r- on the full patch, conditional r+ on the multiday-view chunk with the issues
Separate patches coming in the next few days.

Multiday-view part checked in.  Adding 'uiwanted' for the month-view part.  Basic question for the UI-gurus: Suppose we have an event (a party or a night-shift) lasting from 10pm until 2am.  Should the event be shown on the second day in the month view?  dmose feels it may crowd things.  Previous versions of Sunbird did display the event on the second day.
Keywords: uiwanted
Whiteboard: [uiwanted for comment #12]
I'm not sure if you want user votes, apols for the bugspam if not, but I would prefer not to see the event on the 2nd day. I use multiweek as my "regular" view, and I don't need to see "last night's" events in the morning when I check my schedule for the day.

A pref - a la calendar.overflow.hidehour - would suit me perfectly, thanks.
Attached patch patch for calendar-month-view (obsolete) — Splinter Review
(I'm getting way too familiar with these pref observers).  This patch does everything the previous patch did for the month view, but also includes a hidden preference to handle the case that concerned dmose before.  The preference defaults to 0, but if set to a higher number, multiday events that end before that hour will not be shown on the last day.  Examples make this a bit more clear:

Event from 1/12 (10pm) to 1/13 (2am):
  -If pref = 3, event is shown on 1/12, but not on 1/13
  -If pref = 0, event is shown on both 1/12 and on 1/13

Event from 1/13 (1am) to 1/13 (2am):
  -Event is not multiday. It is always shown.

Event from 1/12 (all-day) to 1/13 (all-day/inclusive):
  -Event is always shown on both days.

(also works in a similar way with tasks)
Attachment #208882 - Flags: first-review?(dmose)
Preferences have a non-zero cost, as was learned from the Suite.  Id be very curious to know how other calendaring software handles this case...
OK, I've been convinced that a pref is indeed the right thing.
Would it be better to generalize this preference to allow the views to break at a time other than midnight, so they can be used by shift workers?  Currently the view can only show midnight to midnight, so it is impossible to show a 'day' that overlaps midnight.  

With an view.day.break.offset.hours preference, if an evening shift worker has a schedule from say, 6pm to 2am, they can see the shift's events in one view by setting the day-break to some time 3am or later (+3 hours).  Similarly, a night shift worker that has a schedule from say, 10pm to 6am, might set the day-break at 9pm (-3 hours).  

(This may also be popular with students who organize their study blocks or study groups to include some of the quiet hours after midnight, and in places where the oppressive heat of the day means many people work better/more-cheaply at night.)

With a view.day.break.offset.hours preference, the "overflowhidehour" preference would not be necessary.
*** Bug 324367 has been marked as a duplicate of this bug. ***
(In reply to comment #17)
> Would it be better to generalize this preference to allow the views to break
> at a time other than midnight, so they can be used by shift workers?

Yes, I think you're right that this is a better long-term fix.  After discussion with Joey, the tentative plan is land a new version of his most recent patch _without_ a pref for now, and to file another bug to allow for different day breaking times not too far in the future.
Comment on attachment 208882 [details] [diff] [review]
patch for calendar-month-view

As per previous discussion, we need a new patch without a pref.
Attachment #208882 - Flags: first-review?(dmose) → first-review-
Without the pref.
Attachment #208882 - Attachment is obsolete: true
Attachment #210091 - Flags: first-review?(dmose)
I believe we're agreed that comment 17 is the right long-term plan, and that obviates the UI question here, so removing the keyword/status stuff about that.
Keywords: uiwanted
Whiteboard: [uiwanted for comment #12]
Comment on attachment 210091 [details] [diff] [review]
patch for calendar-month-view v2

>+          // All our boxes are in default tz, so we need these times in them too.
>           if (aItem instanceof Components.interfaces.calIEvent) {
>-            targetDate = aItem.startDate;
>+            targetDate = aItem.startDate.getInTimezone(this.mTimezone);
>+            finishDate = aItem.endDate.getInTimezone(this.mTimezone);
>           } else if (aItem instanceof Components.interfaces.calITodo) {
>-            targetDate = aItem.startDate || aItem.entryDate;
>+            if (aItem.entryDate) {
>+              targetDate = aItem.entryDate.getInTimezone(this.mTimezone);
>+              if (aItem.dueDate) {
>+                finishDate = aItem.dueDate.getInTimzone(this.mTimezone);

This wants to be getInTimezone(), I bet.  Please test this codepath before checking in.

r=dmose@mozilla.org with the typo fix.
Attachment #210091 - Flags: first-review?(dmose) → first-review+
Summary: Need to support events (not all-day) that span multiple days, in multiday-view → Need to support events (not all-day) that span multiple days
New hard drive, patch checked in, finally closing this bug :-)
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
(In reply to comment #19)
> file another bug to allow for
> different day breaking times

bug 235678 filed.

Comment #25, should be bug 325678.
You need to log in before you can comment on or make changes to this bug.