388414 - [Today Pane] Implement 'MiniDay' Pane

Status: VERIFIED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Berend Cornelius]

This issue is a follow-up to https://bugzilla.mozilla.org/show_bug.cgi?id=386335 and shall implement the second part of the today-pane. The implementation of this issues is to include the miniday-pane. Furthermore the toolbarbutton of the mailtoolbar that toggles the whole pane should be equipped with a new icon.
The user interface is specified in http://wiki.mozilla.org/Calendar:Improving_the_Calendar_Views.

Comment 1

[Thomas Stache]

Attachment: Screenshot of button misalignment in 20070720 nightly

In the 20070720 nightly the toolbar button doesn't look quite right in icon+text mode, the text base line is off. If the icon is subject of this bug, the CSS issue should also be fixed here.

Comment 2

[Stefan Sitter [:ssitter]]

(In reply to comment #1)
This is already filed as Bug 384930 for all calendar icons.

Comment 3

[Patrick]

I really really like the way thunderbird and lightning look in http://wiki.mozilla.org/Calendar:Improving_the_Calendar_Views. Is this due to a certain theme, or is this still to be done. If it is the latter you've got my vote! I just updated my lightning to include the today pane, and it is super! keep it up

Patrick 

Comment 4

[Berend Cornelius]

Attachment: First patch for issue

In the patch attached I (hopefully) resolved all aspects of this issue.
Some considerations that are not yet discussed: I added a midnight schedule that is supposed to switch the miniday one day ahead after midnight. Probably this feature is only desired when the today pane is really in "today-mode". If the user explicitly navigated to the future of past day the midnight schedule is probably not desired.
As can be seen the miniday only is reduced to two lines of text in opposition to the specification http://wiki.mozilla.org/Calendar:Improving_the_Calendar_Views.
This way I could reduce the height of the miniday-pane, but on the other hand the miniday might consume too much width now. If this should be the case a solution would be to abbreviate the monthnames.

Comment 5

[Berend Cornelius]

Attachment: images necessary to apply the patch

The zip-file attached contains the folowing images:
1)image for the toolbar-button within the mail toolbar. This button is still not the final version but a preliminary placeholder hinted by the number "32". The final toolbarbutton is to contain a badge indicating the displayed date of the today-pane. This will be dealt with in a later version.
in reply to comment #3:
The size of the new toolbar-buttons correspond to the size of the other toolbar-buttons in the mail-toolbar. Therefor your issue should not occur anymore
2) images for the buttons to navigate forth and back within the miniday.
2) a background graphic for the miniday-pane

Comment 6

[Berend Cornelius]

in addition to comment #5:
Extractin the zip-file from the calendar directory will automatically place the files at the correct location.

Comment 7

[Berend Cornelius]

Attachment: a new patch

removed a hunk with an image.
Mickey, could you review?

Comment 8

[Berend Cornelius]

Some additional notes about my patch:
Following an animation of Christian I placed the "create new Event" button into the agenda-pane. where it is then located next to the menubutton on top of the agenda-pane. As I estimated this would not be the final location I placed the sourcecode that is triggered by this button to the file "today-pane.js".

Comment 9

[Berend Cornelius]

Attachment: Second patch with reduced sourcecode

I removed some hunks from my patch and pasted them in other patches to resolve issues https://bugzilla.mozilla.org/show_bug.cgi?id=389952 and https://bugzilla.mozilla.org/show_bug.cgi?id=389951

Comment 10

[Michael Büttner (no reviews TFN)]

Comment on attachment 274282 [details] [diff] [review]
Second patch with reduced sourcecode

>+    var agendaPanel = document.getElementById("agenda-box");
>+    var todoPanel = document.getElementById("todo-box");
I suggest naming these elements 'calendar-agenda' and 'calendar-todo' respectively. Furthermore, the groupboxes have been eliminated with bug #389294.

>+    PanePeriod.init();
>+    var date = createDateTime();
>+    date.jsDate = new Date();
>+    date.isDate = true;
>+    date = date.getInTimezone(calendarDefaultTimezone());
>+    scheduleMidnightUpdate(refreshUIBits);
The 'PanePeriod' object has a number of flaws.

1) It shouldn't be a top-level object but rather want to live in the TodayPane scope. I'm wondering why you introduced the new scope but new objects needs to live outside?

2) You don't want to use jsDate at all. Please use calDateTime throughout the implementation. Unless you aren't forced to use jsDate (and here you are not) stick to calDateTime as there are conversion problems, see bug #328442.

3) I'm wondering why you hold the start and end date of the pane if the end date is always just one day ahead of the start? This renders the whole object superfluous, or am I missing anything obvious?

4) Calling scheduleMidnightUpdate() just wasted another timer where there's no need to do so. Please add some code to [1] that updates the today pane stuff. I know that this want to live in an observer but that a story for some other day. Calling scheduleMidnightUpdate() again doesn't make anything better.

>+    var agendapanel = document.getElementById("agenda-box");
>+    var todopanel = document.getElementById("todo-box");
I would like to name variables consistent throughout the code, please see other occurrences of these variables in the same file. For additional help, see [2].

>+  setDay: function(date)
>+  {
>+    var daylabel = document.getElementById("lbldateValue");
>+    daylabel.value = date.day;
>+  
>+    var weekdaylabel = document.getElementById("weekdayNameContainer");
>+    weekdaylabel.selectedIndex = date.weekday;
>+  
>+    var monthnamedeck = document.getElementById("monthNameContainer");
>+    monthnamedeck.selectedIndex = date.month;
>+  
>+    var selMonthPanel = monthnamedeck.selectedPanel;
>+    selMonthPanel.setAttribute("year", date.year);
>+    selMonthPanel.setAttribute("calweek", Math.ceil(date.yearday/7));
>+    selMonthPanel.update();
First, naming scheme. Please don't incorporate types in identifier names (e.g. lbl for label). Hungarian notation is considered bad style, and it doesn't fit in our naming scheme. Furthermore, please adhere to the naming scheme I mentioned in previous patches, names-should-look-like-this.

I wouldn't put the miniday into plain xul into the today-pane as you did with this patch. The miniday should a self-contained entity and not wired up like this. Either create a binding in order to encapsulate it as a new control or make an overlay and add it like the agenda for example. Since the miniday won't be reused throughout the application user interface it's probably better to take the latter approach.

>+// Called at midnight to tell us to redraw the panes
>+function refreshUIBits() {
>+  PanePeriod.advance(1);
>+}
See my comment above regarding refreshUIBits(), but this makes it even worse as you're introducing a name clash. Please don't name top-level functions like already existing ones.

>+<binding id="miniday-monthdisplay" extends="xul:hbox">
>+  <content>
>+    <hbox>
>+      <xul:label anonid="monthdisplay" id="monthlabel" calweek="5555" year="55" xbl:inherits="month,year,calweek"/>
>+      <xul:spacer flex="1"/>
>+    </hbox>
>+  </content>
>+  <implementation>
>+    <field name="mcwlabel">calGetString("calendar", "shortcalendarweek")</field>
>+    <constructor>
>+      <![CDATA[
>+        this.update();
>+      ]]>
>+    </constructor>
>+    <method name="update">
>+        <body>
>+          <![CDATA[
>+            var monthlabel = document.getAnonymousElementByAttribute(this, "anonid", "monthdisplay");
>+            var display = monthlabel.getAttribute("month") + " " +
>+                          monthlabel.getAttribute("year") + 
>+                          ", " + this.mcwlabel + " " +
>+                          monthlabel.getAttribute("calweek");
>+            monthlabel.value = display;
>+          ]]>
>+        </body>
>+    </method>
>+  </implementation>
>+</binding>
As far as I can see, you should get rid of this binding.

>+  <!ENTITY % dtd5 SYSTEM "chrome://calendar/locale/sun-calendar-event-dialog.dtd" > %dtd5;
I can't see a reason for including strings from the event dialog in the today pane. Please move the strings to a more common location, if there are any (I don't see which ones there should be).

>+        <box id="mini-day-box" class="today-subpane">
This probably wants to live in a binding our overlay, see my previous comment above.

>+        <hbox class="miniday-nav-buttons" flex="1" align="start" pack="end">
You don't want to use the 'miniday-nav-buttons' class here.

>+          <vbox flex="1">
>+            <vbox id="agenda-box" persist="height collapsed" flex="1">
>+              <vbox id="agenda-tab-panel" flex="1"/>
>+            </vbox>
>+            <splitter id="today-pane-splitter" persist="hidden"/>
>+            <box id="todo-box" persist="collapsed height" flex="1">
>+              <vbox id="todo-tab-panel" flex="1"/>
>+            </box>
>+        </vbox>
I can't see a reason for putting boxes in boxes in boxes :-) This probably wants just to look like this:

  <vbox id="agenda-tab-panel" flex="1" persist="height collapsed"/>
  <splitter id="today-pane-splitter" persist="hidden"/>
  <vbox id="todo-tab-panel" flex="1" persist="collapsed height"/>

>+    content/calendar/today-pane.xml              (content/today-pane.xml)
Please get rid of this file.

>+#expand    skin/classic/calendar/today-toolbar-large.png  (themes/__THEME__/today-toolbar-large.png)
>+#expand    skin/classic/calendar/today-toolbar-small.png  (themes/__THEME__/today-toolbar-small.png)
Please don't introduce new files for the toolbarbutton. First, it just adds clutter to the themes directory and second (this is more important), it circumvents the established conventions in the *.css files (calendar-toolbar.css). There's a class 'cal-toolbarbutton-1' that points to the file that should contain all icons. That way we just need to add this class to the toolbarbuttons and are free to change the file without remembering any further details. Furthermore, this whole topic should be handled in a different bug as it doesn't have anything to do with the miniday feature.

