Closed Bug 372014 Opened 17 years ago Closed 17 years ago

Unify Sunbird/Lightning preferences code

Categories

(Calendar :: Preferences, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Lightning 0.5

People

(Reporter: ssitter, Assigned: ssitter)

References

Details

Attachments

(2 files, 1 obsolete file)

Instead of adding new preferences UI tin Lightning we should unify Sunbird/Lightning preferences code. Goal of this bug is to reuse the existing preference UI from mozilla/calendar/base/content/preferences in Lightning.
Attached patch rev0 - unify preferences (obsolete) — — Splinter Review
Changes contained in patch:

- Remove the existing Lightning preference UI
- Add General/Alarms/Categories/Views/Timezone tab to Lightning pref. pane
- Add empty box object to Sunbirds pref. panes / Lightnings pref. tabs
- Overlay the box object with the existing preference overlays from Sunbird
- Add some missing default prefs to Lightning and do some code adaption
  to make the preferences work in Lightning

Patch could only been tested with Sunbird/0.5pre and Thunderbird/2.0pre + Lightning/0.5pre builds on Windows 2000.
Attachment #256696 - Flags: first-review?(lilmatt)
(Don't blame me for Windows 2000 being so ugly. Beautification can be done later)
Attachment #256697 - Flags: ui-review?(mvl)
This patch means that we can not use tabs in sunbird preference pages. I'm not sure if that's a bad thing, but we must be aware if it.
(there is no technical reason for this, but tabs in tabs are just plain ugly and very confusing)
True.  We currently only use tabs on the "Advanced" pane, which I'm assuming we don't need to port to Ln.
(In reply to comment #3)
> This patch means that we can not use tabs in sunbird preference pages.

Sure you can. If we want to use tabs in Sunbird in future:
Do not overlay the preference pane anymore. Instead add tabs to the preference pane. Overlay the tabs similar to how it's done in Lightning. In Lightning just add new tabs and overlay it too.

(In reply to comment #4)
> True. We currently only use tabs on the "Advanced" pane, which I'm
> assuming we don't need to port to Ln.

This is right, Advanced pane is not ported to Lightning. But it is included in this patch to make the handling for all panes in Sunbird equal.
Comment on attachment 256696 [details] [diff] [review]
rev0 - unify preferences

>Index: mozilla/calendar/base/content/preferences/preferences.xul
>===================================================================
>     <prefpane id="paneGeneral"
>               label="&paneGeneral.title;"
>-              src="chrome://calendar/content/preferences/general.xul"/>
>+              onpaneload="gGeneralPane.init();"
>+              src="chrome://calendar/content/preferences/general.xul">
>+      <vbox id="calPreferencesBoxGeneral"/>
>+    </prefpane>
This vbox tag and all the others should be indented 2 more spaces. The file is uses 4 space indenting.


>Index: mozilla/calendar/base/content/preferences/categories.js
>===================================================================
>     init: function () {
>         // On non-instant-apply platforms, once this pane has been loaded,
>         // attach our "revert all changes" function to the parent prefwindow's
>         // "ondialogcancel" event.
>         var parentPrefWindow = document.getElementById("CalendarPreferences");
>+        if (!parentPrefWindow) {
>+            parentPrefWindow = document.getElementById("MailPreferences");
>+        }
I'd prefer:
var parentPrefWindow = document.getElementById("CalendarPreferences") ||
                       document.getElementById("MailPreferences");


>Index: mozilla/calendar/lightning/jar.mn
>===================================================================
>-  content/lightning/lightning-preferences.xul		(content/lightning-preferences.xul)
>-  content/lightning/lightning-preferences.js		(content/lightning-preferences.js)
Note that these files should also be cvs rm'd when this patch lands.


>Index: mozilla/calendar/lightning/content/messenger-overlay-preferences.xul
>===================================================================
>-<!DOCTYPE overlay SYSTEM "chrome://lightning/locale/lightning.dtd" >
>+<!DOCTYPE overlay [
>+    <!ENTITY % lightningDTD   SYSTEM "chrome://lightning/locale/lightning.dtd">
>+    <!ENTITY % preferencesDTD SYSTEM "chrome://calendar/locale/preferences/preferences.dtd">
>+    %lightningDTD;
>+    %preferencesDTD;
>+]>
Don't line up the SYSTEMs. If we have a reallyLongThingDTD in the future it will encourage editing all three lines simply for alignment and unnecessarily pollute cvs blame.

> <prefwindow id="MailPreferences">
>-  <prefpane id="paneLightning" label="&lightning.taskLabel;"
>-            src="chrome://lightning/content/lightning-preferences.xul"/>
>+    <prefpane id="paneLightning" label="&lightning.taskLabel;"
>+              onpaneload="gGeneralPane.init(); gAlarmsPane.init(); gCategoriesPane.init(); gTimezonesPane.init();">
Please move label= to its own line

>+        <tabbox id="calPreferencesTabbox" flex="1" onselect="">
>+            <tabs>
>+                <tab id="calPreferencesTabGeneral"
>+                     label="&paneGeneral.title;"/>
>+                <tab id="calPreferencesTabAlarms"
>+                     label="&paneAlarms.title;"/>
>+                <tab id="calPreferencesTabCategories"
>+                     label="&paneCategories.title;"/>
>+                <tab id="calPreferencesTabViews"
>+                     label="&paneViews.title;"/>
>+                <tab id="calPreferencesTabTimezones"
>+                     label="&paneTimezones.title;"/>
>+            </tabs>
>+            <tabpanels flex="1">
>+                <tabpanel orient="vertical">
>+                    <vbox id="calPreferencesBoxGeneral"/>
>+                </tabpanel>
>+                <tabpanel orient="vertical">
>+                    <vbox id="calPreferencesBoxAlarms"/>
>+                </tabpanel>
>+                <tabpanel orient="vertical">
>+                    <vbox id="calPreferencesBoxCategories"/>
>+                </tabpanel>
>+                <tabpanel orient="vertical">
>+                    <vbox id="calPreferencesBoxViews"/>
>+                </tabpanel>
>+                <tabpanel orient="vertical">
>+                    <vbox id="calPreferencesBoxTimezones"/>
>+                </tabpanel>
>+            </tabpanels>
>+        </tabbox>
Should we be persisting which tab was selected so we show it again next time the user opens the prefs?

>+    </prefpane>
> </prefwindow>
Since you're touching everything here, fix the indenting of the <prefwindow> tag as well.

> </overlay>
>\ No newline at end of file
Fix that.


>Index: mozilla/calendar/lightning/content/lightning.js
>===================================================================
>+// categories settings
>+// XXX one day this should contain a list of locale specific default categories
>+pref("calendar.categories.names", "");
One day?  Why are we not using the ones from sunbird.js?


I'd like to see this one more time before checking it in.
Attachment #256696 - Flags: first-review?(lilmatt) → first-review-
Comment on attachment 256697 [details]
rev0 - screenshot of Lightning preferences

ui-review=mvl
Attachment #256697 - Flags: ui-review?(mvl) → ui-review+
Attached patch rev1 - unify preferences — — Splinter Review
Same as rev0 but with the issues from Comment #6 addressed except:

>>Index: mozilla/calendar/lightning/content/messenger-overlay-preferences.xul

>Should we be persisting which tab was selected so 
>we show it again next time the user opens the prefs?

Thunderbird/Sunbird use javascript foo to save/restore the tab order from/to a preference. I don't want to enlarge the patch unnecessary therefore this will be handled in a followup bug as discussed on IRC.

>>Index: mozilla/calendar/lightning/content/lightning.js
>>+pref("calendar.categories.names", "");

>One day? Why are we not using the ones from sunbird.js?

Currently the Lightning default preferences are not locale specific. Therefore I thought it would be better to ship no default categories and let the user create them as needed instead of shipping English categories for all locales.
Attachment #256696 - Attachment is obsolete: true
Attachment #257759 - Flags: first-review?(lilmatt)
Comment on attachment 257759 [details] [diff] [review]
rev1 - unify preferences

r=lilmatt
Attachment #257759 - Flags: first-review?(lilmatt) → first-review+
Blocks: 340601
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Nice work!
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
OS: Windows 2000 → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → Lightning 0.5
Depends on: 376937
Status: RESOLVED → VERIFIED
(In reply to comment #8)
> >Should we be persisting which tab was selected so 
> >we show it again next time the user opens the prefs?
> 
> Thunderbird/Sunbird use javascript foo to save/restore the tab order from/to a
> preference. I don't want to enlarge the patch unnecessary therefore this will
> be handled in a followup bug as discussed on IRC.

Stefan, I can't find the follow-up bug you spoke of here. Any hints?
Component: General → Preferences
QA Contact: general → preferences
(In reply to comment #12)
Bug 373209
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: