92846 - Disable File | Subscribe menu when only POP account is available

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[ji]

With the latest NS6.1 build, File | Subscribe menu is not disabled when there is
only POP account is available. In this case, selecting File | Subscribe menu
will bring up an unusable subscribe window with no accounts and no folders on
it. Since subscrible is for IMAP and news account, we should disable subscrible
menu in this case.

Steps to reproduce:
1. Launch mail with a new profile.
2. Set up a POP account with this profile.
3. On Mail window, select File | Subscribe

Comment 1

[(not reading, please use [e-mail] instead)]

accepting.

Comment 2

[Arun Prasannan (polar)]

This bug is being marked EXPIRED as it has seen no activity in a very long time.

If you think that the issue reported might still be relevant, please test with a recent release of SeaMonkey and if the problem persists feel free to re-open the report. Thank you.

http://www.seamonkey-project.org/

Comment 3

[Mark Banner (:standard8)]

Bulk reopening incorrectly expired bugs - no activity does not constitute no bug - these need proper checking.

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

Confirming: Still an issue with current trunk (SM 2.1b2pre). Note that we also have Feed accounts now, for which File|Subscribe works, too.

Comment 5

[:aceman]

Attachment: patch for TB

This is a version of the fix for Thunderbird.
As this bug is for SM, maybe you'd like to adapt it to SM too.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 8498455 [details] [diff] [review]
patch for TB

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

LGTM, r=mkmelin

Comment 7

[:aceman]

The patch seems to still apply. Let's get it landed so it is not lost.

Comment 8

[:aceman]

rsx11m, I did not assign this to me as I do not intend to do the Seamonkey version as I can't test it. But if you do, I can prepare it.

Comment 9

[:aceman]

(In reply to :aceman from comment #8)
> rsx11m, I did not assign this to me as I do not intend to do the Seamonkey
> version as I can't test it. But if you do, I can prepare it.

I mean whether you can test it if I prepare the patch.

Comment 10

[rsx11m]

Sure, I can test the SM version of the patch. Just ask me for feedback on it, though I don't have a "real" POP account (but I can set one up to verify if the menu item is hidden).

And sorry, I've missed your comment #8.

Comment 11

[:aceman]

Attachment: patch for SM

Comment 12

[:aceman]

(In reply to rsx11m from comment #10)
> Sure, I can test the SM version of the patch. Just ask me for feedback on
> it, though I don't have a "real" POP account (but I can set one up to verify
> if the menu item is hidden).

Actually this patch is not about the context menu on account/folder, but about the main menu item. It should get disabled if there are no IMAP/News accounts existing and you also have not a RSS account selected.

Comment 13

[Philip Chee]

Comment on attachment 8672328 [details] [diff] [review]
patch for SM

> +  let servers = MailServices.accounts.allServers;
> +  for each (let server in fixIterator(servers,
> +                                      Components.interfaces.nsIMsgIncomingServer)) {
1. We aren't supposed to use for-each any more. Please use for-off instead.
2. Ci shortcut isn't available here. Use Components.interfaces.nsIMsgIncomingServer instead.

> for (let server of fixIterator(MailServices.accounts.allServers,
>                                Components.interfaces.nsIMsgIncomingServer)) {

Comment 14

[:aceman]

I do not see Ci. being used anywhere in the patch... Thanks for the for..of hint, support for that (with fixIterator) was only added recently so I am not used to it ;)

Comment 15

[:aceman]

Attachment: patch for SM v1.1

Comment 16

[:aceman]

Attachment: patch for TB v1.1

Comment 17

[Philip Chee]

Comment on attachment 8674447 [details] [diff] [review]
patch for SM v1.1

Ping feedback request?

Comment 18

[rsx11m]

Sorry, I've been rather busy in real life, but hope to get to it soon.

Comment 19

[Ian Neal]

Comment on attachment 8674447 [details] [diff] [review]
patch for SM v1.1

>+++ b/suite/mailnews/mail3PaneWindowCommands.js
>@@ -150,16 +150,17 @@ var DefaultController =
>       case "cmd_viewIgnoredThreads":
>       case "cmd_stop":
>       case "cmd_undo":
>       case "cmd_redo":
> 			case "cmd_expandAllThreads":
> 			case "cmd_collapseAllThreads":
> 			case "cmd_renameFolder":
> 			case "cmd_sendUnsentMsgs":
>+			case "cmd_subscribe":
Please line up with case "cmd_redo" and case "button_print" (no tabs)
> 			case "cmd_openMessage":
>       case "button_print":

>@@ -589,16 +592,19 @@ var DefaultController =

>+			case "cmd_subscribe":
>+				MsgSubscribe();
>+				return;
Please line up with/indent like case "cmd_printSetup" (no tabs)
> 			case "cmd_openMessage":
>                 MsgOpenSelectedMessages();
> 				return;
>             case "cmd_printSetup":
>                 PrintUtils.showPageSetup();
>                 return;

>@@ -844,16 +850,39 @@ function IsSendUnsentMsgsEnabled(folderR

>+/**
>+ * Determine whether there exists any server for which to show the Subscribe dialog.
>+ */
>+function IsSubscribeEnabled()
>+{
>+  // If there are any IMAP or News servers, we can show the dialog any time and
>+  // it will properly show those.
>+  let servers = MailServices.accounts.allServers;
SM doesn't use MailServices yet in this file, accountManager.allServers should work (see http://mxr.mozilla.org/comm-central/source/suite/mailnews/mail3PaneWindowCommands.js#1093)

r=me with those addressed.

Comment 20

[rsx11m]

Comment on attachment 8674447 [details] [diff] [review]
patch for SM v1.1

Works fine for me out of the box with a recent 2.41a1 nightly build. The File > Subscribe menuitem isn't visible with a new profile or after creating any number of POP accounts. It is only shown after creating a Newsgroup or IMAP account, or both. Removing those accounts subsequently disables the menuitem again.

>+++ b/suite/mailnews/mail3PaneWindowCommands.js
>+function IsSubscribeEnabled()
>+{
>+  // If there are any IMAP or News servers, we can show the dialog any time and
>+  // it will properly show those.
>+  let servers = MailServices.accounts.allServers;

I didn't need to apply the change suggested in comment #19, thus it appears that MailServices is defined or included somewhere/somehow already. I therefore didn't test

>-  let servers = MailServices.accounts.allServers;
>+  let servers = accountManager.allServers;

as an alternate patch.

Comment 21

[rsx11m]

Setting status flags as SeaMonkey probably want this on the branches during the upcoming cycle.

Comment 22

[:aceman]

Attachment: patch for SM v1.2

Fixed problems from comment 19, thanks.

Comment 23

[Philip Chee]

Comment on attachment 8687643 [details] [diff] [review]
patch for SM v1.2

a=Ratty for SeaMonkey CLOSED TREE

Comment 24

[:aceman]

https://hg.mozilla.org/comm-central/rev/075959e1c604
https://hg.mozilla.org/comm-central/rev/f99e915e6dac