Code review comment for lp://staging/~victored/beat-box/view-restructuring

Revision history for this message
Victor Martinez (victored) wrote :

Ready or not, I'm marking this for review right now. There are only minor issues (widget settings, etc) remaining (mentioned in the last two comments) and I don't plan to fix them alone. I already touched around ~9000 lines of code and tested this during a month and really would like to get some help to finish this up (In case it gets approved).

WHY DID I MAKE THIS BRANCH? (I mailed this to Scott some time ago)

While working on the miller-column-positioning feature and making some fixes to the view switcher, I noticed that I was adding too many workarounds to LibraryWindow.vala and that we're doing a lot of view-related work there, making it very easy to introduce regressions like bug #929720. (we handled everything there!) [DONE]

In my opinion, we shouldn't manage any internal aspect of the views inside the Library Window. For some views (especially for library items), the welcome screen and miller columns should be inside the ViewWrapper object. That way we don't have to care about hiding/showing elements in the library window. [DONE, except for the welcome screens. That will come later :)]

I also noticed that re-populating miller columns is slower than having separate objects. It also slows down switching between the sidebar elements... [DONE]

The object-dependency tree is much cleaner now (for instance, the album list view now belongs to the AlbumView instead of the LibraryWindow). I've also added a method to the library window to make adding new views a piece of cake. It takes care of adding the sidebar elements and creating the views. The internal changes to the ViewWrapper are still in the works and I'm moving things all over the place between the main window, the views and the view wrapper itself. [DONE]

In conclusion, this has been a bug-fixing branch rather than a feature branch. Normal users wouldn't find any new feature apart from the new configurable column browser. They will find a more responsive application that doesn't access the hard disk lots of time while playing their music.

Now I will highlight improvements and regressions:

IMPROVEMENTS:
1) Fast sidebar and view switching (without using threads!)
2) Automated code inside View Wrapper
3) Clearly-defined view area
4) Ability to have custom views. For example, a different welcome screen for podcasts, radio, etc.
5) Lots of minor improvements

REGRESSIONS (these are well identified and documented in the code):
1) Slower startup (we can make some improvements in other areas to compensate this)
2) Add-to-playlist doesnt work. Actually, it's broken in 0.3 and 1.x as well, so this is not a regression
3) Broken queue
4) Doesn't respect the song list (this is the media we play when switching songs/media) selected by the user. It doesn't work properly in 1.x as well, so this wasn't introduced completely by me. I only need to add a more solid replacement for set_as_current_list() (which I removed).

Looking forward to your opinions :)

« Back to merge proposal