321769 - Day/Week views do not respect preferred view time restrictions

Status: RESOLVED FIXED

(Calendar :: Sunbird Only, defect)

Description

[gekacheka]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051225 Mozilla Sunbird/0.3a1+

calendar-multiday-view always shows views from 8:00-20:00, does not respect sunbird preference.

Reproducible: Always

Steps to Reproduce:
1. Tools | options
2. Views
3. Week and Day Views: restrict views to, say, 9:00--17:00
4. View day view and week view
Actual Results:  
Time bounds of left column remain at 8:00--20:00

Expected Results:  
Times bounds of left column changed to 9:00--17:00

Comment 1

[gekacheka]

Attachment: v1. call setStartEndMinutes from calendarInit, calendarPrefObserver via calendarWindow

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

calendar-multiday-view has method setStartEndMinutes for this purpose, so just need to call it.  But it is encapsulated inside calendar-decorated-day-view and calendar-decorated-week-view.

Adds setStartEndMinutes to calendar-decorated-base.  It calls the view element's (calendar-multiday-view) setStartEndMinutes if it exists (else throws error).

Adds calendarWindow.refreshTimeBoundsFromPrefs which reads current preferences and calls setStartEndMinutes on both day-view and week-view.

Adds calls to calendarWindow.refreshTimeBoundsFromPrefs from both calendarInit and calendarPrefObserver.

Comment 2

[Joey Minta]

Comment on attachment 207044 [details] [diff] [review]
v1. call setStartEndMinutes from calendarInit, calendarPrefObserver via calendarWindow

+                    } else throw new Error("NOT IMPLEMENTED");

This probably should be NS_ERROR_NOT_IMPLEMENTED.

diff -rutp -x '*[~#]' mozilla/calendar/base/content/calendar-decorated-base.xml
I tend to think that this should be directly implemented (or not) in the separate decorated views.  That way we reduce the burden on those who want to implement other views.  They shouldn't need to follow the .setStartEndMinutes formula.

+   // initialize start/end times in day and week view
+   gCalendarWindow.refreshTimeBoundsFromPrefs();
+
Why is this outside CalendarWindow?  It seems like it should be in the constructor.

+  //(misnamed pref: not default EVENT start hour, but default VIEW start hour)
Include an XXX for easy searching, please.

+  var startHour = rootPrefNode.getIntPref("calendar.event.defaultstarthour") ||  8;
Why || 8?  We set defaults for these, and if the pref isn't defined, won't .getIntPref throw?  Also what happens if the default start hour is 0?

In bug 321608 I'm introducing pref-observers for the views.  It might make more sense to add one here for the multiday-view as well to watch this pref.  That way they can be more self-contained.

Comment 3

[Simon Paquet [:sipaq]]

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

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #2)
> (From update of attachment 207044 [details] [diff] [review] [edit])
> +                    } else throw new Error("NOT IMPLEMENTED");
> 
> This probably should be NS_ERROR_NOT_IMPLEMENTED.
> 
> diff -rutp -x '*[~#]' mozilla/calendar/base/content/calendar-decorated-base.xml
> I tend to think that this should be directly implemented (or not) in the
> separate decorated views.  That way we reduce the burden on those who want to
> implement other views.  They shouldn't need to follow the .setStartEndMinutes
> formula.

I agree since even the multiweek and month views don't have that setting.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: lightning & view fixes, v1

This patch fixes the decorated day and week views as well as the Lightning observers.

Comment 6

[Joey Minta]

In bug 321608, mvl pointed out that it would be nice to keep the views as self-contained as possible.  That's why I added the internal pref observers to those views.  I'd be inclined to suggest the same approach here (and removing the old pref observers from sb/ltn).  I'm open to arguments why this isn't the right approach though.

Comment 7

[Calum Mackay]

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

Comment 8

[Joey Minta]

Comment on attachment 207958 [details] [diff] [review]
lightning & view fixes, v1

See the above comment.  I really think we need internal pref observers here.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v2

This moves all pref-management code out of Lightning and into the decorated view.  It didn't look to me as though any Sunbird code needs to be changed for this.  I also added comments to the IDL for both view and decorateview explicitly documenting where pref-dependencies are intended to go.  Note that the pref observers correctly call setStartEndMinutes on the embedded view, but the embedded view refreshes itself to be blank (but with the correct start/end hours).  This is almost certainly a separate bug in the multiday view code.

Comment 10

[Joey Minta]

Comment on attachment 209520 [details] [diff] [review]
patch, v2

