GNOME Bugzilla – Bug 632827
Copy/paste to nautilus should copy files
Last modified: 2018-05-24 15:35:19 UTC
Created attachment 172956 [details] [review] Patch to implement copy/pasting to nautilus or to text contexts. When you're looking at a source browser in rhythmbox, and click "Copy", you should be able to go to Nautilus and click "Paste", and it should copy the files you had selected. Instead, Copying in rhythmbox has no effect on the gtk clipboard. I would also say that if you are pasting into a text context, you should get a list of filenames. It should also fill the X PRIMARY clipboard, so that middle-clicking will paste the list of filenames. Also, if you're pasting into a uri-list context, how about a list of the file uris? This is a patch that implements all these things, against HEAD. --Ryan
Do you know if this works in other file managers (thunar)?
Created attachment 173194 [details] [review] Patch to implement copy/pasting to nautilus or text - fixed a segfault.
Good point, I hadn't tested with other FMs. In my testing, I found a segmentation fault bug and fixed it. Then I retested it in these FMs. Here's the ones it works in: * Nautilus * PCManFM * Thunar * Dolphin These FMs don't seem to support clipboard-based copy/paste: * rox-filer * emelfm2 So it looks like it's ready to go on most popular distros. (where "it" is the new patch). --Ryan
Review of attachment 173194 [details] [review]: not sure what the deletion of the changelog is doing here.. Anyway, no major problems, but I'd like a bit more information on exactly what's going on here to make it easier to deal with this code in the future. ::: shell/rb-shell-clipboard.c @@ +151,3 @@ + // When clearing, we will only do so if the current serial is the same as + // the one we're trying to clear (which will be passed into the clear + // callback). no c++-style comments, please @@ +232,3 @@ + +// MIME-types that we will support on the clipboard. +#define RB_CLIPBOARD_COPYFILES_TYPE "x-special/gnome-copied-files" might be a good idea to explain where x-special/gnome-copied-files comes from @@ +240,3 @@ + RB_CLIPBOARD_COPYFILES, + RB_CLIPBOARD_URI_LIST, + RB_CLIPBOARD_TEXT, I think it'd be good to have a comment here explaining what RB_CLIPBOARD_TEXT is, since there's no #define for it and no entry in the fileselection_types array @@ +245,3 @@ +static const GtkTargetEntry fileselection_types [] = { + {"x-special/gnome-copied-files", 0, RB_CLIPBOARD_COPYFILES}, + {"text/uri-list", 0, RB_CLIPBOARD_URI_LIST}, use the #defines here? @@ +255,3 @@ + + +#define RB_NUM_FILESELECTION_TYPES 2 use G_N_ELEMENTS(fileselection_types) instead? @@ +313,3 @@ g_type_class_add_private (klass, sizeof (RBShellClipboardPrivate)); + + copied_files_atom = gdk_atom_intern ("x-special/gnome-copied-files", FALSE); use the #define here too @@ +328,3 @@ shell_clipboard->priv->deleted_queue = g_async_queue_new (); + + shell_clipboard->priv->serial = 0; no need to initialize this to 0, GObject does that for us @@ +914,3 @@ + case RB_CLIPBOARD_TEXT: + if (info == RB_CLIPBOARD_COPYFILES) { + str = g_string_new("copy"); is there any documentation anywhere of how this format works? @@ +918,3 @@ + str = g_string_new(""); + } + for (it=g_list_first(rb_clipboard->priv->entries), i=0; it; it=g_list_next(it), i++) { i is already 0, you don't need to set it to 0 again. g_list_first is unnecessary too, just start at rb_clipboard->priv->entries. @@ +930,3 @@ + GFile *file; + char *file_path; + file = g_vfs_get_file_for_uri(g_vfs_get_default(), uri); any reason not to just use g_file_new_for_uri here? @@ +957,3 @@ + (g_list_length(rb_clipboard->priv->entries)+1) + * sizeof(char *)); + for (it=rb_clipboard->priv->entries, i=0; it; it=g_list_next(it), i++) { as above
Created attachment 173499 [details] [review] Updated copy/paste patch Updated patch that takes review comments into account.
Oh, also: I couldn't find any document explaining x-special/gnome-copied-files. I just read that out of the nautilus source code and did what they did.
Review of attachment 173499 [details] [review]: I'm a bit dubious about this only working for copy operations, not for cut. I can't quite see how it'd be useful to cut from rhythmbox and paste somewhere else, or exactly what would happen in that case (remove the entries from the library? move the files rather than copying?). Have you thought about this at all? ::: shell/rb-shell-clipboard.c @@ +143,3 @@ + gint n_targets; + + /** Only comments that are actually part of the API documentation or introspection annotations should start with '/**'. Other things can interfere with gtk-doc and gobject-introspection. None of the comments you've added are API documentation or annotation, so they should just start with '/*'. @@ +257,3 @@ + * MIME-types that we will support on the clipboard. + */ +#define RB_CLIPBOARD_COPYFILES_TYPE "x-special/gnome-copied-files" if there's no actual documentation of how this works, then instead we should link to the nautilus code that deals with it, using a link to http://git.gnome.org/browse/nautilus/* @@ +373,3 @@ shell_clipboard->priv->deleted_queue = g_async_queue_new (); + + /* GObject will initialize serial to 0. */ you don't even need to comment on this, actually @@ +1031,3 @@ + // If it's just the X PRIMARY clipboard, we don't need to clear our + // internal data structure. We want paste to still keep working until + // the gtk clipboard is replaced. these are still c++ comments
Yeah, "cut" is supposed to move the files. We could put it in here. It didn't strike me that this was something I wanted rhythmbox to be able to do -- too much power to shoot self in foot. But it may in fact be useful to some people. It would also require removing the files' entries from the database, right? I'd also like to offer a confirmation dialog if we're going to let people do this. I can cook something up if you'd like, but I'd be happy to just go ahead with the copy command. Meanwhile, I believe I've incorporated the latest suggestions.
Created attachment 173854 [details] [review] Updated copy/paste patch
Okay, I've been confused about what the cut command should do. Tell me if this makes sense: - Cut from library, paste into file manager: Move the file to that location, remove it from rhythmbox's library. - Cut from library, paste into playlist: Add the song to the playlist. Don't remove anything. - Cut from library, paste into text editor: Paste the filename, don't move the file. - Cut from playlist, paste to file manager: Move the file to that location, remove it from rhythmbox's library. - Cut from playlist, paste into playlist: Remove the song from one playlist, add to another playlist. - Cut from playlist, paste into text editor: Paste the filename, don't move the file. Does this seem like the correct behavior? I had originally imagined using dialogs to warn you that you were going to remove the file, but I don't think that's a good idea. I'm not used to seeing dialogs when I cut/copy/paste, and file managers don't warn you when you cut.
> - Cut from playlist, paste to file manager: > Move the file to that location, remove it from rhythmbox's library. This is the only one that doesn't seem obviously right, but the only other possibility is removing it from the playlist (but not the library) and moving the file, which doesn't seem much better. I agree that confirmation dialogs don't seem necessary.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/rhythmbox/issues/997.