329225 - minimonth and calendar views are not in sync

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Thomas Benisch]

The minimonth and the calendar views (calendar-month-view.xml, 
calandar-multiday-view.xml) are not in sync.

When navigating in the minimonth, e.g. switching to the next month
by pressing the navigation button, the calendar view is not updated
and still displays the old month/week/day.

In addition, when navigating in the calendar view, the minimonth
is not updated. Although another day is selected in the calendar
view, the minimonth shows the initial selected day.

Comment 1

[Thomas Benisch]

Attachment: patch which keeps minimonth and calendar views in sync

This patch keeps the minimonth and the calendar views in sync.

The minimonth notifies the views via the already existing
'change' event. The views notify the minimonth via the
newly introduced 'daychange' event.

In addition to the date range also the selected day is
kept in sync. This required to change the internal behaviour
of the minimonth. That means when navigating in the 
minimonth the selected day in the minimonth changes.

Just an example: The minimonth displays March 2006 with
March 10th selected. When switching to April 2006 by
clicking on the navigation button, the selected day
changes to April 10th. That's the same behaviour as
in the month view.

As the minimonth is also used in the Lightning datepicker 
and in SunBird, and there the old behaviour is 
probably favoured, I introduced a new binding
(minimonth-extended) which is derived from the minimonth
binding.

Comment 2

[Thomas Benisch]

Attachment: patch v2

removed debugger statements

Comment 3

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

I'm not sure if we want this. I always had the idea that the minimonth was some quick navigation widget. As such, the minimonth and the main view don't need to be in sync.

Comment 4

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

Comment on attachment 214904 [details] [diff] [review]
patch v2

More technical comment: instead of adding a wrapper, I think it would be cleaner to add a attribute. Adding wrappers can end up messy. And the wrapper doesn't seem to depend on anything, and is quite light-weight.

Comment 5

[Thomas Benisch]

