Closed Bug 389854 Opened 17 years ago Closed 17 years ago

Today-pane: implement agenda-pane

Categories

(Calendar :: Calendar Frontend, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: berend.cornelius09, Assigned: berend.cornelius09)

References

Details

Attachments

(5 files, 3 obsolete files)

This issue is a follow-up to issue 388414, where the miniday-pane is about to be implemented. The pane is specified in http://wiki.mozilla.org/Calendar:Improving_the_Calendar_Views.
This issue shall implement only the agenda-pane in all its facets. One aspect that is currently not dealt with in the specification is that within the agenda-pane the periods tomorrow, and soon are only to be displayed when the actual date of the pane is really "today" as displayed in the miniday-pane on top of the agenda-pane.
this patch has been made obsolete with the issue 385900 and 388414 and cannot be applied any longer. However it contains a lot of sourcecode fragments that I will make use of when resolving this issue.
Assignee: nobody → Berend.Cornelius
Status: NEW → ASSIGNED
Target Milestone: 0.7 → ---
Version: Lightning 0.5 → unspecified
One issue of the current agenda-pane is that it only displays event with an enddate later that the current time. This may be Ok when the today-pane is in "showsToday" mode but otherwise the reference time should be the beginning of the day.
Flags: blocking-calendar0.8?
Flags: blocking-calendar0.8? → wanted-calendar0.8+
I'm not sure I agree that hiding events earlier than the current time in "showsToday" mode is such a good idea -- see bug 400736 comment 2 for my reasoning.  (Regardless of the UI choice, the display behavior needs to be based on something less random than the launch time of TB, as in that bug!)  I think showing completed events in a "grayed out" way is preferable to hiding them entirely, since the user isn't surprised by apparent loss of data.
Attached patch patch v.1 (obsolete) — Splinter Review
My implementation follows the ideas in bug 394054, respectively the comment #2 and comment #3. This means that:
- tasks are no longer displayed in the agenda-pane.
- events with an enddate earlier than now are not shown in the agenda pane if the today-pane is in "Today-mode".
- If the today-pane is not in "today-mode" meaning the set date is unequal to today only a one day- period is shown in the agenda-pane. In "today-mode" we still display "today", "tomorrow" and "Next week".
- All current events are displayed with a light blue background. Christian and me spent some time and tried various combinations to depict this and finally found that this is the most reasonable way. Once the current events are not current anymore they are removed from the list.
- Allday events which duration covers multiple periods are also displayed in each period unlike in the current today-pane.
Basically the new agenda-pane is using a richlistbox as this widget is offering more layout options than a tree control. We hope that the users will like the new layout.

Some remarks about the implementation:
As discussed with Christian I am using the calendar-month-day-box-item binding to display allday events in the agenda-pane. Currently this binding expects a calendar-view which the agenda-pane cannot deliver. I'd like to change this in a follow-up bug. In this issues I modified that binding very minimalistically, so that I could use it for my purpose. I am aware that clicking on an allday-event causes an error in "calendar-view-core, line 256". I'd like to fix this in that follow up bug too.

>  var calcolor = "";
>  try {
>     calcolor = getCalendarManager().getCalendarPref_(item.calendar, "color");
>  } catch (e) {
> }
> if (!calcolor) {
>   calcolor = "#a8c2e1";
> }

Setting the default calendar color hardcoded in the source code will certainly meet the reviewer's attention. As this has been done in a similar way several times on other places in the project I would like to work out a clean solution in a follow-up bug, too.
Attachment #274179 - Attachment is obsolete: true
Attachment #291739 - Flags: ui-review?(christian.jansen)
Attachment #291739 - Flags: review?(philipp)
Berend, could you please post a screenshot of your current implementation.
Overall I'm impressed how good the new agenda works.

Some minor things:

- change gray value of the dotted separator lines to #C0C0C0
- change gray value of the line under "Today, Tomorrow, Next Week" to #A7A6AA
- remove unnecessary 2px border (see attachment)
- Would be possible to shorten very long event title by using ellipses
  instead of displaying a horizontal scrollbar? 

I'll give this bug an ui+, but it would be great if you could change the things listed above within this patch. -> r=christian



Things to change/improve in spin-off bugs

- Allow customization of the Agenda Pane
- Allow multi selection of events
- Improve Context Menu
- Improve UI of All Day Events

- It might make sense to split "Next Week" into two sections:
  - "This Week", and
  - "Next Week"

