After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 315389 - Add a "Move to Trash" shortcut for all items backed by files
Add a "Move to Trash" shortcut for all items backed by files
Status: RESOLVED FIXED
Product: rhythmbox
Classification: Other
Component: User Interface
HEAD
Other Linux
: Normal normal
: ---
Assigned To: RhythmBox Maintainers
RhythmBox Maintainers
Depends on:
Blocks:
 
 
Reported: 2005-09-06 20:31 UTC by Bastien Nocera
Modified: 2005-11-30 22:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
rhythmbox-move-to-trash.patch (15.90 KB, patch)
2005-11-08 22:31 UTC, Bastien Nocera
none Details | Review
rhythmbox-move-to-trash2.patch (16.28 KB, patch)
2005-11-09 09:05 UTC, Bastien Nocera
none Details | Review
rhythmbox-move-to-trash3.patch (17.53 KB, patch)
2005-11-09 09:48 UTC, Bastien Nocera
none Details | Review
rhythmbox-move-to-trash6.patch (18.13 KB, patch)
2005-11-17 17:50 UTC, Bastien Nocera
none Details | Review
rhythmbox-move-to-trash7.patch (18.50 KB, patch)
2005-11-17 20:14 UTC, Bastien Nocera
none Details | Review

Description Bastien Nocera 2005-09-06 20:31:13 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.
Comment 1 Nguyen Thai Ngoc Duy 2005-09-19 11:58:45 UTC
I would like to add another option: drag items to Trash (nautilus' Trash) to
delete it.
Comment 2 Bastien Nocera 2005-11-08 22:31:19 UTC
Created attachment 54508 [details] [review]
rhythmbox-move-to-trash.patch
Comment 3 Bastien Nocera 2005-11-08 22:33:00 UTC
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?
Comment 4 Jonathan Matthew 2005-11-08 23:51:09 UTC
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.
Comment 5 James "Doc" Livingston 2005-11-08 23:54:46 UTC
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.
Comment 6 Bastien Nocera 2005-11-09 09:03:02 UTC
> > +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?
Comment 7 Bastien Nocera 2005-11-09 09:05:51 UTC
Created attachment 54521 [details] [review]
rhythmbox-move-to-trash2.patch
Comment 8 James "Doc" Livingston 2005-11-09 09:18:56 UTC
>> * 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.
Comment 9 Bastien Nocera 2005-11-09 09:48:33 UTC
Created attachment 54522 [details] [review]
rhythmbox-move-to-trash3.patch

Fix move to trash being available for the removable media source.
Comment 10 Bastien Nocera 2005-11-13 21:51:05 UTC
> > 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.
Comment 11 Bastien Nocera 2005-11-14 11:21:02 UTC
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])
Comment 12 Bastien Nocera 2005-11-17 17:50:11 UTC
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.
Comment 13 Bastien Nocera 2005-11-17 20:14:38 UTC
Created attachment 54889 [details] [review]
rhythmbox-move-to-trash7.patch

Adds missing popup menu entry for playlists
Comment 14 James "Doc" Livingston 2005-11-21 11:51:48 UTC
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.
Comment 15 Bastien Nocera 2005-11-21 13:01:56 UTC
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?
Comment 16 James "Doc" Livingston 2005-11-22 12:59:49 UTC
Disabling move-to-trash for remote shares could be done using
gnome_vfs_uri_is_local() in rhythmdb_entry_move_to_trash.
Comment 17 Bastien Nocera 2005-11-26 13:38:52 UTC
This needs an update to remove the files completely from the DB if they were
already removed. Right now this fails.
Comment 18 James "Doc" Livingston 2005-11-29 06:57:34 UTC
This is fine to commit once you've dealt with any issues you think need dealing
with.
Comment 19 Bastien Nocera 2005-11-30 22:19:35 UTC
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)