479200 - Detach a single attachment uses a "detach all" dialog instead

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[David Ascher (:davida)]

If you select a single attachment and do detach for it using the context menu, you get a dialog that is a directory picker and says "detach all attachments".

This is because of http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMessenger.cpp#2745, wherein the DetachAttachment call delegates its dirty work to DetachAllAttachments passing in an array.  That's no good, as people a) would get confused, and b) aren't able to pick the file name on disk.

Instead, it should likely follow the SaveAs() code path, followed by a delete w/ no warning.

Comment 1

[David von Oheimb]

Good hint on a potential solution.
The bug still exists in TB 2.0.0.23.
This bug already has been reported in 
https://bugzilla.mozilla.org/show_bug.cgi?id=448576

Comment 4

[David von Oheimb]

Attachment: Proposed fix for bug 479200 regarding DetachAs

Hi guys, 

after waiting for a fix of this bug for about four years,
I've taken about two days to track down the relevant code 
sections and come up with a fix myself, see attached.

I've done some obvious basic testing on the fix (though I could 
not figure out how to invoke the automated tests mentioned in
https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch).

For consistency with "Save As...", I've renamed the context menu entry to "Detach As...". 

Note that the solution is limited to detaching single attachments.
Selecting multiple ones and then right-click "Detach As..." results in the 
previous behavior: one is asked for a directory name (where the attachments
are then stored with their original name) rather than for a (optionally new) file name.

Hope you like this.
  David

Comment 5

[David von Oheimb]

Attachment: Minor update of proposed fix for regarding DetachAs

I just noticed that there was a minor structural error in the patch file I recently supplied. This one does work with the latest version in the repository.

Here are some additional explanations on the bug fix: 
Following David Ascher's suggestion, I basically re-used the existing SaveAttachment method code in mailnews/base/src/nsMessenger.cpp, generalizing it slightly to a new method called SaveOneAttachment. This new method is now called by DetachAttachment (instead of the previously used dirty trick delegating to DetachAttachments(1,...))
I also had to adapt mail/base/content/msgHdrViewOverlay.js such that detaching a single attachment is handled differently (i.e. by the new method) than detaching a multiple selection of attachments. BTW, I believe that the latter case should be handled differently from "detach-all", and thus I've tentatively added the function savedetachAttachment, which is supposed to be called for each of the attachments in the selection, one-by-one. Yet since getting this to work seems more difficult, savedetachAttachment is currently unused and instead detachAllAttachments gets called for multiple selections of attachments as before (see my new comments in the code).
For consistency with SaveAs, I've changed the UI menu label to "Detach As...".

Comment 6

[Ludovic Hirlimann [:Usul]]

Comment on attachment 419423 [details] [diff] [review]
Proposed fix for bug 479200 regarding DetachAs

As you posted a new patch this one is obsolete. Removing requests.

Comment 7

[Ludovic Hirlimann [:Usul]]

Comment on attachment 419897 [details] [diff] [review]
Minor update of proposed fix for  regarding DetachAs

One reviewer is enough :-)

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 419897 [details] [diff] [review]
Minor update of proposed fix for  regarding DetachAs

Thanks for the patch!
Overall this looks good however I don't think the "Detach As..." is better than "Detach...".  The "..." implies there will be additional information required and I don't think the consistency is worth the change.  I'll ui-r+ this without the string change.

Comment 9

[David von Oheimb]

Thanks Bryan for your positive response. I do not know what ui-r+ stands for, but I assume that the patch is being promoted :-) 
Concerning the change of the menu entry to "Detach As..." which I proposed: I'd say that either the "As..." should be there for both "Save" and "Detach" or the "As" should be left out for both. Yet in order to avoid delaying the bug fix with UI discussions, I'm fine with any menu entry (including keeping the existing one).

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

ui-r+ stands for "UI-Review+'.  Any patches that change the user-experience in ways that aren't clearly bug-fixes require it in order to land.  The way to interpret comment 8 is that you could upload a new version of the patch with the fixes that Bryan suggested, you (or anyone else) can in good faith carry forward the ui-r+ flag to the new version of the patch.  

