Created attachment 58842 [details] [review]
make RhythmDBEntry contents opaque

This moves the definition of RhythmDBEntry into rhythmdb-private.h, and fixes all the bits of code that access it directly.

Doing this I've uninlined the rhythmdb_entry_get_* functions, however we may want to keep them inlined for performance reasons.

Looks sensible to me.  A few thoughts that we don't need to worry about yet:

- Alternate rhythmdb backends will probably need to implement rhythmdb_entry_get_* differently, so maybe they should be moved to a separate file, so the backend can use them as is by including that file, but can also reimplement them.  rhythmdb_entry_sync_mirrored most likely needs to go in the same place.

- We can reduce the size of the RhythmDBEntry structure by combining the boolean fields (hidden and inserted) into a bitfield.

- Perhaps we could use a specialised RBRefString-like thing for formatted times, where the equivalent of rb_refstring_get would take the time value and return the formatted time string.  This would further shrink RhythmDBEntry.

(In reply to comment #2)
> - Alternate rhythmdb backends will probably need to implement
> rhythmdb_entry_get_* differently, so maybe they should be moved to a separate
> file, so the backend can use them as is by including that file, but can also
> reimplement them.  rhythmdb_entry_sync_mirrored most likely needs to go in the
> same place.

I think we need to have a think about some fundamental design points here. Should RhythmDB (the base class) know about the internals of RhythmDBEntry?

If so (as is the case now) then the fields of RhythmDBEntry need to be filled in, and then alternate backends won't need different _get implementations because the standard ones will work perfectly and be as fast as they can be.

If not, then we need to move some things to RhythmDBTree, and either make everything go though the rhythmdb_entry_get method (which might be slow) or add type-specific _get methods.


The last two points sounds good.

Patch committed to cvs.

Created attachment 59258 [details] [review]
decouple rhythmdb from rhythmdb-query-model

This lets us implement different types of query model that are not necessarily based on GtkTreeModel.  Some interesting possibilities:
- query models exposed through a dbus interface, which would be useful in a rhythmdb daemon if someone wanted to try that.
- a query model that groups results based on a property and presents the results in a tree; could be used to show podcasts in a tree format, rather than a flat list, and other similar grouped views (albums?)
- something like rhythmdb_do_query_foreach (calling a callback for each entry matching the query, done synchronously), which would come in handy in a few places.

I haven't tested this to any great extent, but it doesn't seem to break anything major.

That looks fine to me.

The latter two things you mentioned can already be done with things based on GtkTreeModel. However this makes it easier, and lets us support other things like the first, and things we haven't though of yet - so it sounds good.

I've committed the patch.

Another thing I think would help would be to create a vtable for rhythmdb entry types.  At the moment we've got a number of places where we do something like 'if (entry->type == RHYTHMDB_ENTRY_TYPE_SONG) { ... }', which for the most part would work better as virtual method calls.

Some methods that would be useful:
- get playback URL (since we use the mountpoint property for podcast episodes)
- get drag URL (same, except we might want to use the location URL for podcast episodes that haven't been downloaded)
- format window title/tooltip text
- construct data to send with song change notification (so it can include the streamed song title for iradio, for example)
- queries: can be played, can be added to the play queue

This would also help for plugins that add new source types, since the code implementing new behaviour for any of the entry type methods would be kept within the plugin.

Other entry-type methods:
- save metadata. some options would be "sync to file" (for the library), no-operation (only updated the db, iradio and podcasts), and disabled. Would basically allow bug 330794.
- "save to disk": whether it should be stored in the on-disk db. Before being useful to plugins, we would need to deal with the fact that the plugin may not be active when the db is loaded.

Created attachment 60470 [details] [review]
commit added entries per-thread

Currently when rhythmdb_commit() is called, all uninserted entries are inserted. This causes problems if a non-main thread is half-way through creating a new entry; it will be inserted before it is completely constructed, and cause critical warnings when later rhythmdb_entry_set_inserted() calls are made.

This patch makes RhythmDB remember which thread created new entries, and only inserts those which were created by the same thread that is now calling rhythmdb_commit().

Currently the patch allows multiple timeout-commits to be queued per thread, which could potentially affect performance. It could be fixed reasonably easily, however I haven't notices any performance degradations, so I've left as-is for now.

(In reply to comment #9)
> This patch makes RhythmDB remember which thread created new entries, and only
> inserts those which were created by the same thread that is now calling
> rhythmdb_commit().

Maybe it'd be better to have a rhythmdb_begin() that returned a structure that would hold the set of changes until they were committed.  (Notice how I'm avoiding the word 'transaction').  This would complicate code that just wants to set a single property, so we could have a shortcut for that.

Is there anything that needs/wants "not-transactions" apart from creating new entries? If so, we could probably do it implicitly by having the transaction comitted when rhythmdb_commit() is called in the thread and starting a new one when rhythmdb_entry_set (or _entry_new) is called.

Patch committed with some typos fixed.

A similar approach could be taken with entry-changes, effectively giving us transactions. The "transaction" would be started automatically, and be commited when that thread calls rhythmdb_commit. In theory we could also add a rhythmdb_rollback or something (if we realyl wanted to).

Created attachment 64316 [details] [review]
add entry-type vfunc for playback uri

(In reply to comment #7)
> Another thing I think would help would be to create a vtable for rhythmdb entry
> types.  At the moment we've got a number of places where we do something like
> 'if (entry->type == RHYTHMDB_ENTRY_TYPE_SONG) { ... }', which for the most part
> would work better as virtual method calls.
> 
> Some methods that would be useful:
> - get playback URL (since we use the mountpoint property for podcast episodes)

This patch adds a entry-type vfunc for getting the playback uri.

Also, the rhythmdb_delete_by_type should probably be rhythmdb_entry_type_unregister, which deletes all the entries and destroys the type. That way we won't leak stuff when removable media are loaded and ejected.

I've committed the patch to cvs. There are probably several more vfunc (including the ones from earlier comments) that could be added.

Created attachment 65931 [details] [review]
split up rhythmdb.c a bit

rhythmdb.c is a bit too large.  This patch moves the file and volume monitoring code to rhythmdb-monitor.c, and the query manipulation code to rhythmdb-query.c.

Looks good to me, I'd actually half-done something similar.

OK, committed.  The other things I was thinking about splitting out were the move-to-trash code (even though there isn't much of it) and the entry get/set property code.

Created attachment 65977 [details] [review]
assorted cleanup

Three things:
- include names in entry types, so we can map names to entry types easily
- add something like gobject's private data to RhythmDBEntry (and convert the podcast data fields to use it)
- reduce code duplication in rhythmdb-tree.c - we've already got code for converting strings to entry property values, and for applying property values to entries.

This changes the auto playlist xml format a bit.  It currently uses <equals prop="type">0</equals> to specify that the query only matches songs, and this changes it to use 'song' instead of '0'.  It reads the existing format correctly.

Created attachment 65984 [details] [review]
fix db entry refcounting

db entry refcounting is wrong in two places:
- when moving entries from the deleted_entries hash table to the deleted_entries_to_emit list, we reference the entry, but the entry was already referenced before adding it to the deleted_entries hash table
- the reference count is initialized to 1 (for the reference in rhythmdb itself), but that reference is never dropped.

Those look fairly sane to me, a couple of comments:

* entry_type_hash needs to be protected with a mutex

* the glib docs say using GValue transformation isn't a good idea for serialisation and the like. Mostly because floating point numbers can deviate a lot after a number of serialise-deserialise operations.



Probably the next step is to make property-types registerable, and have the podcast code do it. Notes:

* we should default to doing gvalue transformation for serialisation/deserialisation, but have vfuncs to override it.

* get/set should be a prop-type vfunc for registerable properties, and a db vfunc for core ones (so a db implementation can override get/set).

* RBRefString should be registered as a GType, plus the stuff so we can put it in GValues.


I'm not sure if we could allow registered property-type in queries. It would cause issues for alternate db implementations.

I've committed the refcounting fix patch.

It's probably not worth changing write_encoded_gvalue to use GValue transformation.  If we add a property that uses a new type, we'll need to make sure that the value transformation works properly anyway, so it doesn't really save us any effort.  We're going to have the same problems with regard to floating point precision loss, and I don't think there's really much of a solution.  I don't think GValue transformation does deserialisation at all.

I was thinking of making get/set vfuncs on the entry type, rather than the property type.  I was mostly looking at the way the podcast properties are handled at the moment, and how I was going to handle storing the original location of trashed files so they could be restored.

Created attachment 66162 [details] [review]
updated patch

Reverts the GValue transformation change, and protects the entry type name map with a mutex.

Looks good to me.

The issue of leaking an entry-type's alias could probably be fixed if we used a RBRefString for the entry type name. The entry-type structure could hold a reference to it, and the hash-table could hold another reference. That way if we every actually supported deleting entry-types, we could clean up properly.

Acutally, it breaks if you have any "ignore" entries, because it doesn't ensure that all the types are registered before loading the db.

Created attachment 66179 [details] [review]
updated patch again

Now hackishly forces the core entry types to be registered before db loading.  This could be done more cleanly, but I think it can wait until we move the iradio and podcast entry types elsewhere.

Looks good to me.

OK, committed.  This fixed bug 337429.

Created attachment 66587 [details] [review]
expose query cancellability, and change rhythmdb_entry_* prototypes

This patch exposes cancellability in the rhythmdb_do_full_query* interfaces, and removed the "db" argument from rhythmdb_entry_{ref,unref}.

It would be good if all the rhythmdb_entry_* methods were consistent about whether they took the db for a parameter or not, rather than exposing the implementation detail of whether they needed it. Personally I think that not having to pass it would be better, and either have the entry know which db it was from or have the entry-type know that.

The changes to rhythmdb_entry_{ref,unref} have already been committed from somewhere else.  The query cancellation stuff looks OK, but I'm not sure where it'd be useful.  Perhaps if more search text is entered before the previous search finishes?

Created attachment 68378 [details] [review]
get rid of rhythmdb_entry_set_{uninserted,nonotify}

This gets rid of rhythmdb_entry_set_uninserted, by simply making rhythmdb_entry_get be smart enough and the right action (it checks anyway). It also gets rid of rhythmdb_entry_set_nonotify, as it isn't used and I can't think of a good use for it (as you can't be sure nothing else cares about the change).

Looks good to me.

Okay, committed to cvs.

To answer your comment about the cancellability patch: I wanted it for a RhythmDBQueryResults implementation I was doing (which put the entries directly in a GList), but didn't end up needing it, so I never finished it.

Created attachment 68649 [details] [review]
handle entries of unknown types in rhythmdb

This allows rhythmdb-tree to load and save entries for which the entry type has not been registered.  It also moves the entry type maps into the RhythmDB object, and allows entry types to decide for themselves whether they should be saved to disk.  This goes some way towards allowing iradio and podcast code to be split out into plugins.

I haven't done any real testing, but it looks fine to me (I assume you have with iradio/podcast as plugins).

OK, committed.

Marking outstanding patch obsolete, since it needed quite a bit of work and I didn't end up needing it.

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