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

Source refactoring #123

Closed asermax closed 10 years ago

asermax commented 11 years ago

Well, as I and foss planned, it's time to start doing some refactoring on the source to correctly divide the functionality in multiple specialized widgets or objects (there may be some logic without a prpper UI) to achieve a better modularized code, easier to modify and extend.

I though of using this post to put down some guidelines about how we should work on it:

Finally, we should use this post to notify of changes and discuss about different aspects of the refactoring.

For starters, I did a clean up of the widgets module, removed some unneded code and moved all the logic related to the specialized buttons there. Next I think I will tackle the main cover view widget if it's posible (if it doesn't break everything).

asermax commented 11 years ago

Well, I decided to do a little more remodeling on the current state of the widgets module. Since we have multiple UI elements that do the same (the buttons on the toolbar and sidebar), I thought of trying a MVC-ish approach and separate the UI from the plugins specific logic. I will be refining this approach on the ui-controllers branch to check it's viability. If I'm succesful with this approach, I think refactoring the toolbar among other things will be a little easier to achieve.

asermax commented 11 years ago

I'm done with the toolbar refactory. I made a lot of changes, reduced the number of custom widgets and made changes to all the buttons and the search entry to use a controller, to which they delegate:

Doing this changes, I was able to:

Also, introduced the ToolbarManager class that takes care of the toolbar initialization and positioning. Currently it creates the 3 toolbars (the topbar and the two sidebars) on initialization, so it may introduce some more memory/cpu usage, but I believe is not enough to worry, and doing so simplifies the code. If it brings some problem tho, it's open to modification.

fossfreedom commented 11 years ago

Cheers for the heads up - I'm going to make some minor changes in this area to deal with some translation issues so I'll be taking a great interest on the refactor stuff you've described.

I'll do this in a "translation" branch for the moment.

asermax commented 11 years ago

Alright, let me know if there's something out of place :3 btw, the idea is to complete the refactoring for the next release? Idk how fast we can go through this, is quite a lot xD

Anyway, another update: moved the quick search to another widget. The logic is merged with the widget for now, I'm probably gonna separate it in the same way I did with the buttons, for the sake of reusability.

fossfreedom commented 11 years ago

@asermax - yes - I would like to get the good stuff you have been working with into the next release.

I agree - lots to do - but if we are both comfortable with everything done so far - we should merge up into the refactoring branch - and if absolutely sure - upto master.

I'm keen to get the ui-controllers branch into master asap - its working good so far for me. Obviously I'm going to give this a complete test whilst making the minor changes to incorporate the translation stuff. I'm going to try (as far as time constraints allow - back to work and all that) to complete my changes in the next couple of days.

asermax commented 11 years ago

Ok, we'll see how it goes. ui-controllers is pretty stable so far, there're still some minor adjustments I would like to make. If you're ok with it, I'm going to keep working on it for the next couple of days and merge it when I feel more comfortable with the results. After the merge there will still be the most important part ahead: refactoring the main view (the icon view) to make a more encapsulated, self-contained code.

fossfreedom commented 11 years ago

@asermax - just to let you know where I'm at at the moment.

I've branched ui-controllers for the translation stuff.

Its still working for "english" with all the new translations in popups.xml. For some reason the decade button doesnt show an image for "All Decades" in spanish. I'll fix that soon.

The other aspect I havent had time to look at yet is if you make the RB main window larger and then smaller then the coverview main_box seems to not expand to full space - indeed you can drag the right-hand side to make smaller. Can't think why I broke that though.

asermax commented 11 years ago

Ok, good to know :F I'm still refining some stuff on the ui-controllers, for instance separated the QuickSearch widget from the album search logic (as you may've noticed). There're some other stuff I want to take a look into, but all seems good so far.

I have no idea why you may be having the problem with resizing tho, neither you or me (I think) touched something related to that :/

fossfreedom commented 11 years ago

Notes (I'll update as I go along).

  1. In XFCE - right click album - properties, the window is readonly - i.e. cannot change any values - N.B. properties editable on music source
  2. issue with resizable cover-view also occurs on ui-controller branch but not master.
  3. Changed track to Clasica - genre was still unknown - need to look at what spanish genres it picked up from popups.xml
  4. add comments around CaseInsensitiveDict on coverart_utils
  5. Tidy up README to have a debian and a yum installation package line. N.B. new prequisite package - python-lxml
  6. Test merge with ui-controllers - supect PO folder will cause difficulty. Remove all contents of po folder in ui-controllers branch - translationfixes branch will supersede this - N.B. similar action required for master branch.
  7. Investigate coverart_album, & controllers python files why cause warning with xgettext
  8. Investigate how to drag comments from python files when extracting translations.
asermax commented 11 years ago

The properties dialog is the same used by rhytmbox itself, I don't see how it's related to the plugin (unless it works correctly for the music library source?).

For 2 you should try bisecting the git tree on ui-controllers to find where the issue appeared first

fossfreedom commented 11 years ago

@asermax - yep - sorry, should make it clearer - those are just notes as I'm testing the translation stuff - just stuff I need to remember and investigate later.

fossfreedom commented 11 years ago

The coverart_browser.ui.main_box becomes user resizable (and you can visually see the right edge of the box) due to this commit

However I dont particularly see any obvious reason why you should be able now to drag the right-hand edge whereas the previous commit (fc97a53d7d742a600abf64b021051f981047) it was ok and main-box completely filled the source

I'll continue looking at the diffs between the two to see if I garner any inspiration.

EDIT:

its something to do with these lines in ToolbarManager

#plugin.shell.add_widget(self._bars['left'], RB.ShellUILocation.SIDEBAR,
        #    expand=False, fill=False)
        #plugin.shell.add_widget(self._bars['right'],
        #    RB.ShellUILocation.RIGHT_SIDEBAR, expand=False, fill=False)

if I comment out as shown all is OK.

EDIT:

specifically its due to the adding of the right widget when there is no right pane actually visible - if a right pane is visible, then all is ok again.

fossfreedom commented 11 years ago

@asermax - I doubt there is a "fix" as such.

However taking the spirit of your approach to be as generic and maintainable as possible, I've made objects of each Toolbar that look after themselves to create, hide & show.

Thoughts?

You had a separate "_search_controller" instance variable - given the dict controller var you had, I thought I should add the search controller as an entry to that dict to make all the objects "controllers". Was this your intention?

asermax commented 11 years ago

Seems ok, no objections on the general idea, it works as intended. I didn't see the need to add the search controller to the dictionary, since it didn't change much; the search entry is created in the code itself, so it wouldn't add any value (the reason to use the dict was to simplify the controller asignment with the for loop on the builder objects). It's ok either way, it doesn't change anything :F

I have one doubt tho, what is the classproperty for? Doesn't asigning a class variable (as the ui variable on all the toolbars) have the same effect?

fossfreedom commented 11 years ago

yep - correct - I was playing around with read-only properties.

Rather than the coverart_browser_source file - should the toolbar stuff be in its own .py file - or maybe coverart_widgets?

I'm done with this branch - thinking of merging with ui-controllers. ok with this?

asermax commented 11 years ago

If you're subclassing GObject, you could as well use GObject's readonly properties using the @GObject.property decorator.

I think they are good where they are, they aren't really widgets, they just encapsulate the logic to manage the toolbars. I don't think it justifies another module either; but feel free to go ahead and do it if you think it will be better organized that way.

And seems good to merge; you can also merge ui-controllers into source-refactoring once you've merged translationfixes; I've revised most of the code on the controllers branch and seems to be ok so far. And if you really need to (for testing or translations reasons), you could also do a provisory merge of the refactoring branch into master.

fossfreedom commented 11 years ago

hmmm - the conflicts with master were much bigger than I was hoping.

I think I got 99.9% correct in the "proposedmaster" branch.

There is a worrying message about the cellrenderer stuff though which I'm not too sure of.

fossfreedom commented 11 years ago

weird - second commit sent up what I sent previously. Anyway - solved the cellrenderer stuff.

I'm going to run through lots of stuff to confirm all is ok before pushing this into master.

I'm going to take a couple of days giving this a good run through before doing this push.

asermax commented 11 years ago

I haven't been able to put much time on this since I'm doing a lot of things around here, but I still think there's much to be done in the source module. Actually, I'm currently working (slowly) on refactoring the cover view to make a more flexible widget, encapsulating all the methods related to retrieving the selected objects, managing the popup, among other things.

fossfreedom commented 11 years ago

Hey - thanks for the heads up.

I'm mainly in testing mode now before release. I need to revamp the README, sort out some wiki stuff and write some stuff for the blogs to get them to produce an article or two about this release.

I'm going to relook at the Discogs stuff - probably will dump the python client module and just use standard json stuff as per musicbrainz and the coverartarchive.

fossfreedom commented 11 years ago

@asermax - I think the source_refactoring branch should be deleted and recreated. Its too far out-of-step with the master branch now. Agree?

asermax commented 11 years ago

That's exactly why I didn't want both branches to get mangled in the first place. Anyway, if you are planning to do something related to the refactoring, go ahead and do what you think would be best, otherwise let it be and I will take care of it when I keep working on it (probably next week).

fossfreedom commented 11 years ago

@asermax - when you get a moment (!) - I've updated the README for this release.

Please can you double-check to see if I've captured this release accurately.

I'll update the linked wiki with more up-to-date screenshots sometime this week.

asermax commented 11 years ago

Seems to be a very well rounded summary :3 if I can think of something else to add on there, I'll let you know. Just in case, the markup I used on the docstrings of some classes is based on sphinx documentation tool. It's not important at all, the documentation on doxygen looks very good anyway, just a heads up if you want to check it out.

asermax commented 11 years ago

@fossfreedom Hey! I'm still kinda busy to sit down and do some coding myself (I always get carried away and I can't afford that right now xD), but I took some time to take a look to your latests commits. I have some comments/recommendations:

