Closed Bug 385900 Opened 17 years ago Closed 17 years ago

Today pane has to be implemented

Categories

(Calendar :: Calendar Frontend, defect)

Product:
Calendar
Component:
Calendar Frontend
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

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

References

Details

(Whiteboard: [roadmap 0.7])

Attachments

(5 files, 1 obsolete file)

category colors (screenshot)
4.73 KB, image/gif
Details
First draft of the today-pane
57.52 KB, patch
michael.buettner
: review-
Details | Diff | Splinter Review
third patch with overworked paths
38.29 KB, patch
michael.buettner
: review-
chris.j.bugzilla
: ui-review-
Details | Diff | Splinter Review
Addressed all issues in the patch attached
42.78 KB, patch
michael.buettner
: review+
chris.j.bugzilla
: ui-review+
Details | Diff | Splinter Review
fourth patch
40.97 KB, patch
Details | Diff | Splinter Review 
For users, who primarily use thunderbird as an email client, there should be a way to observe current or ongoing events and and tasks without leaving the standard e-mail environment. A solution to this problem is to provide an extra pane on the right hand side next to the e-mail message pane. A suggestion of how this could look like can be seen in http://wiki.mozilla.org/Calendar:Improving_the_Calendar_Views. This pane subsequently called "today pane" offers a distinctive view on events and tasks of a certain date and may be resized and collapsed. It shall be closed automatically when a lightning calendar view is shown. The today pane offers a view on either the agenda subpane, the task subpane of both subpanes. This is configurable by the user similarily to the folder pane of thunderbird where it is possible to select only a certain extract of folder information.
The today pane must be equipped with a own datepicker in the header that allows traveling to next or previous dates or opens a monthpicker to select a date further apart. 
In a first step the today pane shall only incorporate the agenda subpane and task subpane that are currently implemented as tab panes next to the calendar pane at the bottom of the messenger sidebar.
Flags: blocking-calendar0.7+
Berend, I know that you have been working on this for some time already. Just a short question:

Will this work for all possible Thunderbird layouts, even those (mail.pane_config.dynamic=3) where the message list and message pane do not
form a 'box'?
OS: SunOS → All
Hardware: PC → All
Version: Trunk → unspecified
Yes, it does (as I just tested). The today pane overlays the "mailContent" box that includes the three panes.
Christian continued on fleshing out the details of the specification, see [1] for the wiki page.

[1] http://wiki.mozilla.org/Calendar:Mail_View_Integration
Berend, what's the ETA for the first revision of this patch? Sven Giermann already asked me about this feature and I'm sure others are also eagerly awaiting this patch :-)
I think I will be able to deliver a first patch by tomorrow.
Status: NEW → ASSIGNED
Attached image category colors (screenshot) — 
Based on what I see on the wiki, I have some requests for the new "Today Pane":

1a) I would like to always see the minimonth instead of the miniday.

1b) A single click in the minimonth could change the today pane to that day.

1c) A double click could change TB to "mail mode" using the most recently used view (e.g. month view), with that day selected.

1d) The minimonth could have a button or something to "Go to Today".  The miniday already has buttons to do this:  < O >.

2) It would be nice if we could choose how many days of events/tasks to see in the today pane instead of being forced to see only one day at a time.  Unfortunately it seems that the 'tomorrow' and 'soon' entries have been removed from the interface, but I loved this feature that existed in Lightning's agenda.  It was so convenient to see events/tasks that will occur in the next few days at a glance without having to manually navigate the interface.

Instead of using a tree view like the agenda used for 'tomorrow' and 'soon', perhaps the today pane could separate each day with a simple horizontal line.  The horizontal line could contain the text 'tomorrow', and subsequent horizontal lines could contain the name of the day.

3) I would like past events to immediately disappear from the today pane (e.g. a meeting from 2:00 to 3:00 disappears at 3:00.  All-day events disappear at 00:00 midnight).  This would optimize screen space by removing obsolete information and, in conjunction with request (2), more future events/tasks could scroll up in the list.  This behavior would be especially useful toward the end of the day when what I have to do tomorrow becomes more important than what I already did today.

I would prefer the behavior described in requests (2) and (3) instead of what I see on the wiki (i.e. only one day at a time, past events are gray, the current event is bold, and I see no events for tomorrow).  Otherwise it seems that we are losing the very useful functionality that we had with Lightning's agenda.

4) It would be nice if tooltips in the today pane would show as much information as tooltips in the main calendar views (e.g. 'Location' and 'description').  In this respect, the today pane could be better than Lightning's agenda was.

5) I'd prefer to not see the striped background colors behind the events/tasks.  I think that the icon on the left of each event/task clearly distinguishes each entry.  I'm hoping that we'll be able to disable the striping, perhaps via CSS.

6) I would like the today pane to use the font colors & attributes that I've defined in UserChrome.css for Categories.  The attached screenshot shows the result of using CSS entries such as the following:

