Closed Bug 1527770 Opened 6 years ago Closed 6 years ago

[de-xbl] Convert Thunderbird preferences to use preferencesBindings.js

Categories

(Thunderbird :: Preferences, task)

Product:
Thunderbird
Component:
Preferences
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 67.0

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(1 file, 5 obsolete files)

1527770-preferences-bindings-6.diff
225.83 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review

In order to finish off bug 1522608, I'm converting our preferences UI to use preferencesBindings.js, which Firefox uses, instead of <preference> XBL bindings.

Summary: Convert Thunderbird preferences to use preferencesBindings.js → [de-xbl] Convert Thunderbird preferences to use preferencesBindings.js

As part of this I had to replace document.getElementById with Preferences.get. I made a list of the ID of every <preference> and searched the code for uses of them, but it's possible some got missed. I can't see anything wrong in the preferences tab or any more errors, and Try was clean.

Attachment #9044041 - Flags: review?(acelists)

Another incarnation of bug 1468167?

I guess, but I am not going to get involved in that discussion.

I see there's a comment in preferencesBindings.js that I should call Preferences.forceEnableInstantApply(); in the pref tab. I'll do that when I get back to this bug.

Comment on attachment 9044041 [details] [diff] [review] 1527770-preferences-bindings-1.diff Review of attachment 9044041 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. I'll try to run with this yet. What about the sanitize dialog? Also uses <preference> elements that you remove the binding for. ::: mail/components/preferences/advanced.js @@ +487,5 @@ > > /** > * When the user toggles the layers.acceleration.disabled pref, > * sync its new value to the gfx.direct2d.disabled pref too. > + * Note that layers.acceleration.disabled is inverted. I wonder why doesn't the 'inverted' attribute in the Preferences.addAll() call not handle this automatically somehow. @@ +492,3 @@ > */ > + updateHardwareAcceleration() { > + if (AppConstants.platforms == "win") { Not your fault, but can you please check this, everywhere else it is 'AppConstants.platform' (no 's'). @@ +789,5 @@ > }, > > }; > + > +Preferences.get("layers.acceleration.disabled", gAdvancedPane.updateHardwareAcceleration); Did you mean to listen to "change" here? ::: mail/components/preferences/colors.xul @@ -23,5 @@ > #endif > > - <script type="application/javascript" src="chrome://communicator/content/utilityOverlay.js"/> > - <script type="application/javascript"><![CDATA[ > - function init() { Why removing this? This initializes the color picker from the pref values. E.g. element id="foregroundtextmenu" still exists. ::: mail/components/preferences/display.js @@ +123,5 @@ > if (!preference) { > + preference = Preferences.add({ > + id: prefs[i].format.replace(/%LANG%/, aLanguageGroup), > + type: prefs[i].type, > + }); At least some improvement from all this churn :) ::: mail/components/preferences/fonts.xul @@ +7,5 @@ > <?xml-stylesheet href="chrome://global/skin/"?> > <?xml-stylesheet href="chrome://messenger/content/charsetList.css"?> > <?xml-stylesheet href="chrome://messenger/skin/preferences/preferences.css"?> > > +<!DOCTYPE dialog [ Thanks for these cleanups. ::: mail/components/preferences/general.js @@ +23,5 @@ > +if (AppConstants.platform != "macosx") { > + Preferences.add({ > + id: "mail.biff.show_alert", > + type: "bool", > + }); Could this be a single line as the others? ::: mail/components/preferences/messagestyle.js @@ +65,5 @@ > .addEventListener("CheckboxStateChange", > previewObserver.showHeaderChanged); > + setTimeout(() => { > + previewObserver.displayTheme(themeName.value); > + this._loaded = true; Why? ::: mail/components/preferences/preferences.css @@ +11,4 @@ > -moz-box-orient: vertical; > } > > prefwindow > .paneDeckContainer { Are there any <prefwindow>s left? .paneDeckContainer is under a <preftab>. ::: mail/components/preferences/preferences.xml @@ +1,1 @@ > <?xml version="1.0"?> So this is a forked copy so that Seamonkey keeps the full version of preferences bindings for now? ::: mail/components/preferences/security.js @@ +18,5 @@ > + { id: "mail.spam.logging.enabled", type: "bool" }, > + { id: "mail.phishing.detection.enabled", type: "bool" }, > + { id: "browser.safebrowsing.enabled", type: "bool" }, > + { id: "mailnews.downloadToTempFile", type: "bool" }, > + // { id: "pref.privacy.disable_button.view_passwords", type: "bool" }, Why is this commented out?
Attachment #9044041 - Flags: review?(acelists) → feedback+
Comment on attachment 9044041 [details] [diff] [review] 1527770-preferences-bindings-1.diff Paenglab, please see if we need any of the prefwindow css rules after this patch. Maybe those need to be changed to 'preftab' or 'dialog' or modified/removed.
Attachment #9044041 - Flags: ui-review?(richard.marti)
Comment on attachment 9044041 [details] [diff] [review] 1527770-preferences-bindings-1.diff ui-r+ for the not changed UI.
Attachment #9044041 - Flags: ui-review?(richard.marti) → ui-review+

I updated the patch to tip and removed the no more needed styles.

The dockoptions and notifications dialogs are empty. So something is wrong, only loading the prefs in the JS files seems not to be enough.

I also get "Missing preference for ID..." errors when opening the colors dialog.

On General page with ticked "Show an alert" and "Play sound" the "Customize..." and "Play" buttons are disabled. I need to untick/tick the checkboxes to enable them...until I reopen the prefs. I haven't checked the other panes if there are similar issues.

Frank, Ian, I'm taking common/bindings/preferences.xml away, and gutting it. I don't think anything else here affects you.

Geoff could you rename it to suite/components/bindings/preferences.xml to preserve the history. I will fixing up the suite build files after landing.

Thanks
frg

Updated based on comments here and some things I found with tests I am creating.

Attachment #9044041 - Attachment is obsolete: true
Attachment #9045427 - Attachment is obsolete: true
Attachment #9046264 - Flags: review?(acelists)

(In reply to :aceman from comment #5)

What about the sanitize dialog? Also uses <preference> elements that you
remove the binding for.

I forgot it, again! In my new patch I've removed all the preferences stuff in that dialog and made it just an ordinary dialog that does things with Services.prefs.

::: mail/components/preferences/advanced.js
@@ +487,5 @@

/**
* When the user toggles the layers.acceleration.disabled pref,
* sync its new value to the gfx.direct2d.disabled pref too.

    • Note that layers.acceleration.disabled is inverted.

I wonder why doesn't the 'inverted' attribute in the Preferences.addAll()
call not handle this automatically somehow.

It does handle it, but that basically means it's layers.acceleration.enabled. Checking the patch now, I seem to have removed this comment. Putting it back in.

::: mail/components/preferences/colors.xul
@@ -23,5 @@

#endif

  • <script type="application/javascript" src="chrome://communicator/content/utilityOverlay.js"/>
  • <script type="application/javascript"><![CDATA[
  • function init() {

Why removing this? This initializes the color picker from the pref values.
E.g. element id="foregroundtextmenu" still exists.

It still works.

::: mail/components/preferences/messagestyle.js
@@ +65,5 @@

         .addEventListener("CheckboxStateChange",
                           previewObserver.showHeaderChanged);
  • setTimeout(() => {
  •  previewObserver.displayTheme(themeName.value);

  •  this._loaded = true;

Why?

Added a comment. I don't really know why, but it's not worth my time investigating.

::: mail/components/preferences/preferences.css
@@ +11,4 @@

-moz-box-orient: vertical;
}

prefwindow > .paneDeckContainer {

Are there any <prefwindow>s left? .paneDeckContainer is under a <preftab>.

Richard has dealt with this.

::: mail/components/preferences/security.js
@@ +18,5 @@

  • { id: "mail.spam.logging.enabled", type: "bool" },
  • { id: "mail.phishing.detection.enabled", type: "bool" },
  • { id: "browser.safebrowsing.enabled", type: "bool" },
  • { id: "mailnews.downloadToTempFile", type: "bool" },
  • // { id: "pref.privacy.disable_button.view_passwords", type: "bool" },

Why is this commented out?

Because this preferences is already added in chat.js, and I forgot this copy was commented. I've removed the one in chat.js instead.

Comment on attachment 9046264 [details] [diff] [review] 1527770-preferences-bindings-3.diff Richard, can you check the Mac dialogs again? I assume they suffer the same problem as I can see with the colours dialog. That is, if the Preferences.jsm code is called too early, everything disappears. No idea why.
Attachment #9046264 - Flags: feedback?(richard.marti)
Comment on attachment 9046264 [details] [diff] [review] 1527770-preferences-bindings-3.diff Thanks, the dialogues opens now with content on Mac and on Windows too.
Attachment #9046264 - Flags: feedback?(richard.marti) → feedback+

(In reply to Richard Marti (:Paenglab) from comment #14)

FX seems to have the same problem. Could this help? https://searchfox.org/comm-central/source/mozilla/browser/components/preferences/colors.xul#90

Yeah, that's how I'm solving it now.

Comment on attachment 9046264 [details] [diff] [review] 1527770-preferences-bindings-3.diff Review of attachment 9046264 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/sanitize.xul @@ -38,5 @@ > - <preferences id="sanitizePreferences"> > - <preference id="privacy.cpd.history" name="privacy.cpd.history" type="bool"/> > - <preference id="privacy.cpd.cookies" name="privacy.cpd.cookies" type="bool"/> > - <preference id="privacy.cpd.cache" name="privacy.cpd.cache" type="bool"/> > - </preferences> So where do you convert these to a Preferences.addAll() call? Is that not needed in this dialog, does it use different handling of prefs? @@ +84,5 @@ > <vbox id="itemList" collapsed="true" persist="collapsed"> > <checkbox label="&itemHistory.label;" > accesskey="&itemHistory.accesskey;" > preference="privacy.cpd.history" > + oncommand="gSanitizePromptDialog.onReadGeneric();"/> Why this change? You didn't touch onsyncfrompreference attributes in the other files (I think you'll do it in the other bug). Also oncommand fires when the checkbox is changed, so it would probbanly correspond to onsync*to*preference. ::: mail/components/preferences/chat.js @@ +185,5 @@ > soundTypeEl.disabled = soundsDisabled; > document.getElementById("chatSoundUrlLocation").disabled = > soundsDisabled || (soundTypeEl.value != 1); > document.getElementById("playChatSound").disabled = > soundsDisabled || (!chatSoundUrlLocation && soundTypeEl.value != 0); The radios do not work right when you toggle them. A similar play sound block in General pane does work fine.

I've removed all the preferences binding stuff from the sanitize dialog. It just uses Services.prefs now. (Which I did say in comment 12.)

Fixed the chat sound and the "auto mark as read" radios too.

Attachment #9046264 - Attachment is obsolete: true
Attachment #9046264 - Flags: review?(acelists)
Attachment #9048003 - Flags: review?(acelists)
Comment on attachment 9048003 [details] [diff] [review] 1527770-preferences-bindings-4.diff Review of attachment 9048003 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/preferences/chat.js @@ +182,5 @@ > + soundsEnabled = Preferences.get("mail.chat.play_sound").value; > + } > + if (isNaN(soundTypeValue)) { > + soundTypeValue = Preferences.get("mail.chat.play_sound.type").value; > + } Somehow this makes the code uglies. Why do you sometimes use Preferences and sometimes the elements? ::: mail/components/preferences/display.js @@ +306,5 @@ > */ > + updateMarkAsReadTextbox(textBoxEnabled, focusTextBox) { > + if (typeof textBoxEnabled == "string") { > + textBoxEnabled = textBoxEnabled == "true"; > + } When would it not be a string?

Why do you sometimes use Preferences and sometimes the elements?

I'm glad you asked that because I was starting to ask myself a similar question.

I was just doing what the original code did, which was sometimes an onchange attribute on a <preference> and sometimes on the element itself. I preferred the latter but now I've gone the other way because it does make the code less ugly.

Attachment #9048003 - Attachment is obsolete: true
Attachment #9048003 - Flags: review?(acelists)
Attachment #9048078 - Flags: review?(acelists)
Comment on attachment 9048078 [details] [diff] [review] 1527770-preferences-bindings-5.diff Review of attachment 9048078 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, the updatePlaySound() now looks much better. Some dialogs (e.g. General->Customize) still has Cancel and OK buttons. Are those useful, don't preferences apply immediately on all platforms now? So maybe the Cancel could be removed. Please file a new bug for that if it is wanted. ::: mail/components/preferences/display.js @@ -280,3 @@ > document.getElementById("markAsReadAutoPreferences").disabled = autoMarkDisabled; > document.getElementById("secondsLabel").disabled = autoMarkDisabled; > - this.updateMarkAsReadTextbox(); Why can this be removed here? @@ +311,1 @@ > delayTextbox.focus(); This is strange. We toggled a radio and suddenly get the focus forced to some element? We do not do that in any other similar preferences set. ::: mail/components/preferences/notifications.xul @@ +40,3 @@ > > + <script type="application/javascript" src="chrome://global/content/preferencesBindings.js"/> > + <script type="application/javascript" src="chrome://messenger/content/preferences/notifications.js"/> Why at the bottom? ::: mail/components/preferences/preferences.js @@ -99,5 @@ > let prefPane = document.getElementById(paneID); > - let tabOnEvent = false; > - // The prefwindow element selects the pane specified in window.arguments[0] > - // automatically. But let's check it and if the prefs window was already > - // open, the current prefpane may not be the wanted one. What is the replacement for this removal?
Attachment #9048078 - Flags: review?(acelists) → review+

(In reply to :aceman from comment #22)

Comment on attachment 9048078 [details] [diff] [review]
1527770-preferences-bindings-5.diff

Review of attachment 9048078 [details] [diff] [review]:

Thanks, the updatePlaySound() now looks much better.

Some dialogs (e.g. General->Customize) still has Cancel and OK buttons. Are
those useful, don't preferences apply immediately on all platforms now? So
maybe the Cancel could be removed. Please file a new bug for that if it is
wanted.

At least on Windows, not tested on Linux, "Cancel" reverts the changes. Maybe this is because they are now <dialog> and no more <prefwindow>.

(In reply to :aceman from comment #22)

Comment on attachment 9048078 [details] [diff] [review]
1527770-preferences-bindings-5.diff

Review of attachment 9048078 [details] [diff] [review]:

Thanks, the updatePlaySound() now looks much better.

Some dialogs (e.g. General->Customize) still has Cancel and OK buttons. Are
those useful, don't preferences apply immediately on all platforms now? So
maybe the Cancel could be removed. Please file a new bug for that if it is
wanted.

The comments in preferencesBindings.jsm say:

// The about:preferences page forces instantApply.
and
// Dialogs of type="child" are never instantApply.

This is consistent with what I'm seeing and with v60. They probably should've had buttons when they were <prefwindow>s since I've not added the dlgbuttons attribute, it was already there.

I'm happy with the buttons coming back, now that I've seen it. It makes more sense, I think.

::: mail/components/preferences/display.js
@@ -280,3 @@

 document.getElementById("markAsReadAutoPreferences").disabled = autoMarkDisabled;
 document.getElementById("secondsLabel").disabled = autoMarkDisabled;
  • this.updateMarkAsReadTextbox();

Why can this be removed here?

It can't, I messed up. :/

@@ +311,1 @@

   delayTextbox.focus();

This is strange. We toggled a radio and suddenly get the focus forced to
some element? We do not do that in any other similar preferences set.

That is how it's supposed to work. I don't know who made it that way, but it does make sense to me.

::: mail/components/preferences/notifications.xul
@@ +40,3 @@

  • <script type="application/javascript" src="chrome://global/content/preferencesBindings.js"/>
  • <script type="application/javascript" src="chrome://messenger/content/preferences/notifications.js"/>

Why at the bottom?

That's how to fix the empty dialogs Richard mentions in comment 8 and discussed in comments 13-16.

::: mail/components/preferences/preferences.js
@@ -99,5 @@

 let prefPane = document.getElementById(paneID);
  • let tabOnEvent = false;
  • // The prefwindow element selects the pane specified in window.arguments[0]
  • // automatically. But let's check it and if the prefs window was already
  • // open, the current prefpane may not be the wanted one.

What is the replacement for this removal?

Nothing. We never wait for this event because it's already happened. I probably should've changed this in bug 1522778 but I hadn't noticed it.

Tidying up some mistakes. I'm going to do another Try run of this and bug 1530271 together.

Attachment #9048078 - Attachment is obsolete: true
Attachment #9048400 - Flags: review+

(In reply to Geoff Lankow (:darktrojan) from comment #24)

I'm happy with the buttons coming back, now that I've seen it. It makes more sense, I think.

I don't think we should add ok/cancel. Firefox doesn't have that, and even web applications are moving more to instant applying prefs, like is also done in the mobile scene.

(In reply to Magnus Melin [:mkmelin] from comment #26)

(In reply to Geoff Lankow (:darktrojan) from comment #24)

I'm happy with the buttons coming back, now that I've seen it. It makes more sense, I think.

I don't think we should add ok/cancel. Firefox doesn't have that, and even web applications are moving more to instant applying prefs, like is also done in the mobile scene.

Checked Firefox under Windows and Linux and it has the ok/cancel buttons in the dialogs. Check "Advanced..." and "Colors" for example.

Ah, those. I guess I skimmed the comments too fast.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/333fa90d259d
Copy preferences.xml into suite to preserve history; rs=me
https://hg.mozilla.org/comm-central/rev/4a4cb3456840
Convert Thunderbird preferences to use preferencesBindings.js; r=aceman

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 67.0
Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/710e3b9acbac Update jar.mn and preferences.xml in suite after move from common. r=frg

I'm note sure if it's caused by this bug, but some of the strings in Daily's Preferences don't show the accesskey. For instance _W_hen Daily launches, show the Start Page in the message area doesn't show accesskey W, but _L_ocation and _R_estore Default on the next line do...

(In reply to Onno Ekker [:nONoNonO UTC+1] from comment #31)

I'm note sure if it's caused by this bug, but some of the strings in Daily's Preferences don't show the accesskey. For instance _W_hen Daily launches, show the Start Page in the message area doesn't show accesskey W, but _L_ocation and _R_estore Default on the next line do...

Looks like for all the checkboxes the access key isn't shown...

I think it's far more likely to be bug 1455433 that caused missing access key display.

Onno, mind checking Firefox trunk and filing a bug if that's the case? (Or a new bug for tb if thunderbird-specific.)

I already checked this and it's also borken in Furefox 67.0a1.
I added a comment to bug 1455433, but haven't filed a new bug yet.

Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: