349520 - working hours for calendar-multiday-view

Status: VERIFIED FIXED

(Calendar :: Calendar Frontend, defect, P1)

Description

[Thomas Benisch]

The calendar-multiday-view should support working hours.
This includes:
- different background colour for working and non-working hours
- preferences for start and end time of working hours

The UI in the Tools/Options dialog may be similar to the recently
removed UI for the start and end time of the view.

Comment 1

[Kay Koll]

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

Comment 2

[Matp75]

some ideas for this bug to improve usability :
- allow custom working time
- allow only show working time
- allow automatic extend if there's a special event running out of working hours (I mean usually there's nothing during the night so I don't wan't to see it but if exceptionnally, I have a event during off hours, the view should extend automagically to include it)
- add a visual effect if some event is out of the window, so the user knows it is interesting to scroll down/up
- if the window is too small to have the whole day and it's possible to move the scrollbar to show more events, do it automatically (exemple : the default view shows 8h00-18h00, there's one event 14h00-15h00 and another event 18h30-23h00 which is seen by default. By automatically scrolling of 5 hours, the screen will show 13h00-23h00, which will include both events )

Comment 3

[Matthew (lilmatt) Willis]

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

Comment 4

[Matthew (lilmatt) Willis]

tbe's salient point from bug 356449:

> I think the main point is, that a user is not interested in configuring the
> start/end time for whole view, but rather in configuring the start/end time for
> the visible region. Therefore an option would be to display all working hours in
> the visible region and adjust the height of the grid boxes accordingly. The
> non-working hours would be accessible by scrolling up or down.

Comment 5

[Greg Wastl]

Please add the ability to customize the hours on the single day view.

Comment 6

[Matthew (lilmatt) Willis]

Per newsgroup thread and user feedback, marking this blocking-calendar0.5+

Comment 7

[Jason M. Hurst Frye]

Personally, being able to restrict the time range of displayed hours in order to better utilize available screen space is my #1 'fault' with an otherwise very promising product.

Perhaps limiting the time ranges for the 'work week' view, while leaving the standard view alone, might make for a good solution.

Comment 8

[Matthew (lilmatt) Willis]

Bumping the priority on this.

This is on my "must fix" hit list for 0.5.  We'll need to make some decisions soon about how to fix this.

Comment 10

[gekacheka]

Bug 366989 (Grid controls) has a prototype implementation that provides controls to zoom in (decrease hours in view) or zoom out (increase hours in view), and the setting is persistent.  This should satisfy requests for ways to change the number of hours in the views, and associated preferences, which seem to be the high priority parts of this request.

Comment 11

[Thomas Benisch]

Attachment: working hours

This patch implements working hours for the calendar-multiday-view.

I added the following preferences for the start and end time of
working hours:

calendar.view.startworkhour
calendar.view.endworkhour

Those preferences can be set in the Tools/Options dialog.
Sunbird:   I added a 'Start time' and a 'End time' field
           to the workweek groupbox.
Lightning: In order to align the Sunbird and the Lightning 
           views options, I added a workweek groupbox with
           'Start time' and 'End time' field. In addition
           I moved the 'Start the week on' field into a
           'General' groupbox.

I used different background colors for working hours and non-working
hours in the week and in the day (!) view. As working hours refer
to working days only, I aligned the background colors for non-working
days and non-working hours.

The size of the grid boxes is adjusted at application startup and
when resizing the application window in that way, that the number
of hours shown in the visible area is equal to the number of working
hours. In addition, at application startup the view scrolls to the
first working hour.

Comment 12

[Thomas Benisch]

Attachment: Lightning view options

Comment 13

[Thomas Benisch]

Attachment: Sunbird view options

Comment 14

[Thomas Benisch]

Attachment: week view after startup

Comment 15

[Thomas Benisch]

Attachment: week view after scrolling

Comment 16

[Thomas Benisch]

Attachment: day view (workday)

Comment 17

[Thomas Benisch]

Attachment: day view (non-workday)

Comment 18

[Simon Paquet [:sipaq]]

We should really align the available settings in the options window between Sunbird and Lightning (in another bug). 

That being said, why can the start and end hour in Lightning be entered in a textbox and in Sunbird it can just be selected from a combobox? IMO it shouldn't be possible to enter a value here?

Comment 19

[Thomas Benisch]

(In reply to comment #18)
You're absolutely right.

I used textboxes in Lightning because the former start and end time
fields I removed in bug #133107 used the same approach.
But I agree, that the Lightning and Sunbird options should be aligned,
that means to use comboboxes also for Lightning.

Before I start to modify this patch, I'd like to have a comment from
the UI reviewers.

Comment 20

[Thomas Benisch]

Attachment: working hours v2

revised version of working hours patch

Comment 21

[Matthew (lilmatt) Willis]

I'm holding off on review until ui-r happens. 

I'll try to poke mvl/dmose about that today.

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

The resizing behavior looks good.

Without this patch, we already have four possible background colors for unused boxes, which already kinda feels like too many.  The weekview screenshots here now show six, which to me seems like confusing visual clutter.  I'm not entirely sure what to do about this however.  Suggestions?

Re the prefs, dropdown menus (a la Sunbird) does sound like the better choice.  One thing that seems unclear to me is that, as a naive user, looking at the pref windows, I wouldn't know what the semantics of something being part of the "work week" would actually be.  

One possible way to address both issues would be to simply ditch the background color changes for non-work hours/days entirely, and change the labels to something like "Default Hours in View".

Thoughts?

Comment 23

[Thomas Benisch]

(In reply to comment #22)
> The resizing behavior looks good.
> 
> Without this patch, we already have four possible background colors for unused
> boxes, which already kinda feels like too many.  The weekview screenshots here
> now show six, which to me seems like confusing visual clutter.  I'm not
> entirely sure what to do about this however.  Suggestions?

You're right, that there are really too much colors in the week view.
That's why I choose the same color for non-working hours and weekend days.
Nevertheless there are six colors left.

I think this problem can be attacked by the following strategy.
In the mid-term I would remove the selected day feature and allow to select
half hour boxes only. Then one can use the same selection color for working
hours and non-working hours. Another option would be to make the today
colors less prominent.

In addition Christian Jansen is working on a new view design. I think his
approach is to have the view less colored, but probably Christian can comment
this.

> Re the prefs, dropdown menus (a la Sunbird) does sound like the better choice. 

I think everybody agrees here.

> One thing that seems unclear to me is that, as a naive user, looking at the
> pref windows, I wouldn't know what the semantics of something being part of the
> "work week" would actually be.  

I didn't get this point, can you please elaborate.

> One possible way to address both issues would be to simply ditch the background
> color changes for non-work hours/days entirely, and change the labels to
> something like "Default Hours in View".

I personally would favour an additional preference, e.g. number of displayed
hours, in the long-term. But this conflicts with gekacheka's approach in 
Bug 366989.

Your proposal of renaming working hours to default hours in view and removing
the different background color for those hours definitely kills the working
hours feature. Therefore I'd like to have a clear statement, if you want to
have working hours or not.

Comment 24

[Matthew (lilmatt) Willis]

Comment on attachment 252587 [details] [diff] [review]
working hours v2

Removing review request until this makes it through ui-r

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #23)
> 
> I think this problem can be attacked by the following strategy.
> In the mid-term I would remove the selected day feature and allow to select
> half hour boxes only. Then one can use the same selection color for working
> hours and non-working hours. Another option would be to make the today
> colors less prominent.

I just looked at iCal, and the way they handle it is a combination of very faint colors and not having the timescale be in separately colored boxes.  mvl, any thoughts here?

> > One thing that seems unclear to me is that, as a naive user, looking at the
> > pref windows, I wouldn't know what the semantics of something being part of the
> > "work week" would actually be.  
> 
> I didn't get this point, can you please elaborate.

As an end user, when I first happen to be in the prefs window of a new program, for whatever reason, I typically explore a bit, to see what exactly is configurable in a program.  If I ran across the pref window pictured, I would be confused, in the sense that I'd wonder "what does changing these settings actually do?".  Of course, the existing dialog text already has this issue.  :-/

> > One possible way to address both issues would be to simply ditch the background
> > color changes for non-work hours/days entirely, and change the labels to
> > something like "Default Hours in View".
> 
> I personally would favour an additional preference, e.g. number of displayed
> hours, in the long-term. But this conflicts with gekacheka's approach in 
> Bug 366989.
> 
> Your proposal of renaming working hours to default hours in view and removing
> the different background color for those hours definitely kills the working
> hours feature. Therefore I'd like to have a clear statement, if you want to
> have working hours or not.

So what you call "the working hours feature" seems to me to be a collection of sub-features.  In particular:

a) sets the number of hours shown
b) chooses what time to scroll to at startup
c) tweaks the background colors
d) relates a) and b) to the days of the workweek

It seems clear to me that a) and b) are important, c) seems useful but perhaps not critical, and d) I'm somewhat skeptical about. In particular, thinking about d) in terms of our two target users: if the student works, it's likely to be a part-time addition to school, so it's not likely that they'll want to only their work hours; the second user is defined as a small-business person in both work and personal life, which makes it seem reasonably likely that they'll want to view more than just their work hours at once also.  As such, I think it's both clearer to first viewers of the pref dialog, and more accurate for our target users not to label these hours as "work hours".  iCal does the additional preference thing that you suggest, and it uses the following UI strings, which seem pretty reasonable:

Day Starts At:
Day Ends At:
Show __ hours at a time

Comment 26

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

I think the most important reason for wanting to have a way to indicate work hours is that the non-workhours are scrolled out of view by default, and the available space can be used to make more room for work hours.
I'm not sure if non-work hours need to be colored. How about just a 2px line between work and non-work, instead of the default 1px?
If that doesn't work, I suggest with playing with brightness and keeping the color the same.

And I agree with dmose's suggested wording of the pref panel. Don't link the work hours to work days.

Comment 27

[Brian Martin]

As a keen user of Sunbird 0.2 & 0.3, I think the issue is about the default range of hours shown in the view. 

These are not necessarily "work hours", they may include events before breakfast or after dinner, there may not even be any "work" as such. Most people whether students, office workers, or business people think in terms of a "normal" day e.g. 6am - midnight, or 8am to 10pm, etc, depending on their lifestyle. Users will mostly prefer to see that chosen range by default, but with automatic resizing for unusual events say at 3am, as already suggested in this thread by others.

The key problem is that Sunbird 0.2 did this nicely, but 0.3 always displays the least likely scenario, 24hour day everyday, with no option to customise it.

I agree with those who find the term "workday hours" either ambiguous or misleading, and suggest "default viewing hours" or similar is much clearer.

Comment 28

[Thomas Benisch]

I'll try to summarize the feedback from UI review and refer to the
sub-features of comment #25:

a) sets the number of hours shown
   ------------------------------

   I think there's an agreement to have preferences for start and end time.
   In the Tools/Options dialog those preferences are called
     Day Starts At:
     Day Ends At:
   Those preferences can be configured by a combobox.
   Also the current resizing behaviour, which adjusts the size of the grid
   boxes to the number of hours between start and end time, seems to be
   accepted.

   Open questions:

   - Where to place those preferences?
     The Workweek groupbox seems to be disfavoured.
     Proposal: Add them to the General groupbox below 'Start the week on'.

   - What about a preference for the number of displayed hours, called
     'Show __ hours at a time' in iCal?
     Is this something you want to have?
     If yes, when (this patch or later)?
	 Please not, that the resizing behaviour will change in this case,
     that means the size of the grid boxes is determined by this preference.

b) chooses what time to scroll to at startup
   -----------------------------------------

   It seems to be accepted to scroll to the hour defined by the start time
   preference.

   Note, that this behaviour might change, if a number of displayed hours
   preference is added.

   Proposal: If the number of displayed hours preference is added, scroll
             in that way, that the display hour range is centered in the
             visible region.

c) tweaks the background colors
   ----------------------------

   I think those colors are important. I personally don't like the 2px proposal
   from mvl and prefer color boxes. Playing with brightness of the same color is
   actually what I did in the patch (at least I tried to). Probably playing with
   opacity gives some better results.

d) relates a) and b) to the days of the workweek
   ---------------------------------------------

   I interpret your comments in that way, that you don't want to have this
   feature. I will accept this, but what I really don't understand is the fact,
   that you agreed to have workdays but don't want to have working hours.
   For me that's rather inconsistent and by reusing your statements about
   our target users I can argue for removing working days.

Comment 29

[Dan Mosedale (:dmosedale, :dmose)]

>    I think there's an agreement to have preferences for start and end time.
>    In the Tools/Options dialog those preferences are called
>      Day Starts At:
>      Day Ends At:
>    Those preferences can be configured by a combobox.

Yes.

>    Also the current resizing behaviour, which adjusts the size of the grid
>    boxes to the number of hours between start and end time, seems to be
>    accepted.

Depends on whether you want to make the "show ___ hours at a time"
preference part of this patch or not.

>    Open questions:
> 
>    - Where to place those preferences?
>      The Workweek groupbox seems to be disfavoured.
>      Proposal: Add them to the General groupbox below 'Start the week on'.

Sounds good.

>    - What about a preference for the number of displayed hours, called
>      'Show __ hours at a time' in iCal?
>      Is this something you want to have?

Yes.

>      If yes, when (this patch or later)?

Your choice.

>          Please not, that the resizing behaviour will change in this case,
>      that means the size of the grid boxes is determined by this preference.

That's fine.

