305182 - Add a "Config Editor..." button to edit the hidden preferences

Status: VERIFIED FIXED

(Calendar :: Preferences, enhancement)

Description

[Riccardo Cretti]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050817 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050817 Firefox/1.6a1

The users that need to change some of the hidden preferences of Sunbird are
forced to create a user.js file and write inside the preferences with the value.
To allow them to see and edit all the hidden preferences easily, it's possible
to "borrow" the "Config Editor..." that is implemented in the preferences of
Thunderbird alpha.

Reproducible: Always

Comment 1

[Riccardo Cretti]

Attachment: The Config Editor... button in Thunderbird

This is the way it's implemented in Thunderbird alpha

Comment 2

[Riccardo Cretti]

Attachment: The about:config window

The about:config window opened makes a lot easier to play with the preferences!

Comment 3

[Robin Edrenius]

Yes, this is a good idea, we should have one, just like Thunderbird.

Comment 4

[Joey Minta]

This is actually pretty simple to implement, but I'm struggling to find a good
place to stick the button for it.  'General' preferences are already quite
crowded, and we don't have an 'Advanced' section.  Really, in my opinion, the
whole preferences organization should be redone at some point, but that should
be handled in another bug.

Confirming this bug for now, to keep it on the radar screen.  Changing component
to 'Sunbird: Front End'.

Comment 5

[Matthew (lilmatt) Willis]

This is being done as part of bug 333923.
Since I'm doing that one, I'll take this one :)

Comment 6

[Matthew (lilmatt) Willis]

(In reply to comment #5)
> This is being done as part of bug 333923.
> Since I'm doing that one, I'll take this one :)

This is now actually being split back off so we can keep the patch size manageable on bug 333923.

We'll land this after nuprefs lands.

Comment 7

[Matthew (lilmatt) Willis]

Attachment: rev0 - adds advanced tab and about:config to Sunbird

Comment 8

[Joey Minta]

Comment on attachment 226227 [details] [diff] [review]
rev0 - adds advanced tab and about:config to Sunbird

How about a screenshot?  

+            <tabpanels flex="1">
+
+                <!-- General -->
+                <tabpanel orient="vertical">
+                    <hbox align="center" pack="start">
+                        <description flex="1">&pref.calendar.advanced.configEdit.caption;</description>
+                        <button id="configEditor"
+                                label="&pref.calendar.advanced.configEdit.button;"
+                                accesskey="&pref.calendar.advanced.configEdit.accessKey;"
+                                oncommand="gAdvancedPane.showConfigEdit();"/>
+                    </hbox>
+                </tabpanel>
Why are we adding tabs when we only have 1 pref for the entire pane?  Seems like very silly UI to me. minusing based on that.

Comment 9

[Thorsten 'nightrat' Fritz]

Attachment: Screenshot of update tab

Screenshot with update tab in Sunbird advanced preferences.

Because of that new feature it is ok to use tabs in this patch.

Comment 10

[Matthew (lilmatt) Willis]

Requesting cal-ui-review to tell me where the about:config button should live.

Original justification for the location:
I was using the same location as Tb for consistency, and planned to have a second tab for aus update stuff, also similar to Ff and Tb for consistency.

Comment 11

[Matthew (lilmatt) Willis]

Moving milestone to 0.3, and adding keyword

Comment 12

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

Comment on attachment 229447 [details]
Screenshot of update tab

I'm not sure what this screenshot is, but my sunbird doesn't ahve the update tab.

Comment 13

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

As long as we don't have the aus tab, using tabs here is just silly. We can add the tabs later, but for now, leave them.
Also, please provide a screenshot.

Comment 14

[Matthew (lilmatt) Willis]

(In reply to comment #13)
> As long as we don't have the aus tab, using tabs here is just silly. We can add
> the tabs later, but for now, leave them.
> Also, please provide a screenshot.

That screenshot was using a previous version of the nuprefs patch which included the aus stuff. 
I had removed that stuff to make nuprefs small for easier review.

One note:
I'm not sure how much we want to track Firefox and Thunderbird prefs, but Firefox 2 has moved the proxy button to an Advanced > Network tab.

Here's a mockup for ui-review:

+---------------------------------------------------------+
| #####  #####     #####     #####   #####     #####      |
| #####  #####     #####     #####   #####     #####      |
| Views  Alarms  Categories  Views  Timezone  Advanced    |
+---------------------------------------------------------+
|                                                         |
| Advanced Configuration           (Config Editor...)     |
|                                                         |
|                                                         |
|                                                         |
|                                                         |
|                                                         |
|                                                         |
|                                                         |
|                                                         |
|                                                         |
|                                                         |
+---------------------------------------------------------+

Comment 15

[Matthew (lilmatt) Willis]

Taking this bug back

Comment 16

[Matthew (lilmatt) Willis]

mvl files bug 350320 to remove the Timezones pane.
I also noticed I had two "Views" tabs in the previous mock-up.

Here's another mock-up showing the about:config button, the box to enable or disable extension/theme update checking (which we have the underlying infrastructure for already), and the proxy settings button, moved to "Advanced", similar to Firefox 2.

If I can get ui-review on this, then I'll file follow-ups to make the other changes.


+-------------------------------------------------------------------------+
|  #####   #####     #####     #####  | #####  |                          |
|  #####   #####     #####     #####  | #####  |                          |
| General  Alarms  Categories  Views  |Advanced|                          |
+-------------------------------------------------------------------------+
|                                                                         |
| [ ] Automatically check for updates to installed Add-ons                |
|                                                                         |
|                                                                         |
| Configure how Sunbird connects to the Internet  (Connection Settings...)|
|                                                                         |
|                                                                         |
| Advanced Configuration                          (Config Editor...)      |
|                                                                         |
|                                                                         |
|                                                                         |
|                                                                         |
+-------------------------------------------------------------------------+

Comment 17

[Matthew (lilmatt) Willis]

Not going to make the 0.3 train.

Comment 18

[Matthew (lilmatt) Willis]

New UI proposal since we found we have no interface for satchel...


+----------------------------------------------------------------------------+
|  #####   #####     #####     #####   #####    | #####  |                   |
|  #####   #####     #####     #####   #####    | #####  |                   |
| General  Alarms  Categories  Views  Timezone  |Advanced|                   |
+----------------------------------------------------------------------------+
|                        _________ ___________ ________                      |
|                       | General | Passwords | Update |                     |
| +---------------------+         +----------------------------------------+ |
| |                                                                        | |
| | Configure how Sunbird connects to the Internet  (Connection Settings…) | |
| |                                                                        | |
| | Advanced Configuration                          (Config Editor…)       | |
| |                                                                        | |
| +------------------------------------------------------------------------+ |
|                                                                            |
+----------------------------------------------------------------------------+


+----------------------------------------------------------------------------+
|  #####   #####     #####     #####   #####    | #####  |                   |
|  #####   #####     #####     #####   #####    | #####  |                   |
| General  Alarms  Categories  Views  Timezone  |Advanced|                   |
+----------------------------------------------------------------------------+
|                        _________ ___________ ________                      |
|                       | General | Passwords | Update |                     |
| +-------------------------------+           +----------------------------+ |
| | Sunbird can remember password information for all of your calendars so | |
| | you do not need to re-enter your login details.                        | |
| |                                                                        | |
| | [ ] Use a master password to encrypt stored passwords                  | |
| |                                                                        | |
| | When set, the Master Password protects all  ( Set Master Password…  )  | |                                                             | |
| | your passwords, but you must enter it once  (Remove Master Password…)  | |
| | per session.                                                           | |
| |                                                                        | |
| | (View Saved Passwords…)                                                | |
| |                                                                        | |
| +------------------------------------------------------------------------+ |
|                                                                            |
+----------------------------------------------------------------------------+


+----------------------------------------------------------------------------+
|  #####   #####     #####     #####   #####    | #####  |                   |
|  #####   #####     #####     #####   #####    | #####  |                   |
| General  Alarms  Categories  Views  Timezone  |Advanced|                   |
+----------------------------------------------------------------------------+
|                        _________ ___________ ________                      |
|                       | General | Passwords | Update |                     |
| +-------------------------------------------+        +-------------------+ |
| |                                                                        | |
| | [ ] Automatically check for updates to installed Add-ons               | |
| |                                                                        | |
| +------------------------------------------------------------------------+ |
|                                                                            |
+----------------------------------------------------------------------------+

Comment 19

[Matthew (lilmatt) Willis]

(In reply to comment #18)
Please ignore the goofy linebreaks from bugzilla.

Comment 20

[Matthew (lilmatt) Willis]

Attachment: ASCII art proposal for Advanced tab

Using this shiny new ui-review flag...

Comment 21

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

So, you want to morph this bug?

Comment 22

[Matthew (lilmatt) Willis]

(In reply to comment #21)
Sure.

Comment 23

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

Comment on attachment 242301 [details]
ASCII art proposal for Advanced tab

ui-review=mvl, assuming you will remove the connection settings pref from the general pane.

Comment 24

[Matthew (lilmatt) Willis]

Attachment: Advanced > General tab screenshot

Comment 25

[Matthew (lilmatt) Willis]

Attachment: Advanced > Passwords tab screenshot

Comment 26

[Matthew (lilmatt) Willis]

Attachment: Advanced > Update tab screenshot

Comment 27

[Matthew (lilmatt) Willis]

Attachment: Adds Advanced tab to preferences

This patch does all the crazy stuff to make the Advanced tab work.

Comment 28

[Cédric Corazza]

(In reply to comment #27)
lilmat,
It would be fine if you could rename these entities which have a capital K in .accessKey in advanced.xul and advanced.dtd: this prevent some localization tools to work efficiently :
pref.calendar.advanced.configEdit.accessKey --> pref.calendar.advanced.configEdit.accesskey
pref.calendar.advanced.update.enableAddonUpdate.accessKey --> pref.calendar.advanced.update.enableAddonUpdate.accesskey

Thanks

Comment 29

[cmtalbert]

Comment on attachment 242956 [details] [diff] [review]
Adds Advanced tab to preferences

This looks good to me. I built it and played with it a bit on windows.

Comment 30

[Joey Minta]

Comment on attachment 242956 [details] [diff] [review]
Adds Advanced tab to preferences

+              xmlns:xhtml2="http://www.w3.org/TR/xhtml2"
+              xmlns:wairole="http://www.w3.org/2005/01/wai-rdf/GUIRoleTaxonomy#"
+              xmlns:aaa="http://www.w3.org/2005/07/aaa">
These namespaces aren't used.  They should be removed.

+    showConfigEdit: function advPaneShowConfigEdit() {
+        document.documentElement.openWindow("Preferences:ConfigManager",
+                                            "chrome://global/content/config.xul",
+                                            "", null);
This needs to be opened modally or we need to check whether it's already open, to avoid multiple copies.

+        if (preference.value === null)
+            return;
+        advancedPrefs.selectedIndex = preference.value;
+
+        this._initMasterPasswordUI();
returning before we init the master password probably isn't what you want to do here.

+            var hasMP = ((status != nsIPKCS11Slot.SLOT_UNINITIALIZED) &&
+                         (status != nsIPKCS11Slot.SLOT_READY));
+            return hasMP;
Creating a variable you're immediately going to return seems a bit weird.  You could just return the line above, but that's a really picky style nit.  Your call.

+    /**
+     * Enables/disables the Master Password button depending on the state of
+     * the "Use Master Password" checkbox, and prompts for Master Password
+     * removal if one is set.
+     */
This comment should include when this function is expected to be called.

+        if (!checkbox.checked) {
+            this._removeMasterPassword();
+        } else {
...
+        this._initMasterPasswordUI();
+    },
...
+    _removeMasterPassword: function advRemoveMasterPassword() {
...
+        }
+        this._initMasterPasswordUI();
As I read this, we're going to call this _init function twice in a row, which can't be right.  (Notwithstanding the fact that an init function should only be called, well, oninit.)

+    _removeMasterPassword: function advRemoveMasterPassword() {
+        var secmodDB = Components.classes["@mozilla.org/security/pkcs11moduledb;1"]
+                                 .getService(Components.interfaces.nsIPKCS11ModuleDB);
Since you seem to be using toolkit style here, you could go all the way in their pattern and cache services you plan to use multiple times with a get().

+    changeMasterPassword: function advPaneChangeMasterPassword() {
+        var url = "chrome://mozapps/content/preferences/changemp.xul";
+        document.documentElement.openSubDialog(url, "", null);
+        this._initMasterPasswordUI();
This'll be another duplicate call into _initMasterPasswordUI

+    viewPasswords: function advPaneViewPasswords() {
+        document.documentElement.openWindow("Toolkit:PasswordManager",
+                                            "chrome://passwordmgr/content/passwordManager.xul",
+                                            "", null);
Another modal-or-check-if-open case.

+                    <checkbox id="useMasterPassword"
+                              label="&pref.calendar.advanced.useMasterPassword.label;"
+                              accesskey="&pref.calendar.advanced.useMasterPassword.accesskey;"
+                              oncommand="gAdvancedPane.updateMasterPasswordButton();"
+                              class="indent"
I don't see the indent class defined anywhere in preferences.css

That's kinda a lot, so I'd like to take another look.

Comment 31

[Matthew (lilmatt) Willis]

Attachment: Fixes jminta's comments

(In reply to comment #30)
> +    showConfigEdit: function advPaneShowConfigEdit() {
> +        document.documentElement.openWindow("Preferences:ConfigManager",
> +                                   "chrome://global/content/config.xul",
> +                                            "", null);
> This needs to be opened modally or we need to check whether it's already open,
> to avoid multiple copies.
This is actually a toolkit bug.
/mozilla/toolkit/components/viewconfig/content/config.xul has no windowtype
attribute set. It should be set to "Preferences:ConfigManager" for this to
only allow one open window automatically. Spinoff bug 358287. Fixing that will
also fix it for Thunderbird automatically.

> +    viewPasswords: function advPaneViewPasswords() {
> +        document.documentElement.openWindow("Toolkit:PasswordManager",
> +                   "chrome://passwordmgr/content/passwordManager.xul",
> +                                            "", null);
> Another modal-or-check-if-open case.
Similar problem. gavin broke this in his checkin for bug 324063.
Spin-off bug 358285.


> 
> +                    <checkbox id="useMasterPassword"
> +                              class="indent"
> I don't see the indent class defined anywhere in preferences.css
It's defined in toolkit's formatting.css which is @imported by global.css

Comment 32

[Joey Minta]

Comment on attachment 243730 [details] [diff] [review]
Fixes jminta's comments

+    _getSecurityModuleDb: function advPaneGetSecModDb() {
+        if (this._secModDb = null) {
+            this._secModDb = Components.classes["@mozilla.org/security/pkcs11moduledb;1"]
+                                       .getService(Components.interfaces.nsIPKCS11ModuleDB);
+        }
+        return this._secModDb;
1.) You want == in the if block
2.) This should be get _secModDb() {}. See, for example, http://lxr.mozilla.org/mozilla/source/browser/components/places/content/bookmarkProperties.js#53

+    _inited: false,
+    init: function advPaneInit() {
+        this._inited = true;
+        this._secModDb = null;
The proper way to initialize _secModDb is to do it like _inited is here.
  
+        if (preference.value === null)
+            return;
Braces around all if-clauses, please.  (Here and elsewhere)

+        var button = document.getElementById("changeMasterPassword");
+        button.disabled = !checkbox.checked;
This looks superfluous, given the eventual call into _initMasterPassword()

+            promptService.alert(window,
+                                bundle.getString("pw_change_failed_title"),
+                                bundle.getString("pw_change2empty_in_fips_mode"));
Umm, there's no promptService defined.

In an ideal world, there'd be a bug open to get the master-password strings into toolkit, for common use.

r2=jminta

Comment 33

[Matthew (lilmatt) Willis]

Patch (with nits) checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Comment 34

[Koen Hendrickx]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061031 Calendar/0.4a1

This seems ok on my system.

Comment 35

[Damian Szczepanik]

Litmus testcase 2890 created