Closed Bug 352696 Opened 13 years ago Closed 12 years ago

Ambiguous meaning of File->Attachments->Delete All

Categories

(SeaMonkey :: MailNews: Message Display, defect, trivial)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mrmazda, Assigned: bugzilla)

References

Details

Attachments

(3 files, 4 obsolete files)

Select some message that has at least one attachment. Then open

     File -> Attachments

to see among other choices

     Delete All

Since this is the File menu and not the Message menu or a context menu, I can think of 5 things it could mean:

1-delete all attachments from selected message
2-delete all attachments from messages in the folder containing this message
3-delete all attachments from messages in the account containing this message
4-delete all attachments from messages in the profile containing this message
5-delete all attachments from messages in profiles contained in $MOZILLA_HOME

The ambiguity needs to be removed. My personal opinion is the context menu meaning would clearly be #1 above, but my goal with the file menu here is actually #2.
Since the attachments of the chosen message are shown above the "... all" commands, I think it's clear, that Delete All refers to the selected message.

Your second command to delete all messages from all attachments in a folder should probably belong to the context menu of that folder.

Bruno
Attached image screenshot
'attachments of the chosen message are shown above the "... all" commands'

I have no idea what the above refers to.

"Since this is the File menu and not the Message menu or a context menu..."

What that means is that if the selections were in the message menu or a context menu instead of the file menu it would be clear to me that they apply only to the attachments on the currently selected message. My desire is to perform an action upon a message or messages, not upon a file or files, and thus the ambiguous meaning from the file menu placement.
(In reply to comment #2)
> I have no idea what the above refers to.
I wanted to say, that you can see on the basis of the attachement names, that the menu belongs to the selected message in the message pane.

But I think you're right, File is probably the wrong place for the attachement list. Moving it to the Messages menu would be much clearer.
Severity: normal → trivial
(In reply to comment #3)
> (In reply to comment #2)
> > I have no idea what the above refers to.
> I wanted to say, that you can see on the basis of the attachement names, that
> the menu belongs to the selected message in the message pane.
 
I still don't see what you mean. How is there any association between attachment names and a file menu? When I get a message with attachments from someone, I only know the attachments have associated names, not that those names have or had anything to do with any files. To me they are nothing but special parts of the message.
Attached patch Patch_v1 (obsolete) — Splinter Review
Patch for the change seen in https://bugzilla.mozilla.org/attachment.cgi?id=244881
Attachment #244882 - Flags: superreview?
Attachment #244882 - Flags: review?
Attachment #244882 - Flags: superreview?(neil)
Attachment #244882 - Flags: superreview?
Attachment #244882 - Flags: review?(neil)
Attachment #244882 - Flags: review?
Comment on attachment 244882 [details] [diff] [review]
Patch_v1

Let's see what Mnyromyr thinks but you should probably give the menuitem a new id and I think the persist="hidden" might be bogus and could be removed.
Attachment #244882 - Flags: review?(neil) → review?(mnyromyr)
Comment on attachment 244882 [details] [diff] [review]
Patch_v1

>Index: mailWindowOverlay.xul
>===================================================================
>    <menuseparator/>
>+   <menu id="fileAttachmentMenu" label="&openAttachmentCmd.label;" accesskey="&openAttachmentCmd.accesskey;" disabled="true" persist="hidden">
>+      <menupopup id="attachmentMenuList" onpopupshowing="FillAttachmentListPopup(this);"/>
>+   </menu>
>+   <menuseparator/>

While I don't think that renaming the menu is necessary (we'd just move the entire menu, its functionality won't change), moving it like this way will result in doubled menuseparators when hiding fileAttachmentMenu due to a hidden message pane (eg by F8; see ll. <http://lxr.mozilla.org/mozilla/source/mailnews/base/resources/content/commandglue.js#767>).

OTOH, why is this item hidden in that special case anyway? It's even visible (though disabled) when showing AccountCentral! Just make the commandglue code _disable_ the menu - and get rid of the persist attribute then...

(On a similar scale: Save As would still be under File, even though it'd fit better into Message as well - but it's "tradionally" there... And we really should have a Folder menubar item, too.)
Attachment #244882 - Flags: review?(mnyromyr) → review-
Comment on attachment 244882 [details] [diff] [review]
Patch_v1

Actually we'd have to rename the menuitem to get rid of the persist attribute to avoid it persisting on to the new menuitem.
Attachment #244882 - Flags: superreview?(neil)
Addressing issues from previous comments. The attachments menu gets disabled now when the message pane is hidden. I used setAttribute and removeAttribute in commandglue.js because the attachments menu gets disabled that way in msgHdrViewOverlay.js.

Another problem is, that the access key 'a' is used for Attachments and Create Filter From Message now. Would 't' be a good access key for the attachments menu?
Attachment #244882 - Attachment is obsolete: true
Attachment #245334 - Flags: review?(mnyromyr)
Attached patch v2 with new AccessKey (obsolete) — Splinter Review
The same as v2 with a changed access key ('h' instead of 'A')
Attachment #245334 - Attachment is obsolete: true
Attachment #245496 - Flags: review?(mnyromyr)
Attachment #245334 - Flags: review?(mnyromyr)
Comment on attachment 245496 [details] [diff] [review]
v2 with new AccessKey

Looks good on Windows, although I had quite some difficulties to test it on Mac - but that's bug 327982 and not your fault.
Attachment #245496 - Flags: review?(mnyromyr) → review+
Attachment #245496 - Flags: superreview?(neil)
Comment on attachment 245496 [details] [diff] [review]
v2 with new AccessKey

While testing this I found two bugs.

The bug caused by this patch is that if there is no loaded message and you show the message pane then the attachments menu is always enabled. Bug 121176 happens to make it work when there is a loaded message.

I discovered another bug 121176 bug by pressing Ctrl+A, F8, Ctrl+A. This means I might have to back out bug 121176 which would completely stop this patch from working.
Attachment #245496 - Flags: superreview?(neil) → superreview-
(In reply to comment #13)
> This means I might have to back out bug 121176 which would completely stop
> this patch from working.

How would that be?  If you back out 121176 now without this patch, does the current File|Attachments submenu stop enabling/disabling correctly?  Or would it only break the code that formerly hid the Attachments submenu for the case of No Message Pane?  (xref bug 88633 comment 9 for the hide-vs-disable issue)
(In reply to comment #14)
>How would that be?
This patch relies on bug 121176's behaviour change of forcing a reload when you show the message pane to update the attachment submenu correctly (except as noted it unconditionally enables the menu when it could let the reload do it).
(In reply to comment #13)
>I discovered another bug 121176 bug by pressing Ctrl+A, F8, Ctrl+A.
Oops, I meant F8, Ctrl+A, F8; but this didn't work in SeaMonkey 1.0b either.
(In reply to comment #13)
> (From update of attachment 245496 [details] [diff] [review] [edit])
> While testing this I found two bugs.
> 
> The bug caused by this patch is that if there is no loaded message and you 
> show the message pane then the attachments menu is always enabled. Bug 121176
> happens to make it work when there is a loaded message.
This is fixed, if I don't enable the attachments menu "by hand".

> F8, Ctrl+A, F8;
I have not found a solution for that one so far. The first message always get loaded after enabling the message pane. Should this get addressed in this bug or in a separate one?
I had an idea for fixing the "enable message pane with multiple messages selected" issue, but it's too late for a proper testing of it. Will attach it probably tomorrow.
Status: NEW → ASSIGNED
Assignee: mail → aqualon
Status: ASSIGNED → NEW
Depends on: 362349
The attachment menu only gets disabled by ChangeMessagePaneVisibility(). Enabling it is done by the reload of the message triggered by setting gDBView.suppressMsgDisplay to true.

The issue found by Neil in #c13 will be fixed by the patch for bug 362349
Attachment #245496 - Attachment is obsolete: true
Attachment #247057 - Flags: superreview?(neil)
Attachment #247057 - Flags: review?(mnyromyr)
Comment on attachment 247057 [details] [diff] [review]
Let the message reload enable the attachments menu

>Index: mailnews/base/resources/content/commandglue.js
>===================================================================
> function ChangeMessagePaneVisibility(now_hidden)
> {
>-  // we also have to hide the File/Attachments menuitem
>-  var node = document.getElementById("fileAttachmentMenu");
>-  if (node)
>-    node.hidden = now_hidden;
>+  // we also have to disable the Message/Attachments menuitem
>+  var node = document.getElementById("msgAttachmentMenu");
>+  if (node && now_hidden)
>+    node.setAttribute("disabled", "true");

You should add a comment here which explains why the removal of the disabled state doesn't happen here.

r=me with that.
Attachment #247057 - Flags: review?(mnyromyr) → review+
Comment on attachment 247057 [details] [diff] [review]
Let the message reload enable the attachments menu

Patch is partly bitrotted, will post a new one next week.
Attachment #247057 - Attachment is obsolete: true
Attachment #247057 - Flags: superreview?(neil)
Attached patch Patch v5Splinter Review
Unbitrotted patch (messenger.dtd resides in suite nowadays) with explaining comment for the change in ChangeMessagePaneVisibility (commandglue.js).
Attachment #273440 - Flags: superreview?(neil)
Attachment #273440 - Flags: review?(mnyromyr)
(In reply to comment #8)
>OTOH, why is this item hidden in that special case anyway?
Originally the message pane could have been in four states:
1. Blank or mail start page
2. Message loaded
3. Message loaded, then message pane collapsed
4. Message pane collapsed, then other message selected
Pressing F8 would take you from state 4 to state 2 by loading the message but from state 3 to state 2 load was optimised away so there was no indication as to whether the attachments menu was valid or not. Hiding the menu rather than disabling it worked around the issue as moving from state 3 to state 2 would simply show the attachments menu that had just been hidden.

After bug 121176 we always load the message after the message pane is shown so that the title bar can be updated correctly when appropriate.
Attachment #273440 - Flags: superreview?(neil) → superreview+
Attachment #273440 - Flags: review?(mnyromyr) → review+
Landed on trunk.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.