396370 - Support for XdndDirectSave

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Marco Pesenti Gritti]

Nautilus recently added support for XdndDirectSave:

http://bugzilla.gnome.org/show_bug.cgi?id=171655

The protocol specification is here:

http://www.newplanetsoftware.com/xds/ 

It would be nice to support it to get drag and drop of images from mozilla to nautilus to really work.

Comment 1

[Marco Pesenti Gritti]

Attachment: Very first go (not ready for review)

Comment 2

[Tom S]

Awesome, I was just working one something similar to this. I hope this works.

Comment 3

[Tom S]

Attachment: wip v2

I refreshed the previous patch. I also removed kFilePromiseFullpathMime and replaced it by splitting the file path and setting kFilePromiseDirectoryMime/kFilePromiseDestFilename.

Comment 4

[Karl Tomlinson (:karlt back Feb 10)]

Comment on attachment 813218 [details] [diff] [review]
wip v2

This is an alternative, probably better, solution to bug 377621, thanks.
It is recommended on http://freedesktop.org/wiki/Draganddropwarts/.

I considered this in bug 377621 comment 61, but ruled it out because
http://www.newplanetsoftware.com/xds/ says "The source should specify the
action XdndActionDirectSave", but GdkDragAction does not support this, so this
would be difficult with GDK.  I don't know why XdndActionDirectSave would be
necessary.

However, the patch on https://bugzilla.gnome.org/show_bug.cgi?id=171655
and https://github.com/piksels-and-lines-orchestra/gimp/blob/master/app/widgets/gimpdnd-xds.c
don't use XdndActionDirectSave, so I guess at least some implementations are
getting by without it.

Looking over this patch quickly, I see two things that should be addressed:

The source "must delete the XdndDirectSave property when it is finished."

The hostname of the URL should be checked before writing the file.
If there is no hostname, then I guess have to just continue blindly.

Flag me for review, please, when ready.

Comment 5

[Tom S]

