Created attachment 53945 [details] [review]
1st patch

This patch adds a "show-hidden" property to rb-query-model which is set for
non-automatic playlists - which means that missing tracks are still shown, and
hence aren't deleted from the playlist.

This could also be useful in implementing a "missing tracks" source that was
discussed a while back.

Two things that still need doing:

1) play-orders need to ignore any entry that has the hidden property set
2) the playlist source should make entries with the hidden property set gray,
have a special "missing" icon, or something to that effect.

*** Bug 323095 has been marked as a duplicate of this bug. ***

Created attachment 55924 [details] [review]
patch

This is updated to cvs. Still to do is making rb-static-playlist-source use a
filter model to hide the HIDDEN entries. That filter model could also be used
for browser/search for static playlists.

*** Bug 325632 has been marked as a duplicate of this bug. ***

Upgrading to critical since this is almost certain to blank all manual playlists when first upgrading from 0.9.2, and occasionally at other times too.

Created attachment 56774 [details] [review]
mostly working patch

This patch fixed the problem of hidden songs being removed and also adds support for the search box to static playlists. This still needs some work to fix ordering issues.


How it works:
The patch adds support for "query model chains", for which a later query model can only contain entries found in the previous model. These are set up by setting the "base-model" property on a query model.

The static playlist source uses these by creating two chained query models - the "base" model contains all tracks that have been added to the playlist, even hidden ones, and the "filter" model is used to remove hidden tracks and those that don't match the search box. The base model is the one that gets written out to disc, so that hidden tracks aren't lost.


To Do:
When entried are added to a model, they need to be put in the same order as the base model. This currently isn't done, which causes the ordering problems.


Notes:
I originally looked into using GtkTreeModelFilter for this (which would seem simpler). The problem is that although a GtkTreeModelFilter is a GtkTreeModel, it is not a RhythmDBQueryModel; most of the code would be fine handling GtkTreeModels, but there are a few places that use RhythmDBQueryModel-specific functions that can't operation on a filter model.



Any comments on whether people think "query model chains" are a sane way of doing this or suggestions would be much appreciated.

A while ago, I decided for some reason that chained query models were necessary, or at least a good idea, but I can't remember what I wanted them for.

I think the query would work better if the filter model copied the base model contents when it was created, and we had a method to run the query over the existing query model contents, removing entries that didn't match.  I have a half-done library browser optimisation patch that does this too.

A couple of nits:
- should rhythmdb_query_model_base_entry_prop_changed only emit signals when the modified entry is in the filtered model?
- rb_static_playlist_source_finalize should call the parent class finalizer
- couldn't the static playlist just display the base model when there's no search text?

(In reply to comment #8)
> I think the query would work better if the filter model copied the base model
> contents when it was created, and we had a method to run the query over the
> existing query model contents, removing entries that didn't match.  I have a
> half-done library browser optimisation patch that does this too.

There are two ways of doing this:
a) take the contents of the base model, and remove the ones that don't match the query (your suggestion)
b) take the results of the query, and see if they are in the base model (how the patch does it).


Which one is more efficient depends on the size of the playlist, and exactly how the database is implemented. (b) will be more efficient when using browsers with large playlists, but this is going to be much less common than any other situation, so (a) is probably better. We can certainly change methods, I just picked one randomly to see how well the concept would work.


> A couple of nits:
> - should rhythmdb_query_model_base_entry_prop_changed only emit signals when
> the modified entry is in the filtered model?
> - rb_static_playlist_source_finalize should call the parent class finalizer

Yep, these need fixing.


> - couldn't the static playlist just display the base model when there's no
> search text?

The base model *must* contain all the entries in the playlist, including ones that are hidden, otherwise they will be lost (which is what this bug is actually about).

We could use the base model with hidden tracks visible, however we would need to ensure that they are never played, or any other synchronous operation done on them (which can lock up if they are a remote file). This is do-able, but would require changes in code above this, such as ignoring entry view activations, skipping them in play orders, never trying to edit tags on them etc.

Created attachment 57135 [details] [review]
updated patch

Now copies the contents of the base model into the chained model, and uses a new method to reapply the query to the existing query model contents.

Also fixes the entry-changed signal emission, and chains up the finalize call.

Created attachment 57146 [details] [review]
updated patch

I've added code to the dnd function to deal with base models - in particular drops and deletes are passed to the base model to handle. Models also now listen for "row-reordered" signals from the base model, and re-order based on that.


The filtering model used by RBStaticPlaylistSource seems to get leaked; after it is unref'd when creating the new filter model, there is still one reference on it. I haven't tried using refdbg to trace this yet.

I've been using this few the past few days and haven't found any issues, so I've committed the patch to cvs.

Created attachment 57452 [details] [review]
patch to fix the build 

This fixes the build for me.

Committed to cvs. thanks.