333923 - Convert Sunbird preferences from xpfe to toolkit

Status: VERIFIED FIXED

(Calendar :: Preferences, defect)

Description

[Matthew (lilmatt) Willis]

Convert Sunbird preferences from xpfe to toolkit.

Use the timezone picker from Lightning.

Comment 1

[Matthew (lilmatt) Willis]

Attachment: rev0 - first cut at migrating from xpfe to toolkit prefs

Comment 2

[Matthew (lilmatt) Willis]

Attachment: rev1 - cleaned out some goofy stuff

Comment 3

[Matthew (lilmatt) Willis]

Attachment: rev2 - fixed some sound file picker stuff

Comment 4

[Joey Minta]

Comment on attachment 218378 [details] [diff] [review]
rev2 - fixed some sound file picker stuff

Some initial thoughts just from scanning through the patch:

-We need to stop packaging the old pref code by ripping out their jar entries.
-There's a lot of strange stuff going on with gFooPanel.init();  The _pane variable doesn't seem to be used anywhere.  In cases like gViewsPanel, the whole js file seems superfluous.  Is there some magic going on there that I can't see?
-I have a personal hatred of global vars and there are quite a few of them here, but that's more stylistic
-A lot of the license headers need tweaking.  I don't think Ben wrote a lot of this code.
-I noticed at least one file in this patch that already exists, and I don't think it was touched (editCategory.xul).  We should just cvs-move that, rather than recreate it.
-Need license headers on the proxy dtds
-I filed a bug awhile back about making more of our files have stripped out licenses, and it was decided to hold off on this until closer to 1.0, since it mess with line numbers for debugging.
-I like the level of commenting, but the //foo// is kind of an odd style.  Not sure that there's anything wrong with it...

Nothing too severe, except for the strange uses of _pane, and the jar stuff.  I'll look more tomorrow.

Comment 5

[Matthew (lilmatt) Willis]

(In reply to comment #4)
> -We need to stop packaging the old pref code by ripping out their jar entries.
Yes. I just didn't want to put that in this patch yet, in case we had to back something out.

> -There's a lot of strange stuff going on with gFooPanel.init();
> The _pane variable doesn't seem to be used anywhere.
I had put it in so I could do _pane.userChangedValue(widget); but apparently didn't end up using it. It can go away.

> In cases like gViewsPanel, the whole js file seems superfluous.
> Is there some magic going on there that I can't see?
No. views.js can go. I had created a .js file expecting to need it for each, but views only uses widgets who track to their preferences automagically.

> -I have a personal hatred of global vars and there are quite a few of them
> here, but that's more stylistic
Other than the gFooPane, do you see some that we should convert over?

> -A lot of the license headers need tweaking.  I don't think Ben wrote a lot of
> this code.
Sure. The original pieces I used to start from were Ff bits, and had those headers. We can adjust as necessary.

> -I noticed at least one file in this patch that already exists, and I don't
> think it was touched (editCategory.xul).  We should just cvs-move that, rather
> than recreate it.
If only cvs had a move command ;)  I did this more for speed.

> -Need license headers on the proxy dtds
Those were copies from Ff. I'll add. 

> -I filed a bug awhile back about making more of our files have stripped out
> licenses, and it was decided to hold off on this until closer to 1.0, since it
> mess with line numbers for debugging.
I just had a wacky thought. Could we put the headers _at the bottom_ of the file, so line numbers aren't hosed? Perhaps with a one-liner at the top to placate legal? You're the soon-to-be lawyer ;)

> -I like the level of commenting, but the //foo// is kind of an odd style.  Not
> sure that there's anything wrong with it...
Sorry, habit from work. I can remove it if necessary.


I'll wait on making another patch until we connect about some of the open items above.

Comment 6

[Matthew (lilmatt) Willis]

Attachment: rev3 - now with jminta's suggestions, and with appropriate dtds in most places

Comment 7

[Matthew (lilmatt) Willis]

Attachment: winstripe preferences css

Comment 8

[Matthew (lilmatt) Willis]

Attachment: rev4 - added missing winstripe css and missing patch to calendar.js

Here's a better version of it all.

Comment 9

[Matthew (lilmatt) Willis]

Attachment: rev4a - add this to rev4

Comment 10

[Matthew (lilmatt) Willis]

Attachment: rev5 - added some missing stuff

Comment 11

[Stefan Sitter [:ssitter]]

Is moving to toolkit the same as moving to the new prefwindow system (as requested in bug 330716)?

Comment 12

[Joey Minta]

Comment on attachment 218475 [details] [diff] [review]
rev5 - added some missing stuff

-function openPreferences()
-{
-  openDialog("chrome://calendar/content/pref/pref.xul","PrefWindow",
-             "chrome,titlebar,resizable,modal");
+function openPreferences() {
+  var features = "chrome,titlebar,toolbar,centerscreen,dialog=no";
+  var url = "chrome://calendar/content/preferences/preferences.xul";
+
+//  var win = getMostRecentWindow("Calendar:Preferences");
+//  if (win) {
+//    win.focus();
+//  } else {
+//    openWindow(null, url, "_blank", features);
+//  }
+
+    openDialog(url, "Preferences", features);
 }
See resouces/content/calAlarmMonitor.js for an example of getMostRecentWindow, if that's what you want to use.  Otherwise, we should at least try to make this modal.

+*  content/calendar/preferences/connection.xul   (preferences/connection.xul)
+*  content/calendar/preferences/connection.js    (preferences/connection.js)
+*  content/calendar/preferences/editCategory.xul (preferences/editCategory.xul)
+*  content/calendar/preferences/general.js       (preferences/general.js)
+*  content/calendar/preferences/general.xul      (preferences/general.xul)
+*  content/calendar/preferences/preferences.xul  (preferences/preferences.xul)
+*  content/calendar/preferences/timezones.js     (preferences/timezones.js)
+*  content/calendar/preferences/timezones.xul    (preferences/timezones.xul)
+*  content/calendar/preferences/views.xul        (preferences/views.xul)
Why do they all get *?

+<?xml version="1.0" encoding="UTF-8"?>
I don't know that I've ever seen an encoding specified in a xul file.  Is this necessary?

+    readSoundLocation: function () {
+        var soundUrl = document.getElementById("alarmSoundFileField");
+        soundUrl.value = document.getElementById("calendar.alarms.soundURL").value;
+        soundUrl.label = this.convertURLToLocalFile(soundUrl.value).leafName;
+        return undefined;
+    },
Per IRC, return undefined is a bit odd.

+            soundIfc.play(url);
+        } catch (ex) {
+            dump("Exception caught! " + ex + "\n");
+        }
+    },
Can you be a bit more descriptive about where this exception was caught, so that the message makes sense in the console.

+        for (var i=0; i < items.length; i++) {
+            items[i].disabled = !alarmsPlaySoundPref.value;
+        }
You could use the new array methods here and elsewhere if you wanted, but that's just bonus.

+        <!-- XXX This sucks and should be moved to a stringbundle -->
+        <script type="application/x-javascript">
+            <![CDATA[
+                var labelLong  = "&pref.dateformat.long;";
+                var labelShort = "&pref.dateformat.short;";
+            ]]>
+        </script>
Firefox's find says there are no uses of these variables.  Are they needed?

+                        <menuitem id="dateformat-short-menuitem"
+                                  label="&pref.dateformat.short;"
+                                  value="1"
+                                  selected="true"/>
What did we decide about the good/bad of selected="true"?

+        //if (itemToSelect) {
+        //    tzListBox.scrollToIndex(tzListBox.getIndexOfItem(itemToSelect));
+        //    tzListBox.selectItem(itemToSelect);
+        //}
Why is this commented out?  Looks pretty useful.

+        <listbox id="timezone-listbox"
+                 preference="calendar.timezone.local"
+                 onselect="document.getElementById('paneTimezones').userChangedValue(event.target);"
+                 onsynctopreference="return gTimezonesPane.getTimezoneResult();"
+                 onsyncfrompreference="gTimezonesPane.setTimezone();"/>
Can we perhaps flex this so that it doesn't look so weird?

Index: calendar/sunbird/themes/pinstripe/sunbird/pref/pref.css
===================================================================
RCS file: calendar/sunbird/themes/pinstripe/sunbird/pref/pref.css
diff -N calendar/sunbird/themes/pinstripe/sunbird/pref/pref.css
--- calendar/sunbird/themes/pinstripe/sunbird/pref/pref.css	19 Jan 2006 00:31:34 -0000	1.3
+++ /dev/null	1 Jan 1970 00:00:00 -0000
@@ -1,196 +0,0 @@
-/*
-# -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
-# ***** BEGIN LICENSE BLOCK *****
-# Version: MPL 1.1/GPL 2.0/LGPL 2.1
-#
I don't think we want to get rid of all this just yet.  Just don't package it.

Still need to get the timezone bit from rootCalendarPref.js (wrong pref observed)

As dmose said, anonymous functions should be avoided if possible.

Comment 13

[Joey Minta]

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

Comment 14

[Gary van der Merwe]

Attachment: Screen shot of View Tab in windows

On the view tab on Win, the contents is to big for the window, and you can't see some of the group box borders are choped off.

In calendar/sunbird/base/preferences/categories.js on line 93 and 108:
+        window.openDialog("chrome://calendar/content/pref/editCategory.xul",
+                          "addCategory", "modal,centerscreen,chrome,resizable=no",
+                          "", null, addTitle);

and

+            window.openDialog("chrome://calendar/content/pref/editCategory.xul",
+                              "editCategory", "modal,centerscreen,chrome,resizable=no",
+                              gCategoryList[list.selectedIndex], currentColor, editTitle);

The url should be chrome://calendar/content/preferences/editCategory.xul

Comment 15

[Robin Edrenius]

(In reply to comment #14)
> Created an attachment (id=218522) [edit]
> Screen shot of View Tab in windows

imho we should have the same "advanced"-icon as Firefox and Thunderbird, the gearwheel kinda thing.

Comment 16

[Gary van der Merwe]

Attachment: Options.png

New Options.png according to Robin's sugestion.

Comment 17

[Gary van der Merwe]

Attachment: Options.png

+ Category Hover

Comment 18

[Matthew (lilmatt) Willis]

Attachment: rev6 - lotsa lotsa fixin' love

Comment 19

[Matthew (lilmatt) Willis]

Attachment: rev7 - fixed busted path (thanks garyvdm)

Comment 20

[Matthew (lilmatt) Willis]

Attachment: rev8 - added missing changes to global.dtd

Comment 21

[Joey Minta]

Attachment: alignment tweaks

Apply to rev8 to improve appearance in a few places.

Comment 22

[Joey Minta]

Comment on attachment 218540 [details] [diff] [review]
rev8 - added missing changes to global.dtd

RCS file: calendar/sunbird/base/preferences/alarms.js
+            // force the menu selection to redraw
+            var saveSelectedIndex = menu.selectedIndex;
+            menu.selectedIndex = -1;
+            menu.selectedIndex = saveSelectedIndex;
This looks really odd.  I realize it's old code, but i'd like to either get rid of this hack or at least have a xul bug # cited with a nice testcase.

+        else
+            soundUrl.label = soundUrl.value;
+        return undefined;
This is still weird too.  Just because it's in mail-code doesn't make it good. :-/  I vote for dropping it.

RCS file: calendar/sunbird/base/preferences/general.js
+        // deselect and reselect to update visible item title
+        var dateFormatMenuList = document.getElementById("dateformat");
+        var selectedIndex = dateFormatMenuList.selectedIndex;
+        dateFormatMenuList.selectedIndex = -1;
+        dateFormatMenuList.selectedIndex = selectedIndex;
More weird menulist stuff.  I really don't think this -1 index stuff needs to be here.

RCS file: calendar/sunbird/base/preferences/preferences.xul
+<!DOCTYPE prefwindow [
+<!ENTITY % brandDTD SYSTEM "chrome://calendar/locale/branding/brand.dtd">
+<!ENTITY % preferencesDTD SYSTEM "chrome://calendar/locale/preferences/preferences.dtd">
+%brandDTD;
+%preferencesDTD;
+]>
Indenting.

RCS file: calendar/sunbird/base/preferences/timezones.js
+        if (itemToSelect) {
+            tzListBox.scrollToIndex(tzListBox.getIndexOfItem(itemToSelect));
+            tzListBox.selectItem(itemToSelect);
+        }
I think we might want to focus() the listbox too for accessibility, but that's just a nit.

My preferences window lost its 'Close' label, but I think that may be a local issue.  As long as that's working for you, don't worry about it.

I'll go ahead and give r=jminta for the front-end (xul/js) bits with the above fixed and the patch I just attached too.  I definitely want mvl to look over the build/code-placement bits especially, and also sign off on such a drastic set of changes.

Comment 23

[Matthew (lilmatt) Willis]

(In reply to comment #15)
> imho we should have the same "advanced"-icon as Firefox and Thunderbird, the
> gearwheel kinda thing.

Ya, I hadn't gotten to pretty pictures yet. I'm planning to reuse whatever icons I can from the appropriate themes from Ff/Tb/Sb.

Comment 24

[Matthew (lilmatt) Willis]

Attachment: rev9 - incorporates jminta's comments/nits and alignment tweaks

r= jminta from rev8

The index -1 tricks are needed, so we should make up a simple test case and create a bug if that's how jminta wants to handle it.

Comment 25

[Matthew (lilmatt) Willis]

Attachment: rev10 - with update crap in advanced tab

Comment 26

[Matthew (lilmatt) Willis]

Attachment: rev11 - added button to restore to using default alert sound

Comment 27

[Matthew (lilmatt) Willis]

I don't think this should go in without removing the old pref loading code if at all possible.

Comment 28

[Matthew (lilmatt) Willis]

Attachment: pinstripe Options.png

Comment 29

[Matthew (lilmatt) Willis]

Attachment: rev12 - adjusted chrome branding path per discussion with bsmedberg

Comment 30

[Matthew (lilmatt) Willis]

Attachment: rev13 - Uses old sk00l menulist timezone picker

Comment 31

[Matthew (lilmatt) Willis]

Attachment: rev14 - removes "advanced" tab altogether (for now)

Comment 32

[Joey Minta]

Comment on attachment 222121 [details] [diff] [review]
rev14 - removes "advanced" tab altogether (for now)

The following are comments from looking through the code.  I may have more comments after building this myself and playing around with it.

Index: calendar/resources/jar.mn
===================================================================
RCS file: /cvsroot/mozilla/calendar/resources/jar.mn,v
You still need to rip out the old preferences code so we stop shipping it.  Removing those files is also more clearly going to show why we need the default prefs file patch first.  This is in addition to a patch to get the pref observer out of rootCalendarPref.js

+  var features = "chrome,titlebar,toolbar,centerscreen,dialog=no";
+  var url = "chrome://calendar/content/preferences/preferences.xul";
+
+  // Check to see if the prefwindow is already open
+  var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
+                     .getService(Components.interfaces.nsIWindowMediator);
+
+  var win = wm.getMostRecentWindow("Calendar:Preferences");
+
+  if (win) {
+    win.focus();
+  } else {
+    openDialog(url, "Preferences", features);
This seems odd to declare features and url so far from where they are used.  It also seems odd to spend time declaring variables for constants that are only used once.

+<!ENTITY  paneAdvanced.title      "Advanced">
We need a follow-up bug for the Advanced pane to be implemented.  I think it already exists though.

+var gAlarmsPane = {
+
+    init: function () {
We need a follow-up bug to make these function non-anonymous.  Really we should probably do a calendar-wide audit for this.

+        if (Number(length) > 1) 
+            newLabelNumber = "labelplural";
+        else
+            newLabelNumber = "labelsingular";
Style nit, use braces, even for 1 line if-else.

+        var items = new Array(4);
+        items[0] = document.getElementById("alarmSoundFileField");
+        items[1] = document.getElementById("calendar.prefs.alarm.sound.useDefault");
+        items[2] = document.getElementById("calendar.prefs.alarm.sound.browse");
+        items[3] = document.getElementById("calendar.prefs.alarm.sound.preview");
This seems like a bit of a strange way to create a constant array.

+    init: function () {
+        // A list of preferences to be reverted when the dialog is cancelled.
+        // It needs to be a property of the parent to be visible onCancel
+        if (!parent.backupPrefList)
+            parent.backupPrefList = [];
We're still going to need to handle the cancel case for Windows.

Comment 33

[Matthew (lilmatt) Willis]

In rev15

(In reply to comment #32)
> This seems odd to declare features and url so far from where they are used.  
fixed

> It also seems odd to spend time declaring variables for constants that are
> only used once.
That is solely for readability when keeping lines under 80 characters in length.

> We need a follow-up bug for the Advanced pane to be implemented.  I think it
> already exists though.
bug 305645

> Style nit, use braces, even for 1 line if-else.
fixed

> This seems like a bit of a strange way to create a constant array.
fixed

Comment 34

[Matthew (lilmatt) Willis]

Attachment: rev15 - addresses some, but not all of jminta's nits

Comment 35

[Matthew (lilmatt) Willis]

Attachment: rev16 - fixes category cancel

Comment 36

[Joey Minta]

Comment on attachment 222181 [details] [diff] [review]
rev16 - fixes category cancel

#ifdef MOZ_SUNBIRD
-    locale/@AB_CD@/calendar/connectionPrefs.dtd (locale/@AB_CD@/connectionPrefs.dtd)
-    locale/@AB_CD@/calendar/prefutilities.properties (locale/@AB_CD@/prefutilities.properties)
+# The following should be moved to /calendar/locales/jar.mn after locales move to l10n
+   locale/@AB_CD@/calendar/branding/brand.dtd           (/calendar/locales/en-US/chrome/branding/brand.dtd)
+   locale/@AB_CD@/calendar/branding/brand.properties    (/calendar/locales/en-US/chrome/branding/brand.properties)
+   locale/@AB_CD@/calendar/preferences/alarms.dtd       (/calendar/locales/en-US/chrome/calendar/preferences/alarms.dtd)
+   locale/@AB_CD@/calendar/preferences/categories.dtd   (/calendar/locales/en-US/chrome/calendar/preferences/categories.dtd)
+*  locale/@AB_CD@/calendar/preferences/connection.dtd   (/calendar/locales/en-US/chrome/calendar/preferences/connection.dtd)
+*  locale/@AB_CD@/calendar/preferences/general.dtd   
Why are we keeping these in #ifdef MOZ_SUNBIRD?

+
+<!ENTITY pref.weekstarts.label "Start the week on:">
This looks awkward, and I don't like having l10n changes buried in here.  Can we please keep all the strings the same?

+  if (win) {
+    win.focus();
+  } else {
+    openDialog(url, "Preferences", features);
+  }
I *think* prevailing style for calendar.js is 4 spaces, but it's a bit of a mess. :-/

+        // convert the file url into a nsILocalFile
+        if (aFileURL) {
+            var ios = Components.classes["@mozilla.org/network/io-service;1"]
+                            .getService(Components.interfaces.nsIIOService);
+            var fph = ios.getProtocolHandler("file")
+                            .QueryInterface(Components.interfaces.nsIFileProtocolHandler);
+            return fph.getFileFromURLSpec(aFileURL);
Nit: align dots.

+        // It needs to be a property of the parent to be visible onCancel
+        if (!parent.backupPrefList)
+            parent.backupPrefList = [];
Nit: braces please

+            try {
+               categoryPrefBranch.clearUserPref(categoryNameFix);
+            } catch (ex) {
+            }
Nit: Looks like wrong identing on categor...

+        <script type="application/x-javascript"
+                src="chrome://calendar/content/calendarUtils.js"/>
We load this script in both general and timezones, but I *think* scripts are loaded to a window, so I'm concerned about unintended consequences of loading this file twice, especially when it contains global vars.

+            id="CalendarPreferences"
+            windowtype="Calendar:Preferences"
+            xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
+            ondialogcancel="gCategoriesPane.panelOnCancel();"
This is my biggest concern.  I don't like having references like this separate from where they're relevant.  Would it be possible to set this attribute within the init method of gCategoriesPane?  It'll also help save us from uninitialized backup-prefs.  Right now if I close the window without showing the preferences pane, I get Error: 
gCategoriesPane is not defined
Source File: chrome://global/content/bindings/dialog.xml
Line: 353

In fact, categories seem just broken.  They don't seem to be saved between sessions, for me.

Comment 37

[Matthew (lilmatt) Willis]

Attachment: rev17 - addresses all of jminta's nits except isolating out l10n changes

rev16 tried to provide "undoability" for category changes for all platform, regardless of if they were instant-apply or not. This is the wrong behavior, since instant-apply platforms provide only one way to close the prefwindow, and that way doesn't mean "throw away my changes"

Comment 38

[Matthew (lilmatt) Willis]

Attachment: rev18 - addresses remaining l10n nits. any others should go in separate bugs

Comment 39

[Joey Minta]

Comment on attachment 223734 [details] [diff] [review]
rev18 - addresses remaining l10n nits. any others should go in separate bugs

+<!ENTITY pref.calendar.view.monthview.caption "Month View">
This entity, and a couple others here, don't seem to be used at all.  Can you remove them before checkin so we don't make our l10n teams do extra work?

+    enableButtons: function () {
+        document.getElementById("editCButton").disabled=null;
+        document.getElementById("deleteCButton").disabled=null;
+    },
spaces around the = 

Categories work almost perfectly for me with this updated version. :-)  We do need a bug on the fact that removing a color completely from a category doesn't update the rule, but that's independent of this patch.

r=jminta! (with the previous mentioned follow-up bugs)

Don't check in until we get the blocker fixed, and make sure to coordinate with the rest of the team, since there could easily be bustage that need quick-fixing.

Comment 40

[Matthew (lilmatt) Willis]

(In reply to comment #39)
> We do need a bug on the fact that removing a color completely from a
> category doesn't update the rule, but that's independent of this patch.

created that as bug 340229

Given my luck with hard drives and laptops lately, I'll address your nits and post one more patch, carrying over the r+

Comment 41

[Matthew (lilmatt) Willis]

Attachment: rev19 - addresses final nits

This is here more as a backup in case my laptop goes poof!

Comment 42

[Matthew (lilmatt) Willis]

blocking0.3?

Filter bugspam out using maggieIsMyCat

Comment 43

[Matthew (lilmatt) Willis]

Attachment: rev20 - moves nuprefs to /m/cal/base/content/preferences per jminta/mvl on IRC

This moves nuprefs to /mozilla/calendar/base/content/preferences based on conversations with both mvl and jminta on IRC. This will help us share the preferences with lightning, eventually.

I also removed an #ifdef from the general.dtd, so as to not regress bug 340723.

None of the old preferences are removed in this patch. I'll clean them up once we're sure this does the trick.

Comment 44

[Matthew (lilmatt) Willis]

Patch v20 landed on MOZILLA_1_8_BRANCH and trunk

-> FIXED!