We have rating now at least.

Created attachment 49730 [details] [review]
Add the useful metadata as criteria for auto playlists

This is the patch posted to the mailing list, with a small bug fix - it adds
support for the following criteria: play count, track number, disc number,
bitrate, duration, last played and first seen.

This will also let us close Bug 141811 and bug 155684.

NOTE: this patch adds two new files widgets/rb-query-creator-properties.c and
widgets/rb-query-creator-private.h

Created attachment 49839 [details] [review]
updated patch

This is an updated version of the patch - new things are:
* implements the extra feature of not recreating the entry widgets, if the
newly selected property is the same type as the old one 
* set the title of the window to "Edit Automatic Playlist" (rather than "Create
Automatic Playlist") if we are editing one
* a couple of minor bug fixes
* a bit more cleanup of rb-query-creator.c

A few nits, but otherwise looks good to me.  Not that this really means anything..

> --- rhythmdb/rhythmdb.c       14 Jul 2005 23:34:17 -0000      1.101
> +++ rhythmdb/rhythmdb.c       27 Jul 2005 13:32:34 -0000
> @@ -2638,6 +2659,8 @@
>                       ENUM_ENTRY (RHYTHMDB_QUERY_PROP_NOT_LIKE, "Inverted
fuzzy property matching"),
>                       ENUM_ENTRY (RHYTHMDB_QUERY_PROP_GREATER, "True if
property1 >= property2"),
>                       ENUM_ENTRY (RHYTHMDB_QUERY_PROP_LESS, "True if property1
<= property2"),
> +                     ENUM_ENTRY (RHYTHMDB_QUERY_PROP_CURRENT_TIME_WITHIN,
"True iff property1 is within property2 of the current time"),
> +                     ENUM_ENTRY (RHYTHMDB_QUERY_PROP_CURRENT_TIME_NOT_WITHIN,
"True iff property1 is not within property2 of the current time"),
>                       { 0, 0, 0 }
>               };

Should be just 'if', to match the others.

> --- widgets/rb-query-creator.c        25 Jul 2005 16:30:58 -0000      1.29
> +++ widgets/rb-query-creator.c        27 Jul 2005 13:32:58 -0000

There are a few whitespace-only changes in this file that should be removed.

> +     /* don't recreate the critera menu and entry if they will be the same*/

Just a spelling mistake.  In a comment.  Possibly the most nit-like nit that
ever nitted.

Might as well move the body of setup_property_option_menu into
create_property_option_menu, and setup_criteria_option_menu into
create_criteria_option_menu, since the create_ functions don't
do much else, and the setup_ functions aren't useful anywhere else.


> --- /dev/null 2005-07-27 16:31:12.477604184 +1000
> +++ widgets/rb-query-creator-private.h        2005-07-26 02:39:01.000000000 +1000


> +typedef GtkWidget*   (*CriteriaCreateWidget)         (gboolean *constrain);
> +typedef void         (*CriteriaSetWidgetData)        (GtkWidget *widget,
GValue *val);
> +typedef void         (*CriteriaGetWidgetData)        (GtkWidget *widget,
GValue *val);
> +
> +typedef struct
> +{
> +     int num_criteria_options;
> +     const RBQueryCreatorCriteriaOption *criteria_options;
> +
> +     CriteriaCreateWidget    createWidget;
> +     CriteriaSetWidgetData   setWidgetData;
> +     CriteriaGetWidgetData   getWidgetData;
> +} RBQueryCreatorPropertyType;

Should be 'criteria_create_widget' etc. for consistency with everything else,
same for the various implementation functions in rb-query-creator-properties.c


> --- /dev/null 2005-07-27 16:31:12.477604184 +1000
> +++ widgets/rb-query-creator-properties.c     2005-07-27 21:44:00.000000000 +1000


> +const RBQueryCreatorCriteriaOption relative_time_criteria_options[] =
> +{
> +     { N_("in the last"), 1, RHYTHMDB_QUERY_PROP_CURRENT_TIME_WITHIN },
> +     { N_("not in the last"), 1, RHYTHMDB_QUERY_PROP_CURRENT_TIME_NOT_WITHIN }
> +};

Do these need some translator notes to clarify that 'last' refers to time here?

Created attachment 50643 [details] [review]
updated patch

All of the operators used to have "iff" rather than "if", and they got fixed
after I wrote the code - I've changed them.

The setup_* methods have been merged into the create_* methods in
rb-query-creator.c

Various code style issues have been fixed.

I've put in some translator notes, but I don't know if they are any more
helpful; does anyone have better suggestions:

"in the last"
Translators: this will match when within <value> of the current time
e.g. "in the last" "7 days" will match if within 7 days of the current time

"not in the last"
Translators: this is the opposite of the above, and will match if not
within <value> of the current time

Created attachment 50801 [details] [review]
even better patch

This patch adds auto playlist sorting, as well as fixing a few minor bugs.

If an auto playlist doesn't have a limit, it can be sorted by clicking on
column headers - just as for the library, etc.

If an auto playlist does have a limit, the sorting is set in the playlist
editor as it is part of what defines the playlist because changing the sorting
will change the results.

Created attachment 50838 [details] [review]
yet another updated patch

This update removes rb_playlist_source_get_model because the model could be
deleted at any time if a resort was to occur, which would cause Bad Things to
happen if it was deleted while something was using it. Currently nothing uses
this function, but it means that it can't cause problems in the future.

It also switches to using the gobject private data functions (re bug 313688),
fixes inconsistant naming of "dlg", "dialog" and "creator" parameters
throughout the source and fixes a few other code style issues and potential
trivial bugs.

As a note, the patch will let us close bug 159232 and bug 155684 (it is in
human readble form now, rather than a uri), fixes bug 131526 and bug 129785 for
automatic playlists (I haven't though about sorting normal playlists) and bug
167659 for playlist criteria (year, etc aren't added as columns or sorting
options, yet).

Created attachment 50841 [details] [review]
patch_count++

Fix a really dumb (crasher) mistake in the previous patch. Oops.

Created attachment 50935 [details]
rb-query-creator-private.h

file goes in the "widgets" directory

Created attachment 50936 [details] [review]
rb-query-creator-properties.c

rb-query-creator-properties.c, goes in the "widgets" folder

I've had a couple of reports from my post on the mailing list, and there haven't
been any real problems reported.

The only issue has been in relation to sorting by time-based (First Seen, Last
Played) and it's wording. The question is what does sorting by First Seen in
ascending order mean? Does it mean ascending order of the time tracks are first
seen (hence older tracks at the top) or ascending in the amount of time they
were first seen (hence newer tracks at the top).

Currently it does the former, because a) the code works that way and b) it works
that way if you click on column headers to sort in ascending order. Some people
have said that they though it would work the latter way. Does anyone have any
suggestion of ways to reword it, to make it clearer to users, or any other ways
of making this better?

It really shouldn't say ascending or descending, but "oldest first" or "newest
first".

I guess we could change the name of the checkbox, depending on which property we
is selected.

"Newest First" for the time-based ones
"Longest First" for duration
"Most played first" for playcount
"Highest Rated first" for rating
"In reverse alphabetical order" for text

Created attachment 51196 [details] [review]
even more betterer patch

This patch includes the two new files in it. It makes the checkbox name
dependant on the sorting order, and removes some more redundant code that was
in rb-query-creator.c. Does anything have an opinion on a) whether this is a
good thing to do, and b) if the names for the checkbox make it clearer what it
means?

