Last Comment Bug 69528 - Can't add multiple attachments at once via dnd [drop files]
: Can't add multiple attachments at once via dnd [drop files]
Status: VERIFIED FIXED
: dataloss
Product: MailNews Core
Classification: Components
Component: Composition (show other bugs)
: Trunk
: All All
: P2 critical with 5 votes (vote)
: mozilla1.0
Assigned To: Håkan Waara
: Trix Supremo
:
Mentors:
: 83953 91001 98532 106045 110881 118055 119036 122844 127239 (view as bug list)
Depends on:
Blocks: 104166
  Show dependency treegraph
 
Reported: 2001-02-20 11:50 PST by Mike Pinkerton (not reading bugmail)
Modified: 2008-07-31 01:22 PDT (History)
28 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Support attaching multiple files at once via DnD. (3.09 KB, patch)
2001-11-20 02:19 PST, James Henstridge
no flags Details | Diff | Splinter Review
Patch for review (3.52 KB, patch)
2002-02-02 08:49 PST, Håkan Waara
no flags Details | Diff | Splinter Review
Updated patch (3.93 KB, patch)
2002-02-18 08:48 PST, Håkan Waara
no flags Details | Diff | Splinter Review
Latest and greatest (5.33 KB, patch)
2002-03-02 18:36 PST, Håkan Waara
vparthas: review+
Details | Diff | Splinter Review
Patch -uw (4.79 KB, patch)
2002-03-07 13:12 PST, Håkan Waara
no flags Details | Diff | Splinter Review
Patch -u (for testing) (5.13 KB, patch)
2002-03-07 13:52 PST, Håkan Waara
no flags Details | Diff | Splinter Review
Patch (5.23 KB, patch)
2002-03-08 13:57 PST, Håkan Waara
vparthas: review+
sspitzer: superreview+
shaver: approval+
Details | Diff | Splinter Review