[item-category="Appointment"]
{ 
     color: red !important;
     background-color: rgb(240,240,240) !important;
}

This is important to me because I have no other way to distinguish events/tasks in the today pane because they're all in a single calendar file (and thus every icon in the left column is the same color).  In addition, it seems that even people who use multiple calendars would also appreciate having the option to format the colors & attributes of text in the today pane based on category.  This could include different text and background colors, for example, for all-day events, appointments, birthdays, holidays, team meetings, multiday events, etc.

7) I think that the bar in the middle of the today pane that contains the "New Event" button could be improved a little.  It looks like it's a header bar for the tasks pane.  I think that it should more clearly belong to the events instead of to the tasks.  Unfortunately I don't have any suggestions on how to improve it.
(In reply to comment #6)
> Created an attachment (id=270984) [details]
> category colors (screenshot)

Thanks for this great feedback:)
Please find my comments below.

> 
> Based on what I see on the wiki, I have some requests for the new "Today Pane":
> 
> 1a) I would like to always see the minimonth instead of the miniday.
Sounds reasonably, but we should do that in a spin-off bug.


> 
> 1b) A single click in the minimonth could change the today pane to that day.

Makes sense.

> 
> 1c) A double click could change TB to "mail mode" using the most recently used
> view (e.g. month view), with that day selected.

You mean "calendar mode", right?
> 
> 1d) The minimonth could have a button or something to "Go to Today".  The
> miniday already has buttons to do this:  < O >.

I'll support this, but this should also be fixed in a spin-off bug.

> 
> 2) It would be nice if we could choose how many days of events/tasks to see in
> the today pane instead of being forced to see only one day at a time. 
> Unfortunately it seems that the 'tomorrow' and 'soon' entries have been removed
> from the interface, but I loved this feature that existed in Lightning's
> agenda.  It was so convenient to see events/tasks that will occur in the next
> few days at a glance without having to manually navigate the interface.
> 
> Instead of using a tree view like the agenda used for 'tomorrow' and 'soon',
> perhaps the today pane could separate each day with a simple horizontal line. 
> The horizontal line could contain the text 'tomorrow', and subsequent
> horizontal lines could contain the name of the day.

Makes also sense from my point of view, I'll update the mock-ups and wiki.

> 
> 3) I would like past events to immediately disappear from the today pane (e.g.
> a meeting from 2:00 to 3:00 disappears at 3:00.  All-day events disappear at
> 00:00 midnight).

I support the approach that events should disappear at midnight, but I'm not sure if events should disappear immediately. This might be confusing for people who want to use the today pane as an aggregation of the day. But maybe we should offer an option for that.

> This would optimize screen space by removing obsolete
> information and, in conjunction with request (2), more future events/tasks
> could scroll up in the list.  This behavior would be especially useful toward
> the end of the day when what I have to do tomorrow becomes more important than
> what I already did today.
> 
> I would prefer the behavior described in requests (2) and (3) instead of what I
> see on the wiki (i.e. only one day at a time, past events are gray, the current
> event is bold, and I see no events for tomorrow).  Otherwise it seems that we
> are losing the very useful functionality that we had with Lightning's agenda.
> 
> 4) It would be nice if tooltips in the today pane would show as much
> information as tooltips in the main calendar views (e.g. 'Location' and
> 'description').  In this respect, the today pane could be better than
> Lightning's agenda was.


It is planned to display the same Tooltips as in the main calendar views.

> 
> 5) I'd prefer to not see the striped background colors behind the events/tasks.
>  I think that the icon on the left of each event/task clearly distinguishes
> each entry.  I'm hoping that we'll be able to disable the striping, perhaps via
> CSS.
> 
> 6) I would like the today pane to use the font colors & attributes that I've
> defined in UserChrome.css for Categories.  The attached screenshot shows the
> result of using CSS entries such as the following:
> 
> [item-category="Appointment"]
> { 
>      color: red !important;
>      background-color: rgb(240,240,240) !important;
> }
> 
> This is important to me because I have no other way to distinguish events/tasks
> in the today pane because they're all in a single calendar file (and thus every
> icon in the left column is the same color).  In addition, it seems that even
> people who use multiple calendars would also appreciate having the option to
> format the colors & attributes of text in the today pane based on category.

Right now Events don't visualize categories, but it is planned to do that. Which means that events with a category set provide a visual marker. This marker occurs in the today pane.
 
> This could include different text and background colors, for example, for
> all-day events, appointments, birthdays, holidays, team meetings, multiday
> events, etc.
> 
> 7) I think that the bar in the middle of the today pane that contains the "New
> Event" button could be improved a little.  It looks like it's a header bar for
> the tasks pane.

Ok, I'll take that into account.

> I think that it should more clearly belong to the events
> instead of to the tasks.  Unfortunately I don't have any suggestions on how to
> improve it.
>
Attached patch First draft of the today-pane — — Splinter Review 
This patch is only a first step for the today-pane. 
Some notes about the implementation:
1) The Todaypane is only visbile in calendar mode. It collapses when the mailmode switches to calendarmode. 
2) Only when the preference 'calendar.todaypane.mailtoolbar' is set to 'false' the todaypane toolbar button is inserted into the toolbarpalette. Of course the user may customize the mailtoolbarpalette in the known manner and remove the today-pane-button. The only possible way to activate the todaypane would then be to restore the default settings of the mail toolbar-palette. For the next patch we must come to an agreement where to place a according menuitem that would trigger the today-pane in the same way as the toolbar-button currently does.
3) Stella, a graphical designer, promised to deliver an image for the toolbar icon. As this is not yet available I used an existing image as a preliminary placeholder.
4) I kept some sourcecode to generate output at the error-console in the patch as it may be beneficial, in case errors might occur.
Attachment #271647 - Flags: review?(michael.buettner)
Comment on attachment 271647 [details] [diff] [review]
First draft of the today-pane

Would you please be so kind to submit a patch that does contain only those parts that belong to this particular feature? It doesn't really make sense to filter out those parts that actually need review.
Attachment #271647 - Flags: review?(michael.buettner) → review-
Attached patch Second draft of the today-pane (obsolete) — — Splinter Review 
It appears that files from other patches with windows lineends made their way into my patch. mickey is taking care of that problem. I cleaned up my patch attached.
Attachment #271674 - Flags: ui-review?(christian.jansen)
Attachment #271674 - Flags: review?(michael.buettner)
I think that it is better to use the normal path in the diff that makes users like me easier to use this patch. :-)

from:
> /net/v20z-so5.germany.sun.com/export/home/bc93774/PIM/pim/mozilla/mozilla_ref/calendar/base/content/today-pane.js
to:
mozilla_ref/calendar/base/content/today-pane.js
If we are going that far, I think we should use |cvs diff|, that also gives you the cvs version info and sane paths ;)
I modified my batch so that gnudiff now displays relative paths.
Attachment #271688 - Flags: ui-review?(christian.jansen)
Attachment #271688 - Flags: review?(michael.buettner)
Some first flyby comments, which I think are worthwhile mentioning before anything else.

First of all, the "Today Pane" button in the toolbar doesn't have a 'checked' state, the button doesn't appear to be pressed if the pane is visible and visa versa. Furthermore, it strikes me that you should introduce a command that operates on this particular issue, since you'll want to add the appropriate menuitem in this or a later patch.

Second, switch to calendar mode and close Thunderbird. After that, restart Thunderbird. The "Today Pane" still is invisible. Clicking on the "Today Pane" button of course brings it back. I also managed to make the button itself disappear, but I'm not able to reproduce it.

Apart from these issues the patch makes a good overall impression :-) I'm going to address the code issues in a separate comment.
Hi,

Just to know ... When today panel will be available at Nightly builds? To test it?!!! 
Comment on attachment 271688 [details] [diff] [review]
third patch with overworked paths

>diff -a -U 8 -pN -r ./mozilla_ref/calendar/base/content/today-pane.js 
...
>+ * The Original Code is Lightning code.
Should this read "...Sun Microsystems..."?

>+
>+
>+
Please just leave a single empty line above...

>+var TodayPane = {
>+  gCurrentPaneView: 0,
The 'g'-Prefix is generally reserved for global variables. So please name this 'currentPaneView' or if you're so inclined 'mCurrentPaneView'.

>+  paneViews: [],
You're constructing 'paneViews' twice. Once during the construction phase of the object, and a second time during your onLoad() function, leaving the initially constructed array-object for the garbage collector. Furthermore, in the sense of 'construct everything immediately' you could write
    paneViews: [ calGetString("calendar", "eventsandtasks"),
                 calGetString("calendar", "tasksonly"),
                 calGetString("calendar", "eventsonly") ];

>+  pref: Components.classes[
>+              "@mozilla.org/preferences-service;1"
>+            ].getService(Components.interfaces.nsIPrefBranch),
First of all, there's no need to cache the reference to the preference service. It doesn't hurt to ask for the service if you need it. This also leaves us with a global object (TodayPane) that holds a constant reference to the preference service. Admittedly, this is rather theoretical, but doesn't leave the chance to release the service (I know that this doesn't apply to this particular scenario, but this premature caching introduces circular references, and causes unnecessary memory consumption). Second, you should use 'nsIPrefBranch2' here and install preference observers.

>+  mailToolbar: null,
Again, this is premature optimization, no need to cache this.

>+  stodaypaneButton: "calendar-show-todaypane-button",
This is a hardcoded string, which should live in a *.dtd file. Furthermore, there's nothing like static variables (which you obviously tried to introduce due to the naming scheme). Please get rid of the 's'-prefix and the whole variable, since you need to move it to some other file.

>+  onLoad: function () {
>+    this.paneViews = [ calGetString("calendar", "eventsandtasks"), calGetString("calendar", "tasksonly"), calGetString("calendar", "eventsonly")];
There's a missing blank before the closing bracket. And see my comment regarding initialization.

>+    var cs = Components.classes["@mozilla.org/consoleservice;1"].getService(Components.interfaces.nsIConsoleService);
Please get rid of debug code in the patch.

>+    this.mailToolbar = document.getElementById("mail-bar2");
You should use the global function getMailBar(), which would make this patch also aware of Thunderbird 1.5.

>+    var defaultSetString = this.mailToolbar.getAttribute("defaultset");
>+    if (defaultSetString.indexOf(this.stodaypaneButton) == -1) {
>+      defaultSetString = this.addButtonToToolbarset(defaultSetString, false);
>+      this.mailToolbar.setAttribute("defaultset", defaultSetString);
>+    }
You're adding the "Today Pane"-button to the defaultset of the mailtoolbar. The function addButtonToToolbarset() doesn't really serve a single purpose. It manages adding the button to the DOM tree and it serves adding it to the defaultset. The behavior is controlled by the boolean passed as second argument. Wouldn't it be much better to clearly separate these responsibilities?

>+    var isInToolbar = this.pref.getBoolPref("calendar.todaypane.mailtoolbar");
>+    cs.logStringMessage("Is in toolbar: " + isInToolbar);
>+    if (!isInToolbar) {
>+      var currentSetString = this.mailToolbar.getAttribute("currentset");
>+      cs.logStringMessage("currentset before: " + currentSetString);
>+      if (currentSetString.indexOf(this.stodaypaneButton) == -1) {
>+        var currentSetString = this.addButtonToToolbarset(currentSetString, true);
>+      }
>+      this.pref.setBoolPref("calendar.todaypane.mailtoolbar", true);
>+    }
"calendar.todaypane.mailtoolbar" serves as an indicator whether or not the "Today Pane" button should initially be added to the currentset of the toolbar. Wouldn't it be much better to introduce the preference initially set and remove it once detected? The approach as it currently stands doesn't work if Lightning is removed and installed again. And you don't detect upgrades as well. Furthermore, I'd want to introduce a general preference (like 'calendar.installed') and handle the appropriate bits and pieces in messenger-overlay-sidebar with a observer mechanism.

>+    cs.logStringMessage("currentset: " + this.mailToolbar.getAttribute("currentset"));
>+    cs.logStringMessage("defaultset: " + this.mailToolbar.getAttribute("defaultset"));
Please get rid of debug code in the patch.

>+    var paneindex = 0;
This is an unreferenced variable.

>+    var agendaPanel = document.getElementById("agenda-groupbox");
>+    var todoPanel = document.getElementById("todo-groupbox");
>+    if (agendaPanel.hasAttribute("collapsed")) {
>+      if (!todoPanel.hasAttribute("collapsed")) {
>+        this.gCurrentPaneView = 1;
>+      }
>+      else{
>+        dump("Cannot display todaypane with both subpanes collapsed");
>+      }
>+    }
>+    else {
>+      if (todoPanel.hasAttribute("collapsed")) {
>+        this.gCurrentPaneView = 2
>+      }
>+    }
You could calculate 'gCurrentPaneView' as so:
this.gCurrentPaneView = document.getElementById("agenda-groupbox").hasAttribute("collapsed") << 1 |
                        document.getElementById("todo-groupbox").todoPanel.hasAttribute("collapsed");
I find this condensed version much more readable, but that's up to you.

>+    var todayheader = document.getElementById("today-header");
>+    todayheader.setAttribute("value", this.paneViews[this.gCurrentPaneView]);
todayheader.value = ... ???

>+  addButtonToToolbarset: function(toolbarSetString, addButton)
Please see my comment above regarding this function.

>+  // we can cycle the pane view forward or backwards
>+  cyclePaneView: function(aCycleForward)
>+  {
>+    // pass the call onto loadFolderView...
>+    var offset = aCycleForward ? 1 : kNumFolderViews - 1;
This is a copy and paste bug, isn't it?

>+    this.gCurrentPaneView = this.gCurrentPaneView + offset;
>+    var nViewLen = this.paneViews.length;
>+    if (this.gCurrentPaneView >= nViewLen) {
>+        this.gCurrentPaneView = 0;
>+    }
>+    else if (this.gCurrentPaneView == 0) {
>+        this.gCurrentPaneView = nViewLen -1;
>+    }
>+    var agendapanel = document.getElementById("agenda-groupbox");
>+    var todopanel = document.getElementById("todo-groupbox");
>+    var todayheader = document.getElementById("today-header");
>+    todayheader.setAttribute("value", this.paneViews[this.gCurrentPaneView]);
todayheader.value = ... ???

>+    switch (this.gCurrentPaneView) {
>+        case 0:
>+          this.collapsePanel(agendapanel, false);
>+          this.collapsePanel(todopanel, false);
>+          break;
>+        case 1:
>+          this.collapsePanel(agendapanel, true);
>+          this.collapsePanel(todopanel, false);
>+          break;
>+        case 2:
>+          this.collapsePanel(agendapanel, false);
>+          this.collapsePanel(todopanel, true);
>+          break;
>+    }
>+    this.pref.setIntPref('calendar.todaypane.view', this.gCurrentPaneView);
This verbose version is hard to read. Wouldn't it make sense to write this in just 2 lines?

>+  collapsePanel: function(oPanel, bCollapse)
>+  {
>+    if (bCollapse) {
>+      oPanel.setAttribute("collapsed", bCollapse)
>+    }
>+    else{
>+      oPanel.removeAttribute("collapsed")
>+    }
>+  }
I would prefer to make this a local function inside of cyclePaneView(). Otherwise, I'd mark 'private' function with a '_'-prefix.

>+function loadTodayPane() {
>+  TodayPane.onLoad();
>+}
>+
>+window.addEventListener("load", loadTodayPane, false);
Please add a comment on why you use this indirection and don't just pass TodayPane.onLoad to addEventListener().

>+   - The Original Code is Calendar view code.
Again, the license block is not correct.

>+   - Contributor(s):
>+   - Berend Cornelius <berend.cornelius@sun.com>
Please adhere to the prevalent indentation.

>+<?xml-stylesheet href="chrome://calendar/skin/today-pane.css" type="text/css"?>
>+<?xml-stylesheet href="chrome://calendar/content/datetimepickers/minimonth.css" type="text/css"?>
>+<?xml-stylesheet href="chrome://messenger/skin/messengercompose/messengercompose.css" type="text/css"?>
Why do you need minimonth.css and messengercompose.css here?

>+<overlay xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" onload="onLoad()">
onLoad() ???

>diff -a -U 8 -pN -r ./mozilla_ref/calendar/base/themes/pinstripe/calendar-toolbar.css ./mozilla/calendar/base/themes/pinstripe/calendar-toolbar.css
>--- ./mozilla_ref/calendar/base/themes/pinstripe/calendar-toolbar.css	2007-07-06 14:49:54.640966000 +0200
>+++ ./mozilla/calendar/base/themes/pinstripe/calendar-toolbar.css	2007-07-10 13:59:47.312782000 +0200
The pinstripe/winstripe versions are not in sync.

>diff -a -U 8 -pN -r ./mozilla_ref/calendar/base/themes/pinstripe/today-pane.css ./mozilla/calendar/base/themes/pinstripe/today-pane.css
>--- ./mozilla_ref/calendar/base/themes/pinstripe/today-pane.css	1970-01-01 01:00:00.000000000 +0100
>+++ ./mozilla/calendar/base/themes/pinstripe/today-pane.css	2007-07-10 13:59:47.313264000 +0200
This file shouldn't be empty.

>+# The Original Code is OEone Calendar Code, released October 31st, 2001.
>+#
>+# The Initial Developer of the Original Code is
>+# OEone Corporation.
>+# Portions created by the Initial Developer are Copyright (C) 2001
>+# the Initial Developer. All Rights Reserved.
>+#
>+# Contributor(s): Berend Cornelius <berend.cornelius@sun.com>
License header.

>     if (!this.filterType)
>-        this.filterType = 'all';
>+        this.filterType = 'events';
Does this really belong to this patch?

>+function toggleTodayPaneInMailMode()
>+{
>+  var oTodayPane = document.getElementById("today-pane-box");
>+  if (oTodayPane.hasAttribute("collapsed")) {
>+    oTodayPane.removeAttribute("collapsed");
>+    oTodayPane.removeAttribute("collapsedinMailMode");
>+  }
>+  else {
>+    oTodayPane.setAttribute("collapsed", true);
>+    oTodayPane.setAttribute("collapsedinMailMode", true);
>+  }
>+}
You probably want to make this a handler for a command yet to be introduced.
> %messengerDTD;

>+  <!ENTITY % dtd4 SYSTEM "chrome://calendar/locale/global.dtd"> %dtd4;
What the global.dtd for?

>+       <separator/>
>+       <vbox id="calendar-panel" flex="1">
>+        <tree id="calendarTree" hidecolumnpicker="true" seltype="single"
>+              onfocus="selectedCalendarPane(event)" flex="1"
>+              onselect="ltnSidebarCalendarSelected(this);"
>+              ondblclick="ltnCalendarTreeView.onDoubleClick(event);"
>+              context="calendartree-context-menu">
>+          <treecols>
>+            <treecol id="col-calendar-Checkbox" cycler="true" fixed="true" width="18" >
>+              <image />
>+            </treecol>
>+            <treecol id="col-calendar-Color" fixed="true" width="18"/>
>+            <treecol id="col-calendar-Calendar" label="&lightning.calendar.label;" flex="1"/>
>+          </treecols>
>+          <treechildren>
>+          </treechildren>
>+        </tree>
>+      </vbox>
Are you sure this belongs to this patch?

>+    // only show the today-pane if was not collapsed during the last 
>+    // "mail-mode session"
>+    var oTodayPane = document.getElementById("today-pane-box");
>+    if (!oTodayPane.hasAttribute("collapsedinMailMode")) {
>+      if (oTodayPane.hasAttribute("collapsed")) {
>+        oTodayPane.removeAttribute("collapsed");
>+      }
>+    }
>+    
That's probably not enough. You also need to handle the case when Thunderbird is closed in calendar mode. Probably you also want to hijack the ltnInitializeMode() function

>+                    oncommand="toggleTodayPaneInMailMode()"/>
...InMailMode() is superfluous here, isn't it?

>+<!ENTITY calendar.showtodaypane.button.tooltip    "Show Today pane" >
I really don't like these ultra-long names which don't add anything significant. Wouldn't it be enough to just  name it todaypane.button and todaypane.button.whatever? That this button is intended to show and hide the today pane is rather obvious. This comment applies to other locations as well.
Attachment #271688 - Flags: review?(michael.buettner) → review-
***** UI Review *****
Please change the following:

* Increase the space between "Right" arrow and Closer by 5px

* "Tasks only" & "Event only" should be changed to
  "Tasks Only" & "Events Only"

* Currently there are white spaces around the group boxes these
  should be replaced, if possible, by dialog color

* A splitter should be place between Events and tasks. This would
  allow users to increase/decrease the size of the event or
  tasks list

* The "Today Pane" Toolbar button should offer a checked state

* The "Today Pane" Toolbar icon should be replaced, soon

* Menu Integration:
  View -> Layout -> Classic View
                    ...
                    ------------
                    Message Pane F8
                    Today Pane   CTRL+D

* Add Keyboard Shortcut:
  "CTRL+D", "CMD+D" for toggling the Pane on or off
Attachment #271688 - Flags: ui-review?(christian.jansen) → ui-review-
(In reply to comment #17)
> ***** UI Review *****
> Please change the following:
> 
> * Increase the space between "Right" arrow and Closer by 5px
> 
> * "Tasks only" & "Event only" should be changed to
>   "Tasks Only" & "Events Only"
> 
I'll need to correct this "Tasks only" should be changed to "Tasks" and "Events only" to "Events".
.
Attachment #272484 - Flags: ui-review?(christian.jansen)
Attachment #272484 - Flags: review?(michael.buettner)
(In reply to comment #17)
I added a splitter as desired. However there might come up a problem in the followup patches: When travelling through the miniday the agendapane and taskpane will be refilled and as both panes are flex they will try to resize themselves in dependency to the amount of their content. This will have to be suppressed. I assigned all other issues in this comment.
in reply to comment #16

>>+  paneViews: [],
> You're constructing 'paneViews' twice. Once during the construction phase of
> the object, and a second time during your onLoad() function, leaving the
> initially constructed array-object for the garbage collector. Furthermore, >in
> the sense of 'construct everything immediately' you could write
>        paneViews: [ calGetString("calendar", "eventsandtasks"),
>                     calGetString("calendar", "tasksonly"),
>                     calGetString("calendar", "eventsonly") ];
    I kept my code as it is. Including the file "calUtils.js"  in "today-pane.js"
    resulted in a nameclash error "Error: redeclaration of const Cc
    Source File: chrome://calendar/content/calUtils.js
    Line: 42"

>>+  stodaypaneButton: "calendar-show-todaypane-button",
> This is a hardcoded string, which should live in a *.dtd file. Furthermore,
> there's nothing like static variables (which you obviously tried to introduce
> due to the naming scheme). Please get rid of the 's'-prefix and the whole
> variable, since you need to move it to some other file.

'stodaypaneButton' referred to a button id and as this is used several times in
 the sourcecode I found that it makes sense to keep this value in a variable

    >+    switch (this.gCurrentPaneView) {
    >+        case 0:
    >+          this.collapsePanel(agendapanel, false);
    >+          this.collapsePanel(todopanel, false);
    >+          break;
    >+        case 1:
    >+          this.collapsePanel(agendapanel, true);
    >+          this.collapsePanel(todopanel, false);
    >+          break;
    >+        case 2:
    >+          this.collapsePanel(agendapanel, false);
    >+          this.collapsePanel(todopanel, true);
    >+          break;
    >+    }
    >+    this.pref.setIntPref('calendar.todaypane.view', 
>    This verbose version is hard to read. Wouldn't it make sense to write this >in
>    just 2 lines?

In the follow-up patches more sourcecode most probably more functionality will
  be called when the inner view of the today pane changes. For example in my
    newest patch the new splitter that Christian demanded will be made invisible.

    >>+                    oncommand="toggleTodayPaneInMailMode()"/>
>   ...InMailMode() is superfluous here, isn't it?
>    I kept the name in order to emphasize that in this function the attribute
>    "collapseinMailMode" is also updated which is not the case when the today >pane
>    is hidden because the calendar-mode is switched on.
>
>    >+<!ENTITY calendar.showtodaypane.button.tooltip    "Show Today pane" >
>    I really don't like these ultra-long names which don't add anything
>    significant. Wouldn't it be enough to just  name it todaypane.button and
>    todaypane.button.whatever? That this button is intended to show and hide > >the today pane is rather obvious. This comment applies to other locations as >well.

I kept the string as it is. I tried to adapt my string to the convention of the
    dtd -file.


I forgot to mention that I no longer keep a preference to add a toolbar button. Instead I inctroduced a boolean attribute, made it persistent and changed its value when the button is added. Admittedly there still remains a problem that, after uninstalling and reinstalling the extension the value of the attribute is still valid.
Comment on attachment 272484 [details] [diff] [review]
Addressed all issues in the patch attached

>+    <vbox id="today-pane-box" minwidth="100" width="200" addtoolbarbutton="true" persist="collapsed collapsedinMailMode addtoolbarbutton width">
I don't like the name of the 'collapsedinMailMode' attribute, following elsewhere established naming convention suggests a name such as 'collapsed-in-mail' or something similar. But that's up to you.

>+      <sidebarheader id="folderPaneHeader" align="center">
This is a copy'n'paste bug, isn't it?

>+        <label id ="today-header"/>
>+        <spacer flex="1"/>
>+        <toolbarbutton id="folderview-cycler-left"  class="folderview-cycler"
>+                       onclick="TodayPane.cyclePaneView(-1);"/>
>+        <toolbarbutton id="folderview-cycler-right" class="folderview-cycler" 
>+                       onclick="TodayPane.cyclePaneView(1);"/>
>+        <spacer style="width: 5px;"/>
>+        <toolbarbutton class="ab-closebutton" command="cmd_toggleTodayPaneWithoutLabel"/>
>+      </sidebarheader>
>+      <vbox flex="1">
>+        <groupbox id="agenda-groupbox" class="today-groupbox" persist="height collapsed" flex="1" minheight="200">
>+          <vbox id="agenda-tab-panel" flex="1"/>
>+        </groupbox>
>+        <splitter id="today-pane-splitter" persist="hidden"/>
>+        <groupbox id="todo-groupbox" class="today-groupbox" persist="collapsed height" flex="1" minheight="200">
>+          <vbox id="todo-tab-panel" flex="1"/>
>+        </groupbox>
>+    </vbox>
>+  </vbox>
>+</box>
There're several attributes that would better fit into the corresponding *.css file (minheight, height, width, etc). I don't think the rationale that it's the same elsewhere in the code should prevent us from getting things straight for new code.

>diff -a -U 8 -pN -r ./mozilla_ref/calendar/base/themes/pinstripe/today-pane.css ./mozilla/calendar/base/themes/pinstripe/today-pane.css
>--- ./mozilla_ref/calendar/base/themes/pinstripe/today-pane.css	1970-01-01 01:00:00.000000000 +0100
>+++ ./mozilla/calendar/base/themes/pinstripe/today-pane.css	2007-07-16 15:15:54.885981000 +0200
>@@ -0,0 +1,49 @@
>+<?xml version="1.0"?>
>+<!--
This is a css file, not an xml file, so this causes a "ruleset ignored due to bad selector" error in the css engine.

>+.today-groupbox {
>+  -moz-border-radius: 0px !important;
>+  min-height:"200px";
>+}
Error in parsing value for property 'min-height' -> get rid of the quotes.

>+function toggleTodayPaneinMailMode()
>+{
>+  var oTodayPane = document.getElementById("today-pane-box");
>+  var todayPaneCommand = document.getElementById('cmd_toggleTodayPane');
>+  if (oTodayPane.hasAttribute("collapsed")) {
>+    oTodayPane.removeAttribute("collapsed");
>+    oTodayPane.removeAttribute("collapsedinMailMode");
>+    todayPaneCommand.setAttribute("checked","true");
>+  }
>+  else {
>+    oTodayPane.setAttribute("collapsed", true);
>+    oTodayPane.setAttribute("collapsedinMailMode", "true");
>+    todayPaneCommand.setAttribute("checked", "false");
>+  }
>+}
This could be handled much more elegant if you would listen to the 'select' event of the 'displayDeck'. See [1] for a reference where this already has been done. I would like to see all this code moved to today-pane.js in order to keep this as close to the relevant scope as possible.

>+    <command id="cmd_toggleTodayPane" oncommand="toggleTodayPaneinMailMode()" label="&calendar.showtodaypane.button.label;"/>
>+    <command id="cmd_toggleTodayPaneWithoutLabel" oncommand="toggleTodayPaneinMailMode()"/>
It's not necessary to have two commands here, please get rid of the second one. I know that you did this just to have access to the label defined in the *.dtd file as you need it to attach it to the dynamically generated menuitem. A possible solution is to add something like this to you today-pane.xul file:

  <script type="application/x-javascript">
    var todayLabel = "&calendar.showtodaypane.button.label;";
  </script>

And just use the global variable 'todayLabel' in your *.js file. I didn't try that but it's probably better to write 'const todayLabel...'.

>+    <toolbarbutton id="calendar-show-todaypane-button"
>+                    class="cal-toolbarbutton-1"
>+                    tooltiptext="&calendar.showtodaypane.button.tooltip;"
>+                    command="cmd_toggleTodayPane"/>
The TodayPane button does initially not appear to be checked, the appropriate attribute is missing.

> <!ENTITY calendar.today.button.tooltip            "Go to today" >
>+<!ENTITY calendar.showtodaypane.button.tooltip    "Show Today pane" >
> <!ENTITY calendar.choosedate.button.tooltip       "Choose date to go to" >
Please name this "calendar.todaypane.button.tooltip"

> <!ENTITY calendar.context.gototoday.label             "Go to Today">
>+<!ENTITY calendar.showtodaypane.button.label          "Today Pane" >
> <!ENTITY calendar.context.gototoday.accesskey         "T">
> <!ENTITY calendar.context.emailevent.label            "Email Selected Events…">
> <!ENTITY calendar.context.emailevent.accesskey        "i">
Please name this "calendar.context.button.label"

In general this patch works as advertised and I don't want to hold it off. r=mickey with all of the above comments addressed.

[1] http://lxr.mozilla.org/mozilla1.8/source/calendar/lightning/content/messenger-overlay-sidebar.js#677
Attachment #272484 - Flags: review?(michael.buettner) → review+
Whiteboard: [roadmap 0.7]
Attached patch fourth patch — — Splinter Review 
I adressed the remaining issues and set up https://bugzilla.mozilla.org/show_bug.cgi?id=388414 ("Today-pane: Implement miniday-pane and toolbarbutton") as a follow-up issue.
Comment on attachment 272484 [details] [diff] [review]
Addressed all issues in the patch attached

r=christian

Sorry for the late approval.
Attachment #272484 - Flags: ui-review?(christian.jansen) → ui-review+
Attachment #271674 - Attachment is obsolete: true
Attachment #271674 - Flags: ui-review?(christian.jansen)
Attachment #271674 - Flags: review?(michael.buettner)
Berend, has this bug been fixed with your checkin yesterday or are there open issues, that are still remain to be fixed in this bug?
I had a look using the 20070720 nightly, only one comment:
in Thunderbird's vertical layout the width of the today pane is taken solely from the message list, while the message pane does not flex. Hence the message list/tree gets very narrow by default. Would it be worthwhile to try how it works with a flexing message pane?
No,as Andreas confirmed everything is fine.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Verified in latest nightly build of lightning, task is fixed.
Status: RESOLVED → VERIFIED
Is it on purpose that Agenda and Task list are no longer visible in Calendar mode? In my opinion this is a major regression because the Today pane doesn't seem to work in Calendar mode either.
in reply to comment #27.
I noticed this too and object to it too. This behaviour is amplified, because I assigned min-widths to  both agenda panel and todo-panel.  While I am currently working on issue https://bugzilla.mozilla.org/show_bug.cgi?id=388414, I removed this attributes and find the behaviour better. But as you said it would probably best to place the today-pane also right to the message pane.
(In reply to comment #30)
> Is it on purpose that Agenda and Task list are no longer visible in Calendar
> mode? In my opinion this is a major regression because the Today pane doesn't
> seem to work in Calendar mode either.
> 

I think the events are not so important. because in mail mode you can see events on the today pane and in calendar mode you can see the events on the calendar.
but i think a list of tasks is necessary. like in sunbird.
for future the task-view will take this job. but up to this i will miss the agenda of tasks.


I just noticed that all the items in the calender have lost their color after this update.... Has a bug been made of this, or should I do that?

Patrick
Patrick, I saw the same problem. Please file a new bug on it and I will confirm it quickly.
done bug 388994
Depends on: 388985
in reply to comment #27.
I confirm this regression by the fact that when you create a task, you need to be in calendar mode, the icons of the toolbar "create new task" is only visible there. But when to see the result you need to come back in Mail mode, to see the result which is very disturbing IMHO
(In reply to comment #30) (In reply to comment #36)
I filed Bug 389150 to track this discussion in a more appropriate place.
(In reply to comment #28)
> No,as Andreas confirmed everything is fine.
> 

Was that related to comment 26 or comment 27?
Did anyone address comment 27, that the mail preview pane is fixed size and doesn't give away room when the today pane is shown?
Berend: I would appreciate if you would follow some rules that all developers adhere to.

First of all, the comments for a check-in should follow this pattern: "Bug #..... - comment, patch=name, r=name". It's fine if you quote the title of the respective bug. For 'name' we're using established nick-names or full email-addresses.

Second, after having a patch checked in, please say so in the bug. Also, please state on which branches you committed. This normally is also the right time to change the resolution of the bug.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: