306079 - Create a proper prefs file (pref/calendar.js)

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Joey Minta]

Sunbird already has the beginnings of a proper prefs file, but there is nothing
similar for the calendar extension.  The best solution would probably be to
place all of the prefs common to the two in one calendar.js file in
resources/content/pref/

This would remove most of the prefs from calendar.properties.  There are a few
there that are locale specific, however, and those should remain.

Currently, pref defaults are set in rootCalendarPrefs.js.  I'm fairly certain
that this entire file can disappear (including
gCalendarWindow.calendarPreferences), along with the get*Pref methods defined in
content/calendar.js once this is done properly.

Comment 1

[Joey Minta]

My current thought on this is to add the following to calendarUtils:
getPref(aPrefName, aType) {}
setPref(aPrefName, aType, aValue) {}

aType can be 'bool', 'int', 'char', or 'complex'.

Unlike the current get*Pref() functions, getPref would not set a default, but
merely read the pref.  In my opinion, this keeps things nice and simple, avoids
continually writing var prefService = Components.classes[..., and would be
easily reusable throughout the code.

I have mixed feelings about whether or not the leading 'calendar.' should be
required in aPrefName.  Currently I'm leaning towards 'no'.

Also, sunbird.js can be added to the profile when built, what sort of magic is
needed to get the calendar extension to read its prefs?

Comment 2

[gekacheka]

The following are the pref defaults that seem likely to be the same for every
locale, so could be removed from calendar.properties.  

defaultWeeksInView=4
defaultPreviousWeeksInView=1
showAlarms=1
showMissed=1
playAlarmSound=0
soundURL=chrome://calendar/content/sound.wav
reloadServersOnLaunch=false
defaultEventLength=60
defaultSnoozeAlarmLength=60
dateFormat=0
storeInGmt=0

defaulteventalarmunit=minutes
defaulttodoalarmunit=minutes

Comment 3

[Joey Minta]

Taking this, just so we don't end up with two people doing double work.  It's
mostly done in my new-views tree.  There's a lot less that needs to be changed
there.  I'll attach a patch as soon as possible after bug 297934 is fixed.

Comment 4

[Joey Minta]

So, I lost this patch with the hard-drive disaster.  It's fairly easy to fix this in Sunbird.  What's significantly harder is keeping the calendar extension alive here.  Am I going to start a riot if I fix Sunbird/break calendar.xpi and put out a call for someone else from the community who feels strongly about the xpi to step up and finish the work?

Comment 5

[Simon Paquet [:sipaq]]

(In reply to comment #4)
> Am I going to start a riot if I fix Sunbird/break calendar.xpi and put out a 
> call for someone else from the community who feels strongly about the xpi to 
> step up and finish the work?

Of course you will get a riot. The question is, do we care. IMO we shouldn't. But this should go in after the LT 0.1 release, so that there's at least a viable alternative for Thunderbird users.

Comment 6

[Joey Minta]

Attachment: fix for Sunbird

This is the patch for Sunbird.  It will, however, completely break calendar.xpi.  To fix it, something much like what I did in bug 298352 for lightning is necessary.  I don't really have the time/desire to mess around with building calendar.xpi at this stage...

Comment 7

[gekacheka]

The days off are not the same in every locale.
Some countries work 6 days a week.
Some countries take Fridays off.

Therefore days off should still be in locale properties.

Comment 8

[Matthew (lilmatt) Willis]

Attachment: rev1 - joey's patch with gekacheka's comment about not hardcoding workdays

Joey,
There are a number of other prefs that should go into sunbird.js, but that should in another bug. I think _this_ should go in with the pref update to toolkit.

Comment 9

[Matthew (lilmatt) Willis]

Oh, and we shouldn't worry about the xpfe xpi at this point.

Comment 10

[Joey Minta]

Comment on attachment 218626 [details] [diff] [review]
rev1 - joey's patch with gekacheka's comment about not hardcoding workdays

We're now not deleting the prefs from the locale file, but I don't see any changes to initLocalePrefs() to load these prefs that we've kept.  Anything that remains in the locale file needs to be set in that function, otherwise callers of that pref will be errors thrown at them.  I also don't like reviewing my own code. :-/

Comment 11

[Joey Minta]

Comment on attachment 218626 [details] [diff] [review]
rev1 - joey's patch with gekacheka's comment about not hardcoding workdays

minusing per previous comment.

Comment 12

[Joey Minta]

Attachment: remove calendarPreferences() v2

This version of the patch still allows for locale-specific days off.  We load all the locale-specific prefs in the initLocalePrefs() function, and all of the general prefs have been moved to sunbird.js.

Comment 13

[Matthew (lilmatt) Willis]

blocking0.3?

Filter bugspam out using maggieIsMyCat

Comment 14

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

(In reply to comment #12)
> This version of the patch still allows for locale-specific days off.  We load
> all the locale-specific prefs in the initLocalePrefs() function, and all of the
> general prefs have been moved to sunbird.js.

I don't like that workaround. http://www.mozilla.org/projects/l10n/mlp_what2.html#defaults seems to imply that you can have default file to be put in the profile in a localization pack. I think that you could add a prefs file that way. That would be a much cleaner solution, and would work for all prefs, not just the one where you wrote this code for.

Comment 15

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

Or even better, add a sunbird-l10n.js, just like firefox has:
http://lxr.mozilla.org/l10n/source/cs/browser/firefox-l10n.js
http://lxr.mozilla.org/seamonkey/source/browser/locales/Makefile.in#81

Comment 16

[Matthew (lilmatt) Willis]

(In reply to comment #15)
> Or even better, add a sunbird-l10n.js, just like firefox has:
> http://lxr.mozilla.org/l10n/source/cs/browser/firefox-l10n.js
> http://lxr.mozilla.org/seamonkey/source/browser/locales/Makefile.in#81
> 

We do now:
http://lxr.mozilla.org/mozilla/source/calendar/locales/en-US/sunbird-l10n.js

Comment 17

[Joey Minta]

Attachment: remove calendarpPreferences() v3

Patch now uses sunbird-l10n.js for the localization sensitive prefs.

Comment 18

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

Comment on attachment 225345 [details] [diff] [review]
remove calendarpPreferences() v3

r=mvl

Comment 19

[Joey Minta]

Patch checked in.