> b) chooses what time to scroll to at startup
>    -----------------------------------------
> 
>    It seems to be accepted to scroll to the hour defined by the start time
>    preference.
>
>    Note, that this behaviour might change, if a number of displayed hours
>    preference is added.
> 
>    Proposal: If the number of displayed hours preference is added, scroll
>              in that way, that the display hour range is centered in the
>              visible region.

If I'm understanding you correctly, by "display hour range", you're
referring to the set of hours bounded by dayStartsAt and dayEndsAt.
Correct?  If so, that works if displayedHours >= dayEndsAt - dayStartsAt,
but seems strange in the other case.  It seems to me that "scroll to
dayStartsAt" works in both situations.  Is there some disadvantage to that
I'm missing?

> c) tweaks the background colors
>    ----------------------------
> 
>    I think those colors are important. I personally don't like the 2px proposal
>    from mvl and prefer color boxes. Playing with brightness of the same color
>    is actually what I did in the patch (at least I tried to). Probably playing
>    with  opacity gives some better results.

The 2px proposal has the problem that it's less visually clear that
something is "out of the normal space".  I'd be interested in seeing
screenshots of much fainter colors here.  How would changing the
opacity make any difference at all?

> d) relates a) and b) to the days of the workweek
>    ---------------------------------------------
> 
>    I interpret your comments in that way, that you don't want to have this
>    feature.

Correct.

>    I will accept this, but what I really don't understand is the fact,
>    that you agreed to have workdays but don't want to have working hours.
>    For me that's rather inconsistent and by reusing your statements about
>    our target users I can argue for removing working days.

I don't believe these are "working hours".  I believe that they are
the hours that the day starts and ends at, but for most of our target
users, this will include more than just the hours that they happen to
be at work.

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

W.r.t. why have workdays, this is a visual indicator for when you can easily schedule events (e.g. trips) that take a day or more.

Comment 31

[Brian Martin]

re WorkDays vs Working Hours, I agree with Dan M ...

quoting Dan...
   "I don't believe these are "working hours".  I believe that they are
   the hours that the day starts and ends at, but for most of our target
   users, this will include more than just the hours that they happen to
   be at work."

Most ppl incl myself work Mon-Fri, so work days means I'll mostly be at work, but may want to schedule events before/after work. So working hours of 9-5 have little relevance to my schedule, I may want to see by default my waking hours say 6am - midnight for example. In my case I only use Sunbird at home, and the "working hours" are simply blocked out 9-5 with a recurring event. The only events of interest to me are before/after working hours on the work days, i.e what I aim to get done that day other than work, but on non-workdays, the interesting events cover the full days hours.

So while I may occassionally note an event for 3AM e.g. a software rollout or similar for work, done from home, most of the time, I only want to see from 6am to midnight, which I would set as my default viewing/scrolling window.

At the same time the workdays concept is good because not everyone works Mon-Fri, my son for example is currently working Tues,Wed,Fri only.

The earlier Sunbird 0.2 actually did this quite nicely, I'm puzzled why this feature disappeared from 0.3

Comment 32

[Thomas Benisch]

Attachment: week view after scrolling with fainter colors

week view after scrolling with fainter colors

Comment 33

[Thomas Benisch]

(In reply to comment #29)
> The 2px proposal has the problem that it's less visually clear that
> something is "out of the normal space".  I'd be interested in seeing
> screenshots of much fainter colors here.  How would changing the
> opacity make any difference at all?

You're right, that using opacity won't solve the original problem.
For a screenshot with fainter colors please have a look at the
attachment of comment #32.

Comment 34

[Thomas Benisch]

(In reply to comment #29)
> If I'm understanding you correctly, by "display hour range", you're
> referring to the set of hours bounded by dayStartsAt and dayEndsAt.
> Correct?  If so, that works if displayedHours >= dayEndsAt - dayStartsAt,
> but seems strange in the other case.  It seems to me that "scroll to
> dayStartsAt" works in both situations.  Is there some disadvantage to that
> I'm missing?

You're right. In the case displayedHours >= dayEndsAt - dayStartsAt
centering the display hour range looks better, but in the other case
it's really strange and probably not what the user wants.
Therefore I will use your proposal and scroll to dayStartsAt.

Comment 35

[Stefan Sitter [:ssitter]]

Comment on attachment 256457 [details]
week view after scrolling with fainter colors

What is the reason for
1. changing color of current day?
2. changing color of selected day?
3. changing color of non-workweek days?

Comment 36

[Thomas Benisch]

(In reply to comment #35)
> (From update of attachment 256457 [details])

This screenshot is a proposal which I made due to comment #29
from Dan: "I'd be interested in seeing screenshots of much
fainter colors here."

My original proposal used the same colors for selection and today,
but wasn't favored by Dan and Michiel. Of course there's also
the possibility to improve the original proposal by choosing better
colors for "non-working" hours (e.g. lighter gray) and for the
overlap regions (blue-gray and yellow-gray).

It seems that choosing the right colors is the only problem
left for getting UI review. Therefore any help or suggestions
are appreciated.

Comment 37

[Thomas Benisch]

Attachment: week view after scrolling v3

This is a modified version of my original proposal.
The colors for the selected and current day are
not changed for "working hours", but only for
"non-working hours". In addition, the gray for
"non-working hours" is a little bit brighter.

Comment 38

[Thomas Benisch]

(In reply to comment #37)
> The colors for the selected and current day are
> not changed for "working hours", but only for

Uups, this statement is wrong. I did change
the colors for the selected day. Unfortunately I
mixed something up. A new screenshot follows.

Comment 39

[Thomas Benisch]

Attachment: week view after scrolling v4

This is a modified version of my original proposal.
The colors for the selected and current day are
not changed for "working hours", but only for
"non-working hours". In addition, the gray for
"non-working hours" is a little bit brighter.

Comment 40

[Thomas Benisch]

Attachment: week view after scrolling (status quo)

Just for comparison, this is the status quo of the
week view. Due to the yellow used for weekend days,
I think that my proposal from comment #39
(week view after scrolling v4) appears to be even
less colorful.

Comment 41

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #40)
> Created an attachment (id=256619) [details]
> week view after scrolling (status quo)
> 
> Just for comparison, this is the status quo of the
> week view. Due to the yellow used for weekend days,
> I think that my proposal from comment #39
> (week view after scrolling v4) appears to be even
> less colorful.

I agree; this looks nice!   ui-r=dmose.  Thanks for you patience sorting through these issues.

Comment 42

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 252587 [details] [diff] [review]
working hours v2

I think the version of this patch for which I've just given ui-review+ hasn't yet been posted, so removing the ui-r? from this one.  Feel free to mark the patch corresponding to v4 as ui-r+ once it's up.

Comment 43

[Thomas Benisch]

Attachment: Lightning view options v4

Comment 44

[Thomas Benisch]

Attachment: Sunbird view options v4

Comment 45

[Thomas Benisch]

Attachment: working hours v4

This patch addresses all comments from ui review.

I added the following preferences for the day start hour
and day end hour:

calendar.view.daystarthour
calendar.view.dayendhour

In addition I added a preference for the number of
visible hours:

calendar.view.visiblehours

In previous discussions there was consent of adding
those preferences to the 'General' groupbox
of the Lightning/Sunbird options dialog.
As those preferences refer to the day and week view
only, I changed my mind and added a 'Day and Week View'
groubox. Those changes can be seen in the screenshots
from comments #43 and #44.

Due to those changes I didn't carry over the ui review
flag and ask for ui review again.

Comment 46

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 256793 [details] [diff] [review]
working hours v4

ui-review+ = dmose, if you change the verbiage from "Day and Week View" to "Day and Week Views".

Comment 47

[Thomas Benisch]

Attachment: working hours v5

This patch addresses comment #46. In addition I fixed a scrolling
problem, which appeared only in Sunbird.

I carried over the ui-review flag from the last patch.

Comment 48

[Stefan Sitter [:ssitter]]

Comment on the preference dialog: 

Sunbird and Lightning now share the same code. You'll only have to add the new controls to m/calendar/base/content/preferences/views.xul, ship views.js in m/calendar/base/jar.mn and call gViewsPane.init() from preferences.xul and messenger-overlay-preferences.xul.

I'm not sure if all the content will fit into Lightning pane due to the extra space required for tabs. One solution might be to merge the General and Workweek section into one section or to hide the Multiweek section in Lightning (e.g. via css rule).

Comment 49

[Thomas Benisch]

(In reply to comment #48)
> Comment on the preference dialog: 
> 
> Sunbird and Lightning now share the same code. You'll only have to add the new
> controls to m/calendar/base/content/preferences/views.xul, ship views.js in
> m/calendar/base/jar.mn and call gViewsPane.init() from preferences.xul and
> messenger-overlay-preferences.xul.

Thanks for those hints.

> I'm not sure if all the content will fit into Lightning pane due to the extra
> space required for tabs. One solution might be to merge the General and
> Workweek section into one section or to hide the Multiweek section in Lightning
> (e.g. via css rule).

You're right, there's not enough space in the views tab. I prefer to hide
the Multiweek section simply due to the fact that Lightning doesn't have
a multiweek view at all. I think instead of adding a css rule the multiweek
groupbox can be hided in messenger-overlay-preferences.xul, because that's
what overlays are for.

Comment 50

[Thomas Benisch]

Attachment: working hours v6

This is a new revision of the patch which broke due to the checkin for
Bug 372014. As the Lightning and Sunbird preferences now share the
same code, I adjusted the patch accordingly.

In addition I removed the multiweek groupbox from the Lightning
views option tab, see also comment #49. I'm not sure if this
requires a new UI review, but I consider this as a bug which came in
with 372014.

Comment 51

[Matthew (lilmatt) Willis]

Comment on attachment 257964 [details] [diff] [review]
working hours v6

I already asked tbe in IRC to make the changes to day and week view in a common location, rather than duplicating code.

>+++ mozilla/calendar/base/content/preferences/views.xul	2007-03-09 12:31:04.921875000 +0100
>+                        <hbox align="center" pack="center">
>+                            <label value="&pref.calendar.view.visiblehours.label;"
>+                                   accesskey="&pref.calendar.view.visiblehours.accesskey;"
>+                                   control="visiblehours" />
Remove the space before the /


>+++ mozilla/calendar/locales/en-US/chrome/calendar/preferences/views.dtd	2007-03-09 12:27:49.890625000 +0100
>+<!ENTITY pref.calendar.view.daystart.label "Day starts at:">
>+<!ENTITY pref.calendar.view.daystart.accesskey "d">
This should be a capital D ^^^

>+<!ENTITY pref.calendar.view.dayend.label "Day ends at:">
>+<!ENTITY pref.calendar.view.dayend.accesskey "y">
>+<!ENTITY pref.calendar.view.visiblehours.label "Show">
Add a : to this ^^^^  "Show:"


>+++ mozilla/calendar/base/jar.mn	2007-03-09 12:27:49.843750000 +0100
>+    content/calendar/preferences/views.js        (content/preferences/views.js)
views.js appears to be missing from the patch.

Comment 52

[Thomas Benisch]

(In reply to comment #51)
> >+++ mozilla/calendar/base/jar.mn	2007-03-09 12:27:49.843750000 +0100
> >+    content/calendar/preferences/views.js        (content/preferences/views.js)
> views.js appears to be missing from the patch.

views.js is an old file which was removed in Bug 133107. It was known, that this file
will be added later again. Therefore the file was only removed from the jar.mn, but
not from CVS. Therefore it's still in the CVS.

Comment 53

[Matthew (lilmatt) Willis]

Thanks for clearing that up.

As discussed in IRC, you have r=lilmatt to land the .dtd changes (including the above review comments) today to beat the string freeze.

Comment 54

[Thomas Benisch]

calendar/locales/en-US/chrome/calendar/preferences/views.dtd
checked in on HEAD and MOZILLA_1_8_BRANCH
due to string freeze

Comment 55

[Thomas Benisch]

Attachment: working hours v7

This patch addresses comment #51.

In order to prevent code duplication I added a
calendar-decorated-multiday-base-view binding,
where the calendar-decorated-day-view and
calendar-decorated-week-view derive binding from.

I also managed to share the code for the pref observer.
As in a derived binding the base method of an overriden
method cannot be called, I made the approach with the
handlePreference() and the handleCommonPreference()
method.

Comment 56

[Matthew (lilmatt) Willis]

Comment on attachment 258306 [details] [diff] [review]
working hours v7

>+++ mozilla/calendar/base/content/calendar-decorated-day-view.xml	2007-03-12 16:20:41.906250000 +0100
>-    <binding id="calendar-decorated-day-view" extends="chrome://calendar/content/calendar-decorated-base.xml#calendar-decorated-base-view">
>+    <binding id="calendar-decorated-day-view" extends="chrome://calendar/content/calendar-decorated-multiday-base-view.xml#calendar-decorated-multiday-base-view">
Since you're touching this line, please put "extends" beneath "id"


>+++ mozilla/calendar/base/content/calendar-decorated-multiday-base-view.xml	2007-03-12 17:18:48.750000000 +0100
>+<?xml version="1.0"?>
>+<!--
>+   - ***** BEGIN LICENSE BLOCK *****
Please format this like so:
<?xml version="1.0" encoding="UTF-8"?>
<!-- ***** BEGIN LICENSE BLOCK *****

>+   - ***** END LICENSE BLOCK *****
>+-->
Please format this like so:
   - ***** END LICENSE BLOCK ***** -->


>+
>+<bindings id="calendar-specific-view-bindings"
>+    xmlns="http://www.mozilla.org/xbl"
>+    xmlns:html="http://www.w3.org/1999/xhtml"
>+    xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+    xmlns:xbl="http://www.mozilla.org/xbl">
>+
>+    <binding id="calendar-decorated-multiday-base-view" extends="chrome://calendar/content/calendar-decorated-base.xml#calendar-decorated-base-view">
Please put "extends" beneath "id".

>+            <field name="mPrefObserver"><![CDATA[
>+            ({ calView: this,
>+               observe: function calDecWeekViewPrefChange(subj, topic, pref) {
This inner function name should probably be changed.

>+                   this.calView.handlePreference(subj, topic, pref);
>+                   return;
>+               }
>+            })
>+            ]]></field>
>+
>+            <method name="handlePreference">
>+                <parameter name="aSubject"/>
>+                <parameter name="aTopic"/>
>+                <parameter name="aPreference"/>
>+                <body><![CDATA[
>+                    this.handleCommonPreference(aSubject, aTopic, aPreference);
>+                ]]></body>
>+            </method>
Do we really need this method since you're defining "handlePreference" in the child views?

>+            <method name="handleCommonPreference">
>+                <parameter name="aSubject"/>
>+                <parameter name="aTopic"/>
>+                <parameter name="aPreference"/>
>+                <body><![CDATA[
>+                    aSubject.QueryInterface(Components.interfaces.nsIPrefBranch2);
>+
>+                    switch (aPreference) {
Everything inside this switch statement should be indented 4 more spaces.
>+
>+                    case "calendar.week.d0sundaysoff":
>+                    case "calendar.week.d1mondaysoff":
>+                    case "calendar.week.d2tuesdaysoff":
>+                    case "calendar.week.d3wednesdaysoff":
>+                    case "calendar.week.d4thursdaysoff":
>+                    case "calendar.week.d5fridaysoff":
>+                    case "calendar.week.d6saturdaysoff":
>+                        debugger;
I'm guessing you didn't mean to leave this line ^^^ in

>+                        if (!this.startDay || !this.endDay) {
>+                            // Don't refresh if we're not initialized
>+                            return;
>+                        }
>+                        this.updateDaysOffPrefs();
>+                        this.goToDay(this.selectedDay);
>+                        break;
>+
>+                    case "calendar.view.daystarthour":
>+                        this.mDayStartMin = aSubject.getIntPref(aPreference) * 60;
>+                        var viewElem = document.getAnonymousElementByAttribute(
>+                                       this, "anonid", "view-element");
>+                        viewElem.setDayStartEndMinutes(this.mDayStartMin,
>+                                                       this.mDayEndMin);
>+                        if (!this.startDay || !this.endDay) {
>+                            // Don't refresh if we're not initialized
>+                            return;
>+                        }
>+                        this.goToDay(this.selectedDay);
>+                        break;
>+
>+                    case "calendar.view.dayendhour":
>+                        this.mDayEndMin = aSubject.getIntPref(aPreference) * 60;
>+                        var viewElem = document.getAnonymousElementByAttribute(
>+                                       this, "anonid", "view-element");
>+                        viewElem.setDayStartEndMinutes(this.mDayStartMin,
>+                                                       this.mDayEndMin);
>+                        if (!this.startDay || !this.endDay) {
>+                            // Don't refresh if we're not initialized
>+                            return;
>+                        }
>+                        this.goToDay(this.selectedDay);
>+                        break;
>+
>+                    case "calendar.view.visiblehours":
>+                        var visibleMinutes = aSubject.getIntPref(aPreference) * 60;
>+                        var viewElem = document.getAnonymousElementByAttribute(
>+                                       this, "anonid", "view-element");
>+                        viewElem.setVisibleMinutes(visibleMinutes);
>+                        if (!this.startDay || !this.endDay) {
>+                            // Don't refresh if we're not initialized
>+                            return;
>+                        }
>+                        this.goToDay(this.selectedDay);
>+                        break;
>+
>+                    case "calendar.timezone.local":
>+                        var viewElem = document.getAnonymousElementByAttribute(
>+                                       this, "anonid", "view-element");
>+                        viewElem.timezone = aSubject.getCharPref(aPreference);
>+                        if (!this.startDay || !this.endDay) {
>+                            // Don't refresh if we're not initialized
>+                            return;
>+                        }
>+                        this.goToDay(this.selectedDay);
>+                        break;
First off, I think we can either move the "var viewElem = doc..." line above the switch statement, since we use it in each case, or we could move it to its own method, since we use it in updateDaysOffPrefs below as well.
Secondly, I'd also like to move the "if (!this.startDay..." bit into a helper function, so as to reduce code duplication.  Maybe it can simply go _after_ the case statement?

>+
>+                    default:
>+                        break;
>+                    }
>+                    return;
>+                ]]></body>
>+            </method>
>+            
Trailing spaces on this line ^^^


>diff -x CVS -a -U 8 -pN -r mozilla_ref/calendar/base/content/calendar-decorated-week-view.xml mozilla/calendar/base/content/calendar-decorated-week-view.xml
>--- mozilla_ref/calendar/base/content/calendar-decorated-week-view.xml	2007-03-12 12:33:24.953125000 +0100
>+++ mozilla/calendar/base/content/calendar-decorated-week-view.xml	2007-03-12 16:54:46.453125000 +0100
>@@ -41,113 +41,70 @@
> <!-- Note that we depend on some helper functions in calUtils.js here-->
> 
> <bindings id="calendar-specific-view-bindings"
>     xmlns="http://www.mozilla.org/xbl"
>     xmlns:html="http://www.w3.org/1999/xhtml"
>     xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>     xmlns:xbl="http://www.mozilla.org/xbl">
> 
>-    <binding id="calendar-decorated-week-view" extends="chrome://calendar/content/calendar-decorated-base.xml#calendar-decorated-base-view">
>+    <binding id="calendar-decorated-week-view" extends="chrome://calendar/content/calendar-decorated-multiday-base-view.xml#calendar-decorated-multiday-base-view">
Since you're touching this line, please put "extends" beneath "id"

>+            <method name="handlePreference">
>+                <parameter name="aSubject"/>
>+                <parameter name="aTopic"/>
>+                <parameter name="aPreference"/>
>+                <body><![CDATA[
>+                    aSubject.QueryInterface(Components.interfaces.nsIPrefBranch2);
>+
>+                    switch (aPreference) {
>+
>+                    case "calendar.week.start":
>+                        this.mWeekStartOffset = aSubject.getIntPref(aPreference);
>+                        viewElem = document.getAnonymousElementByAttribute(
>+                                   this, "anonid", "view-element");
>+                        viewElem.weekStartOffset = aSubject.getIntPref(aPreference);
>+                        if (!this.startDay || !this.endDay) {
>+                            // Don't refresh if we're not initialized
>+                            return;
>+                        }
>+                        // Refresh the view so the settings take effect
>+                        this.goToDay(this.selectedDay);
>+                        break;
>+
>+                    default:
>+                        this.handleCommonPreference(aSubject, aTopic, aPreference);
>+                        break;
>+                    }
The contents of this switch statement should be indented 4 more spaces.


>diff -x CVS -a -U 8 -pN -r mozilla_ref/calendar/base/content/calendar-multiday-view.xml mozilla/calendar/base/content/calendar-multiday-view.xml
>--- mozilla_ref/calendar/base/content/calendar-multiday-view.xml	2007-03-12 12:33:25.031250000 +0100
>+++ mozilla/calendar/base/content/calendar-multiday-view.xml	2007-03-12 12:55:48.984375000 +0100
>@@ -1266,16 +1308,31 @@
>                                             Ci.nsIScriptableDateFormat.timeFormatNoSeconds,
>                                             endhr, endmin, 0);
> 
>           this.fgboxes.startlabel.setAttribute("value", startstr);
>           this.fgboxes.endlabel.setAttribute("value", endstr);
>    
>         ]]></body>
>       </method>
>+      <method name="setDayStartEndMinutes">
>+        <parameter name="aDayStartMin"/>
>+        <parameter name="aDayEndMin"/>
>+        <body><![CDATA[
>+          if (aDayStartMin < this.mStartMin || aDayStartMin > aDayEndMin ||
>+              aDayEndMin > this.mEndMin) {
>+              throw Components.results.NS_ERROR_INVALID_ARG;
>+          }
>+          if (this.mDayStartMin != aDayStartMin ||
>+              this.mDayEndMin != aDayEndMin) {
>+              this.mDayStartMin = aDayStartMin;
>+              this.mDayEndMin = aDayEndMin;
>+          }
>+        ]]></body>
>+      </method>
These should use 2 space indenting inside the if statments to match the rest of the file.

>@@ -2732,34 +2798,70 @@
>             } else {
>               minute = Math.round(x.value/this.mPixPerMin);
>             }
>           }
>           return minute;
>         ]]></body>
>       </method>
> 
>+      <method name="setFirstVisibleMinute">
>+        <parameter name="aMinute"/>
>+        <body><![CDATA[
>+          this.mFirstVisibleMinute = aMinute;
>+        ]]></body>
>+      </method>
also return this.mFirstVisibleMinute for convenience

>+      <method name="setVisibleMinutes">
>+        <parameter name="aVisibleMinutes"/>
>+        <body><![CDATA[
>+          if (aVisibleMinutes <= 0 ||
>+              aVisibleMinutes > (this.mEndMin - this.mStartMin)) {
>+              throw Components.results.NS_ERROR_INVALID_ARG;
>+          }
>+          if (this.mVisibleMinutes != aVisibleMinutes) {
>+              this.mVisibleMinutes = aVisibleMinutes;
>+          }
>+        ]]></body>
also return this.mVisibleMinutes for convenience

Please also use 2-space indenting for the inner methods.  I know the file goes back and forth, but it looks like 2 is the most common in this file.



r=lilmatt with those nits addressed

Comment 57

[Thomas Benisch]

(In reply to comment #56)
> (From update of attachment 258306 [details] [diff] [review])
> >+++ mozilla/calendar/base/content/calendar-decorated-day-view.xml	2007-03-12 16:20:41.906250000 +0100
> >-    <binding id="calendar-decorated-day-view" extends="chrome://calendar/content/calendar-decorated-base.xml#calendar-decorated-base-view">
> >+    <binding id="calendar-decorated-day-view" extends="chrome://calendar/content/calendar-decorated-multiday-base-view.xml#calendar-decorated-multiday-base-view">
> Since you're touching this line, please put "extends" beneath "id"

done

> >+++ mozilla/calendar/base/content/calendar-decorated-multiday-base-view.xml	2007-03-12 17:18:48.750000000 +0100
> >+<?xml version="1.0"?>
> >+<!--
> >+   - ***** BEGIN LICENSE BLOCK *****
> Please format this like so:
> <?xml version="1.0" encoding="UTF-8"?>
> <!-- ***** BEGIN LICENSE BLOCK *****

done

> >+   - ***** END LICENSE BLOCK *****
> >+-->
> Please format this like so:
>    - ***** END LICENSE BLOCK ***** -->

done

> >+<bindings id="calendar-specific-view-bindings"
> >+    xmlns="http://www.mozilla.org/xbl"
> >+    xmlns:html="http://www.w3.org/1999/xhtml"
> >+    xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> >+    xmlns:xbl="http://www.mozilla.org/xbl">
> >+
> >+    <binding id="calendar-decorated-multiday-base-view" extends="chrome://calendar/content/calendar-decorated-base.xml#calendar-decorated-base-view">
> Please put "extends" beneath "id".

done

> >+            <field name="mPrefObserver"><![CDATA[
> >+            ({ calView: this,
> >+               observe: function calDecWeekViewPrefChange(subj, topic, pref) {
> This inner function name should probably be changed.

I changed the name to calDecMultidayViewPrefChange.

> >+            <method name="handlePreference">
> >+                <parameter name="aSubject"/>
> >+                <parameter name="aTopic"/>
> >+                <parameter name="aPreference"/>
> >+                <body><![CDATA[
> >+                    this.handleCommonPreference(aSubject, aTopic, aPreference);
> >+                ]]></body>
> >+            </method>
> Do we really need this method since you're defining "handlePreference" in the
> child views?

I defined handlePreference only for the week view, for the day view the base
implementation is used. An alternative would be to define handlePreference also
for the day view and remove it from the multiday base binding. Nevertheless I
prefer the current approach, because the code path is more obvious
(mPrefObserver calls handlePreference, which calls handleCommonPreference).
But if you want that I change this, please let me know.

> >+            <method name="handleCommonPreference">
> >+                <parameter name="aSubject"/>
> >+                <parameter name="aTopic"/>
> >+                <parameter name="aPreference"/>
> >+                <body><![CDATA[
> >+                    aSubject.QueryInterface(Components.interfaces.nsIPrefBranch2);
> >+
> >+                    switch (aPreference) {
> Everything inside this switch statement should be indented 4 more spaces.

done

> >+                    case "calendar.week.d0sundaysoff":
> >+                    case "calendar.week.d1mondaysoff":
> >+                    case "calendar.week.d2tuesdaysoff":
> >+                    case "calendar.week.d3wednesdaysoff":
> >+                    case "calendar.week.d4thursdaysoff":
> >+                    case "calendar.week.d5fridaysoff":
> >+                    case "calendar.week.d6saturdaysoff":
> >+                        debugger;
> I'm guessing you didn't mean to leave this line ^^^ in

Uups, removed.

> >+                    case "calendar.timezone.local":
> >+                        var viewElem = document.getAnonymousElementByAttribute(
> >+                                       this, "anonid", "view-element");
> >+                        viewElem.timezone = aSubject.getCharPref(aPreference);
> >+                        if (!this.startDay || !this.endDay) {
> >+                            // Don't refresh if we're not initialized
> >+                            return;
> >+                        }
> >+                        this.goToDay(this.selectedDay);
> >+                        break;
> First off, I think we can either move the "var viewElem = doc..." line above
> the switch statement, since we use it in each case, or we could move it to its
> own method, since we use it in updateDaysOffPrefs below as well.

I added a 'viewElem' property to the calendar-decorated-base-view binding.
In the calendar-decorated-multiday-base-view binding I used this property
instead of calling getAnonymousElementByAttribute().
There are about 30 locations in calendar-decorated-base.xml,
calendar-decorated-day-view.xml, calendar-decorated-week-view.xml,
calendar-decorated-multiweek-view.xml and calendar-decorated-month-view.xml
left, which can all use the viewElem property. If you want, I can adjust all
those locations, but probably that bloats this patch too much.

> Secondly, I'd also like to move the "if (!this.startDay..." bit into a helper
> function, so as to reduce code duplication.  Maybe it can simply go _after_ the
> case statement?

This check cannot be moved after the case statement, because it's necessary
that the new pref settings are propagated to the view element. Therefore this
check only prevents a view refresh, which doesn't work, if the view is not
initialized (this.selectedDay gives an error).
While looking at the code I found another error. For the
'calendar.week.d...off' preferences the check is performed after the case
statement and should be moved before the goToDay statement.

Concerning code duplication:
a) Instead of checking this.startDay and this.endDay also the 'initialized'
   property from calendar-decorated-base.xml can be used. This property
   checks in addition this.displayCalendar, but this also works.
b) I personally prefer to have the return statement in the case statement,
   because then the return is more obvious. But if you want I can also move
   the check with return to a helper method in calendar-decorated-base.xml,
   e.g. checkInitialized.

What do you think?

> >+
> >+                    default:
> >+                        break;
> >+                    }
> >+                    return;
> >+                ]]></body>
> >+            </method>
> >+            
> Trailing spaces on this line ^^^

done

> >diff -x CVS -a -U 8 -pN -r mozilla_ref/calendar/base/content/calendar-decorated-week-view.xml mozilla/calendar/base/content/calendar-decorated-week-view.xml
> >--- mozilla_ref/calendar/base/content/calendar-decorated-week-view.xml	2007-03-12 12:33:24.953125000 +0100
> >+++ mozilla/calendar/base/content/calendar-decorated-week-view.xml	2007-03-12 16:54:46.453125000 +0100
> >@@ -41,113 +41,70 @@
> > <!-- Note that we depend on some helper functions in calUtils.js here-->
> > 
> > <bindings id="calendar-specific-view-bindings"
> >     xmlns="http://www.mozilla.org/xbl"
> >     xmlns:html="http://www.w3.org/1999/xhtml"
> >     xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> >     xmlns:xbl="http://www.mozilla.org/xbl">
> > 
> >-    <binding id="calendar-decorated-week-view" extends="chrome://calendar/content/calendar-decorated-base.xml#calendar-decorated-base-view">
> >+    <binding id="calendar-decorated-week-view" extends="chrome://calendar/content/calendar-decorated-multiday-base-view.xml#calendar-decorated-multiday-base-view">
> Since you're touching this line, please put "extends" beneath "id"

done

> >+            <method name="handlePreference">
> >+                <parameter name="aSubject"/>
> >+                <parameter name="aTopic"/>
> >+                <parameter name="aPreference"/>
> >+                <body><![CDATA[
> >+                    aSubject.QueryInterface(Components.interfaces.nsIPrefBranch2);
> >+
> >+                    switch (aPreference) {
> >+
> >+                    case "calendar.week.start":
> >+                        this.mWeekStartOffset = aSubject.getIntPref(aPreference);
> >+                        viewElem = document.getAnonymousElementByAttribute(
> >+                                   this, "anonid", "view-element");
> >+                        viewElem.weekStartOffset = aSubject.getIntPref(aPreference);
> >+                        if (!this.startDay || !this.endDay) {
> >+                            // Don't refresh if we're not initialized
> >+                            return;
> >+                        }
> >+                        // Refresh the view so the settings take effect
> >+                        this.goToDay(this.selectedDay);
> >+                        break;
> >+
> >+                    default:
> >+                        this.handleCommonPreference(aSubject, aTopic, aPreference);
> >+                        break;
> >+                    }
> The contents of this switch statement should be indented 4 more spaces.

done

> >diff -x CVS -a -U 8 -pN -r mozilla_ref/calendar/base/content/calendar-multiday-view.xml mozilla/calendar/base/content/calendar-multiday-view.xml
> >--- mozilla_ref/calendar/base/content/calendar-multiday-view.xml	2007-03-12 12:33:25.031250000 +0100
> >+++ mozilla/calendar/base/content/calendar-multiday-view.xml	2007-03-12 12:55:48.984375000 +0100
> >@@ -1266,16 +1308,31 @@
> >                                             Ci.nsIScriptableDateFormat.timeFormatNoSeconds,
> >                                             endhr, endmin, 0);
> > 
> >           this.fgboxes.startlabel.setAttribute("value", startstr);
> >           this.fgboxes.endlabel.setAttribute("value", endstr);
> >    
> >         ]]></body>
> >       </method>
> >+      <method name="setDayStartEndMinutes">
> >+        <parameter name="aDayStartMin"/>
> >+        <parameter name="aDayEndMin"/>
> >+        <body><![CDATA[
> >+          if (aDayStartMin < this.mStartMin || aDayStartMin > aDayEndMin ||
> >+              aDayEndMin > this.mEndMin) {
> >+              throw Components.results.NS_ERROR_INVALID_ARG;
> >+          }
> >+          if (this.mDayStartMin != aDayStartMin ||
> >+              this.mDayEndMin != aDayEndMin) {
> >+              this.mDayStartMin = aDayStartMin;
> >+              this.mDayEndMin = aDayEndMin;
> >+          }
> >+        ]]></body>
> >+      </method>
> These should use 2 space indenting inside the if statments to match the rest of
> the file.

done

> >@@ -2732,34 +2798,70 @@
> >             } else {
> >               minute = Math.round(x.value/this.mPixPerMin);
> >             }
> >           }
> >           return minute;
> >         ]]></body>
> >       </method>
> > 
> >+      <method name="setFirstVisibleMinute">
> >+        <parameter name="aMinute"/>
> >+        <body><![CDATA[
> >+          this.mFirstVisibleMinute = aMinute;
> >+        ]]></body>
> >+      </method>
> also return this.mFirstVisibleMinute for convenience