After some discussion on IRC about whether the checkbox name changing is a good
idea, I though it best to commit this now (so more people try it) and deal with
any issues afterwards - there haven't been any reported issues apart from the
ascending/descending confusion in a while.


2005-08-24  James Livingston  <jrl@ids.org.au>

        * NEWS:
        * data/glade/create-playlist.glade:
        * rhythmdb/rhythmdb-tree.c: (evaluate_conjunctive_subquery):
        * rhythmdb/rhythmdb.c: (rhythmdb_query_parse_valist),
        (rhythmdb_query_free), (rhythmdb_query_serialize),
        (rhythmdb_query_deserialize), (rhythmdb_query_get_type):
        * rhythmdb/rhythmdb.h:
        * shell/rb-playlist-manager.c:
        (rb_playlist_manager_set_automatic_playlist),
        (rb_playlist_manager_cmd_edit_automatic_playlist):
        * sources/rb-playlist-source.c: (rb_playlist_source_constructor),
        (rb_playlist_source_set_query), (rb_playlist_source_get_query),
        (impl_receive_drag), (rb_playlist_source_new_from_xml),
        (rb_playlist_source_save_to_xml),
        (rb_playlist_source_songs_sort_order_changed_cb),
        (rb_playlist_source_do_query):
        * sources/rb-playlist-source.h:
        * widgets/Makefile.am:
        * widgets/rb-entry-view.c: (rb_entry_view_get_sorting_order),
        (rb_entry_view_set_sorting_order):
        * widgets/rb-query-creator-private.h:
        * widgets/rb-query-creator-properties.c:
        (stringCriteriaCreateWidget), (stringCriteriaSetWidgetData),
        (stringCriteriaGetWidgetData),
        (escapedStringCriteriaSetWidgetData),
        (escapedStringCriteriaGetWidgetData), (set_rating_score),
        (ratingCriteriaCreateWidget), (ratingCriteriaSetWidgetData),
        (ratingCriteriaGetWidgetData), (integerCriteriaCreateWidget),
        (integerCriteriaSetWidgetData), (integerCriteriaGetWidgetData),
        (durationCriteriaCreateWidget), (durationCriteriaSetWidgetData),
        (durationCriteriaGetWidgetData), (create_time_unit_option_menu),
        (relativeTimeCriteriaCreateWidget),
        (relativeTimeCriteriaSetWidgetData),
        (relativeTimeCriteriaGetWidgetData):
        * widgets/rb-query-creator.c: (rb_query_creator_get_type),
        (rb_query_creator_class_init), (rb_query_creator_constructor),
        (rb_query_creator_dispose), (rb_query_creator_set_property),
        (rb_query_creator_get_property), (rb_query_creator_load_query),
        (rb_query_creator_set_sorting), (rb_query_creator_new_from_query),
        (get_entry_for_property), (rb_query_creator_get_query),
        (rb_query_creator_get_limit), (rb_query_creator_get_sort_order),
(limit_toggled_cb), (lookup_row_by_widget),
        (remove_button_click_cb), (append_row),
        (get_property_index_from_proptype), (select_criteria_from_value),
        (property_option_menu_changed), (create_property_option_menu),
        (create_criteria_option_menu), (sort_option_menu_changed),
        (setup_sort_option_menu):
        * widgets/rb-query-creator.h: A fairly huge patch to improve automatic
        playlists. The two most notable improvements are 1) adding more
        properties to the query creator, making it much easier to add more in
        the future, and 2) allow playlists to be sorted, from the query editor
        if they gave limits, and by clicking on column headers if they don't.

        Files added: widgets/rb-query-creator-private.h,
        sources/rb-query-creator-properties.c