309041 - Changing the proportional font does not update the default font until options are re-opened

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Dave Townsend [:mossop]]

This occurs when the options are not using instantapply.

1. Open options to the content pane.
2. Note the default font setting.
3. Click advanced.
4. Change the proportional font option.
5. Click ok.

The default font will be unchanged in the main options. Clicking ok then
re-opening options will change the default font correctly. In an instantapply
setup the default font does change correctly.

Comment 1

[Dave Townsend [:mossop]]

Attachment: Patch

The problem is that when child dialogs are accepted in non-instantapply, they
store their settings in a set of preference elements in the prefpane that opens
them. This patch adjusts the font options in the content pane to use the
correct id's for the preference elements it uses so they correspond with the
ones used by the child dialog. Although not strictly necessary it also places
all the font prefs into the extra preferences element that the child dialog
will create anyway.

This has the added effect that changing the font settings then opening the
advanced will show the changes in the advanced dialog.

Comment 2

[Dave Townsend [:mossop]]

An alternative to this patch would be to make some changes to the prefwindow
binding. Child prefwindows could be made to automatically find a corresponding
preference element in the parent dialog an use that for storage, only resorting
to creating its own with a special id when necessary.

This should fix this situation, possibly other similar situations that I havent
seen, and would also work correctly if two different child dialogs both use the
same pref.

Comment 3

[Dave Townsend [:mossop]]

Attachment: Patch for the prefwindow bindings

When looking for saved state for a child prefwindow, this attempts to find any
preference element on the parent window with the right name.

When saving state the same search is performed and only if no element is found
is a new element created.

Comment 4

[Dave Townsend [:mossop]]

Attachment: Same patch with typo fix

Fixed a minor typo

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 196576 [details] [diff] [review]
Same patch with typo fix

Since this changes the behaviour of the binding a little it might be nice to
get it in for 1.5, don't think its a blocker though.

Comment 6

[Dave Townsend [:mossop]]

Attachment: No longer set preference id

There is no longer any need to set an id on the generated preference element
for the child state.

Comment 7

[Asaf Romano (gone)]

Comment on attachment 198210 [details] [diff] [review]
No longer set preference id

>Index: toolkit/content/widgets/preferences.xml
>===================================================================

>+          var preference = null;
>+          var pprefs = pdoc.getElementsByTagName("preference");
>+          for (var i = 0; i < pprefs.length; ++i) {
>+            if (pprefs[i].name == this.name) {
>+              preference = pprefs[i];
>+              break;
>+            }
>+          }

...

>+                  var preference = null;
>+                  var pprefs = pdoc.getElementsByTagName("preference");
>+                  for (var k = 0; k < pprefs.length; ++k) {
>+                    if (pprefs[k].name == preferences[j].name) {
>+                      preference = pprefs[k];
>+                      break;
>+                    }
>+                  }

As discussed on IRC, make those two loops address the common case a bit better.

>                   if (!preference) {
>-                    var preference = pdoc.createElement("preference");
>+                    preference = pdoc.createElement("preference");
>                     childPrefs.appendChild(preference);
>-                    preference.id       = prefID;

You've to make sure updateElements, disabled and tabIndex don't fail/warn in
the case where there's no id.

Comment 8

[Dave Townsend [:mossop]]

Attachment: Improved patch

Addressed Mano's comments.

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch

Addressed comments from IRC.
Removed break statement.
Wrapped the longer lines.

Comment 10

[Asaf Romano (gone)]

Comment on attachment 198686 [details] [diff] [review]
patch

>-          this._setValue(lastStatePref ? lastStatePref.value 
>-                                       : this.valueFromPreferences, false);
>+
>+          // Try to find a preference element in the parent window with the same
>+          // name as this one.

how about "a preference element for the same preference"?

>+              if (parentPrefs[l].localName == "preference") {
>+                preference = parentPrefs[l];
>+              }
>+            }

remove single line brackets.

>               for (var i = 0; i < panes.length; ++i) {
>                 var preferences = panes[i].preferences;
>                 for (var j = 0; j < preferences.length; ++j) {
>-                  var prefID = window.location.href + "#" + preferences[j].id;
>-                  var preference = pdoc.getElementById(prefID);
>+                

remove the whitespace in this line.


r=mano with these changes.

Comment 11

[Dave Townsend [:mossop]]

Attachment: Final patch

Patch addressing Mano's comments.

Comment 12

[Asaf Romano (gone)]

Comment on attachment 198793 [details] [diff] [review]
Final patch

r=mano.

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in toolkit/content/widgets/preferences.xml;
/cvsroot/mozilla/toolkit/content/widgets/preferences.xml,v  <--  preferences.xml
new revision: 1.42; previous revision: 1.41
done

Comment 14

[Dave Townsend [:mossop]]

Fixed on trunk.

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 198793 [details] [diff] [review]
Final patch

too late for non-critical changes.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Checked in on the 1.8 branch for Firefox 2.
mozilla/toolkit/content/widgets/preferences.xml; new revision: 1.31.2.9;