Description Mike Pinkerton (not reading bugmail) 2001-02-20 11:50:22 PST
- 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 2001-02-22 15:32:10 PST
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) 2001-02-22 15:34:30 PST
i don't see what these ahve to do with each other at all.
Comment 3 laurel 2001-07-02 12:28:18 PDT
*** Bug 83953 has been marked as a duplicate of this bug. ***
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2001-07-17 15:42:06 PDT
*** Bug 91001 has been marked as a duplicate of this bug. ***
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2001-07-17 15:42:24 PDT
OS = all
Comment 6 Not interested in Mozilla any more 2001-07-27 07:31:31 PDT
still not working in 2001072704/Mac
Comment 7 Chad Austin 2001-08-09 07:28:31 PDT
*** Bug 94491 has been marked as a duplicate of this bug. ***
Comment 8 phil 2001-10-08 03:15:03 PDT
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) 2001-10-08 07:58:13 PDT
i'm seeing this with the branch, 10/2, on macos as well.
Comment 10 Mike Pinkerton (not reading bugmail) 2001-10-08 07:59:46 PDT
this is prety bad, i'm not sure why nobody is in a hurry to fix it.
Comment 11 laurel 2001-10-22 11:42:16 PDT
*** Bug 106045 has been marked as a duplicate of this bug. ***
Comment 12 laurel 2001-11-19 16:34:11 PST
*** Bug 110881 has been marked as a duplicate of this bug. ***
Comment 13 James Henstridge 2001-11-20 01:30:14 PST
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 2001-11-20 02:19:42 PST
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 2001-11-20 23:23:50 PST
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 2001-11-24 15:05:50 PST
it might work but I have no such file and/or diretory
Comment 17 Thorsten Gebuhr 2001-11-25 05:39:52 PST
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 2001-11-27 05:29:55 PST
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 2001-11-28 23:08:32 PST
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 2001-12-05 19:46:56 PST
Yeah, file dialog support for multiple files is a must!
Comment 21 James Henstridge 2001-12-12 00:07:56 PST
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) 2001-12-12 07:46:38 PST
tested on mac. it works.
Comment 23 Mike Pinkerton (not reading bugmail) 2001-12-12 07:46:59 PST
the patch works, i mean. it doesn't work w/out the patch.
Comment 24 Sheela Ravindran 2001-12-13 16:10:02 PST
*** Bug 115143 has been marked as a duplicate of this bug. ***
Comment 25 Thorsten Gebuhr 2002-01-02 11:47:36 PST
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 2002-01-05 21:00:31 PST
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 2002-01-09 12:49:30 PST
*** Bug 98532 has been marked as a duplicate of this bug. ***
Comment 28 laurel 2002-01-09 12:55:00 PST
*** Bug 119036 has been marked as a duplicate of this bug. ***
Comment 29 laurel 2002-01-09 14:10:57 PST
*** Bug 118055 has been marked as a duplicate of this bug. ***
Comment 30 Jaime Rodriguez, Jr. 2002-01-18 09:05:27 PST
what's the chances this is gonna make 099?
Comment 31 James Henstridge 2002-01-18 19:02:33 PST
Hopefully.  Is there anything else I need to do to get the patch reviewed and
applied?
Comment 32 R.K.Aa. 2002-01-19 23:58:53 PST
*** Bug 120978 has been marked as a duplicate of this bug. ***
Comment 33 R.K.Aa. 2002-01-31 13:21:47 PST
*** Bug 122844 has been marked as a duplicate of this bug. ***
Comment 34 Christian :Biesinger (don't email me, ping me on IRC) 2002-01-31 13:24:59 PST
changing summary for easier searching
Comment 35 Håkan Waara 2002-01-31 14:06:51 PST
->me to test and review the patch.
Comment 36 Håkan Waara 2002-01-31 14:22:05 PST
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 2002-02-02 08:49:29 PST
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 2002-02-06 10:16:41 PST
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 2002-02-06 11:25:47 PST
why shouldnt it be allowed to drop a folder ?
Comment 40 Jean-Francois Ducarroz 2002-02-06 11:30:34 PST
because the current code doesn't support that! Fell free to file a bug for a
feature request...
Comment 41 Brendan Eich [:brendan] 2002-02-17 13:24:59 PST
Can we get this in by 0.9.9's freeze, in less than three days?

/be
Comment 42 Håkan Waara 2002-02-18 08:01:21 PST
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 2002-02-18 08:23:08 PST
cc'ing varada...
Comment 44 Håkan Waara 2002-02-18 08:48:17 PST
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) 2002-02-20 03:20:17 PST
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 2002-02-20 07:15:13 PST
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 2002-02-22 10:37:48 PST
*** Bug 127239 has been marked as a duplicate of this bug. ***
Comment 48 Håkan Waara 2002-03-02 18:36:36 PST
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 2002-03-02 18:42:31 PST
spun off bug 128653 for the folder-dropping issue.
Comment 50 varada 2002-03-05 12:59:27 PST
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 2002-03-05 13:22:02 PST
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 seth@sspitzer.org instead) 2002-03-07 10:49:19 PST
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 2002-03-07 13:12:30 PST
Created attachment 73036 [details] [diff] [review]
Patch -uw

Addressed Seth's comments.
Comment 54 (not reading, please use seth@sspitzer.org instead) 2002-03-07 13:26:22 PST
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 2002-03-07 13:52:50 PST
Created attachment 73047 [details] [diff] [review]
Patch -u (for testing)

Attaching a patch for testing as well, for Varada.
Comment 56 robinf 2002-03-07 14:39:06 PST
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 2002-03-07 16:02:06 PST
I agree the alert isn't necessary. Don't bug the user with the dialog.
Comment 58 varada 2002-03-08 13:32:56 PST
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 2002-03-08 13:57:49 PST
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 2002-03-11 18:48:55 PST
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 2002-03-11 23:17:22 PST
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 seth@sspitzer.org instead) 2002-03-12 10:05:28 PST
Comment on attachment 73277 [details] [diff] [review]
Patch

sr=sspitzer
Comment 63 Mike Shaver (:shaver -- probably not reading bugmail closely) 2002-03-12 10:24:37 PST
Comment on attachment 73277 [details] [diff] [review]
Patch

a=shaver for checkin to the 1.0 trunk.
Comment 64 Håkan Waara 2002-03-12 13:11:08 PST
fix checked in.
Comment 65 Trix Supremo 2002-04-10 15:44:11 PDT
verified on macos trunk build 2002040908, win32 trunk build 2002040903, linux
trunk build 2002040909

Note You need to log in before you can comment on or make changes to this bug.