Closed Bug 306079 Opened 15 years ago Closed 14 years ago

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

Categories

(Calendar :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jminta, Assigned: jminta)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
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?
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

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.
Status: NEW → ASSIGNED
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?
(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.
Attached patch fix for Sunbird (obsolete) — Splinter Review
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...
Assignee: mostafah → jminta
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.
Blocks: 329582
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.
Attachment #213863 - Attachment is obsolete: true
Attachment #218626 - Flags: first-review?(jminta)
Blocks: 333923
Oh, and we shouldn't worry about the xpfe xpi at this point.
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 on attachment 218626 [details] [diff] [review]
rev1 - joey's patch with gekacheka's comment about not hardcoding workdays

minusing per previous comment.
Attachment #218626 - Flags: first-review?(jminta) → first-review-
Blocks: 337925
Attached patch remove calendarPreferences() v2 (obsolete) — Splinter Review
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.
Attachment #218626 - Attachment is obsolete: true
Attachment #222189 - Flags: first-review?(mvl)
blocking0.3?

Filter bugspam out using maggieIsMyCat
Flags: blocking0.3?(dmose)
Flags: blocking0.3?(dmose) → blocking0.3+
(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.
Patch now uses sunbird-l10n.js for the localization sensitive prefs.
Attachment #222189 - Attachment is obsolete: true
Attachment #225345 - Flags: first-review?(mvl)
Attachment #222189 - Flags: first-review?(mvl)
Comment on attachment 225345 [details] [diff] [review]
remove calendarpPreferences() v3

r=mvl
Attachment #225345 - Flags: first-review?(mvl) → first-review+
Patch checked in.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.