308555 - Move Account Settings to the Mac OS X application menu

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[Scott MacGregor]

It'd be nice if the account settings menu under Tools was moved to the
application men on Mac OS X next to the Preferences menu item.

I saw that Asaf had a similar patch for the update menu item for Firefox and
Thunderbird so maybe it wouldn't be hard to do this.

I'll cc him in case he's interested in working on this. Unlike the update menu
case, this menu item is completely static :)

Comment 1

[Asaf Romano (gone)]

Scott, is it really important for 1.5? We have plans to make it possible (i.e.
without really evil hacks) in the 1.9 timeframe.

Comment 2

[Scott MacGregor]

then it can wait, it looked like a small change to the menu os x toolbar code
based on the update change which is why i wanted to through it outthere. 

Comment 3

[Asaf Romano (gone)]

Also, don't we have a bug on moving the account setting to the Preferences
window? this is the right approach imho.

Comment 4

[Scott MacGregor]

I was confused by the last comment, what's the right approach? What was going on
in the check for updates patch or something different during the 1.9 time frame? 

Comment 5

[Asaf Romano (gone)]

The update patch (other than being very static ;) ) makes widget/ depend on Fx's
and Tbird's XUL, this kind of approach might be OK for the branch, only.

By "Right approach", I referred to mereging the Account Settings prefs into the
main preferences window, assuming it was already in the 2.0 bucket...

Comment 6

[Scott MacGregor]

I wouldn't be adverse to a branch only patch to move it over for the branch.

I would love to have the acocunt manager and options dialog merged of course.
Hopefully that will happen one day if we can get enough UI design work done for 2.0.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Bryan, thoughts?

Comment 8

[Stefan [:stefanh]]

I think this can only be done in widget/src/cocoa. Fwiw, Mail has account settings in the main prefs.

Comment 10

[Rimas Kudelis [use needinfo to ping]]

Until somebody implements the right approach, could we please have what the title of this suggests implemented? That doesn't seem to involve much work, and would certainly make layout of Thunderbird menu in OS X more logical.

Comment 11

[Wayne Mery (:wsmwk)]

I am sure patches would be welcome

Comment 12

[Stefan [:stefanh]]

(In reply to comment #10)
> Until somebody implements the right approach, could we please have what the
> title of this suggests implemented? That doesn't seem to involve much work, and

The application menu is done by the widget code in mozilla-central. Iirc, you'd need to modify the widget code to handle a new xul menuitem. So, it is some work and I also beleive that the current implementation needs a re-write in order to become more generic.

Comment 13

[Wayne Mery (:wsmwk)]

Is this something we want to do?

Comment 14

[Richard Marti (:Paenglab)]

Attachment: 308555-mac-move-account-settings.patch for M-C

Some time ago I tried this. Then it worked besides the issue that in composer the Tools menu is shown but empty.

I had to update the patch and it is now untested because I'd have to change from artifact builds to normal builds.

Alessandro, what do you think about this?

Comment 15

[Richard Marti (:Paenglab)]

Attachment: 308555-hide-accountsettings-menuseparator.patch

Remove the menuseparator in our menu.

Comment 16

[Richard Marti (:Paenglab)]

Attachment: 308555-mac-move-account-settings.patch for M-C

I did now a full build and found a build issue. Fixed now.

M-C did some changes in Mac menu. Now in Composer the Tools still has the AM menuitem, probably because it's the only item in this menu. The Address Book doesn't have this item because it has other menu items in it.

The problem I have is that the item is now in the application menu but it doesn't open the AM.

Mark, I know you have asked for this change sometime ago. I don't know how to make this working on Mac I've never programmed on Mac. Maybe you can help? The skeleton is done and hopefully only a fine tuning is needed.

Comment 17

[Alessandro Castellani [:aleca]]

I'm okay with having the Account Settings menu in the macos Application menu.
I'm not sure I'd be too helpful in reviewing it unfortunately.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 9222777 [details] [diff] [review]
308555-mac-move-account-settings.patch for M-C

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

I'm sorry for having not got back to this much sooner...

The patch seems to work, but fails to call `MsgAccountManager()` from the hidden window because accountUtils.js isn't loaded in there:

`JavaScript error: chrome://messenger/content/hiddenWindowMac.xhtml, line 1: ReferenceError: MsgAccountManager is not defined`

You can also see this without the patch if you minimise any Thunderbird windows and then go to Tools -> Account Settings.

Unfortunately we can't simply add accountUtils.js to be loaded via hiddenWindowMac.xhtml, because it assumes there's a full message window loaded and hence fails. Probably the best alternative would be to add a function to hiddenWindowMac.js that locates the most recently used window (or opens a new one) and calls `window.MsgAccountManager(null)` on that instead.

Not sure if you've done that before, but something similar to this:

https://searchfox.org/comm-central/rev/44aa77c6da7c13b659667b96eb3c149e2096d0f1/mail/base/content/aboutDialog.js#106-128

One comment in-line, not sure if that's necessary to make the patch work, but probably helps with memory leaks maybe.

::: widget/cocoa/nsMenuBarX.mm
@@ +106,5 @@
>    }
>    if (sQuitItemContent == mQuitItemContent) {
>      sQuitItemContent = nullptr;
>    }
> +  if (sPrefItemContent == mAccountItemContent) {

Should be `sAccountItemContent`

Comment 19

[Richard Marti (:Paenglab)]

Attachment: M-C-part

Comment 20

[Richard Marti (:Paenglab)]

Attachment: 308555-move-accountsettings-TB-part.patch

Alex, please could have a look what is wrong/missing?

When I remove the "get selected folder" part (https://searchfox.org/comm-central/source/mailnews/base/prefs/content/accountUtils.js#157-170) the Account Manager tab opens. So it is not an issue of not finding the window.

With adding folderPane.js to the hiddenWindowMac.xhtml I advanced a bit. But now I get a accountManager is undefined error in mailWindowOverlay.js. I thought it is defined from other files but it seems not. So what am I missing here?

Comment 21

[Alessandro Castellani [:aleca]]

I haven't tested this thoroughly because I can't do a full build on macOS, but I don't think adding accountUtils.js and folderPane.js to the hiddenWindowMac.xhtml is correct. As Mark said, those files assume that a window is always available so this wouldn't work.

Did you try following the suggestion from Mark by using something similar to this method? https://searchfox.org/comm-central/rev/44aa77c6da7c13b659667b96eb3c149e2096d0f1/mail/base/content/aboutDialog.js#106-128
I think something like that should be used in a new function inside hiddenWindowMac.js.

Comment 22

[Richard Marti (:Paenglab)]

As I wrote already this seems not to be a problem of a not available window, the menu is shown when the window is active. Without the "get selected folder" part it works. It seems when this part runs it can't get the needed information, but why?

When I add accountUtils.js and folderPane.js it goes further but is now stuck on "accountManager is undefined".

This is all I can do with my knowledge. Maybe someone other can go further with my patches.

Comment 23

[Mark Banner (:standard8)]

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

As I wrote already this seems not to be a problem of a not available window, the menu is shown when the window is active. Without the "get selected folder" part it works. It seems when this part runs it can't get the needed information, but why?

I believe the menu bar is actually formed in the hidden window - on Mac this is a window that's there all the time in the background but you never see any UI for it. So when you minimise Thunderbird or have no other Thunderbird windows open, the hidden window is there and gives you access to the menu bar.

When I add accountUtils.js and folderPane.js it goes further but is now stuck on "accountManager is undefined".

The problem with adding those scripts, is that you're then trying to run them in the hidden window. The hidden window doesn't have most of the chrome UI and js that the main window has. So for example there is no folder pane to find the selected folder.

If let mainWindow = Services.wm.getMostRecentBrowserWindow(); returns a window (vs undefined) that window will be the most recently touched main window by the user.

mainWindow.focus();
mainWindow.MsgAccountManager();

This should then focus it & open the account manager - you might need to check if it un-minimises as well, I think it will.

(note: getMostRecentBrowserWindow() is a shorthand for getMostRecentWindow("mail:3pane");.

If mainWindow doesn't exist, then we need to open one. Based on the combination of those two code snippets, I think this would work:

  window.openDialog(
    "chrome://messenger/content/messenger.xhtml",
    "_blank",
    "chrome,dialog=no,all",
    null,
    {
      tabType: "contentTab",
      tabParams: { url: "about:accountsettings" },
    }
  );

Comment 24

[Mark Banner (:standard8)]

Richard, I've just posted a patch on bug 1758360 to fix opening of the account settings menu when no other window is open. This also fixes it for when this patch displays it in the application menu. It can land independently of this bug, since it fixes its own issue, hence I separated it out.

Do you think you could drive the m-c patch in? My only comment is that I think Account Settings should be below the Settings option in the menu.

Comment 25

[Richard Marti (:Paenglab)]

(In reply to Mark Banner (:standard8) from comment #24)

Do you think you could drive the m-c patch in? My only comment is that I think Account Settings should be below the Settings option in the menu.

I can do this. With your patch there is probably only a change in messengercompose.xhtml needed. But then we have an empty Tools menu in composer (if I remember correctly when I built this last time. I do normally artifact builds because full builds are too slow on my Mac).

Alex, would it be okay when this lands on M-C and we have an entry in Thunderbird and Tools menu in composer after the M-C check-in?
We now have the AM entry everywhere above the Settings entry. Should we move it below in the Thunderbird menu only like Mark proposes?

Comment 26

[Alessandro Castellani [:aleca]]

My only comment is that I think Account Settings should be below the Settings option in the menu

When you say "option menu" do you mean the macOS menu that opens up when you click on "Thunderbird", or do you mean the main app menu?
If it's in the Thunderbird Menu, I think it makes sense.

Comment 27

[Richard Marti (:Paenglab)]

Attachment: Bug 308555 - On Mac move the "Account Settings" menuitem to the Thunderbird menu. r=mstange

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6a476e4ef2d
On Mac move the "Account Settings" menuitem to the Thunderbird menu. r=mstange

Comment 29

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/f6a476e4ef2d

Comment 30

[Richard Marti (:Paenglab)]

Attachment: Bug 308555 - Hide the Account Settings in composer Tools menu. r=aleca

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3a8886dac372
Hide the Account Settings in composer Tools menu. r=aleca