69528 - Can't add multiple attachments at once via dnd [drop files]

Status: VERIFIED FIXED

Description

[Mike Pinkerton (not reading bugmail)]

- open mail compose
- drag 2 files from finder into attachment pane

expected:
- both show up

actual:
- only one shows up. not even sure which one since i can't see the file names 
(only the full path, see other bug)

2/19/01 mac build.

Comment 1

[Keyser Sose]

Came across this dupe. Reopen if i am wrong.

*** This bug has been marked as a duplicate of 43015 ***

Comment 2

[Mike Pinkerton (not reading bugmail)]

i don't see what these ahve to do with each other at all.

Comment 3

[laurel]

*** Bug 83953 has been marked as a duplicate of this bug. ***

Comment 4

[Boris Zbarsky [:bz] (still digging out from vacation mail)]

*** Bug 91001 has been marked as a duplicate of this bug. ***

Comment 5

[Boris Zbarsky [:bz] (still digging out from vacation mail)]

OS = all

Comment 6

[Not interested in Mozilla any more]

still not working in 2001072704/Mac

Comment 7

[Chad Austin]

*** Bug 94491 has been marked as a duplicate of this bug. ***

Comment 8

[phil]

Under Windows 2000 the *last* file selected in the group is the only one that is
added to the attachment list when drag-and-dropped.  All the others are not
added to the list of attached files.

Comment 9

[Mike Pinkerton (not reading bugmail)]

i'm seeing this with the branch, 10/2, on macos as well.

Comment 10

[Mike Pinkerton (not reading bugmail)]

this is prety bad, i'm not sure why nobody is in a hurry to fix it.

Comment 11

[laurel]

*** Bug 106045 has been marked as a duplicate of this bug. ***

Comment 12

[laurel]

*** Bug 110881 has been marked as a duplicate of this bug. ***

Comment 13

[James Henstridge]

This looks fairly trivial to fix.  Just edit
mozilla/mailnews/compose/resources/content/MsgComposeCommands.js and change
attachmentBucketObserver so that it has canHandleMultipleItems set to true.

I will attache a patch once I test it.

Comment 14

[James Henstridge]

Created attachment 58518 [details] [diff] [review]
Support attaching multiple files at once via DnD.

Here is the patch.  I have tested it on Linux (gtk build), and can drag
multiple files from Nautilus without problems (since support for text/uri-list
drops was added recently).  It should work for windows as well, since it
supports multi item drops as well.  I don't have access to any other platforms
to test this on.

Comment 15

[James Henstridge]

From a brief glance at the mac DND code, the above patch should work for Macs as
well (I can't see why it wouldn't).

If anyone wants to test or review the patch, that would be useful.

Comment 16

[Croco Dil]

it might work but I have no such file and/or diretory

Comment 17

[Thorsten Gebuhr]

The file is packed into the messenger.jar in the chrome-Directory.
So you have to unpack it with winzip/ark or the JDK-Utilities, patch it and put
it back into the archive.

But I had errors applying the patch on mozilla 0.96. The 'patch' utility just
generated errors and a backup-copy of the original file, but did not patch it.

Thorsten

Comment 18

[James Henstridge]

Thorsten, is it possible that the reason the patch was rejected is due to line
ending differences?  I produced the patch on Linux, and patch may have issues
with "\r\n" terminated lines.

I just looked at the file to patch in 0.9.6's messenger.jar, and the file
doesn't look too different from the one on the trunk.

Would you be able to apply the patch by hand?  (lines starting with a '-' are
removed, and lines starting with '+' are added).

The paths in the patch are relative to the source tree, rather than a binary
distribution of mozilla (hence the different paths).

Comment 19

[Thomas Swan]

Did a manual test of the patch against win32 nightly 2001112803 and it worked 
for me...

Unpacked messenger.jar put the patched file in and rezipped...

Now, if I could only get the file dialog to allow me to select multiple files

Comment 20

[jesse]

Yeah, file dialog support for multiple files is a must!

Comment 21

[James Henstridge]

just wondering if this bug has been forgotten or not.  I attached a patch a
fortnight ago, which has been tested on both Windows and Linux.  I can see no
reason it wouldn't work (or cause problems) on Mac or any other platform (it
only touches some javascript).

The patch is fairly low risk -- the majority of the +/- lines in the patch are
just whitespace differences.  Any chance of getting this patch applied for the
next milestone?

Comment 22

[Mike Pinkerton (not reading bugmail)]

tested on mac. it works.

Comment 23

[Mike Pinkerton (not reading bugmail)]

the patch works, i mean. it doesn't work w/out the patch.

Comment 24

[Sheela Ravindran]

*** Bug 115143 has been marked as a duplicate of this bug. ***

Comment 25

[Thorsten Gebuhr]

I at last patched mozilla 0.97 running on Win2k by hand.

Workes fine now (after solving a few problems I had with my version of WinAce
and jar-Archives).

I could not try it with KDE 2.1.1 on Linux 2.4.10 - because drag and drop
doesn't work at all on this platform.

