Can't add exceptions to recurring events.

RESOLVED FIXED

Status

Calendar
Sunbird Only
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: Robin Edrenius, Assigned: Mostafa Hosseini)

Tracking

Trunk
x86
Windows XP

Details

Attachments

(1 attachment, 2 obsolete attachments)

3.57 KB, patch
Michiel van Leeuwen (email: mvl+moz@)
: first-review+
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
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

13 years ago
Created attachment 193849 [details] [diff] [review]
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.
Attachment #193849 - Flags: first-review?(mvl)
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)
Attachment #193849 - Flags: first-review?(mvl) → first-review-

Comment 3

13 years ago
Created attachment 193854 [details] [diff] [review]
patch v2

With try/catch since our prefs file needs a lot of work.  I think this is
simpler.
Attachment #193849 - Attachment is obsolete: true
Attachment #193854 - Flags: first-review?(mvl)

Comment 4

13 years ago
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

13 years ago
(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)

Status: UNCONFIRMED → NEW
Ever confirmed: true
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

13 years ago
(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?
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

13 years ago
Created attachment 193948 [details] [diff] [review]
patch v3

Version 3 includes the streamlined try/catch for the second pref.  Bug 306079
has been filed about a proper prefs file.
Attachment #193854 - Attachment is obsolete: true
Attachment #193948 - Flags: first-review?(mvl)

Updated

13 years ago
Attachment #193854 - Flags: first-review?(mvl)
Comment on attachment 193948 [details] [diff] [review]
patch v3

r=mvl (hoping that bug 306079 will indeed be fixed one day)
Attachment #193948 - Flags: first-review?(mvl) → first-review+

Comment 11

13 years ago
Patch checked in.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.