Create automatic playlist. Click on 'Location column' header so the results are sorted by column. Click 'Playlist -> Edit'. Rhythmbox crashes.

Downstream bug:

https://bugzilla.redhat.com/show_bug.cgi?id=1359611

will attach patches shortly

Created attachment 365783 [details] [review]
query-creator: add location and bitrate columns to sort options

Created attachment 365784 [details] [review]
query-creator: check sort index on num_sort_options, not num_property_options

Review of attachment 365784 [details] [review]:

I've pushed the part described by the subject line as commit 99db05027; the other bits need more work.

::: shell/rb-playlist-manager.c
@@ +946,3 @@
 									     sort_key,
 									     sort_direction));
+		g_return_if_fail (creator != NULL);

If this condition can actually happen, we'll need some error handling here, not just a g_return_if_fail.

::: widgets/rb-query-creator.c
@@ +490,3 @@
  * settings.
  *
+ * Return value: (transfer full) (nullable): new query creator widget

The query creator widget isn't made available via introspection, so we don't need to add annotations to it.

(In reply to Jonathan Matthew from comment #6)

> ::: shell/rb-playlist-manager.c
> @@ +946,3 @@
>  									     sort_key,
>  									     sort_direction));
> +		g_return_if_fail (creator != NULL);
> 
> If this condition can actually happen, we'll need some error handling here,
> not just a g_return_if_fail.

should this be fine ?

--- a/shell/rb-playlist-manager.c
+++ b/shell/rb-playlist-manager.c
@@ -945,7 +945,11 @@ edit_auto_playlist_action_cb (GSimpleAction *action, GVariant *parameter, gpoint
                                                                             limit_value,
                                                                             sort_key,
                                                                             sort_direction));
-               g_return_if_fail (creator != NULL);
+               if (creator == NULL) {
+                       rb_error_dialog (NULL, _("Couldn't edit playlist"),
+                                        _("Failed to create query creator"));
+                       return;
+               }
 
                if (limit_value != NULL) {
                        g_variant_unref (limit_value);

(In reply to gkrithi8 from comment #7)
> (In reply to Jonathan Matthew from comment #6)
> 
> > ::: shell/rb-playlist-manager.c
> > @@ +946,3 @@
> >  									     sort_key,
> >  									     sort_direction));
> > +		g_return_if_fail (creator != NULL);
> > 
> > If this condition can actually happen, we'll need some error handling here,
> > not just a g_return_if_fail.
> 
> should this be fine ?

I don't think this really helps much, as it doesn't explain what's wrong or how to fix it.

Let's go back to the first part - does this condition indicate a programming error (I think it does), and if so, can we detect it at build time rather than runtime?

(In reply to Jonathan Matthew from comment #8)

> Let's go back to the first part - does this condition indicate a programming
> error (I think it does), and if so, can we detect it at build time rather
> than runtime?

maybe, a unit test to check that all sortable columns in playlist's view and in query-creator sort_options are in sync ?

1. There are currently 16 visible columns in view [Rhythmbox Preferences -> 15 visible columns] + Title column ( which is mandatory )

2. There are currently 15 options in sort_options array 

const RBQueryCreatorSortOption sort_options[] = {...}

out of which "Album Artist" is not a displayed / visible column. Not sure if it would be helpful to sort on "Album Artist" when it is not displayed in the view.

3. Missing column in view = "Album Artist"

4. Missing columns in sort_options = "location" and "bitrate". This is addressed in attachment 365783 [details] [review].

any suggestions ?

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