Anyway, keep the good work, I'll go back to study xD

fossfreedom commented 11 years ago

cheers - I'll have a close look at your suggestions a little later :)

I'm also looking at the #152 - specifically lastfm.py in /usr/lib/rhythmbox/plugins/artsearch

I note in the code there is an LastFM API_KEY specifically linked against jonathan@d14n.org ... I presume this means I can't pull that module into our code stream? I'm not sure about LastFM licensing - have you come across this issue?

asermax commented 11 years ago

Idk if that would be against the license, but I don't think you should use it without permission. Anyway, you could get an API key and secret for the new cover search plugin yourself, LastFM doesn't have an strict policy to getting one for non-commercial uses. Just go here and apply for one, if I remember correctly you don't even have to wait for someone to revise the application, you get access right away.

fossfreedom commented 11 years ago

excellent - got the API key. What is the secret key used for?

asermax commented 11 years ago

It's part of the user authentication process(OAuth). You don't need to worry about that for the cover search, apparently d:

fossfreedom commented 11 years ago

@asermax - FYI - as requested, I've whizzed through the issues list and moved stuff around so that there is a definitive list for milestone 0.8 vs milestone v0.9. Obviously - stuff can & will be moved back and forth.

Hope the study and exams are going well :)

asermax commented 11 years ago

Heh, thanks for the good whishes d: I'm hanging in there, I should be able to keep working on the plugin after next week.

Gonna check the issue list when I've some time and let you know if I find something odd d:

asermax commented 11 years ago

@fossfreedom I'm testing out some stuff on the cover_search_queue branch, would you test if it's working fine for you? It should work exactly as before, the difference is in the internal mechanism to manage the search queue. If you find anything out of the ordinary, please let me know! Gonna keep doing some refactoring now :F

fossfreedom commented 11 years ago

cool - yes - will test tonight (12 hours time).

fossfreedom commented 11 years ago

if I was supposed to be able to break it - I couldnt.

So looks good :)

asermax commented 11 years ago

Kay, thanks! Gonna merge that branch in then.

asermax commented 11 years ago

@fossfreedom I'm finishing up the CollapsiblePaned widget that replaces the current way we're managing the bottom pane. This new paned widget allows one of it's childs to be wrapped on a GtkExpander and abstracts a bunch of the 'squeeze when collapse' and 'save last position' functionality that we had coded on coverart_browser_source directly. It still haves some rough edges but it's working fairly well for me. If you have some time please test it out and let me know if you notice something that was previously working but it doesn't on the collapsible-paned branch. After polishing some stuff, I'll add some comments to the code and if everything is working alright, gonna merge it to master!

fossfreedom commented 11 years ago

Hi - just a quick look - a bit busy at the moment.

I've checked-out a fresh copy of the plugin - the entry view no longer opens at all - not manually via the expander nor by clicking the album icon twice. No error messages in the terminal. The expander icon toggles from down to right though but you cannot move the expander bar vertically - the pane-bar is fixed in the closed position.

I'll try to figure out what is going on when I get some time this weekend.

fossfreedom commented 11 years ago

ok - realised my error (I think)

Start rhythmbox - coverart

click and drag the pane divider downwards (i.e. dont toggle the expander manually or open by clicking an icon)

Then click an icon twice - you'll see the expander toggling - I didnt realise that I had to drag the divider upwards again.

