329226 - current date is not highlighted in calendar views and minimonth

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

[Thomas Benisch]

The current date is not highlighted in the calendar month/week views
and the minimonth.

Comment 1

[Thomas Benisch]

Attachment: patch which higlights the current date in minimonth and calendar views

This patch highlights the current date in the minimonth, the month view and the
week view. In the day view, the current date is not highlighted.
For highlighting I set the background color to some kind of light blue
(background-color: #dfeaf4;), but the highlight style is of course subject of
discussion.

Comment 2

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

Comment on attachment 214992 [details] [diff] [review]
patch which higlights the current date in minimonth and calendar views

>+++ mozilla/calendar/base/content/calendar-month-view.xml	2006-03-14 11:55:22.000000000 +0100
>@@ -840,16 +840,22 @@
>+          // get today's date
>+          var today = new CalDateTime();
>+          today.jsDate = new Date();
>+          today.isDate = true;
>+          today = today.getInTimezone(calendarDefaultTimezone());

This piece of code is copied three (or more) times in the patch. That's usually a sign that it needs to be in some central place. It already exists in calendarUtils.js, function now(). But you can't include that file in an xbl file. Maybe some addition to calIDateTime.idl? I'm not sure yet what the best solution is.

Using calendarDefaultTimezone() is not the right thing. The ciew already has a timezone set, and that is not always the default timezone. use .timezone of the view, that will always be the right setting, and also doesn't depend on including calendarUtils.js from any xul file that uses the view.

Comment 3

[Thomas Benisch]

(In reply to comment #2)
> This piece of code is copied three (or more) times in the patch. That's usually
> a sign that it needs to be in some central place. It already exists in
> calendarUtils.js, function now(). But you can't include that file in an xbl
> file. Maybe some addition to calIDateTime.idl? I'm not sure yet what the best
> solution is.

In principal you're right, that it's not nice to have the same
code three times in the patch. But we should keep in mind, that
the piece of code we're discussing here is only 4 lines. 

I think we have two issues here. The first one is code consolidation,
the second issue is how to get today's datetime for a given timezone.

First a comment to the first issue. The reason why I didn't
consolidate this code is simply the weakness of XBL. As you
already mentioned, I cannot simply include a *.js file.
I had several alternatives in mind:
- introduce a new method today() in the month and multiweek view
  binding
- implement a method today() in a base binding, derive month and
  multiday view bindings from the base binding
- introduce a new function in calendarUtils.js
I think all of those alternatives have more disadvantages than
the duplication of code.

The second issue is even more important. At the moment, there's
no function available, which gives me today's date for a given
timezone. The implementation of such a function should get the
current time from the operating system (OS). In this case, the
time from the OS refers to the timezone of the OS. Therefore the
time must be converted to the given timezone.
I think even the current implementation of now() in calendarUtils.js
doesn't work correctly. There are always some assumptions, e.g.
that the calendar default timezone is the same as the OS timezone
etc.

In addition, see also #328442 which deals with jsDate versus 
calIDateTime.

With keeping all that in mind, I think the four lines of code
are rather temporary and will be replaced in the future, as soon
as we have a method which gives today's datetime for a given
timezone.

> Using calendarDefaultTimezone() is not the right thing. The ciew already has a
> timezone set, and that is not always the default timezone. use .timezone of the
> view, that will always be the right setting, and also doesn't depend on
> including calendarUtils.js from any xul file that uses the view.

Good point, thanks for that hint. I will change the patch, so that it uses
the timezone of the view.

Comment 4

[Thomas Benisch]

Attachment: patch v2

This patch uses the timezone of the view.
In addition I tried to consolidate the code, so that the 4 lines of code which
calculate today are now only twice in the patch. As already mentioned, I consider
those 4 lines as a temporary fix only. What we need is a function, which 
calculates today or now for a given timezone from the OS clock.

Comment 5

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

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

>+++ mozilla/calendar/base/content/calendar-month-view.xml	2006-03-23 
>+      <method name="today">
>+        <body><![CDATA[
>+          var date = new CalDateTime();

That doesn't work. At least not here. (It does work for components loaded via calItemModule.js). You need to use something like:
          var date = Components.classes["@mozilla.org/calendar/datetime;1"]
                               .createInstance(Components.interfaces.calIDateTime);
which makes the code a bit longer, and is easy to due, due to the unification of the code :)

>+++ mozilla/calendar/base/content/calendar-multiday-view.xml	2006-03-23 
>+              if (d.compare(today) == 0 && this.numVisibleDates > 1) {

uber-nit: change the order in the if, becuase the second check is usually a lot cheaper then the first. With the swapped order, you don't waste time comparing dates while in the dayview.


>+++ mozilla/calendar/resources/content/datetimepickers/minimonth.xml	2006-03-23 

I'm not sure what is wrong here, but this doesn't seem to work in sunbird. A quick check told me that the samedata function is called, and does work. But I see no difference in the minimonth.

Other then that, the patch looks great. Thanks! Just need to fix those issues.

Comment 6

[Thomas Benisch]

(In reply to comment #5)
> >+++ mozilla/calendar/resources/content/datetimepickers/minimonth.xml	2006-03-23 
> 
> I'm not sure what is wrong here, but this doesn't seem to work in sunbird. A
> quick check told me that the samedata function is called, and does work. But I
> see no difference in the minimonth.

SunBird uses a different minimonth.css
(mozilla/calendar/sunbird/themes/winstripe/sunbird/datetimepickers/minimonth.css),
therefore I added my change to all minimonth.css files.

Comment 7

[Thomas Benisch]

Attachment: patch v3

patch with additional fixes for all issues from comment #5

Comment 8

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

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

patch looks good. thanks!

r=mvl

Comment 9

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

patch checked in

Comment 10

[Joey Minta]

*** Bug 331904 has been marked as a duplicate of this bug. ***

Comment 11

[Damian Szczepanik]

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060929 Sunbird/0.3

Comment 12

[Damian Szczepanik]

Litmus testcase 2605 created