Assume today is Monday. Events which scheduled for the day after tomorrow, in that case Wednesday, would be listed in the section "Next Week". Which is wrong. 

From my point of view it would make sense to list this event in a section called "This week". For this case "This Week" would contain all events from Wednesday till Sunday. "Next week" would contain only  events scheduled for the upcoming week.
Comment on attachment 291739 [details] [diff] [review]
patch v.1

See Comment #8
Attachment #291739 - Flags: ui-review?(christian.jansen) → ui-review+
Attached patch patch v.2 (obsolete) — Splinter Review
In the attached I fixed some issues that Andreas had fallen into.
Attachment #291739 - Attachment is obsolete: true
Attachment #291739 - Flags: review?(philipp)
Attachment #292575 - Flags: review?(philipp)
Comment on attachment 291739 [details] [diff] [review]
patch v.1

>       <property name="occurrence">
>         <getter><![CDATA[
>           return this.mOccurrence;
>         ]]></getter>
These lines should be formatted as shown above


>+              var timezone = null;
>+              if (this.calendarView != null) {
>+                timezone = this.calendarView.mTimezone;
>+              }
>+              else {
>+                timezone = calendarDefaultTimezone();
>+              }
Can be shortened to:
var timezone = (this.calendarView ?
                this.calendarView.mTimezone :
                calendarDefaultTimezone())

If you don't like that, at least put the else on a line with its brackets (i.e } else {)


>+      <method name="removeShadows">
>+        <body>
>+          <![CDATA[
>+          removeAnonymousElement(this, "shadow-left-image");
>+          removeAnonymousElement(this, "shadow-right-image");
>+          removeAnonymousElement(this, "shadow-container");
>+          ]]>
>+        </body>
>+      </method>
Same comment as above with the getter, see style guide. Aside from that, does removeShadows permanently remove the xul elements? It would probably be sleek if the tag had a shadows attribute that hides the shadows or not, i.e

<calendar-month-box shadows="false"/>

> function toggleCompletedTasks() {
>-    const kSUNBIRD_ID = "{718e30fb-e89b-41dd-9da7-e25a45638b28}";
>-    var appInfo = Components.classes["@mozilla.org/xre/app-info;1"]
>-                            .getService(Components.interfaces.nsIXULAppInfo);
>-    if (appInfo.ID != kSUNBIRD_ID) {
>-        agendaTreeView.refreshCalendarQuery();
>-    }
>     toDoUnifinderRefresh();
> }
If this function does nothing else other than to refresh the unifinder, maybe we want to get rid of it and call toDoUnifinderRefresh() directly? Please do so if this makes sense.

>+    agendaListbox.setCalendar();
Please at least add a comment to this, or rename the setCalendar function (i.e setupCalendar?) At a first glance, it seems strange that you call setCalendar without any arguments.