(In reply to comment #3)
I thought it's obvious, that the minimonth and the calendar views are in sync.
Probably I'm wrong. Does anybody else have an opinion on this?

Comment 6

[Thomas Benisch]

(In reply to comment #4)
I think this makes sense. I didn't have a good feeling when extending the
minimonth binding.

Comment 7

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

(In reply to comment #5)
> I thought it's obvious, that the minimonth and the calendar views are in sync.
> Probably I'm wrong. Does anybody else have an opinion on this?

I'm just used to them being not in sync, and never considered anything else :)
For what it is worth, evolution has the minical and the real views in sync. Outlook 2000 also syncs. Somebody willing to test mac iCal and newer outlooks?

Another thing: I think that sunbird and lightning should behave the same. I don't see a need for the split there.

Comment 8

[Joey Minta]

(In reply to comment #7)
> I'm just used to them being not in sync, and never considered anything else :)
> For what it is worth, evolution has the minical and the real views in sync.
> Outlook 2000 also syncs. Somebody willing to test mac iCal and newer outlooks?
iCal syncs them up as well, with the current view-range slightly highlighted.  Oulook 2005 has three minimonths, and similar highlighting of the current range.

Comment 9

[Thomas Benisch]

Attachment: patch v3

Instead of deriving from the minimonth binding I added an attribute "autoselect".
In addition, I adapted the patch to the current work tree.

Comment 10

[Joey Minta]

Comment on attachment 215988 [details] [diff] [review]
patch v3

+      <method name="switchMonthYear">
+        <parameter name="aDate"/>
+        <parameter name="aMonth"/>
+        <parameter name="aYear"/>
+        <parameter name="bAutoSelect"/>
+        <body>
+          <![CDATA[
+            var date = Components.classes["@mozilla.org/calendar/datetime;1"]
+                                 .createInstance(Components.interfaces.calIDateTime);
A previous debate/discussion regarding the future of the minimonth widget resulted in a decision to work towards implementing it in the general mozilla toolkit.  Doing this means that it can not utilize calIDateTime, because that depends on libical, which we do not want to ship with toolkit.  I think it would be better if the calculations in this function were done purely with javascript Date objects in order to keep with that decision.

Comment 11

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

Comment on attachment 215988 [details] [diff] [review]
patch v3

This patch is only for lightning. Can you port it to work in sunbird (and possibly even the calendar extension) ?
I think it is adding the attribute to the <minimonth/>, and adding an event listener.

Other then what jminta already said, this patch looks good. Thanks so far!

Comment 12

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

Comment on attachment 215988 [details] [diff] [review]
patch v3

marking r-, based on jminta's comment. We try to keep the base minimonth and timepickers generic enough to move them into toolkit one day.
(I know it's inconsitent with the move to calIDAteTime everywhere else, but such is life...(

Comment 13

[Thomas Benisch]

Attachment: patch v4

In this patch the minimonth uses the JavaScript Date object only, calIDateTime
is not used anymore.
I also added code, so that syncing works for SunBird and the calendar extension.
Please note, that I also reworked some of the code which affects Lightning.

Comment 14

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

Comment on attachment 216744 [details] [diff] [review]
patch v4

The patch looks good to me,  but i'm asking jminta for a second look, because he has more experience in xbl hacking.

Comment 15

[Joey Minta]

Comment on attachment 216744 [details] [diff] [review]
patch v4

Clicking on a day outside of the shown month in the views selects that month in the minimonth, which seems distinctly out of sync.  ie. Select March 15 in the the views. Then select Feb 28 in the views.  The views remain showing march, but the minimonth changes.

The biggest other issue is that there needs to be more documentation about these changes.

-          var d = this.mStartDate.clone();
-          while (d.compare(this.mEndDate) <= 0) {
-            count++;
-            d.day += 1;
-            d.normalize();
+          if (this.mStartDate != null) {
+            var d = this.mStartDate.clone();
+            while (d.compare(this.mEndDate) <= 0) {
+                count++;
+                d.day += 1;
+                d.normalize();
+            }
           }
For awhile now, we've been writing code that implicitly expects .selectedDay to throw if the view has not been initialized.  These changes make it so that an error is no longer thrown.  I'm not sure what other behavioral changes may accompany this change.  Can you describe the motivation for it?  We also need to document more fully in the idl what is to be expected in this case.

-                         onchange="gCalendarWindow.goToDay( this.value );"/>
+                         onchange="onMinimonthChange();"/>
Why are we forking this?  This change creates two separate functions, each of which appears to make the views show a specific day.  Without a comment or two, it's unclear whether they behave differently or which one should be used in various other cases.

+            var autoselect = (this.getAttribute("autoselect") == "true");
This is the biggest need for documentation.  It needs to be crystal clear what this 'autoselect' attribute does.  You and I know it from this bug, but someone who see the minimonth in toolkit has no way of knowing.

+            // normalize month/year
+            var month = aMonth;
+            var year = aYear;
+            if (month < 0 || month > 11) {
+                var offset = (month - month % 12) / 12 - (month < 0 ? 1 : 0);
+                month -= offset * 12;
+                year += offset;
+            }
+
+            var date = new Date(aDate);
+            date.setMonth(month);
+            date.setFullYear(year);
+
+            // correct for accidental rollover into the next month
+            var oldMonth = aDate.getMonth();
+            if ((date.getMonth() - month + oldMonth + 12)%12 != oldMonth) {
+                date.setDate(0);
+            }

Two things: 1) js Date()s have auto-normalization that you can take advantage of to make this much cleaner/faster. 2) This math seems like it will fail if aMonth is less than -12.  While you don't pass anything in like that, it's not clear why someone else couldn't.  I'd suggest something like:
    var date = new Date(aYear, aMonth, aDate.getDate());

    // Correct for accidental rollover into the next month (ie, moving from
    // 30 Jan to February will end up in March).
    var oldMonth = aDate.getMonth();
    if (Math.abs((date.getMonth() - aMonth + oldMonth)%12) != oldMonth) {
        // Setting date to 0 is the same as picking the last day in the
        // previous month
        date.setDate(0);
    }

This behaved exactly the same as the code here in local testing.  If there's other magic in these lines that I'm missing though, then that's just another reason for more code comments.

Comment 16

[Joey Minta]

Comment on attachment 216744 [details] [diff] [review]
patch v4

+    if (minimonthDay && !(viewDay && gCalendarWindow.dateFormater.isOnSameDate(minimonthDay, viewDay.jsDate))) {
+        gCalendarWindow.goToDay(minimonthDay);
gCalendarWindow.dateFormater disappeared today.

Comment 17

[Thomas Benisch]

(In reply to comment #15)
> (From update of attachment 216744 [details] [diff] [review] [edit])
> Clicking on a day outside of the shown month in the views selects that month in
> the minimonth, which seems distinctly out of sync.  ie. Select March 15 in the
> the views. Then select Feb 28 in the views.  The views remain showing march,
> but the minimonth changes.

I will investigate this.

> The biggest other issue is that there needs to be more documentation about
> these changes.
> 
> -          var d = this.mStartDate.clone();
> -          while (d.compare(this.mEndDate) <= 0) {
> -            count++;
> -            d.day += 1;
> -            d.normalize();
> +          if (this.mStartDate != null) {
> +            var d = this.mStartDate.clone();
> +            while (d.compare(this.mEndDate) <= 0) {
> +                count++;
> +                d.day += 1;
> +                d.normalize();
> +            }
>            }
> For awhile now, we've been writing code that implicitly expects .selectedDay to
> throw if the view has not been initialized.  These changes make it so that an
> error is no longer thrown.  I'm not sure what other behavioral changes may
> accompany this change.  Can you describe the motivation for it?  We also need
> to document more fully in the idl what is to be expected in this case.

When calling the getter for the selectedDay property, I personally prefer code
which checks for null rather than catching exceptions, that means I prefer

    var selectedDay = view.selectedDay;
    if (selectedDay) {
        ....
	}

instead of

    var selectedDay;
    try {
        var selectedDay = view.selectedDay;
    } catch(ex) {} // This dies if no view has even been chosen this session

    if (selectedDay) {
        ...
    }

But if the fix above changes the fundamental behaviour, than I have no problem
with catching exceptions.

> 
> -                         onchange="gCalendarWindow.goToDay( this.value );"/>
> +                         onchange="onMinimonthChange();"/>
> Why are we forking this?  This change creates two separate functions, each of
> which appears to make the views show a specific day.  Without a comment or two,
> it's unclear whether they behave differently or which one should be used in
> various other cases.

As you might see in the code, onMinimonthChange() is simply a wrapper, which
calls goToDay(newDate) only, if the selectedDay in the view is different from
newDate. As I need this functionality in order to get rid of infinite callbacks,
I see only those alternatives:
a) write a wrapper which calls goToDay()
b) change the implementation of goToDay(),
   so that always newDate is compared to the selectedDay
c) add an additional parameter to goToDay();
   if this parameter is true, compare newDate and the selectedDay
What are you favouring? Or did I misunderstand you?

> +            var autoselect = (this.getAttribute("autoselect") == "true");
> This is the biggest need for documentation.  It needs to be crystal clear what
> this 'autoselect' attribute does.  You and I know it from this bug, but someone
> who see the minimonth in toolkit has no way of knowing.
> 
> +            // normalize month/year
> +            var month = aMonth;
> +            var year = aYear;
> +            if (month < 0 || month > 11) {
> +                var offset = (month - month % 12) / 12 - (month < 0 ? 1 : 0);
> +                month -= offset * 12;
> +                year += offset;
> +            }
> +
> +            var date = new Date(aDate);
> +            date.setMonth(month);
> +            date.setFullYear(year);
> +
> +            // correct for accidental rollover into the next month
> +            var oldMonth = aDate.getMonth();
> +            if ((date.getMonth() - month + oldMonth + 12)%12 != oldMonth) {
> +                date.setDate(0);
> +            }
> 
> Two things: 1) js Date()s have auto-normalization that you can take advantage
> of to make this much cleaner/faster. 2) This math seems like it will fail if
> aMonth is less than -12.  While you don't pass anything in like that, it's not
> clear why someone else couldn't.  I'd suggest something like:
>     var date = new Date(aYear, aMonth, aDate.getDate());
> 
>     // Correct for accidental rollover into the next month (ie, moving from
>     // 30 Jan to February will end up in March).
>     var oldMonth = aDate.getMonth();
>     if (Math.abs((date.getMonth() - aMonth + oldMonth)%12) != oldMonth) {
>         // Setting date to 0 is the same as picking the last day in the
>         // previous month
>         date.setDate(0);
>     }
> 
> This behaved exactly the same as the code here in local testing.  If there's
> other magic in these lines that I'm missing though, then that's just another
> reason for more code comments.

Refering to my piece of code, you're right, that the correction for
accidental rollover fails for month < -12. But the normalization
of month/year ensures, that month is in the range [0,11]. Therefore the
whole code works, but I agree that it looks magic. The algorithm for
calculating the rollover corrections was not my idea, but I copied it
from the decorated month view.

I tested your piece of code, and the correction for accidental rollover
fails, when e.g. switching from Dec 28th, 2005 to Jan 28th, 2006.
I think the usage of Math.abs() is wrong here. If you get -5 as a
result from (date.getMonth() - aMonth + oldMonth)%12), then Math.abs()
converts it to +5, but what you want is +7 (-5 + 12).

Nevertheless I think using the auto-normalization from jsDate is a good
idea. So after rethinking about the problem I came up with this proposal:

    var date = new Date(aYear, aMonth, aDate.getDate());

    // Correct for accidental rollover into the next month (ie, moving from
    // 30 Jan to February will end up in March).
    if ((date.getMonth() - aMonth)%12 != 0) {
        // Setting date to 0 is the same as picking the last day in the
        // previous month
        date.setDate(0);
    }

Comment 18

[Thomas Benisch]

(In reply to comment #15)
> (From update of attachment 216744 [details] [diff] [review] [edit])
> Clicking on a day outside of the shown month in the views selects that month in
> the minimonth, which seems distinctly out of sync.  ie. Select March 15 in the
> the views. Then select Feb 28 in the views.  The views remain showing march,
> but the minimonth changes.

I propose to fix this in that way, that when setting a new minimonth value the
minimonth doesn't switch its month, if the new value is still visible in the
current minimonth view. As a consequence, also the minimonth behaviour would
change. If the minimonth displays March 2006, and I select in this minimonth
view February 28th, then the minimonth wouldn't change its month view to
February (as it is done now). I think this would be consistent with the
month and week views. What do you think?

Comment 19

[Joey Minta]

(In reply to comment #18)
> (In reply to comment #15)
> > (From update of attachment 216744 [details] [diff] [review] [edit] [edit])
> > Clicking on a day outside of the shown month in the views selects that month in
> > the minimonth, which seems distinctly out of sync.  ie. Select March 15 in the
> > the views. Then select Feb 28 in the views.  The views remain showing march,
> > but the minimonth changes.
> 
> I propose to fix this in that way, that when setting a new minimonth value the
> minimonth doesn't switch its month, if the new value is still visible in the
> current minimonth view. As a consequence, also the minimonth behaviour would
> change. If the minimonth displays March 2006, and I select in this minimonth
> view February 28th, then the minimonth wouldn't change its month view to
> February (as it is done now). I think this would be consistent with the
> month and week views. What do you think?
> 
This seems reasonable with the exception of one small issue:  The minimonth always shows 6 rows, whereas the the month-view sometimes shows 5, sometimes 6 (and in rare cases 4).  We may need a preliminary (or a followup) patch to fix that behavior as well, otherwise the sync still won't be nice.  Overall, since this is how the month-view handles it, this seems reasonable in my book.

Comment 20

[Joey Minta]

(In reply to comment #17)
> (In reply to comment #15)
> > -                         onchange="gCalendarWindow.goToDay( this.value );"/>
> > +                         onchange="onMinimonthChange();"/>
> > Why are we forking this?  This change creates two separate functions, each of
> > which appears to make the views show a specific day.  Without a comment or two,
> > it's unclear whether they behave differently or which one should be used in
> > various other cases.
> 
> As you might see in the code, onMinimonthChange() is simply a wrapper, which
> calls goToDay(newDate) only, if the selectedDay in the view is different from
> newDate. As I need this functionality in order to get rid of infinite
> callbacks,
> I see only those alternatives:
> a) write a wrapper which calls goToDay()
> b) change the implementation of goToDay(),
>    so that always newDate is compared to the selectedDay
> c) add an additional parameter to goToDay();
>    if this parameter is true, compare newDate and the selectedDay
> What are you favouring? Or did I misunderstand you?
There are currently two code-use-cases for goToDay() that I can recall.  One is obviously to go to the current day in the view.  The other is as a way to force the view to refresh itself.  The need for refreshing is shrinking every day, such that I'm not sure there are any calls of it left in non-xbl code.  My main concern is that the patch, as written, didn't make it clear what the differences between the two were (and the function name didn't offer any clue either.)  I'm ok with the fork as long as it's well documented (This is (a)).  The ideal solution would probably be (b), if we can get away with it.  (c) is also feasible, again with proper documentation.

>
> ... So after rethinking about the problem I came up with this proposal:
> 
>     var date = new Date(aYear, aMonth, aDate.getDate());
> 
>     // Correct for accidental rollover into the next month (ie, moving from
>     // 30 Jan to February will end up in March).
>     if ((date.getMonth() - aMonth)%12 != 0) {
>         // Setting date to 0 is the same as picking the last day in the
>         // previous month
>         date.setDate(0);
>     }
This looks much better and less magical.

Comment 21

[Thomas Benisch]

Attachment: patch v5

This patch hopefully fixes all issues from comments #15 to #20.

I changed my mind concerning comment #18. I didn't change the
internal behaviour of the minimonth, that means when clicking
on a day outside of the main month, the minimonth will switch
to the new month. 

Changing this behaviour would have required a method on the
month view (or even extend calIDecoratedView), which allows
to switch the view to a specified month and select a specified
day.

In addition I didn't fix the behaviour of the month view,
which displays different number of rows depending on the
month. I think this can be done in a followup issue.

Comment 22

[Thomas Benisch]

(In reply to comment #21)
I forgot to mention, that I also changed the implementation of the 
moveView method of the calendar-decorated-month-view binding.
When a day outside of the main month was selected, clicking on
the navigation buttons didn't switch the view to the next or
previous month is some cases.

Comment 23

[Thomas Benisch]

(In reply to comment #21)
> In addition I didn't fix the behaviour of the month view,
> which displays different number of rows depending on the
> month. I think this can be done in a followup issue.

The followup issue is #332997.

Comment 24

[Thomas Benisch]

Thomas Benisch -> Joey Minta:
I want to make some progress with this patch.
Any further comments from your side?

Comment 25

[Joey Minta]

(In reply to comment #24)
> Thomas Benisch -> Joey Minta:
> I want to make some progress with this patch.
> Any further comments from your side?
> 
I was looking at this again last night.  (My schedule is finally opening up a bit.)  I'll try and have some more thoughts on this later today.

Comment 26

[Joey Minta]

Comment on attachment 217418 [details] [diff] [review]
patch v5

Some code comments follow.  I still need to go through and test this stuff:

+      <property name="mainStartDate" readonly="true">
+        <getter><![CDATA[
+          // Returns the start date of the main month.
+          // Unless the month actually starts on a Sunday, this means that
+          // mStartDate.month is 1 month less than the main month.
+          var date = this.mStartDate.clone();
+          if (this.mStartDate.day != 1) {

This looks likes its going to return really strange results for the multiweek view.

+              date.month += 1;
+              date.normalize();
+		      // correct for accidental rollover into the next month
+              if ((date.month - this.mStartDate.month - 1)%12 != 0) {
+                  date.month -= 1;
+                  date.normalize();
+            }
+          }
+          return date.startOfMonth;
+        ]]></getter>
+      </property>
Normalizing twice looks weird here, and slow.  Why not set the day to 1 and then +1 then month?  You'll stay normal and get the start of the month that way.

-            this.fireEvent("dayselect");
+            var detail = { selectedDay: val, mainStartDate: val.startOfMonth };
+            this.fireEvent("dayselect", detail);
             return val;
This seems weird to include.  I'm not sure I would expect the mainStartDate to be related to the day selected.

+    var minimonth = document.getElementById("ltnMinimonth");
+    if (minimonth) {
+        minimonth.selectDate(event.detail.selectedDay.jsDate, event.detail.mainStartDate.jsDate);
Under what circumstances won't we have the minimonth?

I'm very scared that the above code is going to run into timezone issues by relying on jsDate.

+    document.getElementById("displayDeck").
+        addEventListener("dayselect", ltnObserveViewDaySelect, false);
+
Styling nit: match the line above.  (same elsewhere)

+          this.sameDay = function (d1, d2) {
+            if (d1 && d2 && 
+                (d1.getDate() == d2.getDate()) &&
+                (d1.getMonth() == d2.getMonth()) &&
+                (d1.getFullYear() == d2.getFullYear())) {
+              return true;
+            }
+            return false;
+          }
This should be in a private (mFoo or _Foo) method.

+            var sameMonth = this.sameDay(aMainDate, this.mEditorDate);
This looks very odd, having a variable named sameMonth based on a sameDay function.  Can you at least comment on why this works?

I'll do some testing later, my main concerns at this stage concern the multiweek view in general and the week view when the week displayed spans 2 months.  Comments from that in a few hours.

Please also ask mvl for another review on the next patch, since it's diverged significantly from his last r+.

Comment 27

[Thomas Benisch]

Before I create a new patch I think it's necessary to get in 
agreement about some issues.

1. What does syncing of minimonth and calendar views mean?
   ------------------------------------------------------

   a) I think it's obvious, that the selected day is the same in both, 
      the minimonth and the calendar views.

   b) In addition, the minimonth main month and the calendar view should 
      fit to each other. 

      Let's take the selected day June 1st, 2006 as an example. If the
      minimonth shows June 2006 as main month, then the month view should
      also show June 2006 as main month. Because in the month view it's
      also possible to show May 2006 as main month and have June 1st, 2006
      selected.

      Whereas for a given selected day in the day and week view the start 
      and end date of the view is well-defined, that's different for the
      month view and the multiweek view.

2. What does navigating in the minimonth and the calendar views mean?
   -----------------------------------------------------------------

   a) minimonth:

      In the minimonth the main month can be changed by either pressing
      the previous and next buttons or selecting a different month/year
      in the month/year popup menu. 

      When changing to a new month, also the selected day changes.
      !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
      Note, that this behaviour is new and only available for 
      autoselect="true"!
      !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
      The new selected day should have the same day number in the new month
      as the old selected day had in the old month, e.g. when switching
      from June 2006 to July 2006 with old selected day June 1st, then the
      new selected day should be July 1st. A special case are so-called
      rollovers, that means when switching from January to February, with
      January 31st selected, then Feburary 28th (29th) will be selected.

      In addition, the behaviour when clicking on a day outside of the
      minimonth main month has to be clarified. At the moment, the minimonth
      changes its main month. E.g. when displaying June and clicking
      on July 1st, the minimonth changes its main month to July.
      Note, that this behaviour is different to that of the month view.

   b) month view:

      In the month view the main month can be changed by using the
      navigation bar. When a new month is selected, also the selected
      day changes. The rules for the new selected day should be the same
      as in the minimonth.

   c) multiweek view:

      I must admit, that I haven't thought about the behaviour in the
      multiweek view, as this view is not available in Lightning.
      But the behaviour can be kept as it is now.

   d) week view:

      When navigating to another week, the same day of the week is
      selected, e.g. Tuesday.

   e) day view:

      I think this case is trivial. 


3. Implementation details
   ----------------------

   a) event notification and listeners
      --------------------------------

      I think the right approach for keeping the minimonth and the calendar
      views in sync is to use an event listener approach.

      If a new day in the month view is selected, an event is fired.
      The information about the new selected day is part of the event.
      The listener switches the minimonth to the same selected day.
      Due to 1.b) the selected day is not always enought information.
      Therefore an additional parameter is needed, which I called in
      patch v5 mainStartDate.

      We have to agree, if one event with two parameters is fired
      or as an alternative if two events each with one parameter are 
      fired. In addition we have to agree, what the second parameter 
      specifies in detail.

      As a third alternative it's also possible to fire an event
      without parameters, and get the relevant information in the listener
      from the view/minimonth. But then, additional APIs for the views
      and the minimonth are needed.

   b) API for switching the calendar view/minimonth
      ---------------------------------------------

      The listener switches the view/minimonth to the new selected day.

      When switching a view to the new selected day, the goToDay(aDate) 
      method of calIDecoratedView is used. This method selects aDate
      as the new selected day. In addition, it centers the view around
      this date. For the month view this means, that when aDate is
      July 1st, then always July is the main month of the month view.
      But there's no API available to switch the month view to June and 
      select July 1st. This would be needed, if the month of the selected
      day in the minimonth is not the same as the minimonth main month.

      The view is only switched to the new selected day, if the old 
      selected day and the new selected day differ. Without this check,
      one would get infinite loops.

      In principal an API at the view is needed, which allows to switch 
      not only the selected day, but also the main month at the month view
      etc. In addition, this API can be implemented in a way, that it
      checks internally, if the new selected day differs from the old
      selected day. If not, nothing is done. Of course, with such an
      implementation one would loose the refresh functionality of
      goToDay().

      At the minimonth I implemented such a method: 
      selectDate(aDate, aMainDate). This method is also much more
      performant than goToDay(), because if the minimonth main month
      is not changed, then only the selected day is changed without
      redrawing the whole minimonth. When calling goToDay() at the
      month view, then the whole view is relayouted, even if only
      the selected day, but not the main month changes.

Comment 28

[Thomas Benisch]

(In reply to comment #26)
> (From update of attachment 217418 [details] [diff] [review] [edit])
> Some code comments follow.  I still need to go through and test this stuff:
> 
> +      <property name="mainStartDate" readonly="true">
> +        <getter><![CDATA[
> +          // Returns the start date of the main month.
> +          // Unless the month actually starts on a Sunday, this means that
> +          // mStartDate.month is 1 month less than the main month.
> +          var date = this.mStartDate.clone();
> +          if (this.mStartDate.day != 1) {
> 
> This looks likes its going to return really strange results for the multiweek
> view.

I actually copied this piece of code from the createDayGrid and reuseExistingGrid
methods.

> 
> +              date.month += 1;
> +              date.normalize();
> +                     // correct for accidental rollover into the next month
> +              if ((date.month - this.mStartDate.month - 1)%12 != 0) {
> +                  date.month -= 1;
> +                  date.normalize();
> +            }
> +          }
> +          return date.startOfMonth;
> +        ]]></getter>
> +      </property>
> Normalizing twice looks weird here, and slow.  Why not set the day to 1 and
> then +1 then month?  You'll stay normal and get the start of the month that
> way.

You're right, that should be much more performant.

> 
> -            this.fireEvent("dayselect");
> +            var detail = { selectedDay: val, mainStartDate: val.startOfMonth
> };
> +            this.fireEvent("dayselect", detail);
>              return val;
> This seems weird to include.  I'm not sure I would expect the mainStartDate to
> be related to the day selected.

Please see my comment #27. For the month view, an additional parameter is needed.
For the multiday view, the additional parameter can be omitted. But then the logic,
which month to select as main month in the minimonth has to implemented in the
listener.

> 
> +    var minimonth = document.getElementById("ltnMinimonth");
> +    if (minimonth) {
> +        minimonth.selectDate(event.detail.selectedDay.jsDate,
> event.detail.mainStartDate.jsDate);
> Under what circumstances won't we have the minimonth?

This is just an additional check for security. I'm a C++ programmer and there
one checks rather frequently if a pointer is null in order to avoid crashes.
I still haven't understood, why you don't like those checks and why performance
in those cases is more important than a simple check for null, but I will
skip this line.

> 
> I'm very scared that the above code is going to run into timezone issues by
> relying on jsDate.

What is the alternative? Creating a calIDateTime object from the minimonth
jsDate and comparing calIDateTimes? I think timezone issues can only be 
avoided if all calendar views and the minimonth refer to the same timezone.
And in this case I think it makes no difference, if I compare calIDateTimes
or jsDates. Or am I wrong?

> 
> +    document.getElementById("displayDeck").
> +        addEventListener("dayselect", ltnObserveViewDaySelect, false);
> +
> Styling nit: match the line above.  (same elsewhere)

That means
       document.getElementById("displayDeck").
                addEventListener("dayselect", ltnObserveViewDaySelect, false);
or write everything in one line?

> 
> +          this.sameDay = function (d1, d2) {
> +            if (d1 && d2 && 
> +                (d1.getDate() == d2.getDate()) &&
> +                (d1.getMonth() == d2.getMonth()) &&
> +                (d1.getFullYear() == d2.getFullYear())) {
> +              return true;
> +            }
> +            return false;
> +          }
> This should be in a private (mFoo or _Foo) method.

How do I write a private method, which cannot be accessed from outside?

> 
> +            var sameMonth = this.sameDay(aMainDate, this.mEditorDate);
> This looks very odd, having a variable named sameMonth based on a sameDay
> function.  Can you at least comment on why this works?

This works, because both, aMainDate and this.mEditorDate refer to the first
day of a month.

Comment 29

[Andrew N Dowden]

per Comment 27
> 1. What does syncing of minimonth and calendar views mean?
>    -----------------------------------------------------------------
>
>    a) I think it's obvious, that the selected day is the same in both, 
>       the minimonth and the calendar views.
Perhaps not so obvious.  Any unnecessary activity should be avoided ..
 -  The minimonth should sync (trigger) with calendar view, ONLY when a task / event is selected, OR a new event / task is added.
 -  The calendar view (month, multi-week, week, day) should ALWAYS sync with minimonth, whenever minimonth is used to select day (left click), or select day (right click) to add a new event / task.
 [ ? should right-click menu (in minimonth) show: 'Edit Selected Event', or 'Delete Selected Event' ]

This avoids main view chasing after, when using minimonth to look forward.  Also minimonth is actually MOST useful in multi-week and week views, because it always shows months (giving context).

>    b) In addition, the minimonth main month and the calendar view should 
>       fit to each other. 
>
>       Let's take the selected day June 1st, 2006 as an example. If the
>       minimonth shows June 2006 as main month, then the month view should
>       also show June 2006 as main month. Because in the month view it's
>       also possible to show May 2006 as main month and have June 1st, 2006
>       selected.
For month view, selecting ANY day visible should bring the minimonth to that actual month.  For minimonth, selecting ANY day visible should bring BOTH the calendar view and minimonth to that actual day (and therefore: week, or month).

>       Whereas for a given selected day in the day and week view the start 
>       and end date of the view is well-defined, that's different for the
>       month view and the multiweek view.
Already defined, I think.

> 2. What does navigating in the minimonth and the calendar views mean?
>    -----------------------------------------------------------------
>
>    a) minimonth:
>
>       In the minimonth the main month can be changed by either pressing
>       the previous and next buttons or selecting a different month/year
>       in the month/year popup menu. 
Yes, but this is speculative: 'What is 4 weeks hence, 17 weeks hence, ..?'

>       .. A special case are so-called
>       rollovers, that means when switching from January to February, with
>       January 31st selected, then Feburary 28th (29th) will be selected.
Yes, if 31,30, or 29 should select last (if needed for Feb. or leapyear).

>       In addition, the behaviour when clicking on a day outside of the
>       minimonth main month has to be clarified. At the moment, the minimonth
>       changes its main month. E.g. when displaying June and clicking
>       on July 1st, the minimonth changes its main month to July.
>       Note, that this behaviour is different to that of the month view.
I believe it should behave as it does.

>    c) multiweek view:
>
>       I must admit, that I haven't thought about the behaviour in the
>       multiweek view, as this view is not available in Lightning.
>       But the behaviour can be kept as it is now.
The MOST important use of minimonth (in my opinion) is for mult-week view, folowed by week view.  It allows us (the user) to keep the big picture.

FYI: Navigates to next week, and same doy of the week (same as week view)

>    d) week view:
>
>       When navigating to another week, the same day of the week is
>       selected, e.g. Tuesday.
This is the current behaviour.  It works fine for me.

Comment 30

[Thomas Benisch]

(In reply to comment #29)
> per Comment 27
> > 1. What does syncing of minimonth and calendar views mean?
> >    -----------------------------------------------------------------
> >
> >    a) I think it's obvious, that the selected day is the same in both, 
> >       the minimonth and the calendar views.
> Perhaps not so obvious.  Any unnecessary activity should be avoided ..
>  -  The minimonth should sync (trigger) with calendar view, ONLY when a task /
> event is selected, OR a new event / task is added.

That's a really interesting interpretation of syncing. I don't understand, why
you require as an additional condition, that an event is selected or created.
For me that doesn't seem to be logical.

>  -  The calendar view (month, multi-week, week, day) should ALWAYS sync with
> minimonth, whenever minimonth is used to select day (left click), or select day
> (right click) to add a new event / task.
>  [ ? should right-click menu (in minimonth) show: 'Edit Selected Event', or
> 'Delete Selected Event' ]

As far as I know it's not possible to add a new event from the minimonth.
Please write a seperate task for this feature/enhancement.

Comment 31

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #8)
> (In reply to comment #7)
> > I'm just used to them being not in sync, and never considered anything else :)
> > For what it is worth, evolution has the minical and the real views in sync.
> > Outlook 2000 also syncs. Somebody willing to test mac iCal and newer outlooks?
> iCal syncs them up as well, with the current view-range slightly highlighted. 
> Oulook 2005 has three minimonths, and similar highlighting of the current
> range.

As far as I can tell, iCal actually approximates our current behavior.  Which is to say that when you click on the arrows, it doesn't sync.  But when you click on a specific date, then it goes to that date.

Comment 32

[Andrew N Dowden]

> That's a really interesting interpretation of syncing. I don't understand, why
> you require as an additional condition, that an event is selected or created.
> For me that doesn't seem to be logical.
I was going for 'lazy syncing', or "less is more".  I sometime use BOTH minimonth and calendar-view to look forward and back (particularly in week or multi-week view).  I haven't personally identified a need for the 'calendar view' to be in strict lock-step.

> As far as I know it's not possible to add a new event from the minimonth.
> Please write a seperate task for this feature/enhancement.
Under Sunbird (latest nightly, w2k) the minimonth has a right-click menu, and it is probably INCORRECT.

(a.) current  (Sunbird latest, w2k)
  New Event  (for current day, in calendar view !?)
  New Task  (for current day, in calendar view !?)
  Edit Selected Event  (greyed out, if no current)
  Delete Selected Event  (greyed out, if no current)
  -----------------------------
  Go to Today
  -----------------------------
  Workweek days only
  Tasks in wiew

(b.) as suggested  (comment 29)
  New Event  (for this day, from minimonth)
  New Task  (for this day, from minimonth)
  -----------------------------
  Go to Today
  -----------------------------
  Workweek days only  (not sure, if useful ?)
  Tasks in wiew  (not sure, if useful ?)

(c.) minimum function ONLY (?)
  Go to Today
  -----------------------------
  Workweek days only  (not sure, if useful ?)
  Tasks in wiew  (not sure, if useful ?)

I raised these issues first here, because these functions are being discussed (and defined).  If there is general agreement on function, then an additional bug may be needed for the right-click menu (which I don't need to own).

I would also like to see a right-click option for a date-picker dialog box.  This should support year, month selection for periods greater than '-2 years, +4 years, any month' as currently supported.  Later enhancements could include date math: 6 months hence, 40 weeks hence, on 50th anniversary (start date, start age, calc ..), etc.  Again, after general discussion this should be moved to a new RFE bug.

Comment 33

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #27)
> Before I create a new patch I think it's necessary to get in 
> agreement about some issues.

Completely agreed.  Let's get the user interaction sorted out first, and then worry about the implementation.  As part of this, let's move the context menu stuff mentioned in comment 32 to another bug.

> 1. What does syncing of minimonth and calendar views mean?

I talked to beltzner a bit about this today, and he suggested that we might get more clarity here if we frame this as two different questions.  Specifically:

a) When the main content view is set to display a specific date range, should the minimonth be switched to also show that date range?

and

b) When the minimonth view is moved explicitly using the direction widgets there, should the content window behave as though the user had selected a day in the newly updated month shown in the minimonth view?

One key use of the minimonth is to provide context for the work that you're doing in the main view.  This is in some ways similar to the high-level map inset displayed in the bottom right corner of google maps these days.  Since you generally want the context to relate to the thing you're inspecting more closely, this would seem to suggest that the answer to question a) is that yes, minimonth should follow the main view.  Outlook 2000 and iCal both behave this way.

Another key use of the minimonth is to select a specific date that is displayed there in order to cause the main view to display that date.  So if browsing forward in the minimonth changes the main view, it's implicitly performing a selection on the user's behalf.  Outlook 2000 does this selection, iCal does not. 

Ideally, we'd have a balanced discussion about these alternatives, pick one, and wait for user feedback from the next milestone.  However, we have one constraint that Outlook does not: right now, view switching in the main view is slow.  We will make it faster, but who knows if that's going to happen between now and the next milestone.  So the Outlook approach would currently provide a painful user experience for anyone trying to browse forward more than one month at time.  Painful enough, I believe, that this would seriously bias any user feedback we got against the Outlook approach.  So this suggests that we should go with the iCal behavior for b) right now, and potentially file a bug to revisit the issue once our view-switching performance is suitably quick.

Comment 34

[Thomas Benisch]

(In reply to comment #33)
> > 1. What does syncing of minimonth and calendar views mean?
> 
> I talked to beltzner a bit about this today, and he suggested that we might get
> more clarity here if we frame this as two different questions.  Specifically:
> 
> a) When the main content view is set to display a specific date range, should
> the minimonth be switched to also show that date range?
> 
> and
> 
> b) When the minimonth view is moved explicitly using the direction widgets
> there, should the content window behave as though the user had selected a day
> in the newly updated month shown in the minimonth view?
> 
> One key use of the minimonth is to provide context for the work that you're
> doing in the main view.  This is in some ways similar to the high-level map
> inset displayed in the bottom right corner of google maps these days.  Since
> you generally want the context to relate to the thing you're inspecting more
> closely, this would seem to suggest that the answer to question a) is that yes,
> minimonth should follow the main view.  Outlook 2000 and iCal both behave this
> way.

If I understand this right, than this implies, that if e.g. the month view
displays July as main month, then also the minimonth should display July
as main month.

> Another key use of the minimonth is to select a specific date that is displayed
> there in order to cause the main view to display that date.  So if browsing
> forward in the minimonth changes the main view, it's implicitly performing a
> selection on the user's behalf.  Outlook 2000 does this selection, iCal does
> not. 
> 
> Ideally, we'd have a balanced discussion about these alternatives, pick one,
> and wait for user feedback from the next milestone.  However, we have one
> constraint that Outlook does not: right now, view switching in the main view is
> slow.  We will make it faster, but who knows if that's going to happen between
> now and the next milestone.  So the Outlook approach would currently provide a
> painful user experience for anyone trying to browse forward more than one month
> at time.  Painful enough, I believe, that this would seriously bias any user
> feedback we got against the Outlook approach.  So this suggests that we should
> go with the iCal behavior for b) right now, and potentially file a bug to
> revisit the issue once our view-switching performance is suitably quick.

I think this is acceptable.

So the view will only follow the minimonth, if a user selects a new day in
the minimonth by clicking on that day. The view won't follow the minimonth,
when navigating in the minimonth by using the previous/next buttons or the
month/year popup menu. That's the current behaviour.

As we now have a common understanding, this immediately leads me to item 2
of my comment #27. 

I think as a consequence, I can drop the autoselect feature of the minimonth,
that means when navigating in the minimonth a new day is selected.
We will keep at the current navigation behaviour of the minimonth, that's
actually the same as in iCal.

In addition, I have no problem with dropping all code, which affects the
navigation behaviour of the views, e.g. the moveView method of the
calendar-decorated-month-view binding.

Comment 35

[Christian Jansen]