So... Sunbird preferences (the abridged version).  Sunbird prefs live in calendar/resources/content/pref/ and the key file here is rootCalendarPref.js.  The basic idea here: Sunbird prefs suck.  They create several problems that you need to work around.  The most severe is that they don't set default preferences until after calendar.xul's onload is called.
+                // get default start/end times from prefs and set on the view
+                this.mStartMin = pb2.getIntPref(
+                    "calendar.view.defaultstarthour") * 60;
+                this.mEndMin = pb2.getIntPref(
+                    "calendar.view.defaultendhour") * 60;
This means that on initial start-up, you'll create a race here between the CalendarPreferences constructor setting these default preferences, and you asking for them.  I'm pretty sure that the view's constructor will win the race 99% of the time, making these preferences throw.  So, please wrap them in a try.  Sunbird will draw the views twice in this case, but that's just something we'll have to live with until I find time to fix bug 306079.

Looking further at rootCalendarPref.js, you'll find the the code you need to remove from the Sunbird/Calendar pref observer at http://lxr.mozilla.org/mozilla/source/calendar/resources/content/pref/rootCalendarPref.js#62

+                       viewElem.setStartEndMinutes(this.calView.mStartMin,
+                           this.calView.mEndMin);
setStartEndMinutes will throw if mEndMin < mStartMin.  I'd like to do something better than that (The best solution would probably be to do valid-value-checking in the preferences windows) so that users don't hang themselves.  That can either be done here, or in a separate bug.  Please file that bug if you don't do it here.

+ * @note Though not all implementations currently do this, the intent
+ * is code that implements this interface to be pure widgetry and thus
+ * not have any preference dependencies.

I can't find any preferences in the current inner views.  If there are any, please file a bug to get them moved.  If there isn't, then this comment needs to be amended.

Just a nit on indenting, 4 space indenting of long lines looks like you're starting a new block.  If possible, I'd like to see it indented from (or lined up with) the start of the clause, something like:
  var viewElem = document.getAnonymousElementByAttribute(
                   this.calView, "anonid", "view-element");
I'm not sure we have good rules governing this style-situation, if we do and my suggestion runs against it, just ignore it.

Looks pretty good overall, mostly just the annoying Sunbird pref stuff to fight with.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

> So, please wrap them in a try.  Sunbird will draw the views twice in this 
> case, but that's just something we'll have to live with until I find time to
> fix bug 306079.

OK, done.

> Looking further at rootCalendarPref.js, you'll find the the code you need to
> remove from the Sunbird/Calendar pref observer at
> http://lxr.mozilla.org/mozilla/source/calendar/resources/content/pref/rootCalendarPref.js#62

Ah, didn't realize that needed to be removed.  Done.

> (The best solution would probably be to do
> valid-value-checking in the preferences windows) so that users don't hang
> themselves.  That can either be done here, or in a separate bug.  Please file
> that bug if you don't do it here.

Bug 324674 filed.  I also just filed bug 324676 about the refresh failure that I mentioned earlier.

> + * @note Though not all implementations currently do this, the intent
> + * is code that implements this interface to be pure widgetry and thus
> + * not have any preference dependencies.
> 
> I can't find any preferences in the current inner views.  If there are any,
> please file a bug to get them moved.  If there isn't, then this comment needs
> to be amended.

Right you are; fixed.

> I'm not sure we have good rules governing this style-situation, if we do and my
> suggestion runs against it, just ignore it.

In general, the main style guidelines is "match the style of the surrounding code so that you don't end up with a big mix of styles."  I've re-indented in a way that I think is more along the lines of what you were looking for.

New patch forthcoming...

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v3

Comment 13

[Joey Minta]

Comment on attachment 209598 [details] [diff] [review]
patch, v3

+                // view.  if we hit an error (eg because sunbird's pref
+                // pref infrastructure hasn't created the pref yet), the

Don't say 'pref' twice in a row. :-)

r=jminta with that fixed in both copies of the comments.  Nice job, this will be very good to have functioning again.

Comment 14

[Joey Minta]

Comment on attachment 209598 [details] [diff] [review]
patch, v3

+                // view.  if we hit an error (eg because sunbird's pref
+                // pref infrastructure hasn't created the pref yet), the

Don't say 'pref' twice in a row. :-)

r=jminta with that fixed in both copies of the comments.  Nice job, this will be very good to have functioning again.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Fixed landed on trunk and 1.8 branch using cross-commit.