done

> >+      <method name="setVisibleMinutes">
> >+        <parameter name="aVisibleMinutes"/>
> >+        <body><![CDATA[
> >+          if (aVisibleMinutes <= 0 ||
> >+              aVisibleMinutes > (this.mEndMin - this.mStartMin)) {
> >+              throw Components.results.NS_ERROR_INVALID_ARG;
> >+          }
> >+          if (this.mVisibleMinutes != aVisibleMinutes) {
> >+              this.mVisibleMinutes = aVisibleMinutes;
> >+          }
> >+        ]]></body>
> also return this.mVisibleMinutes for convenience

done

> Please also use 2-space indenting for the inner methods.  I know the file goes
> back and forth, but it looks like 2 is the most common in this file.

done

Comment 58

[Matthew (lilmatt) Willis]

(In reply to comment #57)
> > Do we really need this method since you're defining "handlePreference" in the
> > child views?
> 
> I defined handlePreference only for the week view, for the day view the base
> implementation is used. An alternative would be to define handlePreference also
> for the day view and remove it from the multiday base binding. Nevertheless I
> prefer the current approach, because the code path is more obvious
> (mPrefObserver calls handlePreference, which calls handleCommonPreference).
> But if you want that I change this, please let me know.

No, I think it's fine as is now that you've explained it.  Maybe you want to add a comment in the base version explaining this?


> > First off, I think we can either move the "var viewElem = doc..." line above
> > the switch statement, since we use it in each case, or we could move it to its
> > own method, since we use it in updateDaysOffPrefs below as well.
> 
> I added a 'viewElem' property to the calendar-decorated-base-view binding.
> In the calendar-decorated-multiday-base-view binding I used this property
> instead of calling getAnonymousElementByAttribute().
> There are about 30 locations in calendar-decorated-base.xml,
> calendar-decorated-day-view.xml, calendar-decorated-week-view.xml,
> calendar-decorated-multiweek-view.xml and calendar-decorated-month-view.xml
> left, which can all use the viewElem property. If you want, I can adjust all
> those locations, but probably that bloats this patch too much.
You're right about bloat.  We should fix the places in the files that we're touching, and clearly use the viewElem in the new multiday base, but file a follow up to fix the rest.

> Concerning code duplication:
> a) Instead of checking this.startDay and this.endDay also the 'initialized'
>    property from calendar-decorated-base.xml can be used. This property
>    checks in addition this.displayCalendar, but this also works.
> b) I personally prefer to have the return statement in the case statement,
>    because then the return is more obvious. But if you want I can also move
>    the check with return to a helper method in calendar-decorated-base.xml,
>    e.g. checkInitialized.
I still think I'd prefer the helper method if we can make it work simply to reduce code duplication.  I haven't tested the .initialized bit so I'm not sure if we want to change to that at this point, but using a helper method would make it easy to do so.  I think we should spin off another bug to use initialized after 0.5 so it can be tested thoroughly.