ok - happy now that I've figured that out. All looks ok to me.

asermax commented 11 years ago

Good good :D thanks for your time. Well, if my test go alright and I finish up the details, I'll merge this :3

asermax commented 11 years ago

Okay, I added some documentation and merged the CollapsiblePaned to master, as you should have noticed. I still think there're some stuff that need to get refactored on the source module:

fossfreedom commented 11 years ago

@asermax

ok - crossing threads - my comments about the popups in the other thread... I presume this is the direction you were thinking?

I was playing with some coverflow stuff... before you say it - I've no intention to release this - its really just a playtime thing.

One area that it brought up is that with new views the coverart_album class does a lot of specific iconview stuff such as cover manipulation, renderer stuff and passing in iconview as a variable.

When (if?) a new view is created such as a coverflow, any thoughts on what the data-model link between the different views should be? - dont think it should be a iconview.

N.B. I pushed up a coverflow branch - it doesnt work! - I was trying to extract the pixbufs from the iconview liststore in coverart_browser_source.load_finished_callback - self.coverflow_box.update_covers(self.covers_view) when I stopped thinking maybe this isnt really the best approach.

asermax commented 11 years ago

Yes, subclassing the GtkMenu is one option to achieve what I mentioned on the other thread. My best bet goes into mimicking the current widget-controllers structure that we're using for the toolbar buttons: we have a subclass a GtkMenu which GtkMenuItems depend on the options of a controller class. The only problem here would be finding a way to listen to changes on the subjacent structure (changes on the current enabled plugins for the external plugins, changes on the currently existing playlists for the playlists items) instead of recreating the menu each time it's shown. The other optión is changing the behavior for this particular widget-controller pair to make it reactive, so the popup delegates to the controller the creation of it's items whenever it's asked to be shown. The main reason for me to propose this separation is that it will make it more reusable if we need to makle use of the popup again somewhere; directly subclassing GtkMenu will make a very specific menu that will work only on the given cases.

from #147 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.

For the views: probably the best approach is to share the AlbumsModel between views (actually, the whole AlbumManager, but the model is the most important data wise), and each view takes care of displaying the covers however it sees fit. There are already some signals that may be util for updating the view on changes of the underlying data, but when we start designing the whole view things (it shouldn't be something duck taped around what we have, we should think of an extension system as the one you made for the external plugins), it will be open to modification to adapt the model to most needs that views may have.

fossfreedom commented 11 years ago

@asermax - when I was regenerating the translation strings I noticed that "Tracks & Covers" in the collapsible pane loses its translation ability. I presume this is the new widget?

Thoughts on the best approach to fix this?

Probably need to do something for this part of the code in coverart_widgets.py

def _on_collapsible_label_changed(self, *args):
        if self._expander:
            self._expander.set_label(self.collapsible_label)
asermax commented 11 years ago

Yup, that probably is it. I added the translatable marker to the label in the ui file; idk if you can arbitrarely add that marker to any field, but please try it out d:

fossfreedom commented 11 years ago

yep - regenerating the po's has pulled in the translatable text. I havent switched my locale to spanish to test - so cross fingers the existing locale stuff picks up the translatable string correctly.

I'm going to freeze the translatable strings now and am going to ask the launchpad translators to do their magic over the next two weeks.

asermax commented 11 years ago

@fossfreedom just moved the statusbar related code to some classes to detach it from the source. I think it's working alright (it wasn't a big change really), but feel free to test it on the refactor-statusbar branch. If nothing comes up, I'm going to merge it to master during the weekend.

fossfreedom commented 11 years ago

yep - looks good. Nice bit of lateral thinking :) Cheers/

fossfreedom commented 11 years ago

@asermax - need some advice.

on coverart-search-providers I have a module called rb3compat.py

on coverart-browser I similarly have a module called rb3compat.py

despite doing a "import rb3compat" or "from rb3compat import xyz" in (for example) coverart_external_plugins in coverart-browser, python/rhythmbox insists on loading the coverart-search-providers version of rb3compat not the local version in coverart-browser.

Now I know that I can just rename rb3compat.py as something else ... but is there a better python way to say "load the local version" first?

asermax commented 11 years ago

I think we already talked about this once: peas loads a module in the global context an it's available to all the plugin's out there. Basically, the first module being loaded is the one accessed by all the plugins, so the only way to do with you want is changing the names :T

fossfreedom commented 11 years ago

ok - cheers. Rename it is then.