295387 - calendar-multiday-view does not display all-day events

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect, P1)

Description

[Stuart Parmenter]

All day events currently show up using the times on the start/end dates instead
of showing up at the top of for the whole duration of the day.

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Load-balancing.  (OMG I AM PDT!!!1!one!)

Comment 2

[Joey Minta]

Reassigning back to default.  From what I can tell, pavlov doesn't have time to get to this.  Anyone else should feel free to pick it up.

Comment 3

[Joey Minta]

Attachment: work in progress

Work in progress.  Should be pretty close to fully functional (including inline-editing) but I want to play around with the css a bit more since it doesn't look very good at this point.  Also, I think there's a bug in the textbox, that makes it expand too far.

Comment 4

[Joey Minta]

Attachment: display (at most 2) all-day events

This patch represents a stage at which I'd be happy landing this patch.  It still has a bug much like bug 312830 in the monnth view.  The box doesn't really have a good way yet of overflowing with lots of events.  

However, this patch is a substantial improvement on the current situation.  We'll now cover the more common use-cases.  Also, I'd like to eventually move most of the code here into a base calendar-editable-item binding that all the other item-displays (in month and multiday view) can inherit from.  This will allow us to easily make the month view have inline editing as well.  This should probably land for some testing before that happens though.

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 206810 [details] [diff] [review]
display (at most 2) all-day events

>Index: mozilla/calendar/base/content/calendar-multiday-view.css
>+calendar-allday-item-box { 
>+  -moz-binding: url(chrome://calendar/content/calendar-multiday-view.xml#calendar-allday-item-box);
>+  margin: 1px;
>+  border: 1px solid #2e4e73;
>+  overflow: hidden;
>+}

Those style rules really should be moved out of this file.

>Index: mozilla/calendar/base/content/calendar-multiday-view.xml

>+  <binding id="calendar-allday-item-box">
>+    <content>
>+<xul:hbox>

indenting. But why have a hbox which contains only a vbox?

>+      <property name="item">
>+        <getter><![CDATA[
>+	  return this.mItem;
>+	]]></getter>
indenting. Looks like tabs...

>+          } else {
>+            //XXX l10n love!
>+            evl.appendChild(document.createTextNode("Untitled Event"));

I think we should try not to create new code with l10n problems. Just do it right from the beginning.

>+          this.setAttribute("class", "calendar-item");
>+          this.setAttribute("item-calendar", val.calendar.uri.spec);
I think you hvae set the class earlier. But why do you need a class at all? the element name says enough.

>+      <method name="stopEditing">
>+              // Note that as soon as we do the modifyItem, this element ceases to exist,
That's an implementation detail of the memory calendar (and thus the ics calendar) IT calls the listeners right away. For the caldav calendar the item will still be there. So try to make the comment less strict, and maybe explain why the element is gone.

>+                      if (!val.startDate.isDate) {
>+                          col.column.selectOccurrence(val);
>+                      } else {
>+                          col.header.selectItem(val);
>+                      }

Why is it called selectOccurrence in one case, and selectItem in they other? Just give them both the same name. They both work on occurences.

>@@ -2196,21 +2460,22 @@
>-              var dayHeaderBox = createXULElement("box");
>-              dayHeaderBox.setAttribute("class", "calendar-event-column-header");
>+              var dayHeaderBox = createXULElement("calendar-header-container");
>               dayHeaderBox.setAttribute("flex", "1");

I know it's existing code, but I really don't like building xul from JS. I'd rather have it written out, in xul. But I guess that's not for this bug.


In general: Why do we need this first, instead of generalizing the editable items first?

Comment 6

[Robin Edrenius]

*** Bug 322358 has been marked as a duplicate of this bug. ***

Comment 7

[Joey Minta]

Given mvl's comment, I've decided to take on the inheritance issue first.  Switching the dependencies around.

Comment 8

[Joey Minta]

Attachment: display all-day events v2

New version using the newly introduced calendar-editable-item.  Still has the same problem as before in that it will only display at most 2 events, but that basically requires fixing bug 312830.

Comment 9

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 210713 [details] [diff] [review]
display all-day events v2

>Index: calendar/base/content/calendar-multiday-view.xml
>               if (estart.isDate) {
>-                  // add it to header
>+                  header.addItem(aEvent);
>               } else {
>                   column.addEvent(aEvent);
>               }

Can you keep the api's of the grid and the all-day header in sync? Then you don't need all those checks, or at least make the only difference be the parent (header or column). In the end, the functions do the same thing, so they should have the same name.
(the same goes for selectOccurrence(null)/unselectItem() and removeItem()/deleteEvent())

Looks good otherwise. r=mvl with this fixed.

Comment 10

[Joey Minta]

patch checked in.