Merge lp://staging/~victored/beat-box/view-restructuring into lp://staging/beat-box/1.x

Proposed by Victor Martinez
Status: Merged
Merge reported by: Victor Martinez
Merged at revision: not available
Proposed branch: lp://staging/~victored/beat-box/view-restructuring
Merge into: lp://staging/beat-box/1.x
Diff against target: 8950 lines (+4129/-2830)
39 files modified
core/Settings.vala (+130/-11)
schemas/org.elementary.beatbox.gschema.xml (+31/-0)
src/CMakeLists.txt (+2/-2)
src/DataBase/DataBaseUpdater.vala (+2/-3)
src/Devices/CDRomDevice.vala (+3/-2)
src/FileOperator.vala (+4/-3)
src/Icons.vala (+4/-5)
src/LibraryManager.vala (+32/-35)
src/LibraryWindow.vala (+473/-463)
src/Objects/Media.vala (+4/-1)
src/Objects/Playlist.vala (+30/-5)
src/Objects/SmartPlaylist.vala (+14/-1)
src/Store/Widgets/StoreView.vala (+4/-4)
src/Views/AlbumView/AlbumIconView-clutter.vala.BACKUP (+0/-720)
src/Views/AlbumView/AlbumIconView.vala (+61/-71)
src/Views/AlbumView/AlbumIconViewClutter.vala (+736/-0)
src/Views/AlbumView/AlbumListView.vala (+8/-6)
src/Views/ContentView.vala (+4/-8)
src/Views/DeviceSummaryWidget.vala (+2/-0)
src/Views/DeviceView.vala (+2/-3)
src/Views/DeviceViewWrapper.vala (+9/-5)
src/Views/ListView/BaseListModel.vala (+5/-2)
src/Views/ListView/BaseListView.vala (+38/-66)
src/Views/ListView/MillerColumns/MillerColumn.vala (+719/-0)
src/Views/ListView/MillerColumns/MillerModel.vala (+376/-0)
src/Views/ListView/MusicTreeModel.vala (+3/-2)
src/Views/ListView/MusicTreeView.vala (+25/-26)
src/Views/ListView/PodcastListView.vala (+14/-22)
src/Views/ListView/PodcastTreeModel.vala (+4/-2)
src/Views/ListView/RadioListView.vala (+10/-17)
src/Views/ListView/RadioTreeModel.vala (+4/-2)
src/Views/ListView/SimilarPane.vala (+88/-6)
src/Views/ViewWrapper.vala (+1056/-535)
src/Widgets/InfoPanel.vala (+4/-3)
src/Widgets/MillerColumns/MillerColumn.vala (+0/-223)
src/Widgets/MillerColumns/MillerModel.vala (+0/-274)
src/Widgets/SideBar.vala (+3/-3)
src/Widgets/SideTreeView.vala (+212/-292)
src/Widgets/WarningLabel.vala (+13/-7)
To merge this branch: bzr merge lp://staging/~victored/beat-box/view-restructuring
Reviewer Review Type Date Requested Status
Victor Martinez (community) Approve
Review via email: mp+98298@code.staging.launchpad.net

This proposal supersedes a proposal from 2012-03-19.

Description of the change

Changes in this branch:

1) Each sidebar item has its own ViewWrapper object, which at the same time wraps all the internal widgets.
2) (1) made possible to have a clearly distinct view area.
3) The view area is a GtkNotebook. This allows super-fast switching between sidebar elements
4) Implementation of https://blueprints.launchpad.net/beat-box/+spec/column-browsing
5) Many minor improvements

To post a comment you must log in.
Revision history for this message
Victor Martinez (victored) wrote : Posted in a previous version of this proposal

Although this is a work in progress, feel free to report bugs as comments in this merge proposal.
I'd appreciate if you read the commit logs before reporting issues :)

Thanks for testing!

Revision history for this message
Victor Martinez (victored) wrote : Posted in a previous version of this proposal

The diff below is not right, since I still need to merge the changes from lp:beat-box/1.x. The "The diff has been truncated for viewing" message will be gone after that :)

Revision history for this message
Victor Martinez (victored) wrote : Posted in a previous version of this proposal

