656045 - Attachment bar actions should be disabled if only a text/x-moz-deleted attachment is present

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[rsx11m]

A minor follow-up to bug 282068, as extended by bug 646032. Take a message with an attachment and delete that attachment, then revisit the message. It will show up with "1 attachment: Deleted: ..." as expected. Right-clicking on either the attachment in the bar itself or the button shows all options disabled, also as expected. However, clicking on the "Save" button will still allow you to save the (meaningless) text/x-moz-deleted part of the message.

For consistency, attachment link and action button should be disabled if no attachment is present for which an action could be performed.

This probably won't affect detached attachments given that the action is performed on the detached file instead.

Comment 1

[rsx11m]

On a side note, the menu items are disabled inconsistently in general:
 - take an e-mail with n attachments, delete one of them

 - right-click menu on the deleted attachment has all options disabled
 - right-click menu on non-deleted attachment has all options enabled
 - right-click menu on non-selected area has "Open All" enabled only

 - button in attachment bar shows "Save All" but has "Open All" enabled only
   (yet executes "Save All" on click based on description above)

 - File > Attachments allows both "Open All" and "Save All"

Comment 2

[Jim Porter (:squib)]

The problems here are that 1) "Open all" is never disabled, 2) "Save all" is never disabled in the File -> Attachments menu, and 3) "save"/"save all" are never disabled in the toolbar.

It should be possible to simply ignore deleted attachments for opening and saving, which I think we should (probably?) do, since we shouldn't restrict the user from doing things unless we have no good way to do it.

I'm not sure what this means for "detach all"/"delete all" when dealing with a message with some detached/deleted attachments. I could enable those too without much effort.

Comment 3

[rsx11m]

The "All" options should probably only be made available if they provide a useful action. Thus, if 7 attachments are "valid" and 1 is "deleted", the action should work safely on the 7 attachments present and skip the deleted one(s).

Comment 4

[Jim Porter (:squib)]

Attachment: Fix this and add tests

This enables the menu items if and only if there is at least one attachment that can accept that action. I also cleaned up some of the code in msgHdrViewOverlay.js, since it was a mess.

The tests check for all the menu states (except for the File -> Attachments menu, since Mozmill can't control Mac menus), but don't actually try to perform the actions, since most of them are hard to control due to using OS dialogs.

Comment 5

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 531527 [details] [diff] [review]
Fix this and add tests

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

So, when I try to run the tests, I get
mozmill.setTest :: {u'name': u'test_detached_attachment', u'filename': u'/Users/bwinton/Programming/thunderbird/src-central/mail/test/mozmill/attachment/test-attachment-menus.js'}
++DOMWINDOW == 20 (0x128c14088) [serial = 24] [outer = 0x120a82580]
error: uncaptured python exception, closing channel <jsbridge.network.BackChannel connected 127.0.0.1:24242 at 0x101239fc8> (<type 'exceptions.KeyError'>:'message' [/usr/local/Cellar/python/2.7.1/lib/python2.7/asyncore.py|readwrite|104] [/usr/local/Cellar/python/2.7.1/lib/python2.7/asyncore.py|handle_read_event|438] [/usr/local/Cellar/python/2.7.1/lib/python2.7/site-packages/jsbridge/network.py|handle_read|93] [/usr/local/Cellar/python/2.7.1/lib/python2.7/site-packages/jsbridge/network.py|process_read|254] [/usr/local/Cellar/python/2.7.1/lib/python2.7/site-packages/jsbridge/network.py|fire_callbacks|269] [/usr/local/Cellar/python/2.7.1/lib/python2.7/site-packages/jsbridge/network.py|fire_event|290] [/usr/local/Cellar/python/2.7.1/lib/python2.7/site-packages/mozmill/__init__.py|__call__|104] [runtest.py|logFailure|365])
[snip…]
endRunner was never called. There must have been a failure in the framework.

So, uh, that's bad.  I've kicked off a try-server build, but it looks like it ran into another problem.  Anyways, you can find it at http://arbpl.visophyte.org/?tree=ThunderbirdTry&pushid=688 and if you don't see the same problem I saw, maybe ask someone to re-run that build to try and get the error…

Other than that, all the things I mention are pretty minor, but the test failure means I've gotta give it an r-.

Thanks,
Blake.

::: mail/base/content/msgHdrViewOverlay.js
@@ +2267,5 @@
> +    {
> +      // Exclude all attachments already deleted
> +      if (attachment.contentType != "text/x-moz-deleted")
> +      {
> +        attachmentContentTypeArray[actionIndex] = attachment.contentType;

I think I would prefer to read this as:
if (attachment.contentType == "text/x-moz-deleted")
  continue;

Also, I notice that we use that string a whole lot.  I wonder if it makes sense to have a define for it somewhere…

::: mail/test/mozmill/attachment/test-attachment-menus.js
@@ +10,5 @@
> + * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
> + * for the specific language governing rights and limitations under the
> + * License.
> + *
> + * The Original Code is mozilla.org code.

"The Original Code is Thunderbird test code."

@@ +13,5 @@
> + *
> + * The Original Code is mozilla.org code.
> + *
> + * The Initial Developer of the Original Code is
> + * Mozilla Messaging.

"The Mozilla Foundation."

@@ +34,5 @@
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +var MODULE_NAME = 'test-attachment-size';

ITYM "test-attachment-menus".  :)

@@ +60,5 @@
> +const missingName = './nonexistent.txt';
> +const deletedName = 'deleted.txt';
> +
> +// create some messages that have various types of attachments
> +var messages = [

How similar are these messages between test-attachment-size and test-attachment-menus?

If they're very similar, perhaps we can move them somewhere common?

@@ +149,5 @@
> +  wh.installInto(module);
> +  let ah = collector.getModule('attachment-helpers');
> +  ah.installInto(module);
> +
> +  messenger = Components.classes['@mozilla.org/messenger;1']

A bunch of this code also looks _really_ similar, and should perhaps go live in attachment-helpers…

::: mail/test/mozmill/shared-modules/test-attachment-helpers.js
@@ +88,5 @@
> +  };
> +}
> +
> +function help_create_detached_deleted_attachment(filename, type) {
> +  return "You deleted an attachment from this message. The original MIME " +

Shouldn't that be "You detached or deleted an attachment…"?

Comment 6

[Jim Porter (:squib)]

So, first, I think I'm going to change the logic here, since trying to delete/detach a bunch of attachments when one of them is detached w/ a missing file will make the detach/delete code bail out without doing anything. I could:

1) Just disable the "delete" and "detach" options when there are
   deleted/detached attachments already (this is the old way)
2) Check for missing external files and remove that attachment from the list
   of selected attachments when operating on them
3) Try to fix the underlying C++ code for deleteAttachments to make it handle
   that error case better

What do you think?

(In reply to comment #5)
> Comment on attachment 531527 [details] [diff] [review] [review]
> Fix this and add tests
> 
> Review of attachment 531527 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> So, when I try to run the tests, I get
[snip]
> endRunner was never called. There must have been a failure in the framework.
> 
> So, uh, that's bad.  I've kicked off a try-server build, but it looks like
> it ran into another problem.  Anyways, you can find it at
> http://arbpl.visophyte.org/?tree=ThunderbirdTry&pushid=688 and if you don't
> see the same problem I saw, maybe ask someone to re-run that build to try
> and get the error…

I saw some error like that before, but I wasn't sure what the issue was. It seemed to be getting angry whenever an exception was thrown and was refusing to log it. I'll look at it again...

> ::: mail/base/content/msgHdrViewOverlay.js
> @@ +2267,5 @@
> > +    {
> > +      // Exclude all attachments already deleted
> > +      if (attachment.contentType != "text/x-moz-deleted")
> > +      {
> > +        attachmentContentTypeArray[actionIndex] = attachment.contentType;
> 
> I think I would prefer to read this as:
> if (attachment.contentType == "text/x-moz-deleted")
>   continue;

Seems like a good idea. I think I had just gone with the way it was originally...

> Also, I notice that we use that string a whole lot.  I wonder if it makes
> sense to have a define for it somewhere…

One step ahead of you. :) In my latest patch for bug 630759 (which I don't think I've uploaded yet), I added an isDeleted property to the attachment object, among other things. Depending on how you want to handle it, I could either keep it in that bug, put it in this bug, or make a new bug to reorganize the attachment object a bit.

> ::: mail/test/mozmill/attachment/test-attachment-menus.js
> @@ +10,5 @@
> > + * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
> > + * for the specific language governing rights and limitations under the
> > + * License.
> > + *
> > + * The Original Code is mozilla.org code.
> 
> "The Original Code is Thunderbird test code."
> 
> @@ +13,5 @@
> > + *
> > + * The Original Code is mozilla.org code.
> > + *
> > + * The Initial Developer of the Original Code is
> > + * Mozilla Messaging.
> 
> "The Mozilla Foundation."
> 
> @@ +34,5 @@
> > + * the terms of any one of the MPL, the GPL or the LGPL.
> > + *
> > + * ***** END LICENSE BLOCK ***** */
> > +
> > +var MODULE_NAME = 'test-attachment-size';
> 
> ITYM "test-attachment-menus".  :)

Noted. :) These should all be easy enough to fix.

