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

VERIFIED FIXED in mozilla1.0

Status

defect
P2
critical
VERIFIED FIXED
19 years ago
11 years ago

People

(Reporter: mikepinkerton, Assigned: hwaara)

Tracking

({dataloss})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

- 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.
Came across this dupe. Reopen if i am wrong.

*** This bug has been marked as a duplicate of 43015 ***
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → DUPLICATE
i don't see what these ahve to do with each other at all.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
*** Bug 83953 has been marked as a duplicate of this bug. ***
*** Bug 91001 has been marked as a duplicate of this bug. ***
OS = all
OS: Mac System 8.5 → All
Hardware: Macintosh → All
still not working in 2001072704/Mac
*** Bug 94491 has been marked as a duplicate of this bug. ***
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.
i'm seeing this with the branch, 10/2, on macos as well.
this is prety bad, i'm not sure why nobody is in a hurry to fix it.
Severity: normal → critical
Keywords: dataloss, nsbeta1
Blocks: 104166
*** Bug 106045 has been marked as a duplicate of this bug. ***
Keywords: nsbeta1+
*** Bug 110881 has been marked as a duplicate of this bug. ***
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.
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.
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.
it might work but I have no such file and/or diretory
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
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).
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
Priority: -- → P2
Yeah, file dialog support for multiple files is a must!
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?
tested on mac. it works.
the patch works, i mean. it doesn't work w/out the patch.
*** Bug 115143 has been marked as a duplicate of this bug. ***
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.
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.
*** Bug 98532 has been marked as a duplicate of this bug. ***
*** Bug 119036 has been marked as a duplicate of this bug. ***
*** Bug 118055 has been marked as a duplicate of this bug. ***
QA Contact: esther → trix
what's the chances this is gonna make 099?
Hopefully.  Is there anything else I need to do to get the patch reviewed and
applied?
*** Bug 120978 has been marked as a duplicate of this bug. ***
*** Bug 122844 has been marked as a duplicate of this bug. ***
changing summary for easier searching
Summary: Can't add multiple attachments at once via dnd → Can't add multiple attachments at once via dnd [drop files]
->me to test and review the patch.
Assignee: varada → hwaara
Status: REOPENED → NEW
Comment on attachment 58518 [details] [diff] [review]
Support attaching multiple files at once via DnD.

This patch doesn't compile any more... obsoleting.
Attachment #58518 - Attachment is obsolete: true
Target Milestone: --- → mozilla0.9.9
Posted patch Patch for review (obsolete) — Splinter 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 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.
Attachment #67581 - Flags: needs-work+
why shouldnt it be allowed to drop a folder ?
because the current code doesn't support that! Fell free to file a bug for a
feature request...
Can we get this in by 0.9.9's freeze, in less than three days?

/be
I'll try to get to this soon. My build environment is still acting up, so I
can't compile. :-(
Target Milestone: mozilla0.9.9 → mozilla1.0
cc'ing varada...
Status: NEW → ASSIGNED
Posted patch Updated patch (obsolete) — Splinter Review
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.
Attachment #67581 - Attachment is obsolete: true
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.
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.
*** Bug 127239 has been marked as a duplicate of this bug. ***
Posted patch Latest and greatest (obsolete) — Splinter Review
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.
Attachment #70077 - Attachment is obsolete: true
spun off bug 128653 for the folder-dropping issue.
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
Attachment #72252 - Flags: review+
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.
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.
Posted patch Patch -uw (obsolete) — Splinter Review
Addressed Seth's comments.
Attachment #72252 - Attachment is obsolete: true
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.
Posted patch Patch -u (for testing) (obsolete) — Splinter Review
Attaching a patch for testing as well, for Varada.
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.
I agree the alert isn't necessary. Don't bug the user with the dialog.
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);
>+          }
Attachment #73047 - Flags: needs-work+
Posted patch PatchSplinter Review
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.
Attachment #73036 - Attachment is obsolete: true
Attachment #73047 - Attachment is obsolete: true
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
Attachment #73277 - Flags: review+
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 on attachment 73277 [details] [diff] [review]
Patch

a=shaver for checkin to the 1.0 trunk.
Attachment #73277 - Flags: approval+
fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago18 years ago
Resolution: --- → FIXED
verified on macos trunk build 2002040908, win32 trunk build 2002040903, linux
trunk build 2002040909
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.