Oh no, wrong target branch xD

review: Needs Resubmitting
Revision history for this message
Danielle Foré (danrabbit) wrote :

Hey Victor, decided to do a little testing. These are the issues I found:

* When entering column view, the column took up the whole width of the screen. At first I was unsure how to get to my library in this view.
* In automatic mode, switching doesn't occur the first time you resize the window. You have to resize it twice to make it work. (Freaking awesome that it does this at all though! I am so stoked!)
* Maybe don't show '0' in ratings or years. We don't typically show songs with no album as something in the column.
* If none of my songs are rated, maybe the ratings column should be made insensitive as an option.
* Unchecking a column messes everything up xD
* What view mode I was using and how big I set my column isn't saved when I close BeatBox.
* Not sure it's a good thing to have different view modes saved for different playlists. Marlin's view mode is persistent no matter directory you're viewing, so this is a bit inconsistent.

That said, this is looking awesome. Switching to different lists is instantaneous here and I'm excited to have the absolutely best column browser there ever was :D

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

Hi Daniel, I see you tested everything. Thank you for doing that :)

> When entering column view, the column took up the whole width of the screen. At first I was unsure how to get to my library in this view.

Yes, I'm aware of that and I know what is provoking it. Expect a fix soon.

> In automatic mode, switching doesn't occur the first time you resize the window. You have to resize it twice to make it work. (Freaking awesome that it does this at all though! I am so stoked!)

Yes. When I implemented this for the first time, it was perfect. Initially I was moving the columns from a vertical container to an horizontal container, and there were 0 issues. The automatic mode worked perfectly and since I had two different GtkPaned widgets the handles were not messed up.

Then I realized that GtkOrientable (the parent class of GtkPaneds and such) allowed the orientation to be changed at runtime, and I modified the code to make use of that. However, since you're having the same bug, I'll switch back to the initial implementation for sure :)

> Maybe don't show '0' in ratings or years. We don't typically show songs with no album as something in the column.

Ok, I'll add that special case

> If none of my songs are rated, maybe the ratings column should be made insensitive as an option.

Agreed.

> Unchecking a column messes everything up xD

If you're talking about the UI or the pane separator, that's COMPLETELY related to the AUTOMATIC mode and what I described above.

When you uncheck a column, the filter is changed to "All" in that column, causing the other columns to be re-populated. Well, I don't think you're referring to that here :P

> What view mode I was using and how big I set my column isn't saved when I close BeatBox.

Known issue :)

> Not sure it's a good thing to have different view modes saved for different playlists. Marlin's view mode is persistent no matter directory you're viewing, so this is a bit inconsistent.

Yes, I think the same. Also, syncing the view mode across the items (i.e. views) that support that kind of view will make my life easier, since I won't have to store individual settings. Does the same apply for searching?

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

There are some special cases I'm not sure how to handle. The following for instance:

Now it's possible to have custom views (some views can have millers, album views, etc. while others don't). Let's say that I configure playlists to only have list and filter view (because the album view doesn't make sense in some of them).

What should I do with the view switcher? Making a single item insensitive looks awfully odd and hiding it provokes an ugly toolbar resizing (well, the latter is fixable).

Since the column view is actually a child of the list view, maybe we should apply the same logic to the UI and enable/disable it elsewhere?

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

... Just to compliment my last comment, removing the column view from the view switcher would leave only two elements in there, making it possible to make the whole switcher insensitive when either the album or list views are not available.

Revision history for this message
Matthew Markell (markellmtthw) wrote :

Victor, this is amazing work, great job! A couple of bugs I'd like to bring to your attention: The Next and Back buttons functions are switched, pressing Next make the previous song play and pressing Back makes the next song play. Also, Next and Back will occasionally play a completely random song :s Related to this is that when a song has finished playing, the song before it will begin playing. I'm sure you'll get these figured out :) You rock dude!

Revision history for this message
Matthew Markell (markellmtthw) wrote :

Another bug: Search appears to be broken in all views except for the Miller View.

Revision history for this message
Danielle Foré (danrabbit) wrote :

