Closed
Bug 69528
Opened 24 years ago
Closed 22 years ago
Can't add multiple attachments at once via dnd [drop files]
Categories
(MailNews Core :: Composition, defect, P2)
MailNews Core
Composition
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.0
People
(Reporter: mikepinkerton, Assigned: hwaara)
References
Details
(Keywords: dataloss)
Attachments
(1 file, 6 obsolete files)
5.23 KB,
patch
|
vparthas
:
review+
sspitzer
:
superreview+
shaver
:
approval+
|
Details | Diff | Splinter Review |
- 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•24 years ago
|
||
Came across this dupe. Reopen if i am wrong. *** This bug has been marked as a duplicate of 43015 ***
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → DUPLICATE
Reporter | ||
Comment 2•24 years ago
|
||
i don't see what these ahve to do with each other at all.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Comment 6•23 years ago
|
||
still not working in 2001072704/Mac
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•23 years ago
|
||
i'm seeing this with the branch, 10/2, on macos as well.
Reporter | ||
Comment 10•23 years ago
|
||
this is prety bad, i'm not sure why nobody is in a hurry to fix it.
Comment 11•23 years ago
|
||
*** Bug 106045 has been marked as a duplicate of this bug. ***
Comment 12•23 years ago
|
||
*** Bug 110881 has been marked as a duplicate of this bug. ***
Comment 13•23 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•23 years ago
|
||
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•23 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.
Comment 16•23 years ago
|
||
it might work but I have no such file and/or diretory
Comment 17•23 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•23 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•23 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•23 years ago
|
Priority: -- → P2
Comment 20•23 years ago
|
||
Yeah, file dialog support for multiple files is a must!
Comment 21•23 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•23 years ago
|
||
tested on mac. it works.
Reporter | ||
Comment 23•23 years ago
|
||
the patch works, i mean. it doesn't work w/out the patch.
Comment 24•23 years ago
|
||
*** Bug 115143 has been marked as a duplicate of this bug. ***
Comment 25•23 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•23 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•23 years ago
|
||
*** Bug 98532 has been marked as a duplicate of this bug. ***
Comment 28•23 years ago
|
||
*** Bug 119036 has been marked as a duplicate of this bug. ***
Comment 29•23 years ago
|
||
*** Bug 118055 has been marked as a duplicate of this bug. ***
Comment 30•23 years ago
|
||
what's the chances this is gonna make 099?
Comment 31•23 years ago
|
||
Hopefully. Is there anything else I need to do to get the patch reviewed and applied?
Comment 32•23 years ago
|
||
*** Bug 120978 has been marked as a duplicate of this bug. ***
Comment 33•23 years ago
|
||
*** Bug 122844 has been marked as a duplicate of this bug. ***
Comment 34•23 years ago
|
||
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•23 years ago
|
||
->me to test and review the patch.
Assignee: varada → hwaara
Status: REOPENED → NEW
Assignee | ||
Comment 36•23 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•23 years ago
|
Target Milestone: --- → mozilla0.9.9
Assignee | ||
Comment 37•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
why shouldnt it be allowed to drop a folder ?
Comment 40•23 years ago
|
||
because the current code doesn't support that! Fell free to file a bug for a feature request...
Comment 41•23 years ago
|
||
Can we get this in by 0.9.9's freeze, in less than three days? /be
Assignee | ||
Comment 42•23 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
Assignee | ||
Comment 44•23 years ago
|
||
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
Comment 45•23 years ago
|
||
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•23 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•23 years ago
|
||
*** Bug 127239 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 48•23 years ago
|
||
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•23 years ago
|
||
spun off bug 128653 for the folder-dropping issue.
Comment 50•23 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•23 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.
Comment 52•22 years ago
|
||
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•22 years ago
|
||
Addressed Seth's comments.
Attachment #72252 -
Attachment is obsolete: true
Comment 54•22 years ago
|
||
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•22 years ago
|
||
Attaching a patch for testing as well, for Varada.
Comment 56•22 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•22 years ago
|
||
I agree the alert isn't necessary. Don't bug the user with the dialog.
Comment 58•22 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•22 years ago
|
||
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•22 years ago
|
Attachment #73047 -
Attachment is obsolete: true
Comment 60•22 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•22 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 62•22 years ago
|
||
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•22 years ago
|
||
fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 22 years ago
Resolution: --- → FIXED
Comment 65•22 years ago
|
||
verified on macos trunk build 2002040908, win32 trunk build 2002040903, linux trunk build 2002040909
Status: RESOLVED → VERIFIED
Updated•20 years ago
|
Product: MailNews → Core
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•