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.

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

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

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.

(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.

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.

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

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

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.

I could imagine that g_filename_from_uri would fail int that case?

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.

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

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

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

Gentle ping.

(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.

(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 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.)

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

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

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 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 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.

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

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.

(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.)

(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!

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

Pushed by evilpies@gmail.com: 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

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

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

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

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.

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?

Maybe Martin might have some contacts?

(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