Hey Victor, Do you think it would be appropriate to move the filter into it's own button? Keep in mind that this would imply that either:
1. you can use the filter and album view at the same time, in which case it wouldn't make sense to show the album column. or
2. filter button is made insensitive when you switch to album view.

Also, I can confirm Matthew's back button bug. I actually filed a report about it thinking it was caused by a song I had where the artists name had a "." in it :P

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

Matthew, thanks for your feedback. I'd really appreciate if you keep testing this branch as I push more changes to it :)

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

Daniel, I think that's a good idea. Not sure about where to put that button though.

In regard to the options... well, it's a tough decision.

1) Showing the filter for all the views. This is exactly how Zune and Banshee do it. The main advantage of this setup is faster browsing when using the AlbumView with very large collections. Another advantage is that I wouldn't have to fix Bug #959885 :)

2) Sounds cool too. Implies no extra changes apart from adding the button. Disadvantage: Bug #959885 still applies here.

Maybe we could play around with the idea. We're already used to (2) since it behaves exactly like the current implementation. I propose implementing (1), testing for a while and then making a decision. Making the required modifications wouldn't take long :)

What do you think?

> Hey Victor, Do you think it would be appropriate to move the filter into it's
> own button? Keep in mind that this would imply that either:
> 1. you can use the filter and album view at the same time, in which case it
> wouldn't make sense to show the album column. or
> 2. filter button is made insensitive when you switch to album view.
>
> Also, I can confirm Matthew's back button bug. I actually filed a report about
> it thinking it was caused by a song I had where the artists name had a "." in
> it :P

Revision history for this message
Danielle Foré (danrabbit) wrote :

I guess we could simply leave it where it is and just have the switcher and stand-alone button right next to each other :p

Yea, let's try #1 and see how it works out :)

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

Okay, I've just pushed a very rough implementation of #1.

Things I don't like:
1) Using the column browser along with the album view kills simplicity.
2) The top-positioned column browser looks weird in the album view.
3) The album list view (the floating album window) is awfully positioned (even if we do it right).
4) Too many widgets. Cluttered UI.

Things I like:
This indeed solves the problems I described in the comments above, but #2 also does.

If you ask me right now, I'd rather go with #2. It keeps the UI simple and beautiful :)

Waiting for your feedback.

Revision history for this message
Scott Ringwelski (sgringwe) wrote :

I thought I would, but I don't like it very much either. Also, the UI seems really laggy now.

What about the possibility of getting rid of the album view? The album view is slow (because of extra algorithms that have to run), clunky, and not very useful IMO.

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

Thank you for your feedback :)

The laggyness is completely my fault. I didn't make the required optimizations and there's a lot of garbage code in the last revision.

I like the album view a lot, and it's a nice way to browse a music library. To be honest, sometimes it feels faster than the list view (well, that could change if we make use of the fast view/model). However, I think I agree partially with you on this. I'd be more than happy to disable it for playlists and other areas where it doesn't make sense (I already did the latter actually). That's just my opinion though.

Revision history for this message
Matthew Markell (markellmtthw) wrote :

Victor, if you are interested in modifying the views, check out this (slightly unfinished) mockup of mine. I think with your current designs you could actually just merge all of the views into one. This mock does not show the top positioned column layout, but it would look similar to Banshee's. Just an idea ;)

http://imgur.com/jLR57

Revision history for this message
Scott Ringwelski (sgringwe) wrote :

That's a nice mockup, but I don't think it's very plausible. It forces you to browse your music Artist->Album->Songs. And, even if you don't browse that way and just click "All artists", you only get a name of a song in the song list and no other information. Other opinions?

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

Matthew: great mockup. You should discuss that with Daniel though. One disadvantage of that design is that you can't add more column filters without making the UI look cluttered.

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

Based on the comments so far, I conclude that none of us liked idea #1.

I will start implementing option #2. This branch is holding on BeatBox's development and should be finished ASAP.

Revision history for this message
Danielle Foré (danrabbit) wrote :

Hey Victor, Yea it sounds like #1 causes a few unexpected problems. Like you, I also do enjoy the album view so I definitely hope we don't get rid of it :p