It looks to me as though Ludo has set the ui-r? as a way to ask Bryan to consider the argumentation you put forward in comment 9.

Separately from this, we've recently put into a place a policy that all patches need corresponding automated tests of the functionality that they're changing in order to land in the tree.  The idea here is for the project to become significantly more agile and of higher quality.  <https://developer.mozilla.org/en/Thunderbird/Thunderbird_Automated_Testing> is a starting point for more information about how that works...

Would you be willing to write a test or two here?  In this case, I suspect a MozMill UI test is the right way to go.  I and others are available to help you sort through the (as-yet-immature) documentation and any issues you may hit along the way...

Comment 11

[Ludovic Hirlimann [:Usul]]

(In reply to comment #10)

> Would you be willing to write a test or two here?  In this case, I suspect a
> MozMill UI test is the right way to go.  I and others are available to help you
> sort through the (as-yet-immature) documentation and any issues you may hit
> along the way...

The place to ask would either be on mozilla.dev.apps.thunderbird mailinglist/newsgroup or on irc on irc.mozilla.org in #maildev (irc is usually faster but sometimes it seems everybody is not around so just stay and wait for an answer don't just leave 3 seconds after asking the question)

Comment 12

[David von Oheimb]

Thanks Dan for your explanations on ui-r(+) and for your invitation (including the offer for help) for taking part in testing. 
Unfortunately, within the next couple of months I will not have the time to get familiar with Mozilla's automated testing to effectively contribute there. I hope someone else can test this patch maybe in conjunction with testing other patches.
I've just done some informal testing when developing the patch in late 2009 and all I can say is I have been using it since then without experiencing problems.

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 419897 [details] [diff] [review]
Minor update of proposed fix for  regarding DetachAs

Giving this the ui-r+ assuming we go with "Detach...".

I understand it seems inconsistent with "Save As..." but we also have "Delete..." which acts in a similar fashion.  Save As... is a well known UI option in many applications and Thunderbird has been using Detach... for a while now.  I just don't see a real user benefit to the change; consistency isn't always a user benefit.

Comment 14

[David von Oheimb]

Well, let's forget about the (very minor) UI change included in the proposed fix. The much more important thing is that the given fix for the broken Detach semantics finally gets (tested and) released.

Comment 15

[Magnus Melin [:mkmelin]]

Can you submit a patch with that change included?

Comment 16

[David von Oheimb]

Attachment: Same as earlier fix regarding DetachAs but without the (very minor) GUI adaptation

Ok guys, as requested, a new version of the fix. 
No changes were needed to make it compile with today's trunk version. I just removed the (very minor) GUI adaptation and adapted a couple of comments and one function name. 
Hope the fix will finally be included in the next release.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 433759 [details] [diff] [review]
Same as earlier fix regarding DetachAs but without the (very minor) GUI adaptation

Carrying fwd ui-r=clarkbw. (But you should still get review and superreview - please set the flags)

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 433759 [details] [diff] [review]
Same as earlier fix regarding DetachAs but without the (very minor) GUI adaptation

>diff -r 8b13001bc702 mail/base/content/msgHdrViewOverlay.js
>--- a/mail/base/content/msgHdrViewOverlay.js	Sat Mar 20 02:16:58 2010 -0700
>+++ b/mail/base/content/msgHdrViewOverlay.js	Sat Mar 20 21:26:55 2010 +0100
>@@ -1607,16 +1607,21 @@ function openAttachment(aAttachment)
> function detachAttachment(aAttachment, aSaveFirst)
> {
>   messenger.detachAttachment(aAttachment.contentType,
>                            aAttachment.url,
>                            encodeURIComponent(aAttachment.displayName),
>                            aAttachment.uri, aSaveFirst);
> }
> 
>+function detachOneAttachment(aAttachment)
>+{
>+  detachAttachment(aAttachment, true);
>+}

Adding this function here seems quite unnecessary. You can use a local function if you need later.

>       rv = messageService->OpenAttachment(PromiseFlatCString(aContentType).get(), PromiseFlatCString(aDisplayName).get(),
>+                                          //TODO: it seems that aDisplayName and aURL should be swapped - please check!

This comment seems wrong.
                                         
Besides that, there are a lot of lines with trailing whitespace. Also please limit the added comment lines to max 80 char.

Most worringly though, this makes Save All detach all!

Comment 19

[David von Oheimb]

Attachment: Minor revision of fix according to review

Magnus, thanks for the review.

>>+function detachOneAttachment(aAttachment)
>>+{
>>+  detachAttachment(aAttachment, true);
>>+}
> 
> Adding this function here seems quite unnecessary.
> You can use a local function if you need later.

I removed detachOneAttachment and now use an unnamed function.

>>       rv = messageService->OpenAttachment(PromiseFlatCString(aContentType).get(), PromiseFlatCString(aDisplayName).get(),
>>+                                          //TODO: it seems that aDisplayName and aURL should be swapped - please check!
> 
> This comment seems wrong.

Are you sure? Just have a look at the respective function definition 
in mailnews/base/public/nsIMsgMessageService.idl:
  void openAttachment(in string aContentType, 
            in string aFileName, 
            in string aUrl,
            in string aMessageUri, 
            in nsISupports aDisplayConsumer, 
            in nsIMsgWindow aMsgWindow,
            in nsIUrlListener aUrlListener);
This makes me pretty sure that my comment points out a bug
(which is unrelated to the given fix of Detach-As, though).

> Besides that, there are a lot of lines with trailing whitespace.
> Also please limit the added comment lines to max 80 char.

Done.

> Most worringly though, this makes Save All detach all!

No. The behavior of Save All is not affected by the fix.

Comment 20

[Magnus Melin [:mkmelin]]

(In reply to comment #19)
> >>       rv = messageService->OpenAttachment(PromiseFlatCString(aContentType).get(), PromiseFlatCString(aDisplayName).get(),
> >>+                                          //TODO: it seems that aDisplayName and aURL should be swapped - please check!
> > 
> > This comment seems wrong.
> 
> Are you sure? Just have a look at the respective function definition 
> in mailnews/base/public/nsIMsgMessageService.idl:
>   void openAttachment(in string aContentType, 
>             in string aFileName, 
>             in string aUrl,
>             in string aMessageUri, 

Exactly, 1st argument contentType, second argument a name (filename/displayname) in both places, no?

> 
> No. The behavior of Save All is not affected by the fix.

Right, not sure what i saw there.

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 434404 [details] [diff] [review]
Minor revision of fix according to review


>+     // Detach(As) on a multiple selection of attachments is so far not really
>+     // supported. Detaching the attachments one-by-one in the below loop,
>+     // even if works backwards through the attachments in order to avoid
>+     // invalidating the positions of the attachments before, does not work.
>+     // See also the comment on Detach(As) below.
>+     // As a workaround, resort to normal detach-'all'.

The sentences in this comment don't really add up. I'm not sure what it's trying to say, esp. the "even if...". Maybe just drop it except for the first sentence.
r=mkmelin with that

Comment 22

[David von Oheimb]

Attachment: Updated the patch by adapting the comments according to review by Magnus

According to the review by mkmelin (Magnus), I have rearranged my comment and also improved the sentence structure. I hope it is clear now.
I've checked again and now agree that my other comment "aDisplayName and aURL should be swapped" on the call of openAttachment was wrong, and thus have deleted it. I had confused openAttachment as contained in nsIMsgMessageService (which is the one relevant here) with the openAttachment in nsIMessenger (where for some reason the second and third argument are in different order).
I hope I've set the right flags, now asking for superreview (and ui-review, if still needed at all, because the UI is not changed anymore by this patch).

Comment 23

[Magnus Melin [:mkmelin]]

You should set the superreview flag to a specific person too, try standard8 or bienvenu perhaps?

Comment 24

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 443937 [details] [diff] [review]
Updated the patch by adapting the comments according to review by Magnus

I haven't tested this because it seems the UI changes were left out so I'll just give a plus to the bug fix. :)

Comment 25

[Mark Banner (:standard8)]

Comment on attachment 443937 [details] [diff] [review]
Updated the patch by adapting the comments according to review by Magnus

Sorry for the delay in looking at this. In general it looks good, however there's a few corrections and tweaks required.

> void
> nsMessenger::GetString(const nsString& aStringName, nsString& aValue)
...
>   if (!mStringBundle)
>     rv = InitStringBundle();
>-
>-  if (mStringBundle)
>+  else
>     rv = mStringBundle->GetStringFromName(aStringName.get(), getter_Copies(aValue));

This is an incorrect change. If mStringBundle isn't initalised, then GetStringFromName won't be called. Indeed the first time I opened the dialog when testing this patch I got "SaveAttachment" (the name of the string for the title of the dialog) rather than "Save Attachment" (the localised version of the name).

The rest of the changes are mainly style nits that I'd like to see addressed as well:


> nsMessenger::SaveAttachment(const nsACString& aContentType, const nsACString& aURL,
>                             const nsACString& aDisplayName, const nsACString& aMessageUri, PRBool aIsExternalAttachment)
> {
>   // open external attachments inside our message pane which in turn should trigger the
>   // helper app dialog...
>   if (aIsExternalAttachment)
>     return OpenURL(aURL);
>+  else
>+    return SaveOneAttachment(PromiseFlatCString(aContentType).get(),
>+                             PromiseFlatCString(aURL).get(),
>+                             PromiseFlatCString(aDisplayName).get(),
>+                             PromiseFlatCString(aMessageUri).get(),
>+                             PR_FALSE);

As you're returning in the first part of the if statement, there's no need for the else here it can just be:

if (aIsExternalAttachment)
  return OpenURL(aURL);

return SaveOneAttachment(...

>+  nsCString dirName;
>+  nsSaveAllAttachmentsState *saveState = nsnull;
>+  rv = localFile->GetNativePath(dirName);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  saveState = new nsSaveAllAttachmentsState(1,

You can move the declaration of saveState down to this line where you assign it. That saves the intermediate initialisation step.

The same applies to similar code later in the file.

>+  rv = SaveAttachment(localFile, nsDependentCString(aURL), nsDependentCString(aMessageUri),
>+                      nsDependentCString(aContentType), (void *)saveState, nsnull);
>   return rv;

Just "return SaveAttachment(..." will do, rather than assigning to rv.

>+  if (aSaveFirst)
>+    return SaveOneAttachment(aContentType, aUrl, aDisplayName, aMessageUri, PR_TRUE);
>+  else
>+    return DetachAttachments(1, &aContentType, &aUrl, &aDisplayName, &aMessageUri, nsnull, withoutWarning);

Again, drop the else.

Comment 26

[David von Oheimb]

Attachment: Adapted the patch according to superreview; should now be final

Thanks Mark for your review, in particular pointing out one mistake. 
I've done all the changes requested (also the two regarding "if ...  return else return ..." where I'd prefer a different style, but ok).

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 448962 [details] [diff] [review]
Adapted the patch according to superreview; should now be final

In general, an r- or sr- on a patch means that the reviewer would like to look at it again after the requested changes have been made.

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 448962 [details] [diff] [review]
Adapted the patch according to superreview; should now be final

That's looking good now. Thanks for the updated patch.

My only comment is that we don't need the removal of the line here:

>@@ -2044,17 +2069,16 @@ nsMessenger::InitStringBundle()
> void
> nsMessenger::GetString(const nsString& aStringName, nsString& aValue)
> {
>   nsresult rv;
>   aValue.Truncate();
> 
>   if (!mStringBundle)
>     rv = InitStringBundle();
>-
>   if (mStringBundle)
>     rv = mStringBundle->GetStringFromName(aStringName.get(), getter_Copies(aValue));

However, given that's just a minor change, I'll do that on check in and save you another round of updating the patch.

Normally we require automated tests for mailnews code, however I think the problem here would be getting the interaction with the file dialogs correct. So I'm happy if we defer to litmus testing for this bug, I've therefore set the in-testsuite? flag which Ludovic should pick up and add something to litmus.

Comment 29

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/6ca7a97e1971

Thanks for fixing this David.

Comment 30

[Ludovic Hirlimann [:Usul]]

11899 and 11890