(In reply to comment #34)
> (In reply to comment #33)
> > > 1. What does syncing of minimonth and calendar views mean?
> > 
> > I talked to beltzner a bit about this today, and he suggested that we might get
> > more clarity here if we frame this as two different questions.  Specifically:
> > 
> > a) When the main content view is set to display a specific date range, should

> > the minimonth be switched to also show that date range?
> > 
> > and
> > 
> > b) When the minimonth view is moved explicitly using the direction widgets
> > there, should the content window behave as though the user had selected a day
> > in the newly updated month shown in the minimonth view?
> > 
> > One key use of the minimonth is to provide context for the work that you're
> > doing in the main view.  This is in some ways similar to the high-level map
> > inset displayed in the bottom right corner of google maps these days.  Since
> > you generally want the context to relate to the thing you're inspecting more
> > closely, this would seem to suggest that the answer to question a) is that yes,
> > minimonth should follow the main view.  Outlook 2000 and iCal both behave this
> > way.
> 
> If I understand this right, than this implies, that if e.g. the month view
> displays July as main month, then also the minimonth should display July
> as main month.
> 
> > Another key use of the minimonth is to select a specific date that is displayed
> > there in order to cause the main view to display that date.  So if browsing
> > forward in the minimonth changes the main view, it's implicitly performing a
> > selection on the user's behalf.  Outlook 2000 does this selection, iCal does
> > not. 
> > 
> > Ideally, we'd have a balanced discussion about these alternatives, pick one,
> > and wait for user feedback from the next milestone.  However, we have one
> > constraint that Outlook does not: right now, view switching in the main view is
> > slow.  We will make it faster, but who knows if that's going to happen between
> > now and the next milestone.  So the Outlook approach would currently provide a
> > painful user experience for anyone trying to browse forward more than one month
> > at time.  Painful enough, I believe, that this would seriously bias any user
> > feedback we got against the Outlook approach.  So this suggests that we should
> > go with the iCal behavior for b) right now, and potentially file a bug to
> > revisit the issue once our view-switching performance is suitably quick.
> 
> I think this is acceptable.
> 
> So the view will only follow the minimonth, if a user selects a new day in
> the minimonth by clicking on that day. The view won't follow the minimonth,
> when navigating in the minimonth by using the previous/next buttons or the
> month/year popup menu. That's the current behaviour.
> 
> As we now have a common understanding, this immediately leads me to item 2
> of my comment #27. 
> 
> I think as a consequence, I can drop the autoselect feature of the minimonth,
> that means when navigating in the minimonth a new day is selected.
> We will keep at the current navigation behaviour of the minimonth, that's
> actually the same as in iCal.
> 
> In addition, I have no problem with dropping all code, which affects the
> navigation behaviour of the views, e.g. the moveView method of the
> calendar-decorated-month-view binding.
> 

(In reply to comment #34)
> (In reply to comment #33)
> > > 1. What does syncing of minimonth and calendar views mean?
> > 
> > I talked to beltzner a bit about this today, and he suggested that we might get
> > more clarity here if we frame this as two different questions.  Specifically:
> > 
> > a) When the main content view is set to display a specific date range, should
> > the minimonth be switched to also show that date range?
> > 
> > and
> > 
> > b) When the minimonth view is moved explicitly using the direction widgets
> > there, should the content window behave as though the user had selected a day
> > in the newly updated month shown in the minimonth view?
> > 
> > One key use of the minimonth is to provide context for the work that you're
> > doing in the main view.  This is in some ways similar to the high-level map
> > inset displayed in the bottom right corner of google maps these days.  Since
> > you generally want the context to relate to the thing you're inspecting more
> > closely, this would seem to suggest that the answer to question a) is that yes,
> > minimonth should follow the main view.  Outlook 2000 and iCal both behave this
> > way.
> 
> If I understand this right, than this implies, that if e.g. the month view
> displays July as main month, then also the minimonth should display July
> as main month.
> 
> > Another key use of the minimonth is to select a specific date that is displayed
> > there in order to cause the main view to display that date.  So if browsing
> > forward in the minimonth changes the main view, it's implicitly performing a
> > selection on the user's behalf.  Outlook 2000 does this selection, iCal does
> > not. 
> > 
> > Ideally, we'd have a balanced discussion about these alternatives, pick one,
> > and wait for user feedback from the next milestone.  However, we have one
> > constraint that Outlook does not: right now, view switching in the main view is
> > slow.  We will make it faster, but who knows if that's going to happen between
> > now and the next milestone.  So the Outlook approach would currently provide a
> > painful user experience for anyone trying to browse forward more than one month
> > at time.  Painful enough, I believe, that this would seriously bias any user
> > feedback we got against the Outlook approach.  So this suggests that we should
> > go with the iCal behavior for b) right now, and potentially file a bug to
> > revisit the issue once our view-switching performance is suitably quick.
> 
> I think this is acceptable.
> 
> So the view will only follow the minimonth, if a user selects a new day in
> the minimonth by clicking on that day. The view won't follow the minimonth,
> when navigating in the minimonth by using the previous/next buttons or the
> month/year popup menu. That's the current behaviour.
> 
> As we now have a common understanding, this immediately leads me to item 2
> of my comment #27. 
> 
> I think as a consequence, I can drop the autoselect feature of the minimonth,
> that means when navigating in the minimonth a new day is selected.
> We will keep at the current navigation behaviour of the minimonth, that's
> actually the same as in iCal.
> 
> In addition, I have no problem with dropping all code, which affects the
> navigation behaviour of the views, e.g. the moveView method of the
> calendar-decorated-month-view binding.
> 

Hi Thomas,
Please find my thoughts on the wiki page
http://wiki.mozilla.org/Calendar:Calendar_View#The_Mini_Month

Comment 36

[Joey Minta]