Revision history for this message
Scott Ringwelski (sgringwe) wrote :

What if we put album images in the miller columns, similar to what banshee does, and got rid of the album view? That way clicking on an album doesn't bring up a popup, it simply shows the tracks on the right like any other miller column? http://banshee.fm/theme/images/slides/scaled/music-450.png

Revision history for this message
Sergey "Shnatsel" Davidoff (shnatsel) wrote :

Rhythmbox's millercolumn-like view worked much better for me than Banshee's view for me today. It's more linear and therefore easier to navigate. Also, is scales better.

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

The new implementation is ready for testing :)

Revision history for this message
Scott Ringwelski (sgringwe) wrote :

Hmmm I don't think it's quite ready for testing. Things are still pretty broken. Search is really slow for me but doesn't work. Nothing persists between different views (miller, selected view mode). Also, was it a final design choice to use a seperate button for the column browser?

Revision history for this message
Scott Ringwelski (sgringwe) wrote :

Actually, the view mode does persist. Although startup is kind of slow still.

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

Yeah, that's true but I was referring to the new View layout (idea #2), NOT the branch as a whole. Otherwise I would have marked it as "Needs Review".

These are known regressions and I'm working on their fixes. Sorry for the confusion :)

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

> Also, was it a final design choice to use a seperate button for the column browser?

I hope so. The button won't be as huge as it's right now in the final revision.

Revision history for this message
Scott Ringwelski (sgringwe) wrote :

Ohhh ok. Sorry.

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

ha no problem :)

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

By the way, are you okay with not having a column browser for playlists?

Revision history for this message
Scott Ringwelski (sgringwe) wrote :

I don't see why it's necessary?

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

Me neither. I removed it from playlists to get rid of lots of bad code I added earlier to sync the miller columns's width and height between view wrappers. I hope no one wants it back.

Revision history for this message
Scott Ringwelski (sgringwe) wrote :

I meant I don't why it's necessary to remove it?

Revision history for this message
Matthew Markell (markellmtthw) wrote :

Victor, some bugs:

Miller columns still don't save their size
When the miller is enabled, the columns in it should be removed from the list view, when the miller is hidden, they would be restored
Like Scott said, search only affects the miller columns and nothing else
The miller columns dont really work :p, selecting an artist doesn't filter down to albums from that artist, etc.

Lots of bugs, but I like the way things are going, keep up the good work dude :)

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

Yea, wait for the next revision. These issues will be fixed :)

In the previous revisions I was only focused in the UI and not in the data, since I wanted to have a clear UI layout before working on that (and that's why it's been broken until now). See Bug #959885

Revision history for this message
Scott Ringwelski (sgringwe) wrote :

I'm really glad to see you working on this. It'll be good to get this finished ASAP and then ironing out the bugs before Luna. The release for Luna has to have NO BUGS, even if that means removing buggy features.

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

Hey guys, it's time to start hunting down regressions. The ones I'm aware of:

1) Slower startup (not sure I can do much here). This is the price of fast sidebar switching.
2) Broken playlist system (WIP)
3) After media is updated, the column browser and list view are not updated (WIP)
4) No column browser for other views (this will be fixed later. Probably won't land in this branch)
5) Column browser doesn't save its width or height in some cases (probably won't land in this branch either)

(1), (4) and (5) should be fixed on separate branches. And since (4) makes BeatBox slower, I'd recommend fixing it after 1.0.

Right now I'm working in (2) and (3). If I missed any regression, please let me know :)

Again, thank you for your feedback!

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

To Scott:
I think you probably noticed that I removed set_as_current_list() from the list views. Well, I'm aware of what it breaks and a fix is in the works. That will properly fix Bug #921060: Playing a song from the new album view doesn't mark it as playing in other views.

To everyone else:
What I wrote above is the reason why you lose your last media selection (yeah, the list you're playing) when you switch to another playlist or library.

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 :)

Revision history for this message
Matthew Markell (markellmtthw) wrote :

Love your work Victor, one regression: Shuffle no longer works and clicking the next and back buttons will sometimes start playing random songs.

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

Thanks Matthew ! :D

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

I forgot to update this

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches