1465061 - Make all preferences in-content and remove stand-alone options window

Status: RESOLVED FIXED

(Thunderbird :: Preferences, enhancement)

Description

[Jorg K (CEST = GMT+2)]

Since bug 925746 (TB 37) TB has had preferences in a tab, but hidden behind the preference mail.preferences.inContent.

The discussion in bug 1460721 indicates, that the old stand-alone options window is becoming increasingly unmaintainable. In any case, we should reduce the maintenance burden and remove it.

Moving fully to in-content prefs was blocked by moving the account manager to a tab as well, bug 1096006. That was mostly blocked by a subtle pane-switching issue when Lightning was also installed.

Lightning is currently disabled since after the removal of support for non-bootstrapped overlays it is not working any more. It is presently unclear how Lighting will be made to function again. Any issues should be addressed when it comes back.

Therefore we are at a good point in time to remove the stand-alone options window and move to accounts in a tab as well.

Comment 1

[Jorg K (CEST = GMT+2)]

Let's see which tests break when switching to in-content prefs:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=60b536f8fc3fa26ad1d87cff8b6702185e9c4070
Push included patch from bug 1460721 as well.

Comment 2

[Jorg K (CEST = GMT+2)]

Announcement here: http://lists.thunderbird.net/pipermail/maildev_lists.thunderbird.net/2018-May/001192.html

Test failures:
mozmill/pref-window/test-attachments-pane.js
mozmill/cloudfile/test-cloudfile-manager.js
That's really minimal and very promising :-)

Aceman, could we look at these two test failures together?
Richard, would you be able to prepare a patch that removes mail.preferences.inContent and all the code looking at that preference. Also, all code used for the stand-alone window.

Comment 3

[Richard Marti (:Paenglab)]

I can try it.

Comment 4

[Jorg K (CEST = GMT+2)]

Looks like both tests use open_pref_window() (which is also used in the switched-off tests in test-font-chooser.js and in test-calendar-utils.js). That function would have to change to open_pref_tab() with appropriate new content.

Comment 5

[Richard Marti (:Paenglab)]

Attachment: inContentPrefs.patch

This sets the in-content prefs as default and removes the old prefs in window.

I haven't touched https://searchfox.org/comm-central/source/mail/components/nsMailDefaultHandler.js#254-265 because I don't know if this should be changed to show the in-content prefs or we should remove this. This is probably a very seldom used command line option.

If we still want this option, this should be changed from someone who knows this better than me.

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 8982417 [details] [diff] [review]
inContentPrefs.patch

Nice, many thanks. This was long overdue. Sadly we need to wait for the test to be changed before we can land this.

Comment 7

[Jorg K (CEST = GMT+2)]

Landing this now to unblock bug 1460721.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1465061-disable-tests.patch

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/22432b42ee7d
Make all preferences in-content and remove stand-alone options window. r=jorgk
https://hg.mozilla.org/comm-central/rev/a649cc4ccd85
temporarily disable tests using open_pref_window(). r=me DONTBUILD

Comment 10

[:aceman]

Comment on attachment 8982417 [details] [diff] [review]
inContentPrefs.patch

Review of attachment 8982417 [details] [diff] [review]:
-----------------------------------------------------------------

Nice cleanup, thanks (even though I do not like the whole idea, but we need to follow FF for now).

Please also fix https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mail/components/nsMailDefaultHandler.js#254 (seems to reference the removed preferences.xul file). There are also some references in mail/test/mozmill/shared-modules/test-pref-window-helpers.js, but those can be done when the tests are fixed, we just shouldn't forget them as they are in comments.

::: mail/base/content/mailCore.js
@@ -426,5 @@
> -      // the dialog must be created
> -      let instantApply = Services.prefs
> -                                 .getBoolPref("browser.preferences.instantApply");
> -      let features = "chrome,titlebar,toolbar,centerscreen" +
> -                     (instantApply ? ",dialog=no" : ",modal");

It seems this removal now basically forces browser.preferences.instantApply = true for all platforms. Also see
https://dxr.mozilla.org/comm-central/rev/9055d9d89a4bca5cf48dda789299559aefca4e54/mozilla/toolkit/content/preferencesBindings.js#77

::: mail/components/preferences/preferences.xul
@@ -21,5 @@
> -<prefwindow type="prefwindow"
> -            id="MailPreferences"
> -            windowtype="Mail:Preferences"
> -#ifdef USE_WIN_TITLE_STYLE
> -            title="&prefWindow.titleWin;"

Can these strings get dropped now?

