I can confirm this bug.
I'm using Rhythmbox 0.10.0 and Gnome 2.18.1 on Ubuntu Feisty.

Jeroen

duplicate of bug 404180 ?

I've just attempted to reproduce this bug on 11.10 using Rhythmbox 2.90.1 and am unable to do so. When I remove a file from my music library using Edit->Remove then reboot Rhythmbox, the file entry is nowhere to be seen, suggesting that it has been successfully removed from rhythmdb.xml. Can anyone else confirm this?

It turns out I was mistaken. This bug does still affect Rhythmbox and I just wasn't doing it right. The 'Remove' feature works as advertised if the file is not in the folder designated as the music library in the Rhythmbox preferences (~/Music by default). If the file is in there and the user attempts to remove it from the database, it is reloaded the next time the library is scanned.

I'm having a go at fixing this just and and a question has just occurred to me: how useful is this feature? Is there really a use case for people not wanting to display all the tracks on their hard drive within Rhythmbox while keeping the files intact? It wouldn't seem to me that such a feature would be widely used.

The average user would want all their music contained within their music library, in much the same way they would want all their CDs on the same shelf (or set of shelves). Having it all in the one place just makes it easier to manage and the 'Remove from library' features just seems rather niche. 

Is it really necessary to keep a little used feature that doesn't even work properly?

I think a simple solution would be to add a boolean attribute called ExcludeFromLibrary to each track in the xml file. If the parser comes across such an attribute with a true value, then it simply ignores it.

When adding new files to the library, it would be a good idea to run through the list of excluded files to ensure that it has not already been excluded. If it is found in that list, then the attribute is flipped to false. This will stop the library file getting clogged up with duplicate excluded tracks.

Created attachment 227742 [details] [review]
Exclude entries from library patch

