264150 - Calendar widget should change number style for days with events [minimonth busy highlight]

Status: VERIFIED FIXED

(Calendar :: General, enhancement)

Description

[Fabien Méghazi]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040614 Firefox/0.9
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040614 Firefox/0.9

When looking at the calendar widget at the upper left of the application frame,
it's impossible to tell which days contains events because the numbers of the
days have the same style wheiter there are event planned or not this day.

Days with registered events should be in bold or in different color so it's
easier to see days with events.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[gekacheka]

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

Comment 2

[gekacheka]

Patch will depend on patch to bug 286841.

Comment 3

[gekacheka]

Attachment: refresh busy days in minimonth tab when calendars change or displayed month changes

(patch -l -p 2 -i file.patch)

Changes:

CalendarWindow.js:
  add method refreshMiniMonthBusyDates, calls minimonth.setBusyDates
      with a set of dates.
    Dates are obtained using the current range of dates displayed
      in the minimonth, from minimonth.beginDisplayDate to
      minimonth.endDisplayDate.
    Set is implemented as set of property names on a JavaScript
      object, where each property is the string of the date in
      milliseconds since 1970.	(Was changed from an array of
      integers to allow for dates that are not in the current month,
      so off-month days in the first and last weeks can also be
      highlighted.)
  add calls to refreshMiniMonthBusyDates to the event source
    change observer, to refresh the minimonth when events are
    loaded, added, changed, or deleted.
calendar.xul
  Add onmonthchange handler to minimonth that calls
    refreshMiniMonthBusyDates so highlighted dates are
    updated when user displays different month in minimonth.
calendarManager.js
  Add call to refreshMiniMonthBusyDates to function refreshView
    so that dates will be refreshed when calendar visibility is
    changed (calendar checkbox).
minimonth.xbl
  Add properties to access range of dates displayed,
      used by refresMinimonthBusyDates.
    monthDisplayedDate (date of 1st of month), 
    beginDisplayDate (date of 1st day of week containing 1st
      of month) and
    endDisplayDate (date AFTER last day of week containing last day in
      minimonth grid). 
  Modify setBusyDates method to accept a set of dates strings instead an
    an array of integer day-of-month, so that displayed days before
    1st day of month or after last day of month can also be
    highlighted when they are busy.
    (set is implemented as strings on a javascript object,
     where each string is the number of milliseconds since 1970
     on a date with events.)
minimonth.css
  Added underline as well as bold to make busy off-month days
    stand out better (they are low contrast grey, so the difference 
    of bold alone can be hard to see).


Tests:
 * when calendar starts up, in minimonth tab days with events are bold.
 * The days are the correct days for the given month.
 * Any displayed last days of the preceding month and
   any displayed first days of the following month are
   grey and bold if they have events.
 * Navigating to a different month by using the arrows or menus
   shows the corresponding month with the correct bold for days
   with events in that month.
 * Clicking the checkbox next to a calendar turns on or off the
   events in the calendar.  If those are the only events on that
   day, then the minimonth shows the differences (bold when events
   appear.no longer bold if no other events on day when calendar's
   events are hidden).
 * Adding, deleting, or moving an event on a day with no other
   events changes the minimonth bold day for that event.

Tested on sb0.2 branch MozCalendar Extension on Moz1.8b1 and TB1.0.
(Can test on trunk after nightly build for w2k becomes available.)

Comment 4

[Simon Paquet [:sipaq]]

gekacheka, 
I'll try to spin up a windows trunk nightly tonight for your testing purposes.

Comment 5

[Simon Paquet [:sipaq]]

