Idea: Rework Genre combobox to create button

[fossfreedom]

The current Genre combobox feels out-of-place with the rest of the toolbar button approach.

What I intend to do is to replace this with a button - when pressed, a popup menu with genres is displayed to choose from.

Hovering over the button displays the genre currently chosen.

[fossfreedom]

@asermax

let me know your thought on this one - I've derived two new GtkButtons - PlaylistButton and GenreButton and have added these new widgets to the coverart_browser.ui glade file.

I'm thinking we can look at something like this implementation approach where common objects should have all events and functionality within a derived object rather than connecting signals on the main coverart_browser_source and adding all signal functionality to the one python module.

For example, move the toolbar into its own widget (for example ToolbarBox)- thus any subsequent view we could create could just add the ToolbarBox widget to the view's glade file itself

[asermax]

I was thinking of something among the same lines about refactoring the source module, separating widgets an their responsibilities seems the right approach to take.

About the issue at hand, I like very much how the pop-up button looks and acts, it sure smoothes the interface an the way to interact with it. The only detail I'm not sure about its using the current selection as a tooltip instead of replacing the button label. But I guess replacing the label isn't right either since the length of the text varies... so it's good.

Maybe we could replace the sorting button for a pop-up as well? It would save lots of space and look way better in my opinion.

[asermax]

Oh some other things I noticed why playing with it a little bit:

• The playlist popup haves no problems, but the genres is kinda slugish. I checked the code and I think it's because you recreate the whole popup each time it pops. With few entries like in the case of the playlist one, it isn't a problem, but since my collection is big (and not really clean), I've lots of genres in there, recreating the popup each time is really costly in that case.
• This new approach doesn't take into account when a genre is emptied (this isn't really a bug, just a nice enhancement)

I did some work on that when refactoring; since we're not using a combo anymore, we can't do the same as before, but we can still use the model. It would be nice if you could reimplement that, but I can do it as well if you want me too. In a nutshell, just keep an eye on the changes on the genres model and update the popup acordingly. Only recreate the popup on the background when a genre is added or removed, and if the removed genre is the selected one, remove the genre filter :3

[fossfreedom]

@asermax - ok - yes agree the sort button approach looks good.

Have implemented that - there is now lots of stuff that can actually be deleted from coverart_browser_source - for the moment I have commented this out. Also, we can now remove the optional display of ratings, year & genre from the settings preferences.

Just one thing I need inspiration on - line 269 of coverart_widgets - the gsettings bind statement against a new gsetting field to store the sort_by value - it keeps throwing an error

  File "/home/dad/.local/share/rhythmbox/plugins/coverart_browser/coverart_widgets.py", line 270, in initialise
    self.sort_by, 'active', Gio.SettingsBindFlags.DEFAULT)
  File "/usr/lib/python2.7/dist-packages/gi/types.py", line 43, in function
    return info.invoke(*args, **kwargs)
TypeError: Expected GObject.Object, but got GObjectMeta

any ideas?

[asermax]

There, fixed that issue for you. Seems like you copied the line from somewhere else and it hadn't much sense there. The first parameter after the key name is the object you want to bind the setting to, and the third is a string with the name of the property of the object to bind to, that's why it wasn't working. Also put the popup creation on the initialise method, since it won't be changing (is not dynamic as the genres or playlists) there is no need to recreate it.

Another thing that would be nice to add (on the base class probably) is a way to indicate the current selected option, like the little dots on the search entry pop-up.

[fossfreedom]

cheers for that. I'll have a look at converting to Gtk.RadioMenuItems instead of standard Gtk.MenuItems.

[fossfreedom]

Note to self - Noticed that selecting a playlist where the model is empty, the result is a full display of covers. Maybe the new filter for playlists cant cope with no rows in the model?

[asermax]

It's not that, I made it like that:

def model_filter(cls, model=None):
        if not model or not len(model):
            return lambda x: True

        albums = set()

        for row in model:
            entry = model[row.path][0]
            albums.add(Track(entry).album)

        def filt(album):
            return album.name in albums

        return filt

if you want it to return nothing, then you can change it for this

def model_filter(cls, model=None):
        if not model or not len(model):
            return lambda x: False

        albums = set()

        for row in model:
            entry = model[row.path][0]
            albums.add(Track(entry).album)

        def filt(album):
            return album.name in albums

        return filt

I didn't find much use of a filter that will return nothing, but maybe it's better that way to keep the consistency.

[asermax]

I made the changes to keep track of the genres model instead of recreating the popup each time.

Btw, the bottom expander doesn't have text anymore and it's resizing behavior is broken. I know we want to reimplement the whole thing anyway, but we should do one thing at the time, trying to not break everything else, otherwise it would be to hard to keep track of what we have to work on :/

[fossfreedom]

I think when I was deleting all the old commented out code, I may have remove the line controlling the expander. I'll have a look back at the cleanup commit and see what I accidentally removed :/

[asermax]

A question related to the implementation: why are you using actions for the radios and their signals? I though actions were used when you wanted the same action be executed by various UI elements, but I'm not sure.

Something related: the changed signal gets fired twice each time an item is selected, one for the item being selected and another for the one being deselected. This efectivelly resorts/refilters the model twice. There should be a check to the active property of the action that threw the signal.

Another weird behavior: on the genres popup, after selecting an item, trying to select another one which is positioned before the selected, doesn't work. It works for items after the current one, I have no idea why.

[asermax]

I made some changes on the genres-no-actions branch, to see if I could fix the issue I mentioned about the genres popup. It seems to have worked, still, I don't have any idea of why. Anyway, if you like it, merge it to the genre branch.

EDIT: also found the missing singals on the bottom pane, fixed that too on the branch.

[fossfreedom]

No particular reason to use actions rather than a standard toggle - other than future flexibility.

Does the refilter still fire twice on your music collection with genre-no-actions ?

In the Gtk documentation it says that using set_active on a radiobutton fires the toggle signal - which would account for the double filter observation.

[asermax]

I added a check on the active property of the item for which the signal is being called, so even when the signal is fired twice, only the active item is the one that refilters the model.

Btw, you had the same issue as me with the genres popup? I still don't get why it was failing, and idk if it was the actions which were doing that or not xD

[fossfreedom]

no I didnt - so confused.

Anyway - I've merged and tidied up the icons - the sidebar toolbar view is especially nice :) Think I'll make that my personal view from now!

[asermax]

It does looks a lot cooler now. I preffer the top toolbar, but still, I like the new buttons, that's why I was so eager to fix the little problems that kept poping up x3

Btw, other things I've found:

• The sort popup menu duplicates it's entries each time you change it's position. We probably are calling initialize each time we call the _toolbar method (haven't seen the code, but pretty sure about it). The solutions are, either call the initialize once on the create_ui method or add a clear_popupmenu on the initialize method body. I think the first option would be more "correct".
• The playlist popup should have the Play Queue as an option too.
• There is no more need of the Cover Art option on the others sources popup menu, since we have the playlist popup now.
• The sort image is kinda out of place xD it should be transparent and black/gray as the others.

Another thing, the changes you've made on the settings UI probably are gonna conflict with the changes I made of it on master. If you want me to, I'll take care of the merge once we're done with this :3

[fossfreedom]

I'll crack on and do this tonight :)

[fossfreedom]

tidied stuff.

I've added a couple of up and down arrows png files to the source base - will use this when doing the toolbar widget. The sort order button should be changed to use the png images. I'll create another task for this.