Hello,

  I am working on this bug as part of the Ubuntu One Hundred Papercuts project (https://launchpad.net/hundredpapercuts/raring).

  I attach a proposed patch (see the comment https://bugzilla.gnome.org/show_bug.cgi?id=388568#c7). The solution I used is pretty much based on the comment above (https://bugzilla.gnome.org/show_bug.cgi?id=388568#c6). I created a new EXCLUDED type and apply this to entries when Remove is clicked (I think having a new type gives more flexibility than using the existing 'ignore' type - for example, for filtering the manually excluded files). The entries are then shown in the Library Import dialog.

  If you have a different proposal or if you notice any problems with this approach, please comment.

Review of attachment 227742 [details] [review]:

This is a decent start, but it definitely needs more work. I don't think showing the excluded entries in the import dialog with no explicit action from the user is the right way to do it. I think people would find it annoying to remove the excluded entries from the import dialog to avoid importing them again.

The behaviour you're adding should be specific to the library source, rather than shared by all browser sources.

::: rhythmdb/rhythmdb-entry-type.h
@@ +111,3 @@
 #define RHYTHMDB_ENTRY_TYPE_IMPORT_ERROR (rhythmdb_get_error_entry_type ())
 #define RHYTHMDB_ENTRY_TYPE_IGNORE (rhythmdb_get_ignore_entry_type ())
+#define RHYTHMDB_ENTRY_TYPE_EXCLUDED (rhythmdb_get_excluded_entry_type ())

I don't actually want new entry types added here, especially special purpose ones like this. The entry type should be defined in the library source where it is used.

::: sources/rb-browser-source.c
@@ +657,3 @@
+	GValue excluded_entry_type = {0};
+	g_value_init(&excluded_entry_type, RHYTHMDB_TYPE_ENTRY_TYPE);
+	g_value_set_object(&excluded_entry_type, RHYTHMDB_ENTRY_TYPE_EXCLUDED);

everywhere else in the code there is a space between the function name and the opening bracket of its arguments.

@@ +660,3 @@
 	sel = rb_entry_view_get_selected_entries (source->priv->songs);
 	for (tem = sel; tem != NULL; tem = tem->next) {
+		// rhythmdb_entry_delete (source->priv->db, tem->data);

if you're removing code, remove it, don't comment it out. and don't use c++ style comments, please.

Hello,

  Thanks for taking the time to review this.

  Regarding how to re-import the songs - I also don't really like the idea of having them in the import dialog, but it is the easiest place to add them without modifying the UI. I have two other ideas for this: one would be to create a new source which will show the excluded files. This is my preferred approach, the only problem being that we get an extra entry in the interface. The other would be to add a checkbox to the import dialog. Let me know if you would prefer one of these, so I can try to implement it. 

  One more question - for the external type, would it be ok with you if I put it in a separate file in rhythmdb? The reason is that I need to use the type both in the library_source (for implementing the delete function) and in the file where I will search for it. And I think it's better if it is in a separate header file, so I don't have to include the library source when I need to use it.

(In reply to comment #10)
> Hello,
> 
>   Thanks for taking the time to review this.
> 
>   Regarding how to re-import the songs - I also don't really like the idea of
> having them in the import dialog, but it is the easiest place to add them
> without modifying the UI. I have two other ideas for this: one would be to
> create a new source which will show the excluded files. This is my preferred
> approach, the only problem being that we get an extra entry in the interface.
> The other would be to add a checkbox to the import dialog. Let me know if you
> would prefer one of these, so I can try to implement it. 

I think the import dialog is the right place to do this. I'm not really sure how it should work, though.


>   One more question - for the external type, would it be ok with you if I put
> it in a separate file in rhythmdb? The reason is that I need to use the type
> both in the library_source (for implementing the delete function) and in the
> file where I will search for it. And I think it's better if it is in a separate
> header file, so I don't have to include the library source when I need to use
> it.

You don't need to have a header for it. Either pass it from one place to another, probably in this case as a property on an object constructed in the library source, or look it up by name using rhythmdb_entry_type_get_by_name (db, "excluded").

(In reply to comment #11)
> (In reply to comment #10)
> > Hello,
> > 
> >   Thanks for taking the time to review this.
> > 
> >   Regarding how to re-import the songs - I also don't really like the idea of
> > having them in the import dialog, but it is the easiest place to add them
> > without modifying the UI. I have two other ideas for this: one would be to
> > create a new source which will show the excluded files. This is my preferred
> > approach, the only problem being that we get an extra entry in the interface.
> > The other would be to add a checkbox to the import dialog. Let me know if you
> > would prefer one of these, so I can try to implement it. 
> 
> I think the import dialog is the right place to do this. I'm not really sure
> how it should work, though.

As I mentioned, my idea would be to add a checkbox to the import dialog. Selecting/deselecting it will toggle between showing and hiding the removed entries from the currently selected directory. I've attached an image with how it would look, let me know what you think about it.

Created attachment 228158 [details]
Show removed file checkbox in import dialog.

Created attachment 228566 [details] [review]
Exclude entries from library patch (2)

I've attached a new version of the patch.

The excluded entry type is now defined in the library source. I've added an impl_delete function to the library source, which modifies the type for the selected entries to 'excluded'.

I've added a checkbox to show the excluded files in the import dialog. When it is selected/deselected, I remove and recreate the query model for the dialog. This might not be the optimal way to do this, but I'm not sure how to add/remove results from an existing model, so comments to this are welcomed.

This still needs some work - the checkbox setting is not persistent yet. For the moment, I'd like to get some feedback, if you'd include this patch in the main codebase (and what changes should be made before inclusion) or if you don't like this solution.

Just a small comment on the patch above.

<property name="tooltip_text">Show also files that have been removed from the library.</property>

Should be either:
Also show files that have been removed from the library.

or 

Show files that have been removed from the library.

After trying this out a bit, I think I've come up with a reasonable way for this to work. Initially, deleted songs are hidden. If any are found under the selected folder, show an info bar saying "this folder contains n songs that have been deleted from the library" with a switch labelled "show deleted songs" which works like the checkbox you've currently got.

We should also make sure excluded entries get deleted when the underlying file goes away, but that's a bit tricky.

Created attachment 230128 [details] [review]
Exclude entries from library patch (3)

Sorry for not following this up earlier, I was busy in the last couple of weeks.

I've added a new proposal - this is mostly the same as the last one, I just replaced the checkbox with an info bar, which is shown only if removed files are present in the currently selected directory.

I'll think also about how to handle files that have been deleted. Let me know if you have any comments on the patch - any functions that should be implemented differently, formatting problems etc.

Review of attachment 230128 [details] [review]:

I think this is pretty much right, just some details to fix up.

::: data/ui/import-dialog.ui
@@ +108,3 @@
+                    <property name="visible">True</property>
+                    <property name="can_focus">False</property>
+                    <property name="label" translatable="yes">This folder contains songs that have been deleted from the library.</property>

We need to be consistent in how we refer to these files. I think 'excluded' is the right term to use.

@@ +120,3 @@
+                    <property name="visible">True</property>
+                    <property name="can_focus">False</property>
+                    <property name="label" translatable="yes">Show removed files:</property>

so this should say "Show excluded files:"

::: sources/rb-library-source.c
@@ +219,3 @@
 	source_class->impl_want_uri = impl_want_uri;
 	source_class->impl_add_uri = impl_add_uri;
+	source_class->impl_delete = impl_delete;

Probably should also implement impl_get_delete_action here. This method returns the name of a GtkAction to use in place of the default 'Remove' action. The library would use an action with 'exclude' as its label.

::: widgets/rb-import-dialog.c
@@ +537,3 @@
+
+static void
+update_removed_files_info_bar_visibility (RBImportDialog *dialog) {

the opening brace should go on a new line by itself

@@ +544,3 @@
+													   dialog->priv->db);
+	if (number_of_entries > 0) {
+		char *item_text = g_strdup_printf (_("This folder contains %d songs that have been deleted from the library."),

should say "excluded" here, not "deleted"

@@ +569,3 @@
+	query = rhythmdb_query_parse (db,
+					  RHYTHMDB_QUERY_PROP_EQUALS, RHYTHMDB_PROP_TYPE, excluded_type,
+					  RHYTHMDB_QUERY_PROP_LIKE, RHYTHMDB_PROP_LOCATION, uri,

should use RHYTHMDB_QUERY_PROP_PREFIX here, not RHYTHMDB_QUERY_PROP_LIKE

@@ +573,3 @@
+
+	excluded_entries = rhythmdb_query_result_list_new ();
+	rhythmdb_do_full_query_parsed (db, RHYTHMDB_QUERY_RESULTS (excluded_entries), query);

I think this query should be done asynchronously.

@@ +579,3 @@
+	if (results != NULL) {
+		ret_val = g_list_length (results);
+	}

g_list_length (NULL) safely returns 0, so you don't need this if block

@@ +581,3 @@
+	}
+
+	g_object_unref (results); 

results is a list here, not an object, and it doesn't need to be freed.

@@ +636,3 @@
+							  RHYTHMDB_QUERY_DISJUNCTION,
+							  RHYTHMDB_QUERY_PROP_EQUALS, RHYTHMDB_PROP_TYPE, excluded_type,
+							  RHYTHMDB_QUERY_PROP_LIKE, RHYTHMDB_PROP_LOCATION, dialog->priv->current_uri,

RHYTHMDB_QUERY_PROP_PREFIX here too

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