305964 - Can't add exceptions to recurring events.

Status: RESOLVED FIXED

(Calendar :: Sunbird Only, defect)

Description

[Robin Edrenius]

Trying to add an exception to a recurring event gives this JS-console error:
Error: dateFormater is not defined
Source File: chrome://calendar/content/eventDialog.js
Line: 1262

Steps to reproduce:
1. Create a recurring event
2. Choose a date to be an exception from this recurrence
3. Press 'Add Exception'

Comment 1

[Joey Minta]

Attachment: patch v1

Several minor problems here.  The most severe was that getIntPref isn't defined
in dateUtils.js because eventDialog.xul doesn't include calendar.js.

Comment 2

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

Comment on attachment 193849 [details] [diff] [review]
patch v1

>+   var prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                            .getService(Components.interfaces.nsIPrefService);
nit: align the dots, please. (same the other spot you do getService)

>-  if ( getBoolPref(gCalendarWindow.calendarPreferences.calendarPref, "date.formatTimeBeforeDate", false )) { 
>+  var prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                            .getService(Components.interfaces.nsIPrefService);
>+  var dateBranch = prefService.getBranch("calendar.date.");
>+  if ( dateBranch.getBoolPref("formatTimeBeforeDate") ) { 
>     return formattedTime+" "+formattedDate;
>   } else { // default
>     return formattedDate+" "+formattedTime; 

the formatTimeBeforeDate pref isn't defined in a default prefs file. So iirc,
getBoolPref will die. I don't think that should happen. You need some try/catch
here, in order to be able to define a default. Or maybe just set it in some
prefs file.

(re-request review if i'm wrong about dying)

Comment 3

[Joey Minta]

Attachment: patch v2

With try/catch since our prefs file needs a lot of work.  I think this is
simpler.

Comment 4

[gekacheka]

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

Suggestion: simplify by making this.datePrefBranch a field of the dateFormater
rather than recomputing it each time.

Suggestion: simplify by making getBoolPref and getIntPref methods on
dateFormater which access the this.datePrefBranch field.  Then it would not be
much trouble to add the exception handlers to set the default value, as they do
in calendar.js.

(An advantage of setting the default value as a pref is it makes it easier to
discover prefs, especially for those that do not appear in the UI.)

Comment 5

[Joey Minta]

(In reply to comment #4)
> (From update of attachment 193854 [details] [diff] [review] [edit])
> Suggestion: simplify by making this.datePrefBranch a field of the dateFormater
> rather than recomputing it each time.
> 
> Suggestion: simplify by making getBoolPref and getIntPref methods on
> dateFormater which access the this.datePrefBranch field.  Then it would not be
> much trouble to add the exception handlers to set the default value, as they do
> in calendar.js.

The two instances of getting prefs that are changed in this patch are the only
ones (I found) that appear in dateUtils.js.  If there were more, I would
certainly consider the datePrefBranch option.  As it is, I feel that for just
these two instances, setting these suggested variables elsewhere would hurt code
readability, moving what is happening away from where the current locations. 
(See also note below)

> (An advantage of setting the default value as a pref is it makes it easier to
> discover prefs, especially for those that do not appear in the UI.)
> 

Agreed.  mvl and I talked about the need for creating a better prefs file with
all the defaults properly declared.  There's no bug for this right now, but I
guess there probably should be.  Once this happens, I believe the get*Pref
methods in calendar.js should disappear completely.

Status: NEW (since nobody did this yet)

Comment 6

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

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

>    try
>    {     
>-      if( getIntPref(gCalendarWindow.calendarPreferences.calendarPref, "date.format", 0 ) == 0 )
>+      if( dateBranch.getIntPref("format") == 0 )

You need another try-block here, to return some default when the pref isn't
set.


>+  try {
>+    if ( dateBranch.getBoolPref("formatTimeBeforeDate") ) { 
>+      return formattedTime+" "+formattedDate;
>+    } else { // default
>+      return formattedDate+" "+formattedTime; 
>+    }
>+  }
>+  catch(ex) { // if the pref is undefined
>+    return formattedDate+" "+formattedTime;

You could make that shorter (with less code duplication), like:

try {
  return formattedTime+" "+formattedDate;
} catch (e) {}
// default:
return formattedDate+" "+formattedTime;

Comment 7

[Joey Minta]

(In reply to comment #6)
> (From update of attachment 193854 [details] [diff] [review] [edit])
> >    try
> >    {     
> >-      if( getIntPref(gCalendarWindow.calendarPreferences.calendarPref,
"date.format", 0 ) == 0 )
> >+      if( dateBranch.getIntPref("format") == 0 )
> 
> You need another try-block here, to return some default when the pref isn't
> set.

The default for this pref seems to be set here
http://lxr.mozilla.org/mozilla/source/calendar/resources/content/pref/rootCalendarPref.js#157
so I don't think this one should ever die.  Do you still want the extra try in
spite of this?

Comment 8

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

That's a pretty weird place way to define defaults, but i guess, for now, we cal
live with it.
Can you file a bug for a proper calendar.js prefs file?

Comment 9

[Joey Minta]

Attachment: patch v3

Version 3 includes the streamlined try/catch for the second pref.  Bug 306079
has been filed about a proper prefs file.

Comment 10

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

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

r=mvl (hoping that bug 306079 will indeed be fixed one day)

Comment 11

[Joey Minta]

Patch checked in.