GNOME Bugzilla – Bug 315389
Add a "Move to Trash" shortcut for all items backed by files
Last modified: 2005-11-30 22:19:35 UTC
Use case: You're listening to a song that you really don't like and even wonder why you have, and want to remove it from Rhythmbox, and the disk. Right now, you need to go to the song's properties, use the "location" to remove it from the disk, then find it in the main library (won't work if you're in an automatic playlist) and find the file to delete it from the library. There should be a "Move song to trash" context menu item for files on the local disk, in the library and every type of playlist.
I would like to add another option: drag items to Trash (nautilus' Trash) to delete it.
Created attachment 54508 [details] [review] rhythmbox-move-to-trash.patch
The patch contains some misc. fixes, including a leak in rb-library-source.c's impl_delete which leaks a list. This version doesn't seem to be able to remove the entries from the DB once they've been moved to the trash though... Should they only be hidden instead?
move_to_trash could be implemented generically in RBSource using rb_source_get_entry_view to get the RBEntryView and g_object_get to get the RhythmDB from RBShell. I'm not sure if it's intended that the playlist implementation of move_to_trash doesn't call rhythmdb_commit where the library implementation does. I don't think it'd make any difference. > +void rhythmbdb_entry_move_to_trash (RhythmDB *db, > + RhythmDBEntry *entry); Does the extra 'b' in 'rhythmbdb' stand for 'B.Y.O.B.' or is it just a typo? Other than that, it looks good to me.
Looks okay to me, although there are a couple of small bugs: * the removable media source (which is derived from the library source) should override can_move_to_trash to return false, so that it doesn't show up for audio cds and ipods. * do you need to call rhythmdb_commit after rhythmbdb_entry_move_to_trash? the library source does, and the playlist source doesn't. * both impl_move_to_trash leak the list * the playlist source implements move_to_trash, but doesn't override can_move_to_trash to return true - so it doesn't work. Currently RB only hides entries where the volume isn't mounted, and deletes entries if the file has just been moved. That needs to be changed anyway to support authenticated remote gnome-vfs mounts.
> > +void rhythmbdb_entry_move_to_trash (RhythmDB *db, > > + RhythmDBEntry *entry); > Does the extra 'b' in 'rhythmbdb' stand for 'B.Y.O.B.' or is it just a typo? Fixed > I'm not sure if it's intended that the playlist implementation of > move_to_trash doesn't call rhythmdb_commit where the library implementation > does. I don't think it'd make any difference. Fixed > * the removable media source (which is derived from the library source) should > override can_move_to_trash to return false, so that it doesn't show up for > audio cds and ipods. There's already a default_can_move_to_trash which does that. > * do you need to call rhythmdb_commit after rhythmbdb_entry_move_to_trash? the > library source does, and the playlist source doesn't. As above, fixed > * both impl_move_to_trash leak the list Fixed > * the playlist source implements move_to_trash, but doesn't override > can_move_to_trash to return true - so it doesn't work. Fixed The playlist changes were an afterthought, and I didn't test them. That will teach me. Any ideas what to do when we can't move the file to the Trash, or there is no trash? Also, know why removing an entry doesn't remove it from the view?
Created attachment 54521 [details] [review] rhythmbox-move-to-trash2.patch
>> * the removable media source (which is derived from the library source) should >> override can_move_to_trash to return false, so that it doesn't show up for >> audio cds and ipods. > > There's already a default_can_move_to_trash which does that. Except that RBRemovableMediaSource derives from RBLibrarySource, which overrides can_move_to_trash to return true. > Also, know why removing an entry doesn't remove it from the view? I'm fairly sure that a couple of the reported crashers are caused by the same root problem. Somewhere a signal seems to be getting lost, and the entry view isn't being updated properly.
Created attachment 54522 [details] [review] rhythmbox-move-to-trash3.patch Fix move to trash being available for the removable media source.
> > Also, know why removing an entry doesn't remove it from the view? > > I'm fairly sure that a couple of the reported crashers are caused by the same > root problem. Somewhere a signal seems to be getting lost, and the entry view > isn't being updated properly. The problem seems to only occur if the view is filtered. It works just fine if it isn't.
A fix for the item not disappearing was committed: 2005-11-14 Bastien Nocera <hadess@hadess.net> * rhythmdb/rhythmdb.c: (rhythmdb_process_stat_event): Patch from Jonathan Matthew <jonathan@kaolin.hn.org> to avoid lingering entry-changed events after stats on startup, fixes entry deleted from filtered views not disappearing I still need to fix the case where the file couldn't be moved to the trash for whatever reason. We decided that the easiest was to use a dialog similar to the one on import failure (lib/rb-load-failure-dialog.[ch])
Created attachment 54878 [details] [review] rhythmbox-move-to-trash6.patch The playback error actually fits the bill nicely (apart from the dialogue's title, obviously). I believe this patch is mergeable.
Created attachment 54889 [details] [review] rhythmbox-move-to-trash7.patch Adds missing popup menu entry for playlists
This looks good. We're aiming to get 0.9.2 out fairly soon, and the patch adds translatable strings, so this is fine to be committed right after we release 0.9.2.
I need to update this patch so that you can't move to the trash files on remote shares (ie. added with smb:, ssh:, etc.) Or can that wait until #168174 is fixed?
Disabling move-to-trash for remote shares could be done using gnome_vfs_uri_is_local() in rhythmdb_entry_move_to_trash.
This needs an update to remove the files completely from the DB if they were already removed. Right now this fails.
This is fine to commit once you've dealt with any issues you think need dealing with.
Committed. This update takes into account already removed files, and deletes remote files from the database if the VFS method doesn't support thrashes. 2005-11-30 Bastien Nocera <hadess@hadess.net> * data/ui/rhythmbox-ui.xml: * rhythmdb/rhythmdb.c: (rhythmdb_entry_move_to_trash_cb), (rhythmbd_entry_move_to_trash_set_error), (rhythmdb_entry_move_to_trash): * rhythmdb/rhythmdb.h: * shell/rb-shell-clipboard.c: (rb_shell_clipboard_sync), (rb_shell_clipboard_cmd_delete), (rb_shell_clipboard_cmd_move_to_trash): * sources/rb-library-source.c: (rb_library_source_class_init), (impl_delete), (impl_move_to_trash): * sources/rb-playlist-source.c: (rb_playlist_source_class_init), (impl_delete), (impl_move_to_trash): * sources/rb-removable-media-source.c: (rb_removable_media_source_class_init): * sources/rb-source.c: (rb_source_class_init), (rb_source_can_move_to_trash), (rb_source_delete), (rb_source_move_to_trash): * sources/rb-source.h: Add move_to_trash member for the sources, implement move to trash for the library and playlists Add a "Move to Trash" context menu item, and menu item (Ctrl+D) to move local files to the trash, and deleting them from the database (Closes: #315389)