(In reply to comment #28)
> (In reply to comment #26)
> > (From update of attachment 217418 [details] [diff] [review] [edit] [edit])
> > Some code comments follow.  I still need to go through and test this stuff:
> > 
> > +      <property name="mainStartDate" readonly="true">
> > +        <getter><![CDATA[
> > +          // Returns the start date of the main month.
> > +          // Unless the month actually starts on a Sunday, this means that
> > +          // mStartDate.month is 1 month less than the main month.
> > +          var date = this.mStartDate.clone();
> > +          if (this.mStartDate.day != 1) {
> > 
> > This looks likes its going to return really strange results for the multiweek
> > view.
> 
> I actually copied this piece of code from the createDayGrid and
> reuseExistingGrid
> methods.
Within the createDayGrid and resuseExistingGrid methods, we use this funciton to shape the days outside of the month.  However, at http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-month-view.xml#951 we disregard the results if we're showing the multiweek view, since all days should be colored the same there.

> 
> > 
> > +    var minimonth = document.getElementById("ltnMinimonth");
> > +    if (minimonth) {
> > +        minimonth.selectDate(event.detail.selectedDay.jsDate,
> > event.detail.mainStartDate.jsDate);
> > Under what circumstances won't we have the minimonth?
> 
> This is just an additional check for security. I'm a C++ programmer and there
> one checks rather frequently if a pointer is null in order to avoid crashes.
> I still haven't understood, why you don't like those checks and why performance
> in those cases is more important than a simple check for null, but I will
> skip this line.
> 
> > 
> > I'm very scared that the above code is going to run into timezone issues by
> > relying on jsDate.
> 
> What is the alternative? Creating a calIDateTime object from the minimonth
> jsDate and comparing calIDateTimes? I think timezone issues can only be 
> avoided if all calendar views and the minimonth refer to the same timezone.
> And in this case I think it makes no difference, if I compare calIDateTimes
> or jsDates. Or am I wrong?

The following simulates the situation in my timezone at 2am.  Actual windows for this error vary by tz.  cdt1 simulates a selected day of May 12.  Notice that it won't match the javascript date of May 12.  One of the keys here is the isDate operator.

js> var jsd = new Date();
js> jsd;
Fri May 12 2006 02:09:40 GMT-0400 (EDT)
js> var cdt1 = Components.classes["@mozilla.org/calendar/datetime;1"].createInstance(Components.interfaces.calIDateTime);
js> cdt1.jsDate = jsd;
Fri May 12 2006 02:09:40 GMT-0400 (EDT)
js> cdt1.isDate = true;
true
js> cdt1.jsDate;
Thu May 11 2006 20:00:00 GMT-0400 (EDT)
js> cdt1.toString();
2006/05/12 00:00:00 UTC


> 
> > 
> > +    document.getElementById("displayDeck").
> > +        addEventListener("dayselect", ltnObserveViewDaySelect, false);
> > +
> > Styling nit: match the line above.  (same elsewhere)
> 
> That means
>        document.getElementById("displayDeck").
>                 addEventListener("dayselect", ltnObserveViewDaySelect, false);
> or write everything in one line?
This means match the line of code above this one in the patch.  Something like
        document.getElementById("displayDeck")
                .addEventListener("dayselect", ltnObserveViewDaySelect, false);
(dots line up)

> 
> > 
> > +          this.sameDay = function (d1, d2) {
> > +            if (d1 && d2 && 
> > +                (d1.getDate() == d2.getDate()) &&
> > +                (d1.getMonth() == d2.getMonth()) &&
> > +                (d1.getFullYear() == d2.getFullYear())) {
> > +              return true;
> > +            }
> > +            return false;
> > +          }
> > This should be in a private (mFoo or _Foo) method.
> 
> How do I write a private method, which cannot be accessed from outside?
Strictly private methods can't be done without lots of messy object stuff.  However, by naming the method mFoo or _Foo, it becomes "private by convention."  That is, no one should ever access an mFoo from outside.

> 
> > 
> > +            var sameMonth = this.sameDay(aMainDate, this.mEditorDate);
> > This looks very odd, having a variable named sameMonth based on a sameDay
> > function.  Can you at least comment on why this works?
> 
> This works, because both, aMainDate and this.mEditorDate refer to the first
> day of a month.
Right, just include a comment saying so in the code.

Comment 37

[Joey Minta]

(In reply to comment #27)
> 3. Implementation details
>    ----------------------
> 
>    a) event notification and listeners
>       --------------------------------
> 
>       I think the right approach for keeping the minimonth and the calendar
>       views in sync is to use an event listener approach.
> 
>       If a new day in the month view is selected, an event is fired.
>       The information about the new selected day is part of the event.
>       The listener switches the minimonth to the same selected day.
>       Due to 1.b) the selected day is not always enought information.
>       Therefore an additional parameter is needed, which I called in
>       patch v5 mainStartDate.
As I said before, I don't think the additional parameter approach is the right one.  A day-select event corresponds to a particular day being selected, and is ambivalent with regards to what other days are shown.  Therefore, from a code-model perspective, the only information the event should carry is that date.  I think I'd be happier with a mainStartDate method being added to the views, to retrieve this information.

>    b) API for switching the calendar view/minimonth
>       ---------------------------------------------
> 
>       The listener switches the view/minimonth to the new selected day.
> 
>       When switching a view to the new selected day, the goToDay(aDate) 
>       method of calIDecoratedView is used. This method selects aDate
>       as the new selected day. In addition, it centers the view around
>       this date. For the month view this means, that when aDate is
>       July 1st, then always July is the main month of the month view.
>       But there's no API available to switch the month view to June and 
>       select July 1st. This would be needed, if the month of the selected
>       day in the minimonth is not the same as the minimonth main month.
> 
>       The view is only switched to the new selected day, if the old 
>       selected day and the new selected day differ. Without this check,
>       one would get infinite loops.
> 
>       In principal an API at the view is needed, which allows to switch 
>       not only the selected day, but also the main month at the month view
>       etc. In addition, this API can be implemented in a way, that it
>       checks internally, if the new selected day differs from the old
>       selected day. If not, nothing is done. Of course, with such an
>       implementation one would loose the refresh functionality of
>       goToDay().
As I understand it, this is no longer an issue as we've dropped sync in this direction, correct?
> 
>       At the minimonth I implemented such a method: 
>       selectDate(aDate, aMainDate). This method is also much more
>       performant than goToDay(), because if the minimonth main month
>       is not changed, then only the selected day is changed without
>       redrawing the whole minimonth. When calling goToDay() at the
>       month view, then the whole view is relayouted, even if only
>       the selected day, but not the main month changes.
> 
I think this seems reasonable, provided that in the case where aMainDate is null, we revert to the traditional behavior, making aMainDate an optional param.  Does this seem reasonable?

Comment 38

[Thomas Benisch]

(In reply to comment #36)
> (In reply to comment #28)
> > (In reply to comment #26)
> > > (From update of attachment 217418 [details] [diff] [review] [edit] [edit] [edit])
> > > I'm very scared that the above code is going to run into timezone issues by
> > > relying on jsDate.
> > 
> > What is the alternative? Creating a calIDateTime object from the minimonth
> > jsDate and comparing calIDateTimes? I think timezone issues can only be 
> > avoided if all calendar views and the minimonth refer to the same timezone.
> > And in this case I think it makes no difference, if I compare calIDateTimes
> > or jsDates. Or am I wrong?
> 
> The following simulates the situation in my timezone at 2am.  Actual windows
> for this error vary by tz.  cdt1 simulates a selected day of May 12.  Notice
> that it won't match the javascript date of May 12.  One of the keys here is the
> isDate operator.
> 
> js> var jsd = new Date();
> js> jsd;
> Fri May 12 2006 02:09:40 GMT-0400 (EDT)
> js> var cdt1 =
> Components.classes["@mozilla.org/calendar/datetime;1"].createInstance(Components.interfaces.calIDateTime);
> js> cdt1.jsDate = jsd;
> Fri May 12 2006 02:09:40 GMT-0400 (EDT)
> js> cdt1.isDate = true;
> true
> js> cdt1.jsDate;
> Thu May 11 2006 20:00:00 GMT-0400 (EDT)
> js> cdt1.toString();
> 2006/05/12 00:00:00 UTC

After rethinking about the problem I came to the conclusion, that comparing
calIDateTime objects doesn't solve the timezone problem. The key issue is,
that the minimonth only works with jsDates, therefore I have to set the
right jsDate in the minimonth. What do you think about the following code
in ltnObserveViewDaySelect:

    var cdt = event.detail;
    var date = new Date(cdt.year, cdt.month, cdt.day);
    minimonth.selectDate(date, ...);

This code should solve the timezone problem.

Comment 39

[Thomas Benisch]

(In reply to comment #37)
> (In reply to comment #27)
> >    b) API for switching the calendar view/minimonth
> >       ---------------------------------------------
> > 
> >       The listener switches the view/minimonth to the new selected day.
> > 
> >       When switching a view to the new selected day, the goToDay(aDate) 
> >       method of calIDecoratedView is used. This method selects aDate
> >       as the new selected day. In addition, it centers the view around
> >       this date. For the month view this means, that when aDate is
> >       July 1st, then always July is the main month of the month view.
> >       But there's no API available to switch the month view to June and 
> >       select July 1st. This would be needed, if the month of the selected
> >       day in the minimonth is not the same as the minimonth main month.
> > 
> >       The view is only switched to the new selected day, if the old 
> >       selected day and the new selected day differ. Without this check,
> >       one would get infinite loops.
> > 
> >       In principal an API at the view is needed, which allows to switch 
> >       not only the selected day, but also the main month at the month view
> >       etc. In addition, this API can be implemented in a way, that it
> >       checks internally, if the new selected day differs from the old
> >       selected day. If not, nothing is done. Of course, with such an
> >       implementation one would loose the refresh functionality of
> >       goToDay().
> As I understand it, this is no longer an issue as we've dropped sync in this
> direction, correct?

Right.

> >       At the minimonth I implemented such a method: 
> >       selectDate(aDate, aMainDate). This method is also much more
> >       performant than goToDay(), because if the minimonth main month
> >       is not changed, then only the selected day is changed without
> >       redrawing the whole minimonth. When calling goToDay() at the
> >       month view, then the whole view is relayouted, even if only
> >       the selected day, but not the main month changes.
> > 
> I think this seems reasonable, provided that in the case where aMainDate is
> null, we revert to the traditional behavior, making aMainDate an optional
> param.  Does this seem reasonable?

So if aMainDate is null, then the main date is calculated from aDate, that
means the minimonth shows the month, which corresponds to aDate.

Comment 40

[Thomas Benisch]

(In reply to comment #37)
> (In reply to comment #27)
> > 3. Implementation details
> >    ----------------------
> > 
> >    a) event notification and listeners
> >       --------------------------------
> > 
> >       I think the right approach for keeping the minimonth and the calendar
> >       views in sync is to use an event listener approach.
> > 
> >       If a new day in the month view is selected, an event is fired.
> >       The information about the new selected day is part of the event.
> >       The listener switches the minimonth to the same selected day.
> >       Due to 1.b) the selected day is not always enought information.
> >       Therefore an additional parameter is needed, which I called in
> >       patch v5 mainStartDate.
> As I said before, I don't think the additional parameter approach is the right
> one.  A day-select event corresponds to a particular day being selected, and is
> ambivalent with regards to what other days are shown.  Therefore, from a
> code-model perspective, the only information the event should carry is that
> date.  I think I'd be happier with a mainStartDate method being added to the
> views, to retrieve this information.

That means, I have to add a mainStartDate method to calIDecoratedView.idl and
calICalendarView.idl. If I understand it right, then from messenger-overlay-sidebar.js
I should only call methods of calIDecoratedView. In this case, do I have to
implement the mainStartDate method also for the calendar-multiday-view?
I think for the day and week view, the month of the selected day can be taken
as mainStartDate. For the multiweek view it gets more complicated.

As an alternative it's also possible to use the startDay and endDay attributes
of calIDecoratedView and calculate from those attributes a mainStartDate in the
ltnObserveViewDaySelect listener method. Is there a way in the
ltnObserveViewDaySelect method to find out, which view has fired the listener?
This information may be helpful, because for the day and week view, the
mainStartDate is simply the month of the selectedDay.

Comment 41

[Joey Minta]

(In reply to comment #38)
 What do you think about the following code
> in ltnObserveViewDaySelect:
> 
>     var cdt = event.detail;
>     var date = new Date(cdt.year, cdt.month, cdt.day);
>     minimonth.selectDate(date, ...);
> 
> This code should solve the timezone problem.
> 

This looks good.

(In reply to comment #39)
> So if aMainDate is null, then the main date is calculated from aDate, that
> means the minimonth shows the month, which corresponds to aDate.
> 
Yep, it would work just like setting the value property.

(In reply to comment #40)
> As an alternative it's also possible to use the startDay and endDay attributes
> of calIDecoratedView and calculate from those attributes a mainStartDate in the
> ltnObserveViewDaySelect listener method. Is there a way in the
> ltnObserveViewDaySelect method to find out, which view has fired the listener?
> This information may be helpful, because for the day and week view, the
> mainStartDate is simply the month of the selectedDay.
> 
You could use event.target (which should be the view).  Then check what the view returns for .supportsDisjointDates. (The multiday-view returns true, the month-view returns false.)

Comment 42

[Thomas Benisch]

Attachment: patch v6

In this patch the minimonth follows the views,
syncing of the views to the minimonth was omitted.
Also the autoselect feature of the minimonth was
dropped.

The main date for the month and multiweek view is
calculated by determining the month with the most
visible days in the view.

Comment 43

[Thomas Benisch]

Comment on attachment 222173 [details] [diff] [review]
patch v6

encountered a problem with SunBird

Comment 44

[Thomas Benisch]

Attachment: patch v7

In this patch the minimonth follows the views,
syncing of the views to the minimonth was omitted.
Also the autoselect feature of the minimonth was
dropped.

The main date for the month view is calculated by 
determining the month with the most visible days 
in the view.

In addition, fixed SunBird problem.

Comment 45

[Joey Minta]

Comment on attachment 222202 [details] [diff] [review]
patch v7

OK, I like this version a lot better.  Just some minor stuff to clean up.

+    // for the month view find the main month,
+    // which is the month with the most visible days in the view;
+    // note, that the main date is the first day of the main month
+    var jsMainDate = null;
+    if (!event.originalTarget.supportsDisjointDates && event.originalTarget.showFullMonth) {
+        var mainDate = null;
+        var maxVisibleDays = 0;
The multiweek view should probably be included in this calculation, since right now, I can click on  July 1 (the only July date displayed), and it shows July, which seems a bit off.  The majority-of-days rule seems like a good one to follow here.

Strictly speaking you don't need to assign jsMainDate = null; you can just do var jsMainDate;

+          this.mSameDay = function (d1, d2) {
+            if (d1 && d2 && 
+                (d1.getDate() == d2.getDate()) &&
+                (d1.getMonth() == d2.getMonth()) &&
+                (d1.getFullYear() == d2.getFullYear())) {
+              return true;
+            }
+            return false;
+          }
Naming this with an m is good, but it still needs to go in a <method>.

Also, this patch has gotten small enough that I'm comfortable reviewing it myself, so don't worry about mvl for the next round.

Comment 46

[Thomas Benisch]

(In reply to comment #45)
> (From update of attachment 222202 [details] [diff] [review] [edit])
> +    // for the month view find the main month,
> +    // which is the month with the most visible days in the view;
> +    // note, that the main date is the first day of the main month
> +    var jsMainDate = null;
> +    if (!event.originalTarget.supportsDisjointDates &&
> event.originalTarget.showFullMonth) {
> +        var mainDate = null;
> +        var maxVisibleDays = 0;
> The multiweek view should probably be included in this calculation, since right
> now, I can click on  July 1 (the only July date displayed), and it shows July,
> which seems a bit off.  The majority-of-days rule seems like a good one to
> follow here.

I found a new problem, when including the multiweek view in this calculation.
As an example, switch to the multiweek view, so that it shows the 
weeks 21-24, 2006. Then June is the main month, so the minimonth shows June.
If you now select one of the days between May 21st and May 27th, then those
days are missing in the minimonth and cannot be selected.
I think there are three solutions:
1.) don't include the multiweek view in the upper calculation
2.) include the multiweek view in the upper calculation and fix the
    problem in the selectDate method of the minimonth:
    if aDate is not in the minimonth range for a given aMainDate, then
    calculate a new aMainDate from aDate.
    This can be implemented by
    a) calling showMonth for aMainDate; if aDate is not in the range
       calculate a new aMainDate and call showMonth again
       (this is not very performant)
    b) calcalute a start and end date for a given aMainDate;
       if aDate is not in the calculated range, calculate a new
       aMainDate and call showMonth
I've chosen 2.b) for the next patch, but probably you have a better idea.

> Strictly speaking you don't need to assign jsMainDate = null; you can just do
> var jsMainDate;

done

> +          this.mSameDay = function (d1, d2) {
> +            if (d1 && d2 && 
> +                (d1.getDate() == d2.getDate()) &&
> +                (d1.getMonth() == d2.getMonth()) &&
> +                (d1.getFullYear() == d2.getFullYear())) {
> +              return true;
> +            }
> +            return false;
> +          }
> Naming this with an m is good, but it still needs to go in a <method>.

I moved the code into a method called _sameDay.

Comment 47

[Joey Minta]

(In reply to comment #46)
> I've chosen 2.b) for the next patch, but probably you have a better idea.
> 
Seems reasonable at least.  Even if it's not the ideal, I'd rather not hold this patch up any longer.

Comment 48

[Thomas Benisch]

Attachment: patch v8

see comment #46

Comment 49

[Joey Minta]

Comment on attachment 222340 [details] [diff] [review]
patch v8

+            } else if (!sameDate) {
+                // select day only
+                var day = this._findDay(aDate);
+                if (day) {
+                    if (this.mSelected) {
+                        this.mSelected.removeAttribute("selected");
+                    }
+                    this.mSelected = day;
+                    day.setAttribute("selected", "true");
+                    this.mValue = aDate;
+                }
Is the if (day) { more c++ over-caution or is there legitimately a way that we can fail to find a day?  I couldn't think of one.

r=jminta with that question answered.  (I'll be happy to tweak prior to landing.)  Thanks for the patience here!

Comment 50

[Thomas Benisch]

(In reply to comment #49)
> (From update of attachment 222340 [details] [diff] [review] [edit])
> +            } else if (!sameDate) {
> +                // select day only
> +                var day = this._findDay(aDate);
> +                if (day) {
> +                    if (this.mSelected) {
> +                        this.mSelected.removeAttribute("selected");
> +                    }
> +                    this.mSelected = day;
> +                    day.setAttribute("selected", "true");
> +                    this.mValue = aDate;
> +                }
> Is the if (day) { more c++ over-caution or is there legitimately a way that we
> can fail to find a day?  I couldn't think of one.

Due to the check
(aDate < this._getStartDate(aMainDate) || aDate > this._getEndDate(aMainDate))
and the subsequent code, which came in in patch v8, one always
finds a day. I'll remove the if (day) check in the next patch.

Comment 51

[Thomas Benisch]

Attachment: patch v9

see comment #50

Comment 52

[Joey Minta]

Comment on attachment 222466 [details] [diff] [review]
patch v9

You can just carry over r+ and set it yourself on the patch in these cases. :-)

Tree is closed, but I'll land this when it opens.

Comment 53

[Joey Minta]

and... patch checked in! whew. :-)