Comment 84 for bug 151162

(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 is missing kFilePromiseURLMime flavor\n"));
> >+ return;
> >+ }
> >+
>
> I expect the transferable may often not have kFilePromiseURLMime even though
> someone asks for target type mimeFlavor == gTextUriListType. (If kURLMime is
> a supported flavour, then gTextUriListType will be in the GtkTargetList.) This
> function should continue to return gTextUriListType data even if there is no
> kFilePromiseURLMime flavour.
Ok, have to check.

> >+ rv = NS_NewFileURI(getter_AddRefs(uri), tmpDir);
>
> The uri will need to contain the hostname. Will NS_NewFileURI do this?
Good to know...

> http://newplanetsoftware.com/xdnd/dragging_files.html
>
> Feel free to use glib functions (g_filename_to_uri() perhaps) when dealing
> with GTK as in this function (and others in this file), if that makes things
> easier.
>
> >+ // 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.)
>
> There is lot of code specific to kFilePromiseURLMime added to SourceDataGet()
> here. This dominates the function (which is actually much more general) and
> makes it harder to follow the flow. Most of the new code should be split out
> into one or more static functions or private methods.
Ok, then I'll try to separate it more and send an updated Patch