> @@ +60,5 @@
> > +const missingName = './nonexistent.txt';
> > +const deletedName = 'deleted.txt';
> > +
> > +// create some messages that have various types of attachments
> > +var messages = [
> 
> How similar are these messages between test-attachment-size and
> test-attachment-menus?
> 
> If they're very similar, perhaps we can move them somewhere common?

About 50/50. There are quite a few different messages (e.g. test-attachment-size checks the size of base64-encoded attachments, and test-attachment-menus checks more combinations of deleted/detached attachments).

> @@ +149,5 @@
> > +  wh.installInto(module);
> > +  let ah = collector.getModule('attachment-helpers');
> > +  ah.installInto(module);
> > +
> > +  messenger = Components.classes['@mozilla.org/messenger;1']
> 
> A bunch of this code also looks _really_ similar, and should perhaps go live
> in attachment-helpers…

I'll see what I can move over...

> ::: mail/test/mozmill/shared-modules/test-attachment-helpers.js
> @@ +88,5 @@
> > +  };
> > +}
> > +
> > +function help_create_detached_deleted_attachment(filename, type) {
> > +  return "You deleted an attachment from this message. The original MIME " +
> 
> Shouldn't that be "You detached or deleted an attachment…"?

Possibly, but that's an exact copy of the text you get when you detach or delete an attachment in Thunderbird, so it's "correct" in the sense that it's what you'd get when you detach an attachment.

Comment 7

[Jim Porter (:squib)]

Attachment: Fix tests, disable on missing external file

This should fix most of the problems. I'm not sure there's an easy way to move any of the code into the attachment-helpers file, since it's all just different enough that I can't factor out much common stuff.

Comment 8

[Jim Porter (:squib)]

Attachment: Fix bitrot from bug 657856

This just fixes some bitrot and improves the code for detecting attachments with no files (i.e. deleted or detached w/ no external file).

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 533167 [details] [diff] [review]
Fix bitrot from bug 657856

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

So, I like it.  Thanks for the changes.  But…

I tried pushing to try again, and this time got build failures!  (http://arbpl.visophyte.org/?tree=ThunderbirdTry&pushid=693)  So I'm going to say r=me after you can post an arbpl link showing a clean run.  :)

Thanks,
Blake.

::: mail/base/content/msgHdrViewOverlay.js
@@ +2007,5 @@
>      let attachmentName  = document.getElementById("attachmentName");
>      let attachmentSize  = document.getElementById("attachmentSize");
>  
> +    let allDeleted = currentAttachments.every(function(attachment) {
> +      return attachment.contentType == "text/x-moz-deleted" ||

Since this is based on top of bug 657856, can we say "attachment.isDeleted" here instead?

@@ +2171,2 @@
>      {
>        if (!gMessengerBundle)

I think the:
if (!item)
  continue;
might make this function easier to read…

::: mail/test/mozmill/shared-modules/test-attachment-helpers.js
@@ +106,5 @@
> + * @return a string representing the attachment
> + */
> +function create_detached_attachment(file, type) {
> +  let fileHandler = Cc["@mozilla.org/network/io-service;1"]
> +                      .getService(Ci.nsIIOService)

Can we use Services.io here?  Cause that would be really nice…

Comment 10

[Jim Porter (:squib)]

I kicked off a try build here: http://arbpl.visophyte.org/?tree=ThunderbirdTry&pushid=700

Comment 11

[Jim Porter (:squib)]

It seems like the try build succeeded, but I'm not sure about the lack of Linux tests running or the exception from Mac OS X 10.6, so I'll wait to make sure bwinton is happy before checking anything in. :)

Comment 12

[Blake Winton (:bwinton) (:☕️)]

I think I want to see successful Linux tests and Mac OS X tests, but bug me about it tomorrow, and I'll try again on my computers.  :(

Thanks,
Blake.

Comment 13

[Mark Banner (:standard8)]

Linux try server builders were stuck, they should hopefully be unstuck now.

Comment 14

[Blake Winton (:bwinton) (:☕️)]

I kicked off a try build here: http://arbpl.visophyte.org/?tree=ThunderbirdTry&pushid=714  We'll see how this one goes…  :)

Later,
Blake.

Comment 15

[rsx11m]

Testing try-server build Linux x86_64 Thunderbird/7.0a1, this works fine in the single-attachment case when it's deleted. However, having a message with five attachments, only one of which is deleted, I still see "Save All" active and clickable as button option whereas all menu items are disabled. When saving it also saves the "Deleted:" attachment, which is probably a separate issue. Only when deleting all attachments, the "Save All" button becomes disabled.

As for the File > Attachments menu, I don't see any attachment names present in this build, which appears to be a regression. The "All" options are available if at least one non-deleted attachment is present, so this corresponds with the status of the "Save All" button but is inconsistent with the drop-down menu.

No messages in the Error Console - sorry for keeping you busy...  ;-)

Comment 16

[rsx11m]

(In reply to comment #15)
> When saving it also saves the "Deleted:" attachment, which is probably a
> separate issue.

Oops, false alarm - that one is fixed, that was a previously saved file from earlier tests. I need to clean out my directory before testing these things...

Comment 17

[Jim Porter (:squib)]

Everything in comment 15 works fine for me.

Comment 18

[rsx11m]

It's the same profile that I've used for the initial testing with 3.3a4pre...
Blake, can you reproduce my observations, maybe it's on Linux only?

Comment 19

[rsx11m]

Nope, I see exactly the same with the b5fa37b8f7c4/try-win32 build on Win7.
But then, I also see the missing File > Attachments items in today's nightly builds *without* your patch!

I'll check if anything has been filed on this issue already, it sure may have contributed the other behavior I've observed in comment #15 as well. Did you use the try-server build for comment #17?

Comment 20

[Jim Porter (:squib)]

Attachment: Update patch

Pulling forward r+. This is just a minor change to fix the issues in comment 15, which I had actually written about a week ago and then promptly forgot to upload!

Comment 21

[rsx11m]

(In reply to comment #19)
> I'll check if anything has been filed on this issue already

That only regressed a few days ago, I've filed bug 661315 on this issue.

Comment 22

[Jim Porter (:squib)]

Try build results: <http://arbpl.visophyte.org/?tree=ThunderbirdTry&pushid=715>. Can't get much greener than that! :)

Looking over this again, I missed your last comment in comment 9 (using Services.io), but that's fixed locally. I fixed it locally; I trust that's simple enough that you don't need to review it. :)

Comment 23

[Blake Winton (:bwinton) (:☕️)]

Huzzah!

Yeah, I trust you can use Services.io without me looking over your shoulder.  :)

Time for checkin?

Later,
Blake.

Comment 24

[Jim Porter (:squib)]

Checked in: http://hg.mozilla.org/comm-central/rev/73546bde038c

Comment 25

[Jim Porter (:squib)]

Reopening, as there are a couple issues with this:

1) The "delete" button is grayed out in newsgroups (it used to be hidden). This
   may or may not be a good thing.
2) The "junk" button isn't properly hidden in .eml messages. Its opacity is just
   set to zero, so it's invisible but takes up space.

Comment 26

[Jim Porter (:squib)]

Attachment: Fix fallout from previous patch

Here's a fix for the problems in comment 25. No tests at the moment, since this really just reverts the behavior to the old way, but I could add tests if they seem important.

Comment 27

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 541953 [details] [diff] [review]
Fix fallout from previous patch

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

> 1) The "delete" button is grayed out in newsgroups (it used to be hidden). This may or may not be a good thing.

I'm not a fan of grayed out buttons.  It's like a tease…  Also, I think I can use the delete key in newsgroups to do something, if not delete messages.

> 2) The "junk" button isn't properly hidden in .eml messages. Its opacity is just set to zero, so it's invisible but takes up space.

Ugh.

On the plus side, the fixes seem reasonable to me, so r=me!  :)

Thanks,
Blake.

Comment 28

[Jim Porter (:squib)]

Comment on attachment 541953 [details] [diff] [review]
Fix fallout from previous patch

This should probably get into 7.0, since it will cause problems (see comment 25).

Comment 29

[Jim Porter (:squib)]

Checked in: http://hg.mozilla.org/comm-central/rev/8d55529ca563

Comment 30

[Mark Banner (:standard8)]

Checked into aurora: http://hg.mozilla.org/releases/comm-aurora/rev/43b778218bea