(In reply to comment #4)
> gekacheka, 
> I'll try to spin up a windows trunk nightly tonight for your testing purposes.

You can find a nightly build from today for testing purposes at
http://www.babylonsounds.com/sunbird/Sunbird_20050320_Nightly.zip

Comment 6

[gekacheka]

Attachment: refresh busy days in minimonth tab when calendars change or displayed month changes (trunk)

(patch -l -p 2 -i file.patch)

Updated patch for trunk.  Patch depends on patch in bug 286841.

CHANGES:

calendar.js:

o Added call to refreshViews at end of initCalendar to make sure
  minimonth busydates are initialized at startup.

calendarWindow.js:

o Added CalendarWindow.refreshViews to make sure all four views are
  refreshed, including minimonth.

o Added calls to refreshViews in calendarWindow's calendarObserver.

o Added miniMonth monthchange event listener that calls
  calendarWindow.refreshMiniMonthBusyDates.

o Added calendarWindow.refreshMiniMonthBusyDates that, for events and
  todos in range of dates displayed in miniMonth, collects dates from
  each day of each occurence and marks dates as busy in the miniMonth.
  (Assumes calls to listener are synchronous, as discussed in bug 291252.)

minimonth.xml:

o Added properties miniMonth.beginDisplayDate and
  miniMonth.endExDisplayDate, bounding dates displayed as set by
  showMonth.

o Modified setBusyDates to use a hash table based on millisecond date
  instead of old approach, an array based on the date of month (which cannot
  include dates before month).	setBusyDatesArray provides simple
  interface, just a collection of dates in an array.

*/minimonth.css:

o Added underline to busy text to make busy dates more visible,
  especially on gray out-of-month dates.
  

TESTING

Tested on trunk build nightrat-win32-20050505.
o busy dates for events appear when first starting up.
o busy dates change when minimonth changes month.
  - forward, back
  - pick month
  - pick year

Known Bug: busy dates for todos do not appear (this might be problem
with calendar.getItems not returning occurrences of todos).

Comment 7

[Simon Paquet [:sipaq]]

mvl, could you please review the patch here. It is a pretty big UI issue, which
we should fix before 0.3a1.

Comment 8

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

No, we are not going to get this in 0.3a1. sb0.2 didn't have it either, so it is
not a regression fix.

Comment 9

[Stefan Sitter [:ssitter]]

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

Comment 10

[Joey Minta]

Attachment: sunbird proposal/patch

Putting this here to try to stimulate a bit of discussion.  This is a functional patch for Sunbird implementing this idea.  If we decide to take this, we should eventually try to find a way for Lightning and Sunbird to share the boxObject code.

Comment 11

[Matthew (lilmatt) Willis]

Reassigning all automatically assigned bugs from Mostafa to nobody@m.o

Bugspam filter: TorontoMostafaMove

Comment 12

[Joey Minta]

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

Comment 13

[Joey Minta]

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

Comment 14

[Joey Minta]

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

Comment 15

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

Comment on attachment 221538 [details] [diff] [review]
sunbird proposal/patch

>+var boxStyler = {
>+    onChange: function boxStylerChange(aBoxNodes) {
>+
>+        // We only want the events for the main month shown.  The first few
>+        // boxes may be in the previous month, but #7 must be in the correct
>+        // month, so use it.

Why do you only want to style changes in the main month? I don't see the reason, and I think it would be better to be consistent, and style all days with events in the same way.

with that fixed, ui-review=mvl

Comment 16

[Joey Minta]

Comment on attachment 221538 [details] [diff] [review]
sunbird proposal/patch

This patch is quite rotten... :(

Comment 19

[jemore]

Is there a build with this patch available ? I use Lightning 0.3.1 (build 2007021403), and i can't see in the mini-month view on the left, when a day as events. This is very important, because when the calendar is integrated in TBird, this is the only place where you can see emails and calendar together.

Comment 20

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Minimonth busy - v1

This patch does it for both lightning and sunbird.

I chose to keep the highlighter code away from the minimonth binding, to make it easier to port our minimonth to toolkit. That has the neat side effect, that most code related to the highlighting is in an extra file.

After talking with Christian, we chose to only highlight the main minimonth.

Comment 21

[jemore]

May I suggest another thing ? please make the highlightment in function of the tasks (or the occupations) of the day. Eg : If I have no task this day, no highlight. If I have one task : light blue ; 2 tasks : darker blue ; 3 tasks or more : red. With this, it will be easy to see, just in a look, the busiest days of the minimonth.

Comment 22

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #21)
> please make the highlightment in function of the
> tasks (or the occupations) of the day.

I'm not sure this should belong to the core functionality. Christian? It should easily be possible through an extension or userChrome.css:

.minimonth-day[busy] {
   color: #ff0000;
}
.minimonth-day[busy="1"] {
   color: #0090FF;
}
.minimonth-day[busy="2"] {
   color: #0000FF;
}

Comment 23

[gekacheka]

(Note: patch attachement 268947 does not provide a filename for first hunk.  did you use diff -N ?)

Comment 24

[Michael Büttner (no reviews TFN)]

Comment on attachment 268947 [details] [diff] [review]
Minimonth busy - v1

>+const kXULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
This constant is never referenced, is there any need to it? If not, I'd suggest to get rid of it.

>+                var n = parseInt(box.getAttribute("busy") || 0) +
>+                        (aState ? 1 : -1);
>+                if (n > 0) {
>+                    box.setAttribute("busy", n);
>+                } else {
>+                    box.removeAttribute("busy");
>+                }
That's nice in terms of customizability, cool stuff...

>+window.addEventListener("load", setupListener, false);
>+window.addEventListener("unload", setdownListener, false);
I would have used onLoad() and onUnload(), but that's probably personal preference. Would you mind changing the names?

>+      <field name="mDaymap">new Array()</field>
...
>+            this.mDayMap = {};
You don't need to initialize the field with 'new Array()'...

>+                switch (aBox.attributes[allowedAttributes].nodeName) {
>+                  case "selected":
>+                  case "othermonth":
>+                  case "today":
>+                  case "value":
>+                  case "class":
>+                  case "flex":
>+                    allowedAttributes++;
>+                    break;
>+                  default:
>+                    aBox.removeAttribute(aBox.attributes[allowedAttributes].nodeName);
I'd suggest to add a 'break' at the end of the default statement, in order to prevent unintended fall-through in case this code gets extended later.

>+              for (var k = 1; k < this.kDaysOfMonthBox.childNodes.length; k++) {
>+                for (i = 0; i < 7; i++) {
Missing 'var' before 'i'...

>-.minimonth-day[busy="true"] {
>+.minimonth-day[busy] {
>   font-weight: bold;
> }
This rule is now contained twice in the *.css file, the first occurrence is in line 135, the one with the typo (budy="true"). It's probably better to fix the typo and get rid of this redundant rule.

>+.minimonth-day[othermonth="true"][busy] {
>+  color: #b2b2b2;
>+}
You don't need this rule at all. .minimonth-day[othermonth="true"] (in line 131) sets the font color to gray, .minimonth-day[busy] (in line 135) sets the font to bold style. So days with an event that belong to the 'other' month are displayed grad+bold, isn't that all what we want?

Please also mark the obsolete patches that are attached to this bug as such. r=mickey with the above comments being addressed. Thanks for this nice feature.

Comment 25

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Minimonth busy - v2

(In reply to comment #24)
> (From update of attachment 268947 [details] [diff] [review])
> >+      <field name="mDaymap">new Array()</field>
> ...
> >+            this.mDayMap = {};
> You don't need to initialize the field with 'new Array()'...
No initialization at all? That used to be an array, now its an object. Should I use new Object(); or null? I chose the latter for now.

> >-.minimonth-day[busy="true"] {
> >+.minimonth-day[busy] {
> >   font-weight: bold;
> > }
> This rule is now contained twice in the *.css file, the first occurrence is in
> line 135, the one with the typo (budy="true"). It's probably better to fix the
> typo and get rid of this redundant rule.
I remove this typo rule in my patch to bug 364381. 


> 
> >+.minimonth-day[othermonth="true"][busy] {
> >+  color: #b2b2b2;
> >+}
> You don't need this rule at all. .minimonth-day[othermonth="true"] (in line
> 131) sets the font color to gray, .minimonth-day[busy] (in line 135) sets the
> font to bold style. So days with an event that belong to the 'other' month are
> displayed grad+bold, isn't that all what we want?
The difference between .minimonth-day[othermonth="true"] and this one is the color (#d2d2d2 vs #b2b2b2). I chose a darker color, since only marking the othermonth events bold doesn't provide enough contrast. I think it looks more natural with the darker color.

Comment 26

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Screenshot of color difference

Shows the difference between #d2d2d2 (left) and #b2b2b2 (right). Both are bold.

Comment 27

[Michael Büttner (no reviews TFN)]

(In reply to comment #25)
> No initialization at all? That used to be an array, now its an object.
I meant something like this...
  <field name="mDaymap"></field>
...but you could also write...
  <field name="mDaymap">{}</field>
...or...
  <field name="mDaymap">null</field>
...just choose the option you find most appealing.

> The difference between .minimonth-day[othermonth="true"] and this one is the
> color (#d2d2d2 vs #b2b2b2). I chose a darker color, since only marking the
> othermonth events bold doesn't provide enough contrast. I think it looks more
> natural with the darker color.
I find it nearly indistinguishable, but I'm fine with that additional rule. We can change colors later.

Comment 28

[Christian Jansen]

Maybe we should change the default color of ".minimonth-day[othermonth="true"]" #b2b2b2. It would assure a better readability. Especially on "older" LCD displays.

Comment 29

[Philipp Kewisch [:Fallen][📱📆]]

I used #b2b2b2 now for all othermonth=true.

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED

Comment 30

[Omar Bajraszewski]

Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.5pre) Gecko/20070630 Calendar/0.7pre

Comment 31

[Omar Bajraszewski]

I noticed that some days are bold but there's no event, task on the days. Trying to investigate what caused it. Other issue: After switching on a remote calendar the view wasn't updated---> will fill a new bug