Attachment Drag n Drop to Desktop creates empty file

[thbone]

Binary package hint: mozilla-thunderbird

If you try to Drag and Drop an email attachment (PDF, MP3, etc.) to the Desktop / or a folder:

1. it opens a window "KDesktop - Filename for dropped file" with an empty filename in the inputbox
2. after you have typed in a filename (in the inputbox) and pressed OK

there would be an empty text file created with an "m" in it.

So the Drag an Drop of attachments from thunderbird to KDE didn't work.

(System: AMD3800X2, Ubuntu Edgy Eft 6.10 with KDE, Thunderbird 1.5.0.10 (20070306))

P.S. sorry for my not so good english

[Freddy Martinez (freddymartinez9)]

I will attempt to reproduce this soon.

[thbone (rabo69)]

Drag and Drop also don't work under Gnome-Ubuntu (doesn't create anything after drop action) and
Xubuntu (Xubuntu only will create a link)

All 5 People I asked can reproduce the Bug as I described before under KDE-Kubuntu

[thbone (rabo69)]

User-Agent: Mozilla/5.0 (X11; U; Linux i686; de; rv:1.8.1.3) Gecko/20060601 Firefox/2.0.0.3 (Ubuntu-edgy)
Build Identifier: Thunderbird Version 2.0.0.0 (20070326) and also Version 1.5.0.10 (20070306)

If you try to move a attachment via Drag and Drop outside Thunderbird to the desktop or a folder on the PC the drag and drop cursor symbols shows up correctly but no file will be created/moved to this place (This works fine under windows)

(The error shows under Gnome Desktop Ubuntu EdgyEft 6.10 and also under Feisty Fawn 7.04)

Reproducible: Always

Steps to Reproduce:
1. drag an email attachment
2. move the cursor to desktop (press shift or control or nothing - same thing)
3. release mouse button
Actual Results:
no file will be created/moved to the drop place

Expected Results:
the attachment should moved/copied to the drop place (in windows this works fine)

[thbone (rabo69)]

The new Version Thunderbird 2.0 RC 1 shows the same Bug no drag and drop to folders or desktop...

[A S Alam (aalam-deactivatedaccount)]

I can confirm this behavior on my machine on Fedora with KDE.
it ask to create file, but actually has no content from attachment

[Cbook (cbook)]

confirmed on fedora fc 6 with thunderbird 2 release build.

[A S Alam (aalam-deactivatedaccount)]

with trunk build thunderbird-3.0a1.en-US.linux-i686, bug can also be reproduced

[Freddy Martinez (freddymartinez9)]

Does this problem still exist? I am going through older issues and would like to know if this problem has been fixed.

[thbone (rabo69)]

yes, the bug still exist in both version I' ve tested (thunderbird
1.5.0.10 (20070403) <- ubuntu edgy eft version and also in the
thunderbird 2.0 version from mozilla.com)

Freddy Martinez schrieb:
> Does this problem still exist? I am going through older issues and would
> like to know if this problem has been fixed.
>
>

[thbone (rabo69)]

yes, the bug still exist in all versions I' ve tested (thunderbird 1.5.0.10 (20070403) <- ubuntu edgy eft version and also in the thunderbird 2.0 version from mozilla.com)

I've made also a bug report at

https://bugzilla.mozilla.org/show_bug.cgi?id=377621

and the problem could be reproduced from other users, too.

[Frank-u-uu (frank-u-uu)]

Similar problem with OS WinXP as well. If you drag the attachment to the desktop you have to keep the left mouse button pressed for some seconds so the attachment is "loaded" (you see a progress bar). Then you can release the button and the attachment is successfull copied. Otherwise (without waiting) an errormessage shows up ("file in use"). You can work with this unconvenience but it is unexpected, not state of the art and contradicts usability a lot (because it took me quite a while to find out).
If you can confirm this behaviour please extend this bug to OS WinXP as well.

[thbone (rabo69)]

bug still exists in new TB 2.0.0.4

[Frank-u-uu (frank-u-uu)]

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4
Build Identifier: Version 1.5.0.12 (20070509)

If you drag the attachment to the
desktop you have to keep the left mouse button pressed for some seconds so the
attachment is "loaded" (you see a progress bar). Then you can release the
button and the attachment is successfull copied. Otherwise (without waiting) an
error message shows up ("file in use"). You can work with this inconvenience but
it is unexpected, not state of the art and contradicts usability a lot (because
most people won't discover the workaround).

Reproducible: Always

Steps to Reproduce:
1. Quickly drag and drop big attachment (>3 MB) to desktop

Actual Results:
1. Error-Message "File in use", click on "OK" then
2. Download dialog with progress bar appears like the attachment is actually downloaded
3. No attachment is copied

Expected Results:
Copying attachment to desktop without any delay

[thbone (rabo69)]

bug stll exists in TB 2.0.0.4

[thbone (rabo69)]

bug still exists in TB 2.0.0.6

[thbone (rabo69)]

bug still exists in TB 2.0.0.6 (Ubuntu/Feisty)

[ddumanis (dave-davedumanis)]

I can confirm this bug in 2.0.0.6 and it has existed since at least Dapper for me. This is a major user interface glitch. Either drag n drop attachments should work properly, or you should not be able to drag the attachment at all.

[Tymon Rzewuski (taymon)]

Bug still exists in TB 2.0.0.9 on Windows XP Professional SP2.

[Nikolay Shopik (shopik)]

Confirm with 2.0.0.12 but NOT with trunk version 3.0a1pre (2008040204) - its ok no problem at all.

[Mkmelin+mozilla (mkmelin+mozilla)]

Please mark branch only bugs, subject or whiteboard.

[Nikolay Shopik (shopik)]

You mean if its reproducible only with trunk but not current?

[Mkmelin+mozilla (mkmelin+mozilla)]

Yes, if you mean current=current stable version. Unfortunately bugzilla's version field is not multi select...

[Nikolay Shopik (shopik)]

o-right then, I will not touch bugs if they only appears in current stable. Will just comment

[Tokoe (tokoe)]

Hej,

the problem is that thunderbird doesn't provide a text/uri-list flavour that
could be evaluated by the desktop environment to copy the attachment somewhere
else.

One idea would be to save the attachment to a temporary file and pass the url
of that file as a test/uri-list flavour in the drag operation.
To make that feature work for all platforms, a java script based implementation
would be preferred. So can somebody point me to the place in the mozilla sources
where the drag for attachments is started?
 mailnews/base/resources/content/msgHdrOverlay.js:attachmentAreaDNDObserver
sounds promising, however that method seems not to be called when I try to
drag an attachment from the message view.

BTW, when I drag the attachment to the desktop, the JS error console shows the following message:

Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsITransferable.getTransferData]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://messenger/content/msgHdrViewOverlay.js :: anonymous :: line 1627" data: no]
Source File: chrome://messenger/content/msgHdrViewOverlay.js
Line: 1627

It seems like the 'application/moz-file-promise-url' flavour is missing...
Can somebody enlighten me what that flavour is for and why it could be missing?

Ciao,
Tobias

[Tokoe (tokoe)]

Created an attachment (id=328330)
Allows drag&drop of attachments from message view to desktop

Hej,

this patch allows drag&drop of attachments to the desktop by saving the requested attachment to a temporary file and passing the URL of that file in the drag object. The patch has still two issues: 1) '/tmp' is hard coded... It seems nsIFileUtilities::getTempDirPath() could help here, but how do I instantiate such a component?
2) The name of the temporary file is the same as stored in the attachment, however as it's a temporary file, it should have a unique name (like returned by mktemp(3) under Unix). nsIFileUtilities::newTempFileName() looks promising, but could it be that there is no implementation for that method?

[Frank-u-uu (frank-u-uu)]

Problem still exists in 2.0.0.14 and MS XP SP3.
See similar problem on OS Linux: https://bugzilla.mozilla.org/show_bug.cgi?id=377621

[Frank-u-uu (frank-u-uu)]

See similar Win XP bug here:
https://bugzilla.mozilla.org/show_bug.cgi?id=384590

Bug only occurs, when attachment is "large" (> 2MB).

[John Vivirito (gnomefreak)]

Ubuntu has this bug in bug tracker the link for it is
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/151162

[Tokoe (tokoe)]

Hej,

any news on that issue? Is the patch ok for review/superreview?

Ciao,
Tobias

[Tokoe (tokoe)]

Created an attachment (id=340309)
Use the directory-service for temporary file now

Hej,

the new patch uses directory-service to retrieve the temporary directory instead of hardcoding "/tmp".

Ciao,
Tobias

[Brian-lu (brian-lu)]

Two comments:
1. The patch need to be updated to apply the trunk
2. I still can't DnD my attachment to my desktop

[David-ascher (david-ascher)]

(From update of attachment 340309)
dmose isn't building on linux much these days, so switching review to magnus.

[Tokoe (tokoe)]

Created an attachment (id=365417)
adapt to HEAD

Adapted to HEAD. Could somebody do a review soon, please?
I really don't want to update the patch here forever :(

[Mkmelin+mozilla (mkmelin+mozilla)]

I hope to get to this later this week, sorry about the delay.

[Mkmelin+mozilla (mkmelin+mozilla)]

(From update of attachment 365417)
Mozilla is using hg now, so for future ref, please make patches against hg tip (cvs HEAD is getting old).

The patch doesn't apply cleanly to hg tip, but I've unbitrotted and played with it a bit. Will attach that in a moment.

(no sr needed for mail/ only changes, and I'm not a super-reviewer)

[Mkmelin+mozilla (mkmelin+mozilla)]

Created an attachment (id=365876)
proposed fix (unbitrotted)

Unbitrotted, and creating the file:// url in a safer way.

This patch works, but it's a bit ugly.

Windows doesn't need to do it this way, so I'm thinking the "proper" fix is probably somewhere in the gtk drag service

http://mxr.mozilla.org/comm-central/source/mozilla/widget/src/windows/nsDragService.cpp#559
http://mxr.mozilla.org/comm-central/source/mozilla/widget/src/gtk2/nsDragService.cpp

What do you think?

[Mkmelin+mozilla (mkmelin+mozilla)]

E.g. bug 267426 may be of interest, though it's windows only afaikt.

[Gary-mozillamessaging (gary-mozillamessaging)]

Moving from General -> MWFE, I was torn between MWFE and Message Reader UI, not so sure, feel free to change though.

We'd need litmus tests to cover this - I'm not sure if MozMill can do DnD out to a non-XUL app (i.e. to Desktop).

[Ludovic-mozillamessaging (ludovic-mozillamessaging)]

(In reply to comment #20)

> We'd need litmus tests to cover this - I'm not sure if MozMill can do DnD out
> to a non-XUL app (i.e. to Desktop).

Good point. Do you feel about adding them ?

[Gary-mozillamessaging (gary-mozillamessaging)]

(In reply to comment #21)
> (In reply to comment #20)
>
> > We'd need litmus tests to cover this - I'm not sure if MozMill can do DnD out
> > to a non-XUL app (i.e. to Desktop).
>
> Good point. Do you feel about adding them ?

Not at the moment - this is a Linux-only bug AFAICT (Windows is covered somewhere else), and I touch Linux only barely. Mac doesn't seem to have this issue (but has issues with DnD of multiple-selected attachments, but that's another thing altogether).

[Mkmelin+mozilla (mkmelin+mozilla)]

Tobias: you need to ask review from a specific person. (Set the review flag to <email address hidden>)

You may want to update the patch according to comment 51 first though.

[Philbaseless-firefox (philbaseless-firefox)]

(In reply to comment #51)
>
> >+ mTempFileCreated = PR_FALSE;
>
> don't quite follow its need, but that may be me.

to clear this point, Tobias indicated that GTK (like windows) tends to call the file creation functions more than once so this prevents multiple copies of the temp file.

[Tokoe (tokoe)]

Created an attachment (id=390806)
New version that removes all references to attachments

Sorry for the late reply, exams at university...

The new patch addresses all issues (references to 'attachment', outdated comments etc.) Phil mentioned in previous comment.

[Roc-ocallahan (roc-ocallahan)]

(From update of attachment 390806)
I'll hand this off to our GTK2 superhero

[Dbucher (dbucher)]

FYI bug still exists in version 2.0.0.22 (20090605) MS XP

[Karlt (karlt)]

(From update of attachment 378778)
I assume that comment 46 effectively made this obselete.

[Karlt (karlt)]

(From update of attachment 390806)
Thank you for this. The general approach looks good, but some of the finer
details should be touched up.

It makes it much easier for reviewers to understand the changes and work out
which code is being modified when patches show function names and have additional lines of context. This can be done automatically by adding a [diff] section to ~/.hgrc or .hg/hgrc: https://developer.mozilla.org/en/Mercurial_FAQ#Configuration

>+ // We can not delete the temporary files immediately after the
>+ // drag has finished, because the target application might have not
>+ // copied the temporary file yet. Instead we collect all temporary
>+ // files in mTemporaryFiles array and remove them here in the destructor.

The destination app should not indicate that it is finished until it has
copied the file. This seems to be implied by both the XDND and Motif
protocols:

http://newplanetsoftware.com/xdnd/dragging_files.html
http://www.lesstif.org/InsideLessTif/node89.html

So it should be safe to delete the temporary files in response to the drag-end
(or drag-failed) signals (in SourceEndDragSession()).

We don't really want to keep accumulating these files when the application
runs for months and the files would not be cleaned up if the app doesn't get a
chance to shut down nicely.

Did you see any apps that were indicating that they had finished before they
had copied the file?

In GetSourceList():

>+ // check if application/x-moz-file-promise url is supported.
>+ // If so, advertise text/uri-list.
>+ if (strcmp(flavorStr, kFilePromiseURLMime) == 0) {
>+ GdkAtom urlAtom = gdk_atom_intern(gTextUriListType, FALSE);
>+ GtkTargetEntry *urlTarget =
>+ (GtkTargetEntry *)g_malloc(sizeof(GtkTargetEntry));
>+ urlTarget->target = g_strdup(gTextUriListType);
>+ urlTarget->flags = 0;
>+ /* Bug 331198 */
>+ urlTarget->info = NS_PTR_TO_UINT32(urlAtom);
>+ targetArray.AppendElement(urlTarget);
>+ }

It looks like this can be done even when numDragItems > 1.

Though I don't think we want two gTextUriListType targets when both kURLMime
and kFilePromiseURLMime are supported flavours (unless there's a reason why
that won't happen). If both kFilePromiseURLMime and kURLMime flavours are
available, then which should be used in the text/uri-list data? (We won't
know whether the destination wants to copy or link.) Perhaps they are
unlikely to both occur together and just picking one will be fine.

I don't think handling multiple files would be too difficult, but even if you
prefer to only implement this for one file right now, make sure that
SourceDataGet() is consistent with GetSourceList().

In SourceDataGet():

>+ void *tmpData = NULL;
>+ nsresult rv;
>+ nsCOMPtr<nsISupports> data;
>+ rv = item->GetTransferData(kFilePromiseURLMime,
>+ ...

[Karlt (karlt)]

(From update of attachment 390806)
>+ tmpDir->CreateUnique(nsIFile::DIRECTORY_TYPE, 0775);

Any reason for g+w here? Is this relying on umask.

>+ rv = NS_NewLocalFileOutputStream(getter_AddRefs(outputStream), tmpDir);

And does this just use umask?

I guess the destination app will copy permissions from the file.

Often mail is considered private and apps seem to use go-r even if umask is more permissive.
We don't know that this data is mail here, but I think it would be better to err on the side of being private for this data of unknown nature: 0700 for the dir, and 0600 for the file.

[Mkmelin+mozilla (mkmelin+mozilla)]

Please try a build from

ftp://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-comm-1.9.1/
http://www.mozillamessaging.com/en-US/thunderbird/early_releases/

->WFM per comment 1. Please reopen if you can reproduce with a recent 3.0 build.

[Tokoe (tokoe)]

(In reply to comment #57)

Hej Karl,

> It makes it much easier for reviewers to understand the changes and work out
> which code is being modified when patches show function names and have
> additional lines of context.
Ok, fixed, thanks for the hint!

> >+ // We can not delete the temporary files immediately after the
> >+ // drag has finished, because the target application might have not
> >+ // copied the temporary file yet. Instead we collect all temporary
> >+ // files in mTemporaryFiles array and remove them here in the destructor.
>
> The destination app should not indicate that it is finished until it has
> copied the file. This seems to be implied by both the XDND and Motif
> protocols:
Right, unfortunately the KDE applications have a bug there (well, it is actually a bug/missing feature in Qt) and they return the XDNDFinished signal
too early before they have started copying the file. I talked with the
Qt developers already, however it won't be fixed in near future, therefor
that delayed deleting would be a helpfull workaround.
(See last entry on http://www.freedesktop.org/wiki/Specifications/XDNDRevision)

> We don't really want to keep accumulating these files when the application
> runs for months and the files would not be cleaned up if the app doesn't get a
> chance to shut down nicely.
Is it so common that a Thunderbird app runs for months?

> In GetSourceList():
>
> >+ // check if application/x-moz-file-promise url is supported.
> >+ // If so, advertise text/uri-list.
> >+ if (strcmp(flavorStr, kFilePromiseURLMime) == 0) {
> >+ GdkAtom urlAtom = gdk_atom_intern(gTextUriListType, FALSE);
> >+ GtkTargetEntry *urlTarget =
> >+ (GtkTargetEntry *)g_malloc(sizeof(GtkTargetEntry));
> >+ urlTarget->target = g_strdup(gTextUriListType);
> >+ urlTarget->flags = 0;
> >+ /* Bug 331198 */
> >+ urlTarget->info = NS_PTR_TO_UINT32(urlAtom);
> >+ targetArray.AppendElement(urlTarget);
> >+ }
>
> It looks like this can be done even when numDragItems > 1.
Ok, will check

> I don't think handling multiple files would be too difficult, but even if you
> prefer to only implement this for one file right now, make sure that
> SourceDataGet() is consistent with GetSourceList().
Ok, will have a look.

> In SourceDataGet():
>
> >+ void *tmpData = NULL;
> >+ nsresult rv;
> >+ nsCOMPtr<nsISupports> data;
> >+ rv = item->GetTransferData(kFilePromiseURLMime,
> >+ getter_AddRefs(data),
> >+ &tmpDataLen);
> >+ if (NS_SUCCEEDED(rv)) {
> >+ foundFilePromiseFlavor = PR_TRUE;
> >+ if (tmpData) {
>
> tmpData is not used.
>
> >+ if (foundFilePromiseFlavor == PR_FALSE) {
> >+ PR_LOG(sDragLm, PR_LOG_DEBUG, ("Transferable i...

[Karlt (karlt)]

(In reply to comment #59)
> (In reply to comment #57)
> > The destination app should not indicate that it is finished until it has
> > copied the file. This seems to be implied by both the XDND and Motif
> > protocols:
> Right, unfortunately the KDE applications have a bug there (well, it is
> actually a bug/missing feature in Qt) and they return the XDNDFinished signal
> too early before they have started copying the file. I talked with the
> Qt developers already, however it won't be fixed in near future, therefor
> that delayed deleting would be a helpfull workaround.
> (See last entry on http://www.freedesktop.org/wiki/Specifications/XDNDRevision)
>
> > We don't really want to keep accumulating these files when the application
> > runs for months and the files would not be cleaned up if the app doesn't get a
> > chance to shut down nicely.
> Is it so common that a Thunderbird app runs for months?

I assume mail apps tend to get run continuously (rather than started and quit
repeatedly) so that users are notified of new mail, so I hope Thunderbird will
run for months.

I don't know but I wonder whether or not the app shuts down cleanly when the X
session is ended.

This could be used by any app of unknown function too.

I think deleting the file on a timer could be suitable workaround. (It would
still need to be deleted on exit as well, assuming the timer is not guaranteed
to run before exit.)

Either a glib timer (g_timeout_add? - beware we need to support glib-2.12.x) or
nsITimer should be helpful.
http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsITimer.idl

Once the destination has a file handle, it should be safe to unlink, but it
could take a little while for the destination to get a file handle if it
prompts the user for anything. I guess a timeout of a few minutes should be
suitable.

[Karlt (karlt)]

Just a record of my investigation into XDS:

I looked at the possibility of using the Direct Save Protocol (http://www.newplanetsoftware.com/xds/) as an alternative (more similar to MS behavior) because a blog (http://rodney.id.au/dev/gnome/an-xds-example) said that it was "supported by ROX, Thunar, Konqueror, and Nautilus file managers".

However, I can't see how GTK apps could easily implement this protocol because
GDK doesn't support XdndActionDirectSave. Apparently that action was added
(October 26, 2000) but I don't know why it's needed. Further, even if this
action could be provided, it would be difficult for destinations not
supporting XDS to know how to fall back to text/uri-list given the
unrecognized action.

[Tokoe (tokoe)]

(In reply to comment #57)
Hej Karl,

> In GetSourceList():
>
> >+ // check if application/x-moz-file-promise url is supported.
> >+ // If so, advertise text/uri-list.
> >+ if (strcmp(flavorStr, kFilePromiseURLMime) == 0) {
> >+ GdkAtom urlAtom = gdk_atom_intern(gTextUriListType, FALSE);
> >+ GtkTargetEntry *urlTarget =
> >+ (GtkTargetEntry *)g_malloc(sizeof(GtkTargetEntry));
> >+ urlTarget->target = g_strdup(gTextUriListType);
> >+ urlTarget->flags = 0;
> >+ /* Bug 331198 */
> >+ urlTarget->info = NS_PTR_TO_UINT32(urlAtom);
> >+ targetArray.AppendElement(urlTarget);
> >+ }
>
> It looks like this can be done even when numDragItems > 1.
>
> Though I don't think we want two gTextUriListType targets when both kURLMime
> and kFilePromiseURLMime are supported flavours (unless there's a reason why
> that won't happen). If both kFilePromiseURLMime and kURLMime flavours are
> available, then which should be used in the text/uri-list data? (We won't
> know whether the destination wants to copy or link.) Perhaps they are
> unlikely to both occur together and just picking one will be fine.
So what shall I do? Checking whether an kURLMime/gTextUriListType already exists and only create a new gTextUriListType entry if not?

> I don't think handling multiple files would be too difficult, but even if you
> prefer to only implement this for one file right now, make sure that
> SourceDataGet() is consistent with GetSourceList().
What exactly do you mean with consistent?

> >+ // remove and re-add the text/uri-list flavor to discard any previous
> >+ // data of that flavor
> >+ item->RemoveDataFlavor(gTextUriListType);
> >+ item->AddDataFlavor(gTextUriListType);
> >+
> >+ // set the file:///tmp/dnd_file/<filename> URL for text/uri-list flavor
> >+ item->SetTransferData(gTextUriListType, genericDataWrapper, ucs2Str.Length() * 2);
>
> The side-effect of modifying the transferable here doesn't feel right to me.
> The transferable is an app-internal reference to the data, while the external
> uri list, generated here for the GtkSelectionData for other apps, is likely to
> be different. (Internally the data can be referenced from the original URI,
> rather than from the file intended for external use.)
So how can I return the url of the temporary file instead of changing the transferable?!?

Ciao,
Tobias

[Karlt (karlt)]

(In reply to comment #62)
> (In reply to comment #57)

> > If both kFilePromiseURLMime and kURLMime flavours are
> > available, then which should be used in the text/uri-list data? (We won't
> > know whether the destination wants to copy or link.) Perhaps they are
> > unlikely to both occur together and just picking one will be fine.

> So what shall I do? Checking whether an kURLMime/gTextUriListType already
> exists and only create a new gTextUriListType entry if not?

In GetSourceList() when numDragItems > 1, gTextUriListType is the only target
that will be supported, so it's fine to break from the loop when the first is
added.

The real decision will be in SourceDataGet() (or CreateUriList) when choosing
whether to provide the uri from the kFilePromiseURLMime or kURLMime flavor. I
don't know what the right answer is. I'd be happy with even just a comment
indicating that it is not clear which is better.

> > make sure that SourceDataGet() is consistent with GetSourceList().
> What exactly do you mean with consistent?

It's probably enough to make sure that SourceDataGet does
gtk_selection_data_set for targets that are advertised in GetSourceList. (I
can't remember but I was probably also thinking that data should not be set
for targets that were not advertised, but that principal doesn't appear to be
satisfied in current code anyway.)

> So how can I return the url of the temporary file instead of changing the
> transferable?!?

Return it using gtk_selection_data_set(). I expect CreateUriList() will
probably need some modification.

[Tokoe (tokoe)]

Created an attachment (id=403730)
Patch that addresses the problems from previous review

The new patch has the following improvements
  - uses a timer to delete the list of temporary files
  - uses gtk_selection_data_set instead of changing the Transferable object
  - code that creates temporary file is located in separated method

[Karlt (karlt)]

(From update of attachment 403730)
>diff --git a/widget/src/gtk2/nsDragService.cpp b/widget/src/gtk2/nsDragService.cpp
>--- a/widget/src/gtk2/nsDragService.cpp
>+++ b/widget/src/gtk2/nsDragService.cpp
>@@ -53,26 +53,31 @@
> #include "nsNetUtil.h"
> #include "prlog.h"
> #include "nsVoidArray.h"
> #include "nsPrimitiveHelpers.h"
> #include "prtime.h"
> #include "prthread.h"
> #include <gtk/gtkinvisible.h>
> #include <gdk/gdkx.h>
>+#include <unistd.h>
> #include "nsCRT.h"
>
> #include "gfxASurface.h"
> #include "gfxXlibSurface.h"
> #include "gfxContext.h"
> #include "nsImageToPixbuf.h"
> #include "nsIPresShell.h"
> #include "nsPresContext.h"
> #include "nsIDocument.h"
> #include "nsISelection.h"
>+#include "nsDirectoryService.h"
>+#include "nsDirectoryServiceDefs.h"
>+#include "nsEscape.h"
>+#include "nsString.h"
>
> // This sets how opaque the drag image is
> #define DRAG_IMAGE_ALPHA_LEVEL 0.5
>
> // These values are copied from GtkDragResult (rather than using GtkDragResult
> // directly) so that this code can be compiled against versions of GTK+ that
> // do not have GtkDragResult.
> // GtkDragResult is available from GTK+ version 2.12.
>@@ -141,16 +146,19 @@ nsDragService::nsDragService()
> PR_LOG(sDragLm, PR_LOG_DEBUG, ("nsDragService::nsDragService"));
> mTargetWidget = 0;
> mTargetDragContext = 0;
> mTargetTime = 0;
> mCanDrop = PR_FALSE;
> mTargetDragDataReceived = PR_FALSE;
> mTargetDragData = 0;
> mTargetDragDataLen = 0;
>+ mTempFileCreated = PR_FALSE;
>+ mTimer = do_CreateInstance("@mozilla.org/timer;1");
>+
> }
>
> nsDragService::~nsDragService()
> {
> PR_LOG(sDragLm, PR_LOG_DEBUG, ("nsDragService::~nsDragService"));
> }
>
> NS_IMPL_ISUPPORTS_INHERITED2(nsDragService, nsBaseDragService,
>@@ -173,16 +181,36 @@ nsDragService::Observe(nsISupports *aSub
> } else {
> NS_NOTREACHED("unexpected topic");
> return NS_ERROR_UNEXPECTED;
> }
>
> return NS_OK;
> }
>
>+// nsITimer
>+
>+NS_IMETHODIMP
>+nsDragService::Notify(nsITimer *timer)
>+{
>+ // We can not delete the temporary files immediately after the
>+ // drag has finished, because the target application might have not
>+ // copied the temporary file yet. Instead we collect all temporary
>+ // files in mTemporaryFiles array and remove them here in the timer event.
>+ PRUint32 count = mTemporaryFiles.Count();
>+
>+ while (count > 0) {
>+ --count;
>+ mTemporaryFiles[count]->Remove(PR_TRUE);
>+ }
>+ mTemporaryFiles.Clear();
>+
>+ return NS_OK;
>+}
>+
> // nsIDragService
>
> NS_IMETHODIMP
> nsDragService::InvokeDragSession(nsIDOMNode *aDOMNode,
> nsISupportsArray * aArrayTransferables,
> nsIScriptableRegion * aRegion,
> PRUint32 aActionType)
> {
>@@ -321,16 +349,18 @@ nsDragService::SetAlphaPixmap(gfxASurfac
> gdk_pixmap_unref(pixmap);
> return PR_TRUE;
> }
>
> NS_IMETHODIMP
> nsDragService::StartDragSession()
> {
> PR_LOG(sDragLm, PR_LOG_DEBUG, ("nsDragService::StartDragSession"));
>+ mTempFileCreated = false;
>+
> return nsBaseDragService::StartDragSession();
> }
>
> NS_IMETHODIMP
> nsDragServi...

[Karlt (karlt)]

Oops, sorry, clicked in the wrong place!

[Karlt (karlt)]

(From update of attachment 403730)
Thanks, again! Sorry for the delay on this.

>+ // We can not delete the temporary files immediately after the
>+ // drag has finished, because the target application might have not
>+ // copied the temporary file yet. Instead we collect all temporary
>+ // files in mTemporaryFiles array and remove them here in the timer event.

Can you be more specific about the Qt issue please, so that we will know when
it is fixed and this code can be removed? A link to a bug report would be
ideal.

>+ PRUint32 count = mTemporaryFiles.Count();
>+
>+ while (count > 0) {
>+ --count;
>+ mTemporaryFiles[count]->Remove(PR_TRUE);
>+ }

A "for" loop would work well here.

>+ mTempFileCreated = false;

Use PR_FALSE for PRBools.

>+nsresult GetDownloadDetails(nsITransferable * aTransferable, nsIURI **aSourceURI, nsAString &aFilename)

This function can be static and file style is to put the function name on a
new line.

Can you move this to just above CreateTempFile(), where it is used, please?

>+ NS_ENSURE_TRUE(aTransferable, NS_ERROR_FAILURE);

NS_PRECONDITION is better here. It is a debug-only check; it shouldn't be
necessary to check in production code.

>+ NS_ENSURE_TRUE(srcUrlPrimitive, NS_ERROR_FAILURE);

Consensus seems to be to deprecate NS_ENSURE_TRUE, and I agree:
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/ccb87d7e7d9e688f

Instead, use something like:

  if (!srcUrlPrimitive)
    return NS_ERROR_FAILURE;

>+ mTimer->Cancel();
>+ mTimer->InitWithCallback(static_cast<nsITimerCallback*>(this),
>+ NS_DND_TIMEOUT,
>+ nsITimer::TYPE_ONE_SHOT);

This looks like a sensible place to cancel the previous timer.

A better place to start the timer would be SourceEndDragSession().

Can you update the comment for NS_DND_TIMEOUT to explain the new use, please?

>+ outputStream->Close();

I think this is the place to check that buffers have been successfully flushed
to disk. (No need to check inputStream->Close().)

>+ char hostname[255];
>+ gethostname(hostname, sizeof(hostname));

Need to ensure that there is a NUL terminator here.
(Just make the last byte NUL.)

>+ nsCAutoString hostnameStr;
>+ hostnameStr.AssignASCII(hostname);
>+
>+ fileURL->SetHost(hostnameStr);

The string copy can be skipped by using:

  fileURL->SetHost(nsDependentCString(hostname))

>+ if (strcmp(mimeFlavor, gTextUriListType) == 0) {
>+ PRUint32 i, count;
>+ mSourceDataItems->Count(&count);
>+
>+ // check whether transferable contains FilePromiseUrl flavor...
>+ PRBool foundFilePromiseFlavor = PR_FALSE;
>+ for (i = 0; i < count; i++) {

kFilePromiseURLMime was only advertised (in GetSourceList) as a uri-list when
numDragItems == 1 and only one temporary file will be created, so only
consider providing the temporary file when numDragItems == 1.

Can this block be merged with the previous "strcmp(mimeFlavor,
gTextUriListType) == 0" block?

>+ NS_ConvertUTF16toUTF8 tempFileUrl(mTempFileUrl);

By making mTempFileUri ...

[Tokoe (tokoe)]

Created an attachment (id=411924)
Patch that addresses the issues found on previous review

This patch should fix all mentioned issues. Only the 'check that all data are flushed' confused me... doesn't Close() flush the stream automatically before it closes?

[Philbaseless-firefox (philbaseless-firefox)]

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

[Karlt (karlt)]

(From update of attachment 411924)
(In reply to comment #68)
> This patch should fix all mentioned issues. Only the 'check that all data are
> flushed' confused me... doesn't Close() flush the stream automatically before
> it closes?

Yes, it tries to, but that may not succeed. Check the return value of
outputStream->Close() to see whether it succeeded.

>+ // (http://bugreports.qt.nokia.com/browse/QTBUG-5441)

Thanks for filing that. That looks like a sensible suggestion (from my
limited understanding of the Qt APIs).

>+ tmpDir->CreateUnique(nsIFile::DIRECTORY_TYPE, 0700);

I think there should be a check for success here before adding to
mTemporaryFiles to be deleted.

The directory is created here, but this function can still fail to set
mTempFileUri (after cancelling the timer) ...

>+ if (!mTempFileUrl.IsEmpty()) {
>+ // start time to remove temporary files
>+ mTempFileTimer->InitWithCallback(static_cast<nsITimerCallback*>(this),
>+ NS_DND_TIMEOUT,
>+ nsITimer::TYPE_ONE_SHOT);

so I think this should check mTemporaryFiles.Count() instead of mTempFileUrl.

>+ // check whether transferable contains FilePromiseUrl flavor...
>+ PRBool foundFilePromiseFlavor = PR_FALSE;
>+ PRUint32 i;
>+ for (i = 0; i < numDragItems; i++) {

No need for a loop here as numDragItems == 1.

[Philbaseless-firefox (philbaseless-firefox)]

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

[drx (drx)]

Thunderbird in 10.04 still doesn't support drag&drop of attachments to nautilus or the desktop.

If you try to, the attached error dialog shows up, stating (my translation):

An error occured trying to read information about »fetch>UID>.INBOX>14282«.
(Error Details:) The specified location is not supported.

[Paolo Montrasio (paolo-paolomontrasio)]

The bug is still there in Thunderbird 3.1.7 (Ubuntu 10.4)

[Lorenzo (lsutton)]

I can also confirm this in Thunderbird 3.1.7 on Ubuntu 10.4 (Gnome
With a warning message:
Error while copying.
There was an error getting information about "[folder_name]>[number]".
An in the show more details:
The specified location is not supported

Lorenzo.

[Paolo Montrasio (paolo-paolomontrasio)]

Bug still here, TB 3.1.7 + Ubuntu 10.10 + gnome 2.32.0 and almost 4 years since the creation of the bug (2007-04-16)

[Wintermute (olivier-desportes)]

I also have this bug in all my ubuntu 10.x boxes (64) when trying to drag n drop attached files in TB 3.1x. Why is it related to the first occurrence, four years ago ? The symptoms are totally different : a message box pops up telling "Error while copying. There was an error getting information etc etc" like Lorenzo wrote above.
Please handle this quickly, my linux-hating users are jumping on this stupid bug to show how clunky linux is... :s

[Dylan Justice (dsjstc)]

Confirmed still present in Thunderbird 5, Ubuntu 10.10.

[Mozilla-stukjewebgebeuren (mozilla-stukjewebgebeuren)]

Just so no-one forgets about this bug, I'm happy to announce that Thunderbird 5.0 on Ubuntu 10.10 still has this bug.

;(

[Romano Giannetti (romano-giannetti)]

Yep, confirmed here too. Quite unnerving, if I may say. It's a 4 years old bug on something that users will notice in the first 5 minutes of usage.

[O-register (o-register)]

Confirmed using TB 7.0a2 nightlies on OSX Lion, when I drag an attachment to desktop it creates an empty file named "undefined" instead of the expected attachment.

[aova (hot-alex-mail)]

Bug confirmed, TB 3.1.11 + Ubuntu 10.04 + gnome 2.32.0

[Paolo Montrasio (paolo-paolomontrasio)]

Still there in Ubuntu 11.04, TB 3.1.12
I'm sure there is some good reason for the fix is taking so long to deliver after that spur of activity back in 2009. It would be appreciated if the developers could explain what is it. Maybe we can think all together about how to remove any hurdle preventing the resolution of this issue.

[Sagarwal (sagarwal)]

Repeated comments are not going to help get the bug fixed any faster. Please stop commenting unless you're willing to take on the bug or have a patch.

[Paolo Montrasio (paolo-paolomontrasio)]

Your comment reminds me of http://www.reddit.com/r/programming/comments/c2b4l/open_source_openofficeorg_we_have_a_problem/c0pqvz6

[Chris Coulson (chrisccoulson)]

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

[Romano Giannetti (romano-giannetti)]

The behaviour changed. Now when I drag and drop, on Ubuntu 11.10, i have a ".desktop" file created with a "link" to the mail object. The problem is that

1) clicking on the icon give the error Could not display "imap://romano%2Egiannetti%40...1.2&filename=13008%202011.pdf".

2) ...and even if I manage to open it (I can, with some opening and cut and paste and based on the fact that the application understand the URI) what happens if I go offline?

So I think it's worst than ever --- before you had an error, now it could seem that the operation succeeded.

[Paolo Montrasio (paolo-paolomontrasio)]

Not sure it's worse but it seems not what we are used to.
All mail clients I know (and also Thunderbird) create a new file with a full copy of the attachment. That seems to create a link to the attachment. If wonder what happens if one edits the saved attachment (let's say it's a .doc on .odt) but I bet that it's just a bug, not the intended behavior.

[Romano Giannetti (romano-giannetti)]

Still here and TB is default mailer now...
Isn't this a duplicate of bug #151162?

[Lo_pescofi (corbieres)]

This bug run until long times...
I've install a new fresh build: Ubuntu 13.10 (with thunderbird 24.2.0), it's the same.
Drag & Drop an email attachment to the Desktop or other folder don't work.