@@ -47,5 @@
> -  <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/>
> -  <script type="application/javascript" src="chrome://communicator/content/contentAreaClick.js"/>
> -  <script type="application/javascript" src="chrome://messenger/content/preferences/preferences.js"/>
> -
> -#include general.inc.xul

These files now no longer need to stay separate (as now only one xul document includes them, but let's leave them for logical grouping. Also Firefox kept them like that.

::: mail/themes/linux/mail/preferences/preferences.css
@@ -61,5 @@
> -  color: HighlightText;
> -}
> -
> -radio[pane=paneGeneral] {
> -  list-style-image: url("chrome://messenger/skin/preferences/general.png");

Do we have to remove the icons? Can't they be used in the pref panes names (albeit smaller) besides the label?

::: mail/themes/windows/mail/preferences/preferences.css
@@ -40,5 @@
>  }
>  
>  prefwindow[type="child"] .prefWindow-dlgbuttons {
>    padding: 0;
> -}

Where is the closing bracket if this one gets removed? Typo?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b86ffd09784f
Follow-up: restore accidentally removed closing brace in preferences.css. r=me DONTBUILD

Comment 12

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #10)
> Comment on attachment 8982417 [details] [diff] [review]
> inContentPrefs.patch
> 
> Review of attachment 8982417 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice cleanup, thanks (even though I do not like the whole idea, but we need
> to follow FF for now).
> 
> Please also fix
> https://dxr.mozilla.org/comm-central/rev/
> 51678c5b0b3edd827c57a70f1c5baf02d44acead/mail/components/
> nsMailDefaultHandler.js#254 (seems to reference the removed preferences.xul
> file). There are also some references in
> mail/test/mozmill/shared-modules/test-pref-window-helpers.js, but those can
> be done when the tests are fixed, we just shouldn't forget them as they are
> in comments.

Yes, this is what I asked in comment 5. Do we want this to open the in-content prefs or should we remove it?

> ::: mail/base/content/mailCore.js
> @@ -426,5 @@
> > -      // the dialog must be created
> > -      let instantApply = Services.prefs
> > -                                 .getBoolPref("browser.preferences.instantApply");
> > -      let features = "chrome,titlebar,toolbar,centerscreen" +
> > -                     (instantApply ? ",dialog=no" : ",modal");
> 
> It seems this removal now basically forces browser.preferences.instantApply
> = true for all platforms. Also see
> https://dxr.mozilla.org/comm-central/rev/
> 9055d9d89a4bca5cf48dda789299559aefca4e54/mozilla/toolkit/content/
> preferencesBindings.js#77

Yes, this is also the behavior on FX. Only the Account manager still uses the "Cancel" button in dialogs.

> ::: mail/themes/linux/mail/preferences/preferences.css
> @@ -61,5 @@
> > -  color: HighlightText;
> > -}
> > -
> > -radio[pane=paneGeneral] {
> > -  list-style-image: url("chrome://messenger/skin/preferences/general.png");
> 
> Do we have to remove the icons? Can't they be used in the pref panes names
> (albeit smaller) besides the label?

If we would use icons for every category, then we would use the ones we use in the in-content prefs.

Comment 13

[Jorg K (CEST = GMT+2)]

Can someone take care of the left-over reference to preferences.xul and the following problem:
When you select an action for an attachment in "Options > Attachments, Incoming", that choice is not stored.

Aceman is already NI'ed.

Comment 14

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+2) from comment #13)
> Can someone take care of the left-over reference to preferences.xul.

So you agree to remove it?

Comment 15

[Jorg K (CEST = GMT+2)]

We need to investigate what that code does and then decide.

Comment 16

[Richard Marti (:Paenglab)]

With "thunderbird -options" on the command line it starts TB and opens the Prefs window.

Comment 17

[:aceman]

Did it open only the Prefs window or was there also the main 3pane window in the background?
With the new tab we can only just ignore the option or open the 3pane window and open the prefs in a tab inside it. I'm not sure how hard it is to pass such a request from the code in question which I think runs far before the main window is up.
But we could test this.

Comment 18

[Jorg K (CEST = GMT+2)]

OK, can we make it open the prefs tab instead?

Comment 19

[Richard Marti (:Paenglab)]

It opened the 3pane window and the prefs.

Comment 20

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #10)
> Comment on attachment 8982417 [details] [diff] [review]
> inContentPrefs.patch
> 
> Review of attachment 8982417 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: mail/components/preferences/preferences.xul
> @@ -21,5 @@
> > -<prefwindow type="prefwindow"
> > -            id="MailPreferences"
> > -            windowtype="Mail:Preferences"
> > -#ifdef USE_WIN_TITLE_STYLE
> > -            title="&prefWindow.titleWin;"
> 
> Can these strings get dropped now?

They are still needed for the tab title.

Comment 21

[:aceman]

Also the *style* ones?

Comment 22

[Richard Marti (:Paenglab)]

Attachment: Bug1465061-rm-strings.patch

Ah, this ones aren't needed anymore.

Comment 23

[:aceman]

Comment on attachment 8984262 [details] [diff] [review]
Bug1465061-rm-strings.patch

Review of attachment 8984262 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e67228e765b5
Remove strings only used by obsolete stand-alone options window. r=aceman

Comment 25

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #13)
> Can someone take care of the left-over reference to preferences.xul and the
> following problem:
> When you select an action for an attachment in "Options > Attachments,
> Incoming", that choice is not stored.
Richard, this works in the in-content preferences in TB 60 beta, right? So its a new issue introduced here, yes?

Comment 26

[Jorg K (CEST = GMT+2)]

I'm trying to look into the
  "Options > Attachments, Incoming", action choice not stored"
problem. With the inspector I found that a) |gApplicationsPane.onSelectAction(event.originalTarget)|, b) |gApplicationsPane.chooseApp(event)| and c) |gApplicationsPane.manageApp(event)| are used on those menus.

a) |gApplicationsPane.onSelectAction(event.originalTarget)| comes from
https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mail/components/preferences/handlers.xml#52
so some "handler" bindings. 

b) |gApplicationsPane.chooseApp(event)| is set up here:
https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mail/components/preferences/applications.js#1385

chooseApp() is here:
https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mail/components/preferences/applications.js#1602

The strange thing is that the chosen action is actually shown, until the next content type is picked. Then it's reverted.

Comment 27

[Jorg K (CEST = GMT+2)]

I forgot to ask: Is this handler stuff related to bug 1460392?

Comment 28

[Richard Marti (:Paenglab)]

Yes, on 60 beta it works and it broke later. It breaks too with bug 1460392 applied where changes in applications.* are done.

Comment 29

[:aceman]

(In reply to Jorg K (GMT+2) from comment #13)
> Can someone take care of the left-over reference to preferences.xul and the
> following problem:
> When you select an action for an attachment in "Options > Attachments,
> Incoming", that choice is not stored.

Possibly the leftover line:
let loadInContent = Services.prefs.getBoolPref("mail.preferences.inContent");

Fetching non-existent prefs throws, just that it seems the exception is not shown in error console for some reason.
This may have hosed the pane.

I'll fix it in the patch fixing tests.

Comment 30

[:aceman]

Attachment: 1465061-fix tests

This should fix the 2 tests in TB opening the prefs. Font tests are left for bug 1460721. The one calendar test also needs update but I'll let darktrojan do it as it needs decision whether calendar wants to keep openLightningPrefs() in some way.

Comment 31

[Jorg K (CEST = GMT+2)]

Comment on attachment 8986613 [details] [diff] [review]
1465061-fix tests

Geoff doesn't need to comment on the patch but rather file another bug to fix the Calendar MozMill test test-calendar-utils.js which still uses the now removed open_pref_window() when MozMill comes back online.

Comment 32

[Jorg K (CEST = GMT+2)]

Comment on attachment 8986613 [details] [diff] [review]
1465061-fix tests

Thanks, both tests pass for me now. Dispatching a new event "paneSelected" is a little hacky, but it's OK if we need it for the tests.

The attachment action mostly sticks now, the only case where it doesn't stick is that I have a content type "about" which appears somewhat defective. However, the "Use other..." doesn't stick at all, and that's already broken in TB 60 beta, so we might move this to another bug.

Comment 33

[:aceman]

Comment on attachment 8986613 [details] [diff] [review]
1465061-fix tests

Review of attachment 8986613 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/preferences/preferencesTab.js
@@ +93,5 @@
> +
> +        if ("onLoad" in aArgs) {
> +          aArgs.onLoad(event, aTab.browser);
> +        }
> +      }, {capture: true, once: false}

Before landing, please change this back to 'once: true', the 'false' is a leftover from my experiments.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4111f577309c
fix tests opening TB preferences. r=jorgk

Comment 35

[:aceman]

Re: comment 5, can you try replacing that window opening code with openOptionsDialog() ?

Comment 36

[:aceman]

Not that easy. As suspected the 3pane window is not yet open when that code is run.

Comment 37

[Jorg K (CEST = GMT+2)]

To be continued in bug 1470304 and bug 1470307.