Closed Bug 295387 Opened 15 years ago Closed 14 years ago

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

Categories

(Calendar :: Lightning Only, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Lightning 0.1

People

(Reporter: pavlov, Assigned: jminta)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Assignee: shaver → vladimir
Load-balancing.  (OMG I AM PDT!!!1!one!)
Assignee: vladimir → pavlov
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → Lightning 0.8
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.
Assignee: pavlov → dmose
Status: ASSIGNED → NEW
QA Contact: shaver → lightning
Summary: All day events not displayed properly in lightning → calendar-multiday-view does not display all-day events
Assignee: dmose → nobody
OS: Windows XP → All
Hardware: PC → All
Blocks: 321164
Attached patch work in progress (obsolete) — Splinter Review
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.
Assignee: nobody → jminta
Status: NEW → ASSIGNED
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.
Attachment #206764 - Attachment is obsolete: true
Attachment #206810 - Flags: first-review?(mvl)
Blocks: 321546
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?
Attachment #206810 - Flags: first-review?(mvl) → first-review-
*** Bug 322358 has been marked as a duplicate of this bug. ***
Given mvl's comment, I've decided to take on the inheritance issue first.  Switching the dependencies around.
No longer blocks: 321546
Depends on: 321546
No longer depends on: 321546
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.
Attachment #206810 - Attachment is obsolete: true
Attachment #210713 - Flags: first-review?(mvl)
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.
Attachment #210713 - Flags: first-review?(mvl) → first-review+
patch checked in.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.