fossfreedom / coverart-browser

Browse your cover-art albums in Rhythmbox v2.96 - 3.0+
http://xpressubuntu.wordpress.com/
GNU General Public License v3.0
74 stars 19 forks source link

Make 'Open Containing Folder' plugin also work with coverart-browser #147

Closed iGadget closed 11 years ago

iGadget commented 11 years ago

As per title. I now have to move back to the 'Music' view to be able to use this feature. Alternative solution: integrate this feature into coverart-browser, rendering the plugin obsolete :-)

asermax commented 11 years ago

Well, the initial idea was to try to integrate all the plugins into our source too, but there's a problem into how the current plugin system is implemented in Rhythmbox's: each 'default' source's popup have a placeholder for plugins ui. Each plugin that wants it's ui integrated on those popups, adds a new menu item there.

So there isn't an easy way to do that, but here are some ideas:

  1. Add a placeholder into our plugin and ask every plugin dev to add the ui to our popup too (I don't think this is feasible)
  2. 'Borrow' another source's popup and try to use it in ours. This can lead to bugs and other kind of problems (since the popups have some dependency on the source they belong, e.g. the currently selected entry), so we should be careful.
  3. Build some kind of extension system to be able to write 'adapters' for each plugin we want to integrate. So for each interesting plugin, we should write an adapter that would do some custom logic to include it in our source, trying to avoid the rewrite of the same functionality on our plugin.

It's an insteresting idea but we need to discuss it a little more, so be patient d:

fossfreedom commented 11 years ago

Re 3:

Some notes -

Possible bridge-work - each plugin we support should have a unique ActionGroup name. Thus the combination of "supported plugin" + ActionGroup we can get the list of Actions Gtk::UIManager::get_action_groups & Glib::RefPtr< Action > get_action (const Glib::ustring& action_name)

Thus from the action-name we need to bridge the callback used with our plugin methods.

We can "activate" the action associated with an action via is activate method, thus - for example, if supporting the SendFirst plugin, we'll need to "activate" several times for each track-entry in an Album.

We'll need to take into account the view we are using - so each supported plugin needs also a map between view & action-names.

i.e. QueuePlaylistViewPopup, BrowserSourceViewPopup, PlaylistViewPopup


ok - problem areas - we can only add right-click menu options for each supported plugin if that plugin is activated. Is there a Peas method to do this?

We'll need to remove the right-click menu option if the plugin is deactivated - is this possible? Maybe we can kludge this by building the right-click menu EVERYTIME the right-click menu is invoked.

I suppose we need also to take into account if the action is disabled/enabled - I presume actions become disabled/enabled if the associated MenuItem is also disabled/enabled - is this a true statement? I suppose the Action get_sensitive & get_visible methods would be useful here - especially if we are building the right-click menu EVERYTIME.


Implementation? obviously base-class and derived classes where derived classes are for each supported plugin and base-class has common functions.

I suppose the base-class should have a method that creates a menu-item with our own "ActionGroup" & Action. The derived classes will respond to the Action and call the supported plugin Action.activate() method.

asermax commented 11 years ago

The PeasEngine class haves a method to get the currently loaded plugins (by loaded I think it means activated) and a couple of signals to know when a plugin is loaded or unloaded. This seems like a shitload of work to do, it think we should prioritize the things we're working on right now and let this for a future release.

fossfreedom commented 11 years ago

@iGadget

I've added some functionality that hopefully adds this feature - if you have sometime, can you checkout the master branch and test this?

The following external plugins are "supported" by the coverart-browser

"Supported" means a menu item is made available when you right-click either an album or a track(s) in the Track Entry View.

asermax commented 11 years ago

@fossfreedom wow, neatly implemented! I like how you tightly packed all into it's own module :3 Here is what I experienced and my opinions:

  1. I have all of the supported plugins activated, but only the SendFirst and SendTo options are showing up. Here's the relevant trace.
  2. The plugins I mentioned above are working flawlessly!
  3. The LastFMPlugin may not be working given the changes I made on the plugin structure (not sure in which version you based the correspondant ExternalPlugin, gonna read the code with more detail later).
  4. I think the EnhancedIconView should know nothing about the external plugins. I think it may be better to build a wrapper class that acts like a popup, but that internally contains the external plugin's group. Something like:
    class ExternalPluginGroupPopup(object)
        def __init__(self, group, popup):
             self.group = group
             self.popup_menu = popup

        def popup(self, *args, **kwargs):
             #code that creates/add the menu items
             self.popup_menu.popup(*args, **kwargs)

That way you keep the widget and the external plugins mechanism decoupled.

Kepp the good work :+1:

EDIT: Oh, nevermind the fingerprint issue, it doesn't show up on the IconView but it does on the entry level (as it should :F). Another thing that I noticed is that they are missing the icons, probably you didn't got into that yet.

fossfreedom commented 11 years ago

@asermax - cheers for the feedback

yep - my fault with the opencontainingfolder - I was playing with my own local name version with the wrong name. Thats fixed now.

re the wrapper class - I presume this is a wrapper around GtkMenu - and the new widgets handles any popups including the building of the external-plugin menu items?

coverart_entryview.ui and coverart_browser.ui should use this new widget instead of the default GtkMenu as present.

Let me know if that's the direction you were leaning towards - I'll then crack-on and refactor that way.

Re missing icons - screenshot? I've never noticed any icons with menu options... maybe that is the way my desktop-environment is setup.