The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla1.0

Status

MailNews Core
Composition
P2
critical
VERIFIED FIXED
16 years ago
9 years ago

People

(Reporter: Mike Pinkerton (not reading bugmail), Assigned: Håkan Waara)

Tracking

({dataloss})

Trunk
mozilla1.0
dataloss

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

16 years ago
- 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

16 years ago
Came across this dupe. Reopen if i am wrong.

*** This bug has been marked as a duplicate of 43015 ***
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → DUPLICATE
(Reporter)

Comment 2

16 years ago
i don't see what these ahve to do with each other at all.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---

Comment 3

16 years ago
*** 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

Comment 7

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

Comment 8

16 years ago
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.
(Reporter)

Comment 9

16 years ago
i'm seeing this with the branch, 10/2, on macos as well.
(Reporter)

Comment 10

16 years ago
this is prety bad, i'm not sure why nobody is in a hurry to fix it.
Severity: normal → critical
Keywords: dataloss, nsbeta1

Updated

16 years ago
Blocks: 104166

Comment 11

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

Updated

16 years ago
Keywords: nsbeta1+

Comment 12

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

Comment 13

16 years ago
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

16 years ago
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

16 years ago
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.
Keywords: patch, review

Comment 16

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

Comment 17

16 years ago
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

16 years ago
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

16 years ago
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

Updated

16 years ago
Priority: -- → P2

Comment 20

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

Comment 21

16 years ago
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?
(Reporter)

Comment 22

16 years ago
tested on mac. it works.
(Reporter)

Comment 23

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

Comment 24

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

Comment 25

15 years ago
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

15 years ago
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

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

Comment 28

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

Comment 29

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

Updated

15 years ago
QA Contact: esther → trix

Comment 30

15 years ago
what's the chances this is gonna make 099?

Comment 31

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

Comment 32

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

Comment 33

15 years ago
*** 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]
(Assignee)

Comment 35

15 years ago
->me to test and review the patch.
Assignee: varada → hwaara
Status: REOPENED → NEW
(Assignee)

Comment 36

15 years ago
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
(Assignee)

Updated

15 years ago
Target Milestone: --- → mozilla0.9.9
(Assignee)

Comment 37

15 years ago
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 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+

Comment 39

15 years ago
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
(Assignee)

Comment 42

15 years ago
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
(Assignee)

Comment 44

15 years ago
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.
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.

Comment 46

15 years ago
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

15 years ago
*** Bug 127239 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 48

15 years ago
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.
Attachment #70077 - Attachment is obsolete: true
(Assignee)

Comment 49

15 years ago
spun off bug 128653 for the folder-dropping issue.

Comment 50

15 years ago
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+
(Assignee)

Comment 51

15 years ago
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.
(Assignee)

Comment 53

15 years ago
Created attachment 73036 [details] [diff] [review]
Patch -uw

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.
(Assignee)

Comment 55

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

Attaching a patch for testing as well, for Varada.

Comment 56

15 years ago
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

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

Comment 58

15 years ago
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+
(Assignee)

Comment 59

15 years ago
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.
Attachment #73036 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #73047 - Attachment is obsolete: true

Comment 60

15 years ago
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+
(Assignee)

Comment 61

15 years ago
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

sr=sspitzer
Attachment #73277 - Flags: superreview+
Comment on attachment 73277 [details] [diff] [review]
Patch

a=shaver for checkin to the 1.0 trunk.
Attachment #73277 - Flags: approval+
(Assignee)

Comment 64

15 years ago
fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago15 years ago
Resolution: --- → FIXED

Comment 65

15 years ago
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.