Somebody should take care of the integration of this patch into the next release
and mark this bug as FIXED.

Comment 26

[James Henstridge]

Any chance of this patch getting reviewed and checked in?  It has been tested on
unix, windows and macos, and fixes the bug on all systems.

I just don't want to see the patch bitrot, and it is something that should work.

Comment 27

[Erik Hensema]

*** Bug 98532 has been marked as a duplicate of this bug. ***

Comment 28

[laurel]

*** Bug 119036 has been marked as a duplicate of this bug. ***

Comment 29

[laurel]

*** Bug 118055 has been marked as a duplicate of this bug. ***

Comment 30

[Jaime Rodriguez, Jr.]

what's the chances this is gonna make 099?

Comment 31

[James Henstridge]

Hopefully.  Is there anything else I need to do to get the patch reviewed and
applied?

Comment 32

[R.K.Aa.]

*** Bug 120978 has been marked as a duplicate of this bug. ***

Comment 33

[R.K.Aa.]

*** Bug 122844 has been marked as a duplicate of this bug. ***

Comment 34

[Christian :Biesinger (don't email me, ping me on IRC)]

changing summary for easier searching

Comment 35

[Håkan Waara]

->me to test and review the patch.

Comment 36

[Håkan Waara]

Comment on attachment 58518 [details] [diff] [review]
Support attaching multiple files at once via DnD.

This patch doesn't compile any more... obsoleting.

Comment 37

[Håkan Waara]

Created attachment 67581 [details] [diff] [review]
Patch for review

Here's a modified version of James' patch. I've tested it, and it works just
great.

Ducarroz, can I get your r=?

Comment 38

[Jean-Francois Ducarroz]

Comment on attachment 67581 [details] [diff] [review]
Patch for review

The patch looks good but I see 2 problems (that exist in the actual code too):
1) the strings DuplicateFileErrorDlogTitle and DuplicateFileErrorDlogMessage

 are missing in composeMsg.properties! can you put them back.

2) We can drop a file folder! we should block that.

Comment 39

[Dirk O. Brückner]

why shouldnt it be allowed to drop a folder ?

Comment 40

[Jean-Francois Ducarroz]

because the current code doesn't support that! Fell free to file a bug for a
feature request...

Comment 41

[Brendan Eich [:brendan]]

Can we get this in by 0.9.9's freeze, in less than three days?

/be

Comment 42

[Håkan Waara]

I'll try to get to this soon. My build environment is still acting up, so I
can't compile. :-(

Comment 43

[Jean-Francois Ducarroz]

cc'ing varada...

Comment 44

[Håkan Waara]

Created attachment 70077 [details] [diff] [review]
Updated patch

This patch is updated to the trunk, and fixes #2 of the review nits. #1
includes changing lots of other mail code as well, so we can spin off a new bug
for that before resolving this.

Comment 45

[Vidar Haarr (not reading bugmail)]

I secong Brendan.. could we get this for .9.9 ?

I would r= your patch, but I don't know if I'm allowed to do so.

The only thing I notice is
+DuplicateFileErrorDlogMessage=Error: the file is already attached
I don't think that is a very good message .. what about something like:
"I'm sorry, but this file has already been attached to your email."
Just a tad more "user-friendly" =)

Or better yet, this bug is about letting multiple files be dropped into the
window, so what about looping thru all the files, and when done, just display a
error message, if one or more of the files couldn't be added, with a list of the
relevant files. Did that make sense ? :)

Just my 2 cents.

Comment 46

[Robert Kieffer]

With regard to warning the user when they've already attached a file, I don't
think this is necessary - just ignore the duplicate files and be quiet about it  :-)

This is, in fact, the current behavior when attaching a single file and it works
just fine.  There's no reason to behave differently when attaching multiple files.

Comment 47

[laurel]

*** Bug 127239 has been marked as a duplicate of this bug. ***

Comment 48

[Håkan Waara]

Created attachment 72252 [details] [diff] [review]
Latest and greatest

Fixed some subtle bugs (and a major screw-up from my last attachment). I've
tested it quite extensively -- mass-attaching 20 files at once, dropping in
emails as attachments and so on. It works great!

Ducarroz, to respond your review comments (from private emails):

* The "duplicate file" alert will be really, rare - frankly, I don't see why
anyone would end up with it at all, but if you still want me to fix the wording
-- feel free to file a bug on me, and once I jglick has changed the spec I can
fix it.

* I fixed some major bugs, and tested it more this time - so it should work
fine for you.

Over to you again, JF, for review.

Comment 49

[Håkan Waara]

spun off bug 128653 for the folder-dropping issue.

Comment 50

[varada]

Comment on attachment 72252 [details] [diff] [review]
Latest and greatest

The patch looks good. I have to agree with JF on whether we should allow
dropping of folders on the attachment pane. If he doesnt have a problem with
that being a separate spin off bug 128653 - then r=varada

Comment 51

[Håkan Waara]