Comment 59

[Thomas Benisch]

(In reply to comment #57)
> (In reply to comment #56)
> > (From update of attachment 258306 [details] [diff] [review] [details])
> While looking at the code I found another error. For the
> 'calendar.week.d...off' preferences the check is performed after the case
> statement and should be moved before the goToDay statement.

I also fixed that.

Comment 60

[Thomas Benisch]

(In reply to comment #58)
> (In reply to comment #57)
> > > Do we really need this method since you're defining "handlePreference" in the
> > > child views?
> > 
> > I defined handlePreference only for the week view, for the day view the base
> > implementation is used. An alternative would be to define handlePreference also
> > for the day view and remove it from the multiday base binding. Nevertheless I
> > prefer the current approach, because the code path is more obvious
> > (mPrefObserver calls handlePreference, which calls handleCommonPreference).
> > But if you want that I change this, please let me know.
> 
> No, I think it's fine as is now that you've explained it.  Maybe you want to
> add a comment in the base version explaining this?

done

> > > First off, I think we can either move the "var viewElem = doc..." line above
> > > the switch statement, since we use it in each case, or we could move it to its
> > > own method, since we use it in updateDaysOffPrefs below as well.
> > 
> > I added a 'viewElem' property to the calendar-decorated-base-view binding.
> > In the calendar-decorated-multiday-base-view binding I used this property
> > instead of calling getAnonymousElementByAttribute().
> > There are about 30 locations in calendar-decorated-base.xml,
> > calendar-decorated-day-view.xml, calendar-decorated-week-view.xml,
> > calendar-decorated-multiweek-view.xml and calendar-decorated-month-view.xml
> > left, which can all use the viewElem property. If you want, I can adjust all
> > those locations, but probably that bloats this patch too much.
> You're right about bloat.  We should fix the places in the files that we're
> touching, and clearly use the viewElem in the new multiday base, but file a
> follow up to fix the rest.

I filed bug 373888 as follow-up issue.

> > Concerning code duplication:
> > a) Instead of checking this.startDay and this.endDay also the 'initialized'
> >    property from calendar-decorated-base.xml can be used. This property
> >    checks in addition this.displayCalendar, but this also works.
> > b) I personally prefer to have the return statement in the case statement,
> >    because then the return is more obvious. But if you want I can also move
> >    the check with return to a helper method in calendar-decorated-base.xml,
> >    e.g. checkInitialized.
> I still think I'd prefer the helper method if we can make it work simply to
> reduce code duplication.  I haven't tested the .initialized bit so I'm not sure
> if we want to change to that at this point, but using a helper method would
> make it easy to do so.  I think we should spin off another bug to use
> initialized after 0.5 so it can be tested thoroughly.

As discussed with lilmatt in IRC I didn't introduce a helper method.

[2007-03-13 21:04:06] <tbe> I found another problem with the helper function for checking if the view is initialized.
[2007-03-13 21:04:15] <lilmatt> ok?
[2007-03-13 21:04:53] <tbe> If I move the whole if statement into a helper function, the return statement is somehow useless, because return means return from that function.
[2007-03-13 21:05:18] <tbe> That means I must have a return in the case statement anyway.
[2007-03-13 21:05:41] <lilmatt> So ignore that and leave that as is
[2007-03-13 21:05:41] <tbe> In addition I want to have the no refresh comment in the case statement.
[2007-03-13 21:06:37] <tbe> O.K., I think in a follow-up one can use this.initialized instead of this.startDay and this.endDay

For using the initialized property I filed Bug 373892 as follow-up issue.

Comment 61

[Thomas Benisch]

Attachment: working hours v8

Just for reference, this is the patch I finally checked in.

Comment 62

[Thomas Benisch]

patch checked in on HEAD and MOZILLA_1_8_BRANCH

Comment 63

[Thomas Benisch]

Here's a list of follow-up issues:

Bug 349518  visual markers for out-of-sight events in calendar-multiday-view
Bug 373888  code cleanup: use viewElem property in decorated views
Bug 373892  use initialized property in decorated views
Bug 373898  rounding issues for grid boxes in calendar multiday view

Comment 64

[Martin Schröder [:mschroeder]]

VERIFIED with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4pre) Gecko/20070522 Calendar/0.5pre