[de-xbl] Convert Thunderbird preferences to use preferencesBindings.js
Categories
(Thunderbird :: Preferences, task)
Tracking
(Not tracked)
People
(Reporter: darktrojan, Assigned: darktrojan)
References
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
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.
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
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.
Assignee | ||
Comment 4•6 years ago
|
||
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?
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.
Comment 7•6 years ago
|
||
Comment on attachment 9044041 [details] [diff] [review] 1527770-preferences-bindings-1.diff ui-r+ for the not changed UI.
Comment 8•6 years ago
|
||
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.
Assignee | ||
Comment 9•6 years ago
|
||
Frank, Ian, I'm taking common/bindings/preferences.xml away, and gutting it. I don't think anything else here affects you.
![]() |
||
Comment 10•6 years ago
|
||
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
Assignee | ||
Comment 11•6 years ago
|
||
Updated based on comments here and some things I found with tests I am creating.
Assignee | ||
Comment 12•6 years ago
|
||
(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.
Assignee | ||
Comment 13•6 years ago
|
||
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.
Comment 14•6 years ago
|
||
FX seems to have the same problem. Could this help? https://searchfox.org/comm-central/source/mozilla/browser/components/preferences/colors.xul#90
Comment 15•6 years ago
|
||
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.
Assignee | ||
Comment 16•6 years ago
|
||
(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 17•6 years ago
|
||
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.
Assignee | ||
Comment 18•6 years ago
|
||
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.)
Assignee | ||
Comment 19•6 years ago
|
||
Fixed the chat sound and the "auto mark as read" radios too.
![]() |
||
Comment 20•6 years ago
|
||
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?
Assignee | ||
Comment 21•6 years ago
|
||
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.
![]() |
||
Comment 22•6 years ago
|
||
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?
Comment 23•6 years ago
|
||
(In reply to :aceman from comment #22)
Comment on attachment 9048078 [details] [diff] [review]
1527770-preferences-bindings-5.diffReview 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>.
Assignee | ||
Comment 24•6 years ago
|
||
(In reply to :aceman from comment #22)
Comment on attachment 9048078 [details] [diff] [review]
1527770-preferences-bindings-5.diffReview 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.
Assignee | ||
Comment 25•6 years ago
|
||
Tidying up some mistakes. I'm going to do another Try run of this and bug 1530271 together.
Comment 26•6 years ago
|
||
(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.
Comment 27•6 years ago
|
||
(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.
Comment 29•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Comment 30•6 years ago
|
||
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
Comment 31•6 years ago
|
||
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...
Comment 32•6 years ago
|
||
(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...
Assignee | ||
Comment 33•6 years ago
|
||
I think it's far more likely to be bug 1455433 that caused missing access key display.
Comment 34•6 years ago
|
||
Onno, mind checking Firefox trunk and filing a bug if that's the case? (Or a new bug for tb if thunderbird-specific.)
Comment 35•6 years ago
|
||
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.
Updated•5 years ago
|
Description
•