The folder dropping problem requires more extensive changes, throughout the
mailnews module, and perhaps a bit of design discussion. I would rather not hold
up this patch because of that.  Let's continue that discussion in the spin-off.

Comment 52

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

dropping a folder is a bigger beast, let's not make this part of this bug.

It looks like the user will get the:

"Duplicate file error" / "Error: the file is already attached" error if they try
to drop the same message twice, in addition if they try to drop the same file twice.

If so, that doesn't seem right.

Let's spin off the alert issue to another bug, and for now, just dump to the
console.

So the code would be something like this (notice the removal of the ! and the
switching of the if and else logic)

+          if ((DuplicateFileCheck(rawData))) 
+          {
+            dump("Error, attaching the same item twice\n");
+          }
+          else 
+          {
+            attachment =
Components.classes["@mozilla.org/messengercompose/attachment;1"]
+                         .createInstance(Components.interfaces.nsIMsgAttachment);
+            attachment.url = rawData;
+            attachment.name = prettyName;
+            AddAttachment(attachment);
+          }

Let's spin this UI issue off to a new bug, and cc jglick and robinf.

Please attach a new patch.

Comment 53

[Håkan Waara]

Created attachment 73036 [details] [diff] [review]
Patch -uw

Addressed Seth's comments.

Comment 54

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

hwaara points out (over aim) that the existing code (that uses the prompt 
service) uses strings that aren't defined!

varada, can you re-review and then I'll sr?

I've sent mail to jglick / robinf about the UI issue which we'll spin up 
seperately.

Comment 55

[Håkan Waara]

Created attachment 73047 [details] [diff] [review]
Patch -u (for testing)

Attaching a patch for testing as well, for Varada.

Comment 56

[robinf]

I don't think we need to alert the user about the duplicate file/message drag
and drop. However, it's jglick's call. I'll supply error message text if we
decide to alert the user.

Comment 57

[jglick]

I agree the alert isn't necessary. Don't bug the user with the dialog.

Comment 58

[varada]

Comment on attachment 73047 [details] [diff] [review]
Patch -u (for testing)


>+        if (item.flavour.contentType == "text/x-moz-url" ||
>+            item.flavour.contentType == "text/x-moz-message-or-folder")
>+        {
>+          if (item.flavour.contentType == "application/x-moz-file")

 You will never hit this condition inside the outer if loop. This has to be
made into another
separate condition. the older switch code seems to deal with all the cases
properly.

>+          {
>+            var ioService = Components.classes["@mozilla.org/network/io-service;1"]
>+                            .getService(Components.interfaces.nsIIOService);
>+            rawData = ioService.getURLSpecFromFile(aData.data);
>+          }
>+

 The rawData from the application/x-moz-file and the other two cases are
obtained differently.
Dont use the following string manipulation on the getURLSpecFromFile.
>+          var separator = rawData.indexOf("\n");
>+          if (separator != -1) 
>+          {
>+            prettyName = rawData.substr(separator+1);
>+            rawData = rawData.substr(0,separator);
>+          }

Comment 59

[Håkan Waara]

Created attachment 73277 [details] [diff] [review]
Patch

Thanks for your review comments.

> You will never hit this condition inside the outer if loop. This has to 
> be made into another separate condition.

Added "application/x-moz-file" as a condition to the outer if(), so it will
enter that code.

> The rawData from the application/x-moz-file and the other two cases 
> are obtained differently. Dont use the following string manipulation on 
> the getURLSpecFromFile.

Changed the code so it will just do the string manipulation if we're *not*
dealing with an application/x-moz-file.  This is exactly what the old code did.

Comment 60

[varada]

Comment on attachment 73277 [details] [diff] [review]
Patch

>+        if (item.flavour.contentType == "text/x-moz-url" ||
>+            item.flavour.contentType == "text/x-moz-message-or-folder" ||
>+            item.flavour.contentType == "application/x-moz-file")
>+        {
>+          if (item.flavour.contentType == "application/x-moz-file")
>+          {

   Instead of including the application/x-moz-file in the first if{} and then
again inside
wouldnt it be better to evaluate it as an if-else condition and then obtain the
rawData.

if (item.flavour.contentType == "text/x-moz-url" ||
    item.flavour.contentType == "text/x-moz-message-or-folder")
{  
   ...
   rawData = foo;
} 
else if (item.flavour.contentType == "application/x-moz-file")
{
  ...
  rawData = bar;
}
Other than that it is ok.
R=varada

Comment 61

[Håkan Waara]

Varada and I discussed the if()else() issue over AIM, and it was not as easy as
it looked.  The code needs to be the way it is now, in order to not allow other,
unsupported types to go through.  r=varada still applies.

Comment 62

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

Comment on attachment 73277 [details] [diff] [review]
Patch

sr=sspitzer

Comment 63

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 73277 [details] [diff] [review]
Patch

a=shaver for checkin to the 1.0 trunk.

Comment 64

[Håkan Waara]

fix checked in.

Comment 65

[Trix Supremo]

verified on macos trunk build 2002040908, win32 trunk build 2002040903, linux
trunk build 2002040909