(In reply to Karl Tomlinson (:karlt) from comment #4)
> Comment on attachment 813218 [details] [diff] [review]

> Looking over this patch quickly, I see two things that should be addressed:
> 
> The source "must delete the XdndDirectSave property when it is finished."
> 
Good catch
> The hostname of the URL should be checked before writing the file.
> If there is no hostname, then I guess have to just continue blindly.
> 
I don't understand what you mean by this? We don't really care about the hostname in this code. Did you mean empty filename?
> Flag me for review, please, when ready.

Comment 6

[Tom S]

Attachment: wip v3

Cleaned up the previous patch a bit and we also remove the property now. Didn't fix anything with hostnames as indicated in the previous comment.

Comment 7

[Tom S]

Oh and tested it with rox-filer. Sadly Linux Mint/Cinnamon Desktop doesn't even support this, right :(

Comment 8

[Tom S]

I am still not sure about the SetTransferData aDataLen parameter. Some instances seem to pass length() * sizeof(PRUnichar) ?

Comment 9

[Karl Tomlinson (:karlt back Feb 10)]

I'm wanting to review the new patch, but have had too many distractions, so
will at least answer the question to which I know the answer. 

(In reply to Tom Schuster [:evilpie] from comment #5)
> > The hostname of the URL should be checked before writing the file.
> > If there is no hostname, then I guess have to just continue blindly.
> > 
> I don't understand what you mean by this? We don't really care about the
> hostname in this code.

The URL from the file manager should be of the form file://host/path/name, as
described in the XDS spec.

Consider, for example, the case of someone starting an ssh connection to host
B from a terminal running on host A, and then starting a file manager on host
B to display on host A.  If the user drags from a local application on host A
to the file manger, the file manager would return "file://b/path/name".  The
local application may not be able to save to host B, and can report a failure,
as indicated in the spec, but it must not write to /path/name on host A.

http://www.newplanetsoftware.com/xdnd/dragging_files.html describes a
different protocol but says to use gethostname() for the hostname, so I
suggest using the same function for consistency in XDS.  It also points out that
WM_CLIENT_MACHINE (on the source window) can be used if there is no host in the url, so we don't need to continue blindly if there is no hostname in the URL.

Comment 10

[Tom S]

I could imagine that g_filename_from_uri would fail int that case?

Comment 11

[Karl Tomlinson (:karlt back Feb 10)]

g_filename_from_uri does not check that the hostname in the uri matches the local hostname.  If the uri has no hostname (e.g. file:///path/name) then NULL with be stored in the hostname parameter, if provided.  The error status of g_filename_from_uri does not depend on whether the hostname parameter is provided.

(In reply to Karl Tomlinson (:karlt) from comment #9)
> WM_CLIENT_MACHINE (on the source window) can be used if there is no host in
> the url

That's actually on the destination window, sorry, but I'm not so concerned about dealing with broken file managers.  That can be added separately in the future, if required.

Comment 12

[Tom S]

I think this more trouble than it's worth. I doubt anyone actually implemented such a check.

Comment 13

[Tom S]

Attachment: v1

I implemented such a check, but haven't tested, so it might be totally wrong.

Comment 14

[Tom S]

Let's clarify comment 12. Not a single implementation of XDS that I have found has such a check.

Comment 15

[Tom S]

Gentle ping.

Comment 16

[Karl Tomlinson (:karlt back Feb 10)]

(In reply to Tom Schuster [:evilpie] from comment #8)
> I am still not sure about the SetTransferData aDataLen parameter. Some
> instances seem to pass length() * sizeof(PRUnichar) ?

It does seem very pointless passing this around everywhere, instead of getting it when required from the data object.
It is used as the number of bytes (not characters) in a string, without the nul terminator I think, at
http://hg.mozilla.org/mozilla-central/annotate/396e59370945/widget/xpwidgets/nsTransferable.cpp#l153
Note that CreateDataFromPrimitive currently only returns a non-null buff for text/plain nsISupportsCString primitives and nsISupportsString primitives for other mime types.

I doubt aDataLen is used for nsIFile.  It would be non-sensical to try to cache an nsIFile object or its pointer to disk.  I would pass 0, but I see nsChildView.mm passes sizeof(nsIFile*), so I guess copying that would be consistent FWIW.

Comment 17

[Karl Tomlinson (:karlt back Feb 10)]

(In reply to Tom Schuster [:evilpie] from comment #14)
> Let's clarify comment 12. Not a single implementation of XDS that I have
> found has such a check.

Oh.  I'm sad to hear that.  nsContentAreaDragDrop.cpp uses nsIFile::CreateUnique() so at least skipping the WM_CLIENT_MACHINE check won't overwrite existing files when writing to the wrong host.

Comment 18

[Karl Tomlinson (:karlt back Feb 10)]

Comment on attachment 819187 [details] [diff] [review]
v1

Thank you, Tom, for tidying this up and making it ready to go.  We should also include Marco in the author list.

The only thing below that isn't trivial I think is the timing of deleting the
property.

>+        PR_LOG(sDragLm, PR_LOG_DEBUG, ("actualFlavor is %s\n", actualFlavor));

This should be the same as typeName logged above, so please leave this out.

>+            if (!gdk_property_get(aContext->source_window,
>+                                  property, type, 0, 4096,
>+                                  FALSE, NULL, NULL,
>+                                  &length, &data)) {
>+                return;
>+            }

Can you replace NULLs with nullptr, please?  That offers a little more type
safety, in arguments for example, than a plain 0.

Please use gdk_drag_context_get_source_window for GTK3, and add this to
widget/gtk/compat/gdk/gdkdnd.h.  There are 2 other instances also.

I assume 4096 comes from PATH_MAX.  A URL could be considerably longer if it
contains escaped characters.  You can use INT32_MAX for the length.  That
would work around problems with gdk_property_get not handling
bytes_after_return, but truncating strings.  (Even though the parameter is
unsigned long, Xlib truncates to unsigned int for the wire protocol.
UINT32_MAX would cause problems on platforms where that is equal to
G_MAX_ULONG because gdk_property_get adds 3.)

>+
>+            // Zero-terminate the string.
>+            data = (guchar *)g_realloc(data, length + 1);
>+            if (!data)
>+                return;
>+            data[length] = '\0';

Writing this down, just because it took me some time to work out:

"XGetWindowProperty always allocates one extra byte in prop_return (even if
 the property is zero length) and sets it to zero so that simple properties
 consisting of characters do not have to be copied into yet another string
 before use."

However, gdk_property_get then copies prop_return to another array that is not
null-terminated :(  Perhaps that's the reason for this disclaimer:

"The XGetWindowProperty() function that gdk_property_get() uses has a very
 confusing and complicated set of semantics. Unfortunately, gdk_property_get()
 makes the situation worse instead of better (the semantics should be
 considered undefined), and also prints warnings to stderr in cases where it
 should return a useful error to the program. You are advised to use
 XGetWindowProperty() directly until a replacement function for
 gdk_property_get() is provided."

But at least gdk_property_get handles format to length conversion for us.

>+            // Direct save has some weird way of indicating
>+            // success/failure, it's probably not even used by
>+            // anything.
>+            const char *result = NS_SUCCEEDED(rv) ? "S" : "F";
>+            gtk_selection_data_set(aSelectionData,
>+                                   aSelectionData->target,

It is a bit weird.  It is a way to return 3 possible values, one success, and
two different failures.

Thunar and Nautilus, at least, use this.
https://git.gnome.org/browse/nautilus/tree/libnautilus-private/nautilus-tree-view-drag-dest.c?id=9fd0e21817ef1392a2d1f81308ec835dd2fa373d#n851
http://git.xfce.org/xfce/thunar/commit/thunar/thunar-standard-view.c?id=134d0187afd0c2ebc2d0e47fbde4bda71b25fbb9

"F" means that the source can't save to the location because it is on a
different machine, and that the destination should try
application/octet-stream.  Here saving the file has failed for some other
reason, and Gecko doesn't seem to offer application/octet-stream, so 'E' would
be the appropriate character to return.

Perhaps the other early return paths should also respond with 'E', to follow
the specified way to indicate an error.  One way to do that would be to call
gtk_selection_data_set(aSelectionData, target, (const guchar*)"E", 1)
immediately after the strcmp(mimeFlavor, gDirectSaveType) test and then it can
be changed to 'S' if everything succeeds.

Please use "target" instead of "aSelectionData->target" for GTK3.

>+            gdk_property_delete(aContext->source_window, property);

The spec says that this property should be removed after receiving
XdndFinished.  This would translate into the drag-end signal or the
earlier drag-failed signal would be fine, if received.  Before the early
returns in SourceEndDragSession() would be fine.

(The spec only talks about the destination modifying the property if it
receives 'F', but it also talks about the source checking for changes in the
property even when 'S' is sent, so the destination may assume the property
still exists before it sends XdndFinished.)

Comment 19

[Tom S]

Thanks for the review. I will try to fix this, but I find writing that code to not be very enjoyable.

Comment 20

[Tom S]

I don't think I will do this anytime soon.

Comment 21

[Tom S]

Attachment: v1.1

This is almost the same patch as v1 (from 5 years ago!). I tried to address your review comments, but might have forgotten something. Apparently Nemo still can't handle XdndDirectSave correctly (https://github.com/linuxmint/nemo/issues/407), so I used Thunar.

Comment 22

[Tom S]

Comment on attachment 9011007 [details] [diff] [review]
v1.1

Review of attachment 9011007 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gtk/nsDragService.cpp
@@ +1688,5 @@
> +
> +            guchar *data;
> +            gint length;
> +            if (!gdk_property_get(gdk_drag_context_get_source_window(aContext),
> +                                  property, type, 0, 4096,

I guess I forgot to change this to INT32_MAX.

Comment 23

[Karl Tomlinson (:karlt back Feb 10)]

Comment on attachment 9011007 [details] [diff] [review]
v1.1

Please use 8 lines of context for patches, or Phabricator.

+@@ -1308,6 +1312,8 @@ nsDragService::GetSourceList(void)
+             do_QueryElementAt(mSourceDataItems, 0);
+ 
+         if (currItem) {
++            currItem->SetRequestingPrincipal(mSourceNode->NodePrincipal());
++

Can this change be done in a separate patch, with a commit message explaining
the change, please?

Is there a reason why a getter method is setting this?

I assume this is related to the regression reported in
https://bugzilla.mozilla.org/show_bug.cgi?id=1487263 ?

But if all implementations need to add the principals, then why were they not
added when the transferable was constructed?

If that is not appropriate, then wouldn't
nsBaseDragService::InvokeDragSession() be a better place to do this?

I don't know the difference between a triggering principal and a requesting
principal, and the documentation has not been kept up to date.  We'll need
someone who knows to review or advise and the documentation updated.
https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/widget/nsITransferable.idl#211

I suggest looking at the blame of associated code to find a reviewer.

++    // Remove this property to avoid confusion.

"Remove this property, if it exists, to satisfy the Direct Save Protocol."
(or explain what confusion is avoided.)

-+            char *fullpath = g_filename_from_uri((const gchar *)data, &hostname, NULL);
-+            g_free(data);
-+            if (!fullpath)
++            char *gfullpath = g_filename_from_uri((const gchar *)data, &hostname, NULL);
++            if (!gfullpath)
 +                return;

Don't remove g_free(data), or it will be leaked.

++            nsCString fullpath;
++            fullpath.Assign(gfullpath);

nsCString fullpath(gfullpath) should work, I expect.

++                    if (!host.EqualsASCII(hostname)) {

Use Equals(hostname).
No need for the ASCII assertion.

-+                                (guchar *)fileNameCStr.get(),
-+                                fileNameCStr.Length());
++                                (guchar*) g_strdup((gchar*)fileNameCStr.get()),
++                                fileNameCStr.Length() + 1);

g_strdup() looks like it will leak.
Why was that needed?

Items in previous review not addressed:
We should also include Marco in the author list.
Comment 22.

Comment 24

[Tom S]

Hey Christoph! Can you answer the question about SetRequestingPrincipal in the previous comment? I was mostly following the Windows code when adding this (https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/widget/windows/nsDragService.cpp#202). The principal is required for the code that actually saves the file to disk: https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/dom/base/nsContentAreaDragDrop.cpp#348

Comment 25

[Tom S]

Thanks for reviewing.

(In reply to Karl Tomlinson (:karlt) from comment #23)
> Comment on attachment 9011007 [details] [diff] [review]
> v1.1
> 
> -+                                (guchar *)fileNameCStr.get(),
> -+                                fileNameCStr.Length());
> ++                                (guchar*)
> g_strdup((gchar*)fileNameCStr.get()),
> ++                                fileNameCStr.Length() + 1);
> 
> g_strdup() looks like it will leak.
> Why was that needed?
> 
I thought this was causing crashes, but now I removed it and can't reproduce them. I guess I can just use the .get() directly. Judging from the gdk_property_get code above, Length() + 1 should probably also just be Length(). Sadly the documentation is not great.

Comment 26

[Karl Tomlinson (:karlt back Feb 10)]

(In reply to Tom Schuster [:evilpie] from comment #25)
> Judging from the
> gdk_property_get code above, Length() + 1 should probably also just be
> Length(). Sadly the documentation is not great.

Yes, Length() sounds right thanks.  (This convention in text properties is probably in a different document, but I don't know where to look for documentation.  GTK doesn't include the NUL byte fwiw.)

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tom Schuster [:evilpie] from comment #24)
> Hey Christoph! Can you answer the question about SetRequestingPrincipal in
> the previous comment?

I think Karl is right and we can set the RequestingPrincipal within InvokeDragSession(). I am not sure if we need to file a separate bug for this, or if we can incorporate the change here. Since you are on it, please also set the ContentPolicyType - thanks!

Comment 28

[Tom S]

Attachment: Bug 396370 - Set RequestingPrincipal and ContentPolicy in BaseDragService. r?ckerschb

Comment 29

[Tom S]

Attachment: Bug 396370 - Gtk (XDnd) Image/File drag and drop support r?karlt

Based on a patch from Marco Pesenti Gritti (11 years ago)

Depends on D7407

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7790aa7225e2
Set RequestingPrincipal and ContentPolicy in BaseDragService. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/f49d04b37bb2
Gtk (XDnd) Image/File drag and drop support r=karlt

Comment 31

[Tom S]

I think this might qualify for a Linux specific release not considering for how long we haven't supported this.

Comment 32

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/7790aa7225e2
https://hg.mozilla.org/mozilla-central/rev/f49d04b37bb2

Comment 33

[Julien Cristau [:jcristau]]

What is this doing and what is the suggested text for a release note?

Comment 34

[Tom S]

Something like: "Firefox now supports Drag and Drop for images from the browser to other Linux/X11 applications like file managers", not super happy with that wording, especially because I wanted to have "Drag and Drop" in there. Not sure if this important enough for a release note.

Comment 35

[Tom S]

Maybe we should hold of on this for now. I couldn't get this to work with Nautilus (Gnome), Thunar (Xfce) works out of the box, and nemo (Mint/Cinnamon) requires this patch: https://github.com/linuxmint/nemo/pull/1964. 

Is there somebody from the Gnome team who could look into this?

Comment 36

[Karl Tomlinson (:karlt back Feb 10)]

Maybe Martin might have some contacts?

Comment 37

[Martin Stránský [:stransky] (ni? me) (PTO, back on Feb 17)]

(In reply to Tom Schuster [:evilpie] from comment #35)
> Maybe we should hold of on this for now. I couldn't get this to work with
> Nautilus (Gnome), Thunar (Xfce) works out of the box, and nemo
> (Mint/Cinnamon) requires this patch:
> https://github.com/linuxmint/nemo/pull/1964. 
> 
> Is there somebody from the Gnome team who could look into this?

Please file an issue at https://gitlab.gnome.org/GNOME/nautilus/issues