>   setShortWeekdays: function()
>   {
>     var weekdisplaydeck = document.getElementById("weekdayNameContainer");
>     var childNodes = weekdisplaydeck.childNodes;
>     for (var i = 0; i < childNodes.length; i++) {
>       childNodes[i].setAttribute("value", calGetString("dateFormat","day." + (i+1) + ".Mmm"));
>     }
>- },
>+  },
> 
>+  setDaywithjsDate: function(aNewDate)
>+  {
>+    var newdatetime = jsDateToDateTime(aNewDate);
>+    newdatetime = newdatetime.getInTimezone(calendarDefaultTimezone());
>+    document.getElementById("aMinimonthPopupset").hidePopup();
>+    return this.setDay(newdatetime);
>+  },
>+  getDay: function(aNewDate)
>+  {
>+      return this.start;
>+  },
> 
>   setDay: function(aNewDate)
>   {
Please fix the brackets here. If you feel so inclined, you could also un-anonymize the functions.


>       </sidebarheader>
>-      <vbox flex="1">
>          <box id="mini-day-box" class="today-subpane">
>            <stack flex="1">
Fix indentation if you remote the vbox

>     skin/classic/calendar/gradient-overlay.png   (themes/common/gradient-overlay.png)
>+    skin/classic/calendar/calendar-overlay.png   (themes/common/calendar-overlay.png)
>     skin/classic/calendar/category-overlay.png   (themes/common/category-overlay.png)
Try to keep files in alphabetical order, even though many are not.

>+/**
>+ * sets the value of a boolean attribute by either setting the value or 
>+ * removing the attribute
>+ *
>+ * @param
>+ *      aXulElement
>+ * @param
>+ *      aAttribute
>+ * @param
>+ *      aValue the boolean value
>+ */
put @param on same line as argument, and add small description of each param

>+        if (aValue) {
>+            aXulElement.setAttribute(aAttribute, aValue);
>+        }
>+        else {
>+            if (aXulElement.hasAttribute(aAttribute)) {
>+                aXulElement.removeAttribute(aAttribute);
>+            }
>+        }
Fix } else {. Also, I believe calling removeAttribute doesn't do any harm if the attribute isn't there, so you can save the hasAttribute call.


>+function removeAnonymousElement(aParentNode, aId) {
>+    var child = document.getAnonymousElementByAttribute(aParentNode,"anonid", aId);
>+    child.parentNode.removeChild(child);
>+}
spaces after commas


>+.checkbox-label-box {
>+  -moz-margin-start: 4px;
>+}
>+
>+.checkbox-icon {
>+  -moz-margin-end: 2px;
>+}
>+
>+.checkbox-label {
>+  margin: 0 !important;
>+}

These rules are quite general and will apply to every checkbox, correct? Try to be more specific if possible, in case extension developers want to extend the today pane and want a "normal" checkbox somwhere.




I'm quite tired, more review soon.
Ack I think I commented on the first patch instead of the second. Did I say I'm tired yet? :-/
Comment on attachment 292575 [details] [diff] [review]
patch v.2

Here the remaining comments, this time to the correct patch. Comments from the last reply that have code not in this patch can of course be ignored.


>   onLoad: function () {
>+    debugger;
Remove debugger statement

>+  setDaywithjsDate: function(aNewDate)
>+  {
>+    var newdatetime = jsDateToDateTime(aNewDate);
>+    newdatetime = newdatetime.getInTimezone(calendarDefaultTimezone());
>+    document.getElementById("aMinimonthPopupset").hidePopup();
>+    return this.setDay(newdatetime);
>+  },
I know this is the way it was before, but is it correct to call getInTimezone here? Should we not set the timezone by passing calenarDefaultTimezone() to jsDateToDateTime()? 


>+function setBooleanAttribute(aXulElement, aAttribute, aValue) {
>+    if (aXulElement) {
>+        if (aValue) {
>+            aXulElement.setAttribute(aAttribute, aValue);
since aValue is boolean, maybe you want setAttribute(aAttribute, "true"); If boolean automatically casts to string when calling setAttribute you can disregard this comment.

>--- mozilla_ref/calendar/base/themes/pinstripe/today-pane.css	2007-10-30 12:28:46.031250000 +0100
>+++ mozilla/calendar/base/themes/pinstripe/today-pane.css	2007-12-10 15:14:51.500000000 +0100
Please use 4-space indent for css rule bodies.

>--- mozilla_ref/calendar/lightning/content/agenda-listbox.js	1970-01-01 01:00:00.000000000 +0100
>+++ mozilla/calendar/lightning/content/agenda-listbox.js	2007-12-10 16:54:20.093750000 +0100
It looks like you copied this file from somewhere, but if you like feel free to change agendaListbox to include the functions directly, i.e

var agendaListbox = {
    agendaListboxControl: null,
    pendingRefresh: null,
    eventlistItem: null,
    newalldayeventlistItem: null,
    kDefaultTimezone: null,
    showsToday: false
    init: function  aL_initAgendaListbox() {
        ...
    },
    ...
};

It would be great if you could fix the style on this file, but nothing mandatory. There are a lot of places where
if () {
...
}
else {
...
}

should be replaced with

if () {
...
} else {
...
}

This happens often, please fix at least everywhere you added the code yourself.

>diff -a -x CVS -U 8 -pN -rN mozilla_ref/calendar/lightning/content/agenda-listbox.xml mozilla/calendar/lightning/content/agenda-listbox.xml
>--- mozilla_ref/calendar/lightning/content/agenda-listbox.xml	1970-01-01 01:00:00.000000000 +0100
>+++ mozilla/calendar/lightning/content/agenda-listbox.xml	2007-12-10 15:06:39.640625000 +0100
This file also has a lot of the above mentioned } else { problems. Also check the style guide for how to CDATA blocks.

>+          this.mAllDayItem = document.getAnonymousElementByAttribute(this, "anonid", "allday-item");
>+          this.mAllDayItem.removeShadows();
>+          this.mAllDayItem.removeAttribute("tooltip");
>+          this.mAllDayItem.addEventListener("dblclick",
>+            function(aEvent) {
>+                document.getElementById('agenda_edit_event_command').doCommand();
>+                aEvent.stopPropagation();
>+                aEvent.preventDefault();
>+            },
>+            true);
>+        ]]>
>+      </constructor>
The event listener needs to be removed somewhere, so the function needs to be put somewhere else.

>+            try {
>+                var calcolor = this.mItem.calendar.getProperty("color");
>+            } catch (e) {
>+            }
There is no reason getProperty should fail, or did I miss something?

>+  content/lightning/agenda-listbox.js			(content/agenda-listbox.js)
>+  content/lightning/agenda-listbox.xml   (content/agenda-listbox.xml)
Go ahead and sort these alphabetically



r=philipp with these chanages fixed.
Attachment #292575 - Flags: review?(philipp) → review+
I applied the changes as Phillip said. Some notes:

>>+            try {
>>+                var calcolor = this.mItem.calendar.getProperty("color");
>>+            } catch (e) {
>>+            }
>There is no reason getProperty should fail, or did I miss something?

See my comment #5. As I found out for the default calendar it is necessary to set the color manually

>>+  setDaywithjsDate: function(aNewDate)
>>+  {
>>+    var newdatetime = jsDateToDateTime(aNewDate);
>>+    newdatetime = newdatetime.getInTimezone(calendarDefaultTimezone());
>>+    document.getElementById("aMinimonthPopupset").hidePopup();
>>+    return this.setDay(newdatetime);
>>+  },
>I know this is the way it was before, but is it correct to call getInTimezone
>here? Should we not set the timezone by passing calenarDefaultTimezone() to
>jsDateToDateTime()? 

There is an open issue about timezones in the today-pane within which I would like to fix this in a new bug, too.

>>+.checkbox-label-box {
>>+  -moz-margin-start: 4px;
>>+}
>>+
>>+.checkbox-icon {
>>+  -moz-margin-end: 2px;
>>+}
>>+
>>+.checkbox-label {
>>+  margin: 0 !important;
>>+}
>
>These rules are quite general and will apply to every checkbox, correct? Try to
>be more specific if possible, in case extension developers want to extend the
>today pane and want a "normal" checkbox somwhere.

I left these rules as they are. As my agenda-checkbox derives from a "normal" checkbox I had to use the existing classes to change the rules.

>It would probably be sleek
>if the tag had a shadows attribute that hides the shadows or not, i.e

As I stated in comment #5 already my implementation within the month-view-binding is very minimalistic and I would like to change that in a follow-up bug.
I also applied the desired UI-changes.
Additionally Andreas has tested my patch under Solaris Intel, Windows and Mac and found some small issues that I fixed on the way. There was one bug that caused me some work: Removing a single item of a series from the agenda-listbox was not possible. 
To solve this problem I had to change the "modifyItem" listener in agendaListbox.js and dumped the function "modifylistItem". Maybe Philipp can have a look on this.
There is one regression my patch brings in: The open state of the "today" "tomorrow" and "Next Week" checkboxes are not persistent anymore. I tried to apply these attributes but it would not work. I think this has the same cause as  bug 405034. I would like to fix this in a follow up.
Attached patch patch v.3Splinter Review
Attachment #292575 - Attachment is obsolete: true
patch checked in on trunk and MOZILLA_1_8_BRANCH
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Verified with Lt 2007121918.
One bug:
There is an event from tomorrow in section "Next week" (it starts tomorrow and finishes on Saturday)
Status: RESOLVED → VERIFIED
Attached image Screenshot
There's no space between icon and "New Event" and because of it looks ugly
in reply to comment #18:
>There is an event from tomorrow in section "Next week" (it starts tomorrow and
>finishes on Saturday)
This was done on purpose; pleas see comment #5
>- Allday events which duration covers multiple periods are also displayed in
>each period unlike in the current today-pane.
Comment on attachment 293827 [details] [diff] [review]
patch v.3

r+ was discussed per phone with berend, I just didn't have time to set the review bit.
Attachment #293827 - Flags: review+
Target Milestone: --- → 0.8
Berend, I think you used an incorrect license header in agenda-listbox.xml:

   - The Original Code is Calendar attendee code.
   -
   - The Initial Developer of the Original Code is
   -   Mozilla Corp
   - Portions created by the Initial Developer are Copyright (C) 2006
   - the Initial Developer. All Rights Reserved.
You need to log in before you can comment on or make changes to this bug.