97413 - Dragging non-link image to Finder/Explorer should save image, not link shortcut

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect, P4)

Description

[Benjamin Mucci]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Macintosh; U; PPC; en-US; rv:0.9.3+) Gecko/20010828
BuildID:    2001082805

Actually when I drag images on Finder the image path (Image w/o link) or the URL
(image with link) is saved: not the image file as in NS4.7. I don't know why
it's the case but it would be nice to recover this behavior.

Comment 1

[Henri Sivonen (:hsivonen)]

Confirmed and setting to normal.

Comment 2

[Peter Trudelle]

Looks like similar behavior on Win98 ("Do you want to add an Active Desktop item
to your desktop?")   Not sure who should get this, but based on related bugs,
->ben/enhancement/p4

Comment 3

[timeless]

<sdagley> pinkerton is generally doomed to D&D bugs

Comment 4

[Mike Pinkerton (not reading bugmail)]

since we don't yet support image dragging, this is by design i guess. brade can 
comment on if this is already a dupe or a real bug.

Comment 5

[sairuh (rarely reading bugmail)]

spam: over to File Handling. haven't changed the current owners, but i did take
qa contact for the nonce. pls do retriage/reassign if needed.

Comment 6

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 7

[m_mozilla]

IIRC, a modified key is used in NN to request that a link be created instead of
saving the image. It would be nice if when this bug is fixed, that alternate
behavior is also verified to work.

Comment 8

[Alfonso Martinez]

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

Comment 9

[Pete Boyd]

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

Comment 10

[Pete Boyd]

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

Comment 11

[Pete Boyd]

I suggest changing this bugs title to 'Dragging (dnd) an image to desktop or
other application drags URL (linK) rather than image' as its no-longer
Mac-specific. I'd do it myself but can't figure out how to

Comment 12

[Dave Miller [:justdave]]

Isn't this a regression rather than an enhancement?  I could swear this used to
work in Moz....

Comment 13

[m_mozilla]

this is a NN4.x parity issue too.

-matt

Comment 14

[cor bently]

I'll likely never see that vampira erotica again, 
and all I've got are a bunch of links.
 
WOE is me!

What could have possibly went wrong?!?

;o)

Comment 15

[Vicki Brown]

It would be nice to make this an option. Saving the link could be useful but the
exected behaviour (ie. the behaviour from IE and Netscape) is to download the
image as an image file. ([e-mail])

Comment 16

[saari (gone)]

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

Comment 17

[saari (gone)]

toss this dagley's way

Comment 18

[Steve Dagley]

Flinging bug back to component owner.  Not that I think it's necessarily set to
the right component but I don't want this bug on my plate :-)

Comment 19

[Matthew T (active 1999-2002)]

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

Comment 20

[Matthew T (active 1999-2002)]

This isn't an enhancement, it's a bug. No other browser behaves like Moz does.

Comment 21

[Bill Law]

Resetting the component to better suit the bug, I think.

Comment 22

[Ed Sabol]

This is a regression from NS 4.7x and before. Could someone target a fix for a
milestone more recent than "Future"?

Comment 23

[Simon Fraser [no longer active]]

Watch bug 45823 for Mac fixes for this problem.

Comment 24

[Daniel Wang]

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

Comment 25

[Ido Beeri]

I think that also link dragging should be saved not as a shortcut if the user
presses "Ctrl" while dragging. It should save the source.

Comment 26

[Simon Fraser [no longer active]]

Fixed on Mac. Giving to Nisheeth for Windows love.

Comment 27

[Nisheeth Ranjan]

So, I have an issue here.  On the Mac, it is possible to get the drop location 
of a drag-drop operation.  I've read up on the Windows drag/drop APIs and can't 
figure out how to get that information.  The only communication between the 
drop target and the drop source that I know about is via the IDataObject and 
the IDropSource interfaces.  None of the methods on those interfaces give me 
the drop location.  Is there some other way to get that info?

If you know of any Windows hackers who can answer this question, please forward 
it to them or point them to this bug.  Thanks!

CCing Bill Law and Mike Judge in case they know what to do here.

For now, I'm going forward with the terrible terrible hack of saving the 
dragged file in a known temporary location and passing the filename back to the 
drop target which copies the file from the temp location to the actual drop 
location.  Effectively, the dropped file is copied twice.  :-(

Help!

Comment 28

[Nisheeth Ranjan]

Cutting/pasting some email discussion that happened on my earlier question:

Nisheeth wrote:

Simon, I wonder if Mac's drag/drop isn't faced with the same "target can
be file system or any application" problem?  How do your changes handle
the case where an image is drag-dropped from Mozilla into another Mac
application like Adobe Photoshop?

Thanks for the help, Bill!  Much appreciated!

My main goal here was to avoid writing too much Windows specific code.
Seems like that ain't a possibility.  The drop handler extension
approach suggested on the second google link will only work if the file
is dropped on a file system folder and will probably be more code (drop
handlers need to be separate DLLs with their own class factories,
implement IPersistFile and IDropTarget) than fixing this the right way
-- stream the data out from the drop source to the data target via the
IStream interface (I only need to implement IStream on the existing data
object).   The IStream approach will work if the file is dropped
anywhere -- file system or another Windows app.

So, I'll go the IStream route as a second step after I get things
working with the hacky double copy...

Nisheeth
-- 
Bill Law wrote:

> I researched this a little this evening, and found this:
>
>
http://groups.google.com/groups?hl=en&lr=&ie=UTF-8&oe=UTF-8&selm=OU3a7KKT%
23GA.225%40uppssnewspub04.moswest.msn.net

>
>
> It seems that maybe you really don't want to know where the file was
> dropped :-).  Probably because then you wouldn't do the right thing when
> your file was dropped on some spiffy new Windows application that wanted
> to handle the data itself.  Makes a certain amount of sense.
>
> So the problem is how to get the target to transfer the data
> efficiently.  I can't grok the IDataObject details sufficiently to know
> how to do that, sorry.
>
> On the other hand, where there's a will, there's usually a way (in
> Windows).  See
>
http://groups.google.com/groups?hl=en&lr=&ie=UTF-8&oe=UTF-
8&threadm=eQfoKpH09GA.121%40uppssnewspub05.moswest.msn.net&rnum=6&prev=/groups%
3Fq%3Ddrop%2Bhandler%2Bshell%2Bextension%26ie%3DUTF-8%26oe%3DUTF-8%26hl%3Den

>
>
> Bill 

Comment 29

[Nisheeth Ranjan]

Attachment: Work in progress

This patch is a work in progress so please don't review yet.  Simon, your XP
code is working great and the dropped file is getting saved in the temp
directory.  But, I get a crash up in a windows DLL as I do the drag/drop.  I've
stepped through the Windows specific code I wrote and don't find anything
obviously wrong.  Will debug this more tomorrow.  If any Windows hacker out
there sees something that I don't please comment.  Thanks!

Comment 30

[Nisheeth Ranjan]

Attachment: Patch for first review

OK, after tons of experimentation with Windows drag/drop API code, this patch
allows images to be dragged from Mozilla to the desktop.  It leverages Simon's
XP code to first save the dragged image to a temporary location and then uses
the CF_HDROP clipboard format to tell Windows to copy the file from the temp
location to the drop destination.

The issues fixed by this patch are:
- the crash in Windows drag/drop code I was running into with the earlier
patch.
- the temporary file is only created once even though GetData() can be called
multiple times by different drop targets as the mouse hovers over them during
the drag operation.
- the temporary file is moved to the final destination instead of getting
copied.

I think this is a good enough fix for now, at least for image dragging where
all the data is on the local disk.

I am going to file another bug on myself to implement drag drop via the
CF_FILEDESCRIPTOR and CF_FILECONTENT clipboard formats and pass the file's data
to the drop target through the IStream interface.  But, that will be a lot of
Windows specific code.	We'll basically have to serve the dragged file through
a Necko stream to an IStream that the drop target will pull its data from.

Comment 31

[Nisheeth Ranjan]

Comment on attachment 121635 [details] [diff] [review]
Patch for first review

Asking Rod for a review and Simon for a super review.  Please see comment 30
for an explanation of what the patch does.

Comment 32

[Nisheeth Ranjan]

bug 203307 filed to track implementing drag and drop via the IStream interface.

Comment 33

[Ido Beeri]

Can you please tell what you thing about comment #25 ?

Comment 34

[Nisheeth Ranjan]

Comment on attachment 121635 [details] [diff] [review]
Patch for first review

rods is off working on non mozilla stuff.

Changing reviewer to smontagu, the new owner.  Simon, please see comment 30 and
call me if you have any questions on the patch.

Also, since the patch is in windows only code and kin is windows guy,  changing
sr request to point to him.  sfraser, please feel free to take a look at this
if you want as well.   Another pair of eyes can only help!  :-)

Comment 35

[rods (gone)]

I wasn't able to debug it, but I reviewed it closely and it sems to look ok. r=rods

Comment 36

[kinmoz]

Comment on attachment 121635 [details] [diff] [review]
Patch for first review

==== In CEnumFormatEtc::InsertFEAt() shouldn't we be incrementing mNumFEs even
in the case where you don't resize the array? Also, shouldn't we be
incrementing mMaxNumFEs after the reallocation? I'm assuming the number of FEs
will usually be small and that there is no need to optimize the reallocation
code to just insert the new FE since it has to copy the data from the old to
new array.


====  Also the one concern I have is the possibility of cluttering the OS temp
directory if the user drags and drops into an app instead of the finder. I
think that could probably be addressed in another bug after you've landed this.

Comment 37

[Nisheeth Ranjan]

Attachment: Patch that addresses Kin's comments

Kin, I now increment mMaxNumFEs and mNumFEs properly.  Good catch!

About to file a new bug to take care of file cluttering issue in the temp
directory...

Comment 38

[kinmoz]

Comment on attachment 122054 [details] [diff] [review]
Patch that addresses Kin's comments

sr=[e-mail]

Comment 39

[kinmoz]

Comment on attachment 122054 [details] [diff] [review]
Patch that addresses Kin's comments

I forgot to ask if these temp files are created when just dragging images
around inside the same composer/mail compose window?

Comment 40

[Nisheeth Ranjan]

bug 203847 filed to fix issue regarding file cluttering in temp directory...

Kin, no these temp files aren't created while dragging images inside Composer.  
That raises another issue though.  Currently, dragging images from Composer out 
to the desktop doesn't invoke sfraser's XP image drag/drop code changes to 
nsContentAreaDragDrop.cpp.  This is because Composer implements its own 
drag/drop helper objects.

We are gonna have to make XP changes to Composer and make sure the the file 
promise flavor is entered into the nsITransferable when images are dragged.  
I'll file a new bug for that and work with sfraser on it...

Comment 41

[Nisheeth Ranjan]

bug 203852 filed to track drag image from composer to desktop issue.

Comment 42

[Nisheeth Ranjan]

Comment on attachment 122054 [details] [diff] [review]
Patch that addresses Kin's comments

bringing r= from rods forward to this patch.

Comment 43

[Simon Montagu :smontagu]

Comment on attachment 122054 [details] [diff] [review]
Patch that addresses Kin's comments

I see rods beat me to r=, but I will make my comment anyway :-)

If ::GlobalAlloc() fails, don't you want GetPreferredDropEffect() and GetFile()
to fail? (as GetFileDescriptorInternetShortcut() and
GetFileContentsInternetShortcut() do now)

Comment 44

[Ed Sabol]

This patch has r= and sr=... Can we get it landed for 1.4???

Comment 45

[Nisheeth Ranjan]

I've checked in this patch into 1.5 alpha.  About to attach the final patch that
got checked in.  It addresses Simon Montagu's comment.

I don't want to check in this patch into 1.4 until I fix bug 203847.  I have a
fix in hand which I will attach to the bug shortly.  Once its passed reviews,
I'll check it into 1.5 alpha.  Then, if things look good on the trunk, I'll
check in both the fix for this bug and the fix for bug 203847 into 1.4...

Comment 46

[Nisheeth Ranjan]

Attachment: Final patch addressing smontagu's comments

This is the patch I checked into 1.5 alpha.

Comment 47

[Ed Sabol]

Wonderful! Thank you for your efforts, Nisheeth!

Comment 48

[Nisheeth Ranjan]

OK, so the fix to bug 203847 exposed a pre-existing bug (bug 209593) on the
trunk related to drag/drop of emails.  Kin has checked in a fix to bug 209593 on
the trunk.  But, this "pseudo-regression" has made me wary of checking in the
fixes to this bug, bug 203847, and bug 209593 on the 1.4 branch so close to the
1.4 final release.  Drag/drop is used in many places in the app and I'd much
rather have these fixes bake properly over the 1.5  app cycle.

I'm marking this bug fixed.  If people feel strongly about seeing these bug
fixes land on the 1.4 branch, please say so!

Thanks!

Comment 49

[Paolo Marani]

I'm under W98se, mozilla 1.6a build 2003092304.
Unfortunately, when i drag an image from a page to
desktop or any other folder) i got just a bad
system error message:

    Unable to copy: <some blank spaces> File system error (1026)

I don't feel this issue has been completely fixed, at least
not in W98. What's wrong ?

Comment 50

[Scott MacGregor]

Over in Bug #83803, I leverage the work Nisheeth and Simon did to support
dragging attachments from the mail window to the desktop. I ran into a problem
though because of the temp file solution we use on windows. 

Large mail attachments can take a long time to download from the server. If you
drag an attachment, we start downloading the attachment. When you release the
drop, the OS copies the temporary file we have been downloading to (as described
by the windows fix for this bug), over to the final destination such as the
desktop. If we have not finished downloading the attachment, you get an
operating system error saying "Another process is accessing this file and it
cannot be moved".

If we were able to write directly to the drop destination instead of having the
OS give us a temporary file to write it to, we might avoid this error. 

I wonder how other applications make this work.

Comment 51

[Scott MacGregor]

Hmm, looks like 4.x didn't have this problem. So there must be a better way to
fix this problem that does not involve copying to a temporary file first. With
4.x, the desktop icon does not show up either until we are done downloading the
attachment. 

Comment 52

[Yuri Kovalchuk]

Attachment: Changes to Drag and Drop of files

When one tries to drag and drop a large file from browser's window to file
system (eg. Windows Explorer) one can get an "Sharing Violation" error, or get
not what was expected. For example go to:
http://www.pakistan-tradenetwork.com/admin/images/vase-%20large.jpg, wait while
the image downloads and then choose Edit->Preferences... Advanced->Cache and
choose clear cache. Then drag the image out of the browser window to, say
desktop and check the results.	You'll get something but not the image you've
dragged. This happens because WebBrowserPersist is not done downloading the
image to the temp location on your computer and explorer already tries to move
a file to the desktop (download isn't done yet).
Here I suggest a fix to this problem.
Just before the drag starts, create a hidden window and register it to recieve
notifications from the shell about events in the file system. And in
nsDataObj::GetFile instead of starting the download to the temp dir, just
create	an empty file there and let the user drag that empty file. When the
drop occurs shell will notify our hidden window that SHCNE_RENAMEITEM event
occurred and pass the path of the new item. We store that path and when drag
loop exits we kick off the download to that path. Check the code for the
details.

Comment 53

[Alexander Opitz]

@Yuri

I think that's another Drag'n'Drop bug and haven't todo with the problem
described in this bug.

Maybe there is another bug with this issue or create a new bug.

Comment 54

[timeless]

Comment on attachment 164149 [details] [diff] [review]
Changes to Drag and Drop of files

please don't use tabs.
please use cvs diff -up10 (pick 10 to be a number which gives reasonable
context)

Comment 55

[timeless]

oh, and lastly, please manage your changes in a new bug (reference the bug
number here), this bug is resolved.

Comment 56

[Yuri Kovalchuk]

Attachment: Suggested changes to drag and drop code.

Fixed previous version of an attachment (done some minor refactoring).

Comment 57

[Yuri Kovalchuk]

Comment on attachment 164257 [details] [diff] [review]
Suggested changes to drag and drop code.

When dragging large images or files out of the browser window using
"application/x-moz-file-promise" with the file or image that has not been
cached yet, you can get a "Sharing Violation" or an incomplete file on Windows.
This patch fixes this issue.

Comment 58

[Kathleen :Brade]

Comment on attachment 164257 [details] [diff] [review]
Suggested changes to drag and drop code.

Please generate a new diff with context:  cvs diff -u15 nsD*

It looks like there are some tab characters in some of the code added; please
convert those to spaces.  Thanks!

Comment 59

[Yuri Kovalchuk]

As people suggested I filed a new bug #267426. And posted my patch there with
tabs removed:).