GNOME Bugzilla – Bug 321341
[Patch] Include "Year" as criteria for match in "Automatic Playlists"
Last modified: 2006-01-06 10:29:21 UTC
In rhythmbox in CVS, now that "Year" tags are being read and can appear as columns, it should be possible to add them as criteria for automatic playlists. E.g. all songs in a given year (2002), or in a range (1972-1975), or all songs without a year (Unknown). A minimal initial version would just allow grouping by a single year.
Created attachment 55875 [details] [review] Implements using year as criterion in auto playlists Attached is a patch that implements this feature request, can be used for equals to a given year, or for above or below a threshold. Doesn't implement ranges as yet.
Update description, noting presence of patch that fixes this bug.
Created attachment 56065 [details] [review] Updated against CVS HEAD for 2005-12-16 Updated patch against CVS HEAD for 2005-12-16
This will have issues if a track has an actual date not just a year (which some formats support). "Year equals" needs to check that the date is between January 1 of the year and the day before January 1 of the next year; similarly with "Year at most". However such tracks probably aren't too common, so we can probably commit this, and try to fix it after that.
(In reply to comment #4) > This will have issues if a track has an actual date not just a year (which some > formats support). "Year equals" needs to check that the date is between January > 1 of the year and the day before January 1 of the next year; similarly with > "Year at most". > However such tracks probably aren't too common, so we can probably commit this, > and try to fix it after that. Yes, I will try and work on it after committing.
Any chance this patch could be committed? It does the job for the moment. I will look at adding the full date check after that.
Created attachment 56367 [details] [review] Updated patch that handles full dates This patch should handle full dates by checking whether the metadata date is between 1-JANUARY-YEAR and 31-DECEMBER-YEAR for a given YEAR. The mp3 libmad backend only supports YEAR, not DD-MM-YEAR, but it works fine there. Needs testing with metadata backends that support full dates.
Created attachment 56651 [details] [review] Updated patch against CVS HEAD 2006-01-01
A couple of issues: - evaluate_conjunctive_subquery really shouldn't allocate anything on the heap. Not very important in light of my next comment. - The year to Julian day conversion should be done once in rhythmdb_query_parse_valist rather than once per entry compared. A slight amount of trickery may be required to do this for equality comparisons. rhythmdb_query_serialize and rhythmdb_query_deserialize would also need to be changed to handle this. - All other greater/less than comparisons are actually implemented as greater-or-equal/less-than-or-equal. Maybe year comparisons should work the same way.
The conversion is probably best to put in rhythmdb_query_preprocess (rhythmdb.c), because these kinds of transformations is exactly what that function got added to do.
(In reply to comment #9) > A couple of issues: > > - evaluate_conjunctive_subquery really shouldn't allocate anything on the heap. > Not very important in light of my next comment. > > - The year to Julian day conversion should be done once in > rhythmdb_query_parse_valist rather than once per entry compared. A slight > amount of trickery may be required to do this for equality comparisons. > rhythmdb_query_serialize and rhythmdb_query_deserialize would also need to be > changed to handle this. > > - All other greater/less than comparisons are actually implemented as > greater-or-equal/less-than-or-equal. Maybe year comparisons should work the > same way. I don't have much time to work on this, so perhaps commit my first (very simple!) patch (http://bugzilla.gnome.org/attachment.cgi?id=56367&action=edit). It doesn't handle full dates but it will give the basic functionality, then add back the full date stuff later, since it is probably more complicated than I had envisaged and probably beyond my familiarity with the code at this point.
(In reply to comment #9) > - All other greater/less than comparisons are actually implemented as > greater-or-equal/less-than-or-equal. Maybe year comparisons should work the > same way. Agreed.
(In reply to comment #11) > I don't have much time to work on this, so perhaps commit my first (very > simple!) patch (http://bugzilla.gnome.org/attachment.cgi?id=56367&action=edit). As suggested by James in comment #4. ;-) I'm mainly worried about version skew in keep the patch up to date.
Created attachment 56837 [details] [review] better patch Moves the year->julian conversion up to the rhythmdb_query_preprocess function, so that it only gets performed once per query. Also make less/greater behave the same way as for other things. The YEAR_EQUALS bit could probably be made better by replacing that criteria with two, rather then putting them into a subquery - however I'm not sure if there is an easy to to insert something in the middle of a GPtrArray.
Works fine here, thanks for working on this. I think I see how the rhythmdb_query_preprocess() function works, so I've learned something. Please do commit.
Committed to cvs, thanks.
Shouldn't the patch "Status" be switched to "committed"? I can't figure out how to do this myself, even though I opened the bug.
Yep. Fixed
There appears to be an addition between the patch you described and what was committed to CVS, because now it doesn't work. When I try to create a playlist with a Year criteria, and click "New" rb just hangs. Here's the output I get before the hang running this with rhythmbox -d: <snip> [0x8b4b730] [rhythmdb_read_enter] rhythmdb.c:764 (23:21:13): counter: 4 [0x8f43e10] [query_thread_main] rhythmdb.c:3216 (23:21:13): entering query thread [0x8f43e10] [rhythmdb_query_internal] rhythmdb.c:3193 (23:21:13): doing query [0x8f43e10] [do_query_recurse] rhythmdb-tree.c:1764 (23:21:13): doing recursive query, 1 conjunctions [0x8f43e10] [rhythmdb_query_model_add_entries] rhythmdb-query-model.c:702 (23:21:13): adding 0 entries [0x8f43e10] [rhythmdb_query_internal] rhythmdb.c:3199 (23:21:13): completed I tracked it down to this line in rhythmdb_query_preprocess(): /* redo this criteria, in case the criteria we change to need transformation */ restart_criteria = TRUE; If I comment this out, it works again.
Oops, I must have uploaded the wrong version to bugzilla. It should be fixed in cvs now.
Thanks, seems that it is fixed now. Closing.