> #calendar-show-todaypane-button {
>-        -moz-image-region: rect(0px 544px 32px 512px);
>+  list-style-image: url("chrome://calendar/skin/today-toolbar-large.png");
>+  -moz-image-region: rect(0px 32px 32px 0px);
> }
See my comment above, please don't switch to another image file.

>+.miniday-nav-buttons > .previous-day-button:hover,
>+.miniday-nav-buttons > .previous-day-button:hover:active {
>+  -moz-image-region: rect(13px 5px 26px 0px);
>+}
In light of 'Question all usages of the child selector' (see [4]), I'm wondering why this rule is necessary.

>+overlay chrome://calendar/content/today-pane.xul chrome://lightning/content/todo-list-overlay.xul
Please address this issue in a separate bug.

>+++ ./mozilla/calendar/resources/content/datetimepickers/minimonth.xml	2007-07-27 
Please don't touch the minimonth control in this patch as it is not necessary. You're just using a wrong class (minimonth-navbtn) and that's why you also get the logic from the binding in the minimonth. But please don't touch the minimonth.

One more thing that I found is that the miniday looks quite different than what the specification says, see [3]. The buttons (left,today,right) are set apart more than they should.

Last but not least, there are a ton of style nits to be fixed. Please see [2] for the newly created style guide (thanks again to philipp for setting this up).

[1] http://lxr.mozilla.org/mozilla1.8/source/calendar/lightning/content/messenger-overlay-sidebar.js#481
[2] http://wiki.mozilla.org/Calendar:Style_Guide
[3] http://wiki.mozilla.org/Calendar:Improving_the_Calendar_Views#Today_Pane_Switched_On
[4] http://developer.mozilla.org/en/docs/Writing_Efficient_CSS#Question_all_usages_of_the_child_selector.21

Comment 11

[Michael Büttner (no reviews TFN)]

Attachment: un-bitrotten patch

This is an updated version of the latest patch, as it was quite bitrotten.

Comment 12

[Berend Cornelius]

Attachment: overworked patch

in reply to comment #10
Thank you for your thorough revision.I overworked the patch as you demanded. Your objections were well founded, so there is no need to discuss about them.
I did not apply all of the rules of http://wiki.mozilla.org/Calendar:Style_Guide
because I was implementing in files older than the style guide and found it sometimes more appropriate to stick to the existing coding style. I hope that this is fine. 
I extracted the sourcecode for the toolbar button out of this patch and will set up a new issue for this.

Comment 13

[Berend Cornelius]

Attachment: images necessary to run the patch

unzipping the zip-file in 'mozilla/calendar' folder will extract the images to their destination folder

Comment 14

[Berend Cornelius]

Attachment: zipped images necessary to run the patch

unzipping the zip-file in 'mozilla/calendar' folder will extract the images to
their destination folder

Comment 15

[Berend Cornelius]

in reply to comment #11
> One more thing that I found is that the miniday looks quite different than what
> the specification says, see [3]. The buttons (left,today,right) are set apart
> more than they should.
This is true. Christian - just like me - found the buttons too crowded and might lead the user to wrongly click adjacent controls. In general the miniday is different from the specification in various aspects. Majorly we found that it should consume less height which is why in the current patch only two rows of labels are presented.

Comment 16

[Michael Büttner (no reviews TFN)]

First of all, the miniday currently just displays an arbitrary date but doesn't synchronize with the agenda or task panel. I'd consider this as the basic functionality which should be contained in this patch, wouldn't you agree? In order to add this feature I would suggest to let the miniday fire events that are getting hooked up by the agenda and the task panel. See document.createEvent() for further information.

Second, the minimum width of the today pane has been increased quite dramatically with this patch. As you're using a deck that holds all possible names you're forcing the deck to be as wide as the largest string. Of course, I'll obey to Christian's final decision. Furthermore, the Today panel can't be collapsed just as the folder pane on the left, is this intentional?

Third, you renamed the today panel element but didn't update all references. For example, pressing the close button at the top of the today pane reveals this exception:

Error: oTodayPane has no properties
Source File: chrome://lightning/content/messenger-overlay-sidebar.js
Line: 521

Fourth, you moved some strings from sun-calendar-event-dialog.dtd to calendar.dtd but didn't update the necessary includes in the event dialog. Just try opening the custom recurrence dialog in order to see the missing include. Furthermore, it's probably better to move those strings to the global.dtd as this strikes me to be a better match.

Fifth, the minimonth that's embedded in the miniday seems to be quite broken. First, it doesn't close after a new date has been selected. As this is the case at all other occurrences where the minimonth is used I'd consider this as inconsistent. Second, using any one of the selection popups in the minimonth (month, year) causes the minimonth control to appear no longer functional.

Comment 17

[Michael Büttner (no reviews TFN)]

Some more flyby comments. First, I just tested this patch on a Mac and unfortunately the buttons (left, today, right) as well as the popup don't receive their border as they do on Windows. Second, I'd suggest to consolidate the arrow icons that are used in this patch and the ones used in the original minimonth control. Feel free to file a separate bug on this issue, but I think it's worth while to do so.

Comment 18

[Michael Büttner (no reviews TFN)]

Comment on attachment 277159 [details] [diff] [review]
overworked patch

>+  start: null,
Of course this variable is intended to hold the current date of the today pane, respectively the miniday control. Wouldn't it be better to honor that with a more descriptive name instead of naming it just 'start'? The name that springs out of my mind would be 'currentDate'.

>+  cwlabel: null,
This variable is intended to hold the literal string for calGetString("calendar", "shortcalendarweek"). First, the initialization should be as early as possible, so this could just read cwlabel: calGetString(...). Second, you need this only locally inside the upcoming function, so there's no need to hold this at the object scope of the today pane. Third, instead of hardwiring the composition order of the display string it would be superior to use the property mechanism, see nsIStringBundleService::formatStringFromName().

>+    var todaypanebox = document.getElementById("today-pane-panel");
I would suggest to also rename the variable names to match the names of the elements. Also, please compose variable names using mixed case letters starting with a lower case letter.

>+    var agendapanel = document.getElementById("agenda-tab-panel");
>+    var todopanel = document.getElementById("todo-tab-panel");
>+    if (agendapanel.hasAttribute("collapsed")) {
>+      if (!todopanel.hasAttribute("collapsed")) {
Please compose variable names using mixed case letters starting with a lower case letter.

>+    for (var i = 0; i < 12; i++) {
It would be better to use childNodes.length instead of relying on the fact that this particlar array contains 12 elements.

>+      var monthlabel = childNodes[i];
>+      this.setMonthDescription(monthlabel, kYEARINIT, kCALWEEKINIT);
>+    }
I would suggest to get rid of the deck in favor of a simple label that gets cropped if the space doesn't allow for the a larger string instead of this approach.

>+  setMonthDescription: function(aMonthLabel, aYear, aCalWeek)
>+  {
>+    return aMonthLabel.value = aMonthLabel.getAttribute("month") + " " + aYear 
>+                        +  ", " + this.cwlabel + " " +  aCalWeek;
>+  },
See nsIStringBundleService::formatStringFromName() for a better way of doing this.

>+  getDateFromMinimonth: function()
>+  {
>+    document.getElementById("todayMinimonth").update(this.start.jsDate);
>   },
Please name xul elements according to the established naming scheme -> "today-minimonth" in this case.

>     if (this.CurrentPaneView >= nViewLen) {
>-        this.CurrentPaneView = 0;
>+      this.CurrentPaneView = 0;
>     }
>     else if (this.CurrentPaneView == -1) {
>-        this.CurrentPaneView = nViewLen -1;
>+      this.CurrentPaneView = nViewLen -1;
>     }
We agreed on 4 space indentation.

>+  setDaywithjsDate: function(aNewDate)
>+  {
>+    var newdatetime = jsDateToDateTime(aNewDate);
>+    newdatetime = newdatetime.getInTimezone(calendarDefaultTimezone());
>+    return this.setDay(newdatetime);
>+  },
We also agreed on placing opening braces on the same line as the function signatures. Also, I'd write this as follows:
    return this.setDay(
        jsDateToDateTime(aNewDate)
            .getInTimezone(calendarDefaultTimezone());

>+  setDay: function(aNewDate)
>+  {
>+    this.start = aNewDate.clone();
>+
>+    var daylabel = document.getElementById("datevalue-label");
>+    daylabel.value = this.start.day;
>+
>+    var weekdaylabel = document.getElementById("weekdayNameContainer");
>+    weekdaylabel.selectedIndex = this.start.weekday;
>+
>+    var monthnamedeck = document.getElementById("monthNameContainer");
>+    monthnamedeck.selectedIndex = this.start.month;
>+
>+    var selMonthPanel = monthnamedeck.selectedPanel;
>+    return this.setMonthDescription(selMonthPanel, this.start.year, Math.ceil(this.start.yearday/7));
>+  },
Brace placement, and the setMonthDescription() line should adhere to the 80 chars column rule. Apart from that, the date gets cloned twice, once with getInTimezone() and once more explicitly with clone() above. You could save one operation by writing
    this.start = aNewDate..getInTimezone(calendarDefaultTimezone());
and getting rid of this statement in the function above. Just a suggestion...

>+    var oTodayPane = document.getElementById("today-pane-panel");
Please name the variables according to their element names and get rid of these prefixes.

>-        <spacer style="width: 5px;"/>
>+        <spacer width="5"/>
I still prefer to have this specified in a css file.

>+         <box id="mini-day-box" class="today-subpane">
>+           <stack flex="1">
>+             <image id="mini-day-image" flex="1"/>
>+             <hbox flex="1">
>+               <deck id="dateContainer" selectedIndex="0">
>+                 <hbox>
>+                   <spacer flex="1"/>
>+                   <label id="datevalue-label" class="dateValue" pack="end"/>
>+                 </hbox>
>+                 <label class="dateValue" value="55"/>
>+               </deck>
The spacer is superfluous, just move the pack-attribute to the enclosing hbox. Furthermore, the whole deck seams to be not strictly necessary.

>+               ...
>+                 <hbox>
>+                   <deck id ="monthNameContainer">
>+                     <label class="monthlabel" month="&month.1.name;"/>
>+                     <label class="monthlabel" month="&month.2.name;"/>
>+                     <label class="monthlabel" month="&month.3.name;"/>
>+                     <label class="monthlabel" month="&month.4.name;"/>
>+                     <label class="monthlabel" month="&month.5.name;"/>
>+                     <label class="monthlabel" month="&month.6.name;"/>
>+                     <label class="monthlabel" month="&month.7.name;"/>
>+                     <label class="monthlabel" month="&month.8.name;"/>
>+                     <label class="monthlabel" month="&month.9.name;"/>
>+                     <label class="monthlabel" month="&month.10.name;"/>
>+                     <label class="monthlabel" month="&month.11.name;"/>
>+                     <label class="monthlabel" month="&month.12.name;"/>
>+                   </deck>
>+                 </hbox>
I don't see why we need the deck here, you could as easily use a simple label that crops the text with ellipsis for free, instead of forcing a giant minimum width for the today pane.

>+         <vbox flex="1">
>+           <vbox id="agenda-tab-panel" flex="1" persist="height collapsed"/>
>+           <splitter id="today-pane-splitter" persist="hidden"/>
>+           <vbox id="todo-tab-panel" flex="1" persist="collapsed height"/>
>+         </vbox>
>+       </vbox>
In general, I'm wondering why the hierarchy can't be something like?

  <vbox>
    <miniday/>
    <vbox id="agenda-tab-panel" flex="1" persist="height collapsed"/>
    <splitter id="today-pane-splitter" persist="hidden"/>
    <vbox id="todo-tab-panel" flex="1" persist="collapsed height"/>
  </vbox>

If there's no compelling reason for the above version, I would prefer this hierarchy setup.

>diff -a -U 8 -pN -r -x '*.png' ./mozilla_ref/calendar/base/themes/pinstripe/today-pane.css ./mozilla/calendar/base/themes/pinstripe/today-pane.css
The css files are in perfect shape...

>     // refresh the current view, if it has ever been shown
>     var cView = currentView();
>     if (cView.initialized) {
>         cView.goToDay(cView.selectedDay);
>     }
>+    TodayPane.advance(1);
Why not 'TodayPane.setDay(currentView().selectedDay)'? If for some reason this function gets called more than once per day the today pane would step ahead of time...

>+<!ENTITY month.1.name "January" >
>+<!ENTITY month.2.name "February" >
>+<!ENTITY month.3.name "March" >
>+<!ENTITY month.4.name "April" >
>+<!ENTITY month.5.name "May" >
>+<!ENTITY month.6.name "June" >
>+<!ENTITY month.7.name "July" >
>+<!ENTITY month.8.name "August" >
>+<!ENTITY month.9.name "September" >
>+<!ENTITY month.10.name "October" >
>+<!ENTITY month.11.name "November" >
>+<!ENTITY month.12.name "December" >
Probably this should live in global.dtd. Sorry for being insane and having suggested this in the first place.

Comment 19

[Christian Jansen]

(In reply to comment #17)
> [...] Second, I'd suggest to consolidate
> the arrow icons that are used in this patch and the ones used in the original
> minimonth control. Feel free to file a separate bug on this issue, but I think
> it's worth while to do so.

We should wait with this till we know how the mini month will look like.

Comment 20

[Michael Büttner (no reviews TFN)]

(In reply to comment #19)
> We should wait with this till we know how the mini month will look like.
The minimonth *currently* uses visually indistinguishable imagery, thus it should be consolidated as soon as possible. Experience attests that this won't ever be done if we don't at least file a bug on that.

Comment 21

[Berend Cornelius]

Attachment: intermediate patch

This is just an intermediate patch where I assigned some of mickey's objected issues. I rearranged the boxes of the miniday and reduced the margins and paddings in order to reduce the minimal width of the todaypane. In this process I changed the behaviour of the splitter, so that the splitter is now able to close and reopen the todaypane. Meanwhile this behaviour has been demanded by bug 390982, so  I will shift the according implementation to the patch of that bug.

Comment 22

[Berend Cornelius]

In reply to comment #
> We agreed on 4 space indentation.
..
> We also agreed on placing opening braces on the same line as the function
> signatures. 

I suggest to extend the styleguide in http://wiki.mozilla.org/Calendar:Style_Guide by an additional instruction like:
The style of added implementations should also incorporate themselves to the style of the existing implementation. If the existing coding style of a file differs from  the existing styleguide it will sometimes make more senses to pursue this.

Comment 23

[Michael Büttner (no reviews TFN)]

(In reply to comment #22)
> The style of added implementations should also incorporate themselves to the
> style of the existing implementation. If the existing coding style of a file
> differs from  the existing styleguide it will sometimes make more senses to
> pursue this.
Allowing to incorporate existing style would render the style guide practically useless, since we currently don't have a consistent style. The whole idea was to have a set of rules that everybody can easily follow without the need to discuss about this particular annoying issue.

Comment 24

[Berend Cornelius]

> Allowing to incorporate existing style would render the style guide practically
> useless,
I totally disagree. A guide is supposed to give the user an orientation about how to do something, but it should not necessarily demand slavish obedience when there might be a better solution for a certain context.

Comment 25

[Stefan Sitter [:ssitter]]

Michael, Berend: I suggest to move the discussion about how and when to apply style guidelines / style rules to a more appropriate place (e.g. newsgroup) because it's not related to the issue tracked with this bug.

Comment 26

[Berend Cornelius]

Attachment: patch with additional functionality

In this patch I addressed the concerns of mickey:
The following additional features were added/changed:
-The miniday now also changes the date of the agenda-box by applying the start date  as the "today" date in the agenda-box.
This raised the problem how to display this date when it's not equal to "today": For this purpose I added a method "showsToday()" to the today-pane. If this method returns 'false' the celltexts of the agenda-box that usually contain "today", "tomorrow" and "soon" are then replaced by a formatted string indicating the respective period. 
-midnight updates are only invoked when the today pane is not in "showsToday-mode".
-I changed the arrangement of the boxes in the miniday-pane, as Christian was concerned about a too large minimal width of the today-pane. Because of that I also display monthnames only as shortnames (eg. "Dec").
- The splitter now also allows to collapse and reopen the today-pane.
- The minimonth works now consistent to the other minimonths. I could not reproduce any freezing.
Some notes about the implementation:
Including "calUtils.js" into the xul-files of agenda-tree-overlay.xul and today-pane.xul raised an error in line 43 of calUtils.js. ("redeclaration of const Cc"). I worked around this problem by a later definition of some variables  in the today-pane.js (mickey objected to this in comment #18) and by duplicating the method "sameDay" right into "agenda-tree.js". I know that this must be changed but I suggest to do that in a later issue.

Comment 27

[Berend Cornelius]

> -midnight updates are only invoked when the today pane is not in
> "showsToday-mode".

This is wrong. It must be: -midnight updates are only invoked when the today pane is in "showsToday-mode".

Comment 28

[Christian Jansen]

Berend, thanks for the patch. I have installed the patch on a Mac (see attachment) and works really great.

Regarding the UI, please find my 2 cts below:
 
General:
 - Total width should be reduced by 25 to 30% 

Day Number:
 - Left space should be reduced if number is below 10

Month & Year Information
 - Should be left aligned with Weekday
 
Nav Buttons:
 - Spaces between Back, Today, Forward should be reduced
 - Feedback for (mouse down) could be improved, but this
   seems to be a Mac only problem
 
Drop Down
 - Should be reduced to a total width of approx. 15px

Comment 29

[Christian Jansen]

Attachment: Screenshot of Mini Day on MAc

Comment 30

[Michael Büttner (no reviews TFN)]

Comment on attachment 280140 [details] [diff] [review]
patch with additional functionality

First of all, the images inside of the buttons are left-aligned. It would probably look better if there wouldn't be any space between the images and the border of the buttons. This applies to the left-, today-, right- and 'drop down'-button of the mini day.

Second, the listener that gets triggered in case the default timezone is missing. This is minor but since it's not that big deal I suspect it should go into this patch as otherwise it gets lost after this has landed.

>+    return aMonthLabel.value = this.dateFormatter.shortMonthName(aIndex)
>+            + " " + aYear +  ", " + this.cwlabel + " " +  aCalWeek;
Since we're already in string freeze this is a bit unfortunate. This should be located in a properties files in order to allow different languages a different element order. Please file a spin-off bug which can be resolved as soon as 0.7 is out of the door.

>+  getDateFromMinimonth: function()
>+  {
>+    document.getElementById("todayMinimonth").update(this.start.jsDate);
This function isn't referenced.

>+  setDay: function(aNewDate)
      ...
>+    if (agendaTreeView.initialized) {
>+      this.updatePeriod();
>+    }
In order to further reduce dependencies between unrelated modules I still vote for using generic events instead of first-class coupling (i.e. call directly the agenda, etc.).

>+  updatePeriod: function() {
>+    var date = this.start.clone();
>+    return agendaTreeView.refreshPeriodDates(date);
See my previous comment.

>-    <splitter id="gray_vertical_splitter" collapse="true" persist="state" resizebefore="closest">
>+    <splitter id="today-splitter" collapse="after" persist="state" resizebefore="closest">
Originally this was part of Bug 390982. I would prefer to not mix different patches as it makes me insane... :-)

>+function sameDay(date1, date2) {
>+   if (date1 && date2) {
>+       if ((date1.day == date2.day) &&
>+          (date1.month == date2.month) &&
>+          (date1.year == date2.year)) {
>+          return true;
>+       }
>+   }
>+   return false;
>+ }
> }
This function is *inside* sendMailTo()...

> agendaTreeView.getCellText =
> function getCellText(row, column)
> {
>+    var dateFormatter = Components.classes["@mozilla.org/calendar/datetime-formatter;1"]
>+                                  .getService(Components.interfaces.calIDateTimeFormatter);
The today pane caches the calIDateTimeFormatter service but in this function you don't. I vote for a consistent way of accessing components and my preference is like you did it here.

>+                    var startString = new Object();
>+                    var endString = new Object();
>+                    dateFormatter.formatInterval(
>+                        this.soon.start, this.soon.end, startString, endString);
>+                    var dateString = startString.value + " - " + endString.value;
>+                    return dateString;
Another candidate for a properties file.

>+agendaTreeView.addListener = 
>+function addListener(aListener) {
>+    this.mListener = aListener;
>+}
This wouldn't be necessary if you'd employ custom events.

> function setAgendaTreeView()
> {
>     agendaTreeView.setCalendar(getCompositeCalendar());
>     document.getElementById("agenda-tree").view = agendaTreeView;
>+    var cs = Components.classes["@mozilla.org/consoleservice;1"]...
Debug code?

>+function sameDay(date1, date2) {
>+   if (date1 && date2) {
>+       if ((date1.day == date2.day) &&
>+          (date1.month == date2.month) &&
>+          (date1.year == date2.year)) {
>+          return true;
>+       }
>+   }
>+   return false;
>+ }
You already had this function in calUtils. The 'redeclaration of Components.classes' is due to the fact that it is a redeclaration of a top-level const variable, see Bug 390842. I would provide clean solutions instead of introducing more and more workarounds. Admittedly, the time is short, but the patch for bug 390842 is a two-liner.

> function refreshUIBits() {
>-    agendaTreeView.refreshPeriodDates();
>     document.getElementById("ltnMinimonth").refreshDisplay();
> 
>     // refresh the current view, if it has ever been shown
>     var cView = currentView();
>     if (cView.initialized) {
>         cView.goToDay(cView.selectedDay);
>     }
>+    if (TodayPane.showsToday()) {
>+      TodayPane.setDay(today());
>+    }
> }
You removed the call to scheduleMidnightUpdate()...

>+<!ENTITY month.1.name "January" >
>+<!ENTITY month.2.name "February" >
>+<!ENTITY month.3.name "March" >
>+<!ENTITY month.4.name "April" >
>+<!ENTITY month.5.name "May" >
>+<!ENTITY month.6.name "June" >
>+<!ENTITY month.7.name "July" >
>+<!ENTITY month.8.name "August" >
>+<!ENTITY month.9.name "September" >
>+<!ENTITY month.10.name "October" >
>+<!ENTITY month.11.name "November" >
>+<!ENTITY month.12.name "December" >
This should wait until 0.7 is out of the door.

Comment 31

[Michael Büttner (no reviews TFN)]

I forgot to mention that this patch still has the problem that the minimonth freezes after one of the child popups has been accessed. At least this happens on Windows. Open for example the year popup and select an arbitrary year or click somewhere else to close the popup. After that the minimonth won't close if a date has been selected or clicking outside of the control. The only way to make it disappear is to press the escape key.

Comment 32

[Simon Paquet [:sipaq]]

(In reply to comment #30)
> >+<!ENTITY month.1.name "January" >
> >+<!ENTITY month.2.name "February" >
> >+<!ENTITY month.3.name "March" >
> >+<!ENTITY month.4.name "April" >
> >+<!ENTITY month.5.name "May" >
> >+<!ENTITY month.6.name "June" >
> >+<!ENTITY month.7.name "July" >
> >+<!ENTITY month.8.name "August" >
> >+<!ENTITY month.9.name "September" >
> >+<!ENTITY month.10.name "October" >
> >+<!ENTITY month.11.name "November" >
> >+<!ENTITY month.12.name "December" >
>
> This should wait until 0.7 is out of the door.

Yes, please file a followup-bug for this. And please also check (if you haven't already) if sun-calendar-event-dialog-attendees.xul needs to include global.dtd as well with that change since it is the only file that I could find, that includes sun-calendar-event-dialog.dtd but not global.dtd.

Comment 33

[Stefan Sitter [:ssitter]]

(In reply to comment #31)
Known issue: Bug 356505

Comment 34

[Stefan Sitter [:ssitter]]

Comment on attachment 280140 [details] [diff] [review]
patch with additional functionality

>+++ ./mozilla/calendar/lightning/content/agenda-tree.js
>@@ -330,27 +354,27 @@ function agendaDoubleClick(event)
>-    createEventWithDialog(calendar, today(), today());
>+    createEventWithDialog(calendar, this.today.start, this.today.start);

>-        createEventWithDialog(calendar, today(), today());
>+        createEventWithDialog(calendar, this.today.start, this.today.start);

>         createEventWithDialog(calendar, tom, tom);

It would be nice if you could remove the third parameter. Otherwise I need to update the patch in Bug 390300 that is currently waiting for your review.

Comment 35

[Christian Jansen]

I've tested the patch on Mac, thus all my comments are relevant
to that platform, only. From my point of view this feature should look
visually good before the patch lands. If not we might ran into the
situation that our don't like using the Today Pane. I'd be happy
if somebody could comment on my issues, who has applied the patch on
Windows:

 
 * Width:
   - Please reduce the space between the "Previous" "Home"
    "Next" buttons to a minimum
   - Please reduce the width of the drop-down
   - We should try to have an initial width of approx. 200px (on Mac
     & Win)
 
 * Day Number:
   - If possible it would be great to center the numbers from
     1 to 9, this would reduce the space left, in front of the
     day number.
   
 * Month & Year
   - Please left-align this with the Weekday

 * Weekday
   - Please short the day do an abbreviated version. I'll file a
     spin-off bug for having the same behavior like the Weekday title in
     the Weekview. (Thanks for the hint, Mickey) ;-)
 
 * Navigation
   - Only for "Today" the Event for Today, Tomorrow & Soon should
     be displayed

 * UI Feedback
   - On mouse over and on mouse click should provide feedback on Mac.



UI+ with the changes above. Centering the day number, the Navigation, plus the UI Feedback on Mac should not block this patch. I'll write spin-off bugs for that. It would be great if Weekday "auto-abbrivatie" would work in that patch, if not it might make sensego with an abbreviated day nomenclature.
 
Please note that I'll be a conference next week (Sept 18 - 21). I'm not sure  if I'm able to check mails frequently.
Thanks. Christian

Comment 36

[Berend Cornelius]

in reply to comment #35:
I addressed the width issue. The margin between the navigation buttons are reduced to 0. The  width of the dropdown button and the navigation buttons was very large under Mac and I reduced this too. I used the abbreviated version of the weekday labels and will file a follow-up bug to implement the same behaviour as the weekday-title.
Tomorrow and soon will be displayed for all days but under a different label than "tomorrow" and "soon" denoting the exact day.
UI feedback under Mac is not provided. I suggest to add grey borders around the navigation buttons.

Comment 37

[Berend Cornelius]

Attachment: new patch that addressed Christian's concerns [checkedin]

Comment 38

[Michael Büttner (no reviews TFN)]

Comment on attachment 281637 [details] [diff] [review]
new patch that addressed Christian's concerns [checkedin]

r=mickey.

Comment 39

[Christian Jansen]

Hi Berend,
the patch works pretty well, but I found the following nits (Win XP)

- The Nav. Button "Previous" seams to be Left aligned, please align it right
- "Home" Button seams to be Left aligned, please change it to centered
- The Drop Down Indicator seams to be left aligned, please change to centered
- If the minimonth is opened, a click on the dropdown should close mini month pop-up.

Comment 40

[Berend Cornelius]

in reply to comment #39. I don't think I will be able to fix the alignment problem  by the 0.7 release and the problem with the minimonth as well. A solution for the latter problem would be to remove the drop-down button from the miniday.

Comment 41

[Michael Büttner (no reviews TFN)]

(In reply to comment #40)
> problem  by the 0.7 release and the problem with the minimonth...
I'm really wondering why it's working in the date/time-picker controls (used for example in the event dialog) where the same scenario works flawlessly.

Just for clarification, I'm fine with addressing the remaining bits and pieces in one or more spin-off bugs as I don't want to hold this off for 0.7. On the contrary it should be noted that the above stated review comments are still outstanding and the spin-off bugs haven't been filed.

Comment 42

[Berend Cornelius]

I filed Bug 396898 as a followup where I collected remaining issues.
In reply to comment #30

> The today pane caches the calIDateTimeFormatter service but in this function
> you don't. I vote for a consistent way of accessing components and my
> preference is like you did it here.

In the todaypane.xul I implemented the calIDateTimeFormatter from the scratch on my own but in the agendatree.js I did not want to change an existing implementation more than necessary.

Comment 43

[Berend Cornelius]

Comment on attachment 281637 [details] [diff] [review]
new patch that addressed Christian's concerns [checkedin]

patch checked in on trunk and MOZILLA_1_8_BRANCH.
I am sorry for forgetting to add "r=mickey" when checking in.

Comment 44

[Omar Bajraszewski]

Verified with lightning 2007092403

Comment 45

[seshu.parvataneni]

It is nice to see this implemented. I've tested it and the UI looks good. However, at first I could not understand what the CW stood for in the UI. Maybe clock wise? maybe Clock Work? may be some garbage? But none of this made any sense. After I looked for it in this bug I realized that it stands for Calendar Week which I am well aware of. I think the abbreviation should be a bit more obvious and should definitely be mentioned in the documentation for lightning. Once again good work on implementing this feature. It looks good. Thanks.

Comment 46

[Berend Cornelius]

in reply to comment #45:
Pleas see bug 396898 where I addressed this issue

Comment 47

[Christian Jansen]

CW = Calendar Week