Closed Bug 92846 Opened 20 years ago Closed 6 years ago

Disable File | Subscribe menu when only POP account is available

Categories

(SeaMonkey :: MailNews: Message Display, defect)

defect
Not set
minor

Tracking

(seamonkey2.40 affected, seamonkey2.41 affected, seamonkey2.42 affected)

RESOLVED FIXED
seamonkey2.42
Tracking Status
seamonkey2.40 --- affected
seamonkey2.41 --- affected
seamonkey2.42 --- affected

People

(Reporter: ji, Assigned: aceman)

References

Details

Attachments

(2 files, 3 obsolete files)

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
accepting.
Status: NEW → ASSIGNED
Blocks: 236667
Product: Browser → Seamonkey
Assignee: sspitzer → mail
Status: ASSIGNED → NEW
Component: MailNews: Subscribe → MailNews: Message Display
QA Contact: stephend → search
Assignee: mail → nobody
QA Contact: search → message-display
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/
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → EXPIRED
Bulk reopening incorrectly expired bugs - no activity does not constitute no bug - these need proper checking.
Status: RESOLVED → REOPENED
Resolution: EXPIRED → ---
Status: REOPENED → UNCONFIRMED
Confirming: Still an issue with current trunk (SM 2.1b2pre). Note that we also have Feed accounts now, for which File|Subscribe works, too.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Severity: normal → minor
Attached patch patch for TB (obsolete) — Splinter Review
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.
Attachment #8498455 - Flags: review?(mkmelin+mozilla)
Attachment #8498455 - Flags: feedback?(iann_bugzilla)
Comment on attachment 8498455 [details] [diff] [review]
patch for TB

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

LGTM, r=mkmelin
Attachment #8498455 - Flags: review?(mkmelin+mozilla) → review+
The patch seems to still apply. Let's get it landed so it is not lost.
Keywords: checkin-needed
Whiteboard: [keep open for SM part]
Assignee: nobody → acelists
Status: NEW → ASSIGNED
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.
(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.
Flags: needinfo?(rsx11m.pub)
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.
Flags: needinfo?(rsx11m.pub)
Attached patch patch for SM (obsolete) — Splinter Review
Attachment #8672328 - Flags: feedback?(rsx11m.pub)
(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 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)) {
Flags: needinfo?(acelists)
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 ;)
Flags: needinfo?(acelists)
Attached patch patch for SM v1.1 (obsolete) — Splinter Review
Attachment #8672328 - Attachment is obsolete: true
Attachment #8672328 - Flags: feedback?(rsx11m.pub)
Attachment #8674447 - Flags: feedback?(rsx11m.pub)
Attachment #8498455 - Attachment is obsolete: true
Attachment #8498455 - Flags: feedback?(iann_bugzilla)
Attachment #8674448 - Flags: review+
Keywords: checkin-needed
Whiteboard: [keep open for SM part]
Comment on attachment 8674447 [details] [diff] [review]
patch for SM v1.1

Ping feedback request?
Flags: needinfo?(rsx11m.pub)
Attachment #8674447 - Flags: review?(iann_bugzilla)
Sorry, I've been rather busy in real life, but hope to get to it soon.
Flags: needinfo?(rsx11m.pub)
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.
Attachment #8674447 - Flags: review?(iann_bugzilla) → review+
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.
Attachment #8674447 - Flags: feedback?(rsx11m.pub) → feedback+
Setting status flags as SeaMonkey probably want this on the branches during the upcoming cycle.
Fixed problems from comment 19, thanks.
Attachment #8674447 - Attachment is obsolete: true
Attachment #8687643 - Flags: review+
Keywords: checkin-needed
Comment on attachment 8687643 [details] [diff] [review]
patch for SM v1.2

a=Ratty for SeaMonkey CLOSED TREE
https://hg.mozilla.org/comm-central/rev/075959e1c604
https://hg.mozilla.org/comm-central/rev/f99e915e6dac
Status: ASSIGNED → RESOLVED
Closed: 12 years ago6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.42
You need to log in before you can comment on or make changes to this bug.