playlist subsystem overhaul

[Ian Monroe]

On Mon, Oct 6, 2008 at 1:01 PM, Soren Harward <stharward at gmail.com> wrote:
> As I mentioned on amarok-devel last week, I've been working on an overhaul of
> the playlist subsytem, and it's now ready to commit to the SVN tree.  It fixes
> a bunch of bugs, and (from my perspective) vastly cleans up and simplifies the
> subsystem.  But it's a big (about 16k lines, 6k of which are entirely new code)
> invasive patch, so I'd like to have a couple people look over it before I
> commit it.  The full patch (against r868502) is available at:
>
> http://www.mandy-and-soren.org/misc_files/playlist_overhaul-r868502.patch.gz
>
> And here's what the SVN commit log would be if I were committing it now:
>
> ---------
> Major overhaul of the playlist subsystem
>
> == Changes for end users ==
> Mouse and keyboard event handling, and the selection system have both been
> significantly improved.  The undo/redo commands now work properly in all cases.
> Album grouping works consistently.  Some of the eye candy (like animations) has
> been temporarily removed, but will hopefully return in the future.
>
> == Changes in general Amarok code ==
> The PlaylistModel has been split into four different classes:
> 1) PlaylistActions
> 2) PlaylistController
> 3) PlaylistModel
> 4) PMGroupingProxy

I actually thought about a Controller class from the beginning, but
thought it would be too coupled to Model to have a point. It does
appear to be highly coupled to Model, but at the same time its nice to
have all that cruft out of the PlaylistModel.

I wanted to separate out the PMGroupingProxy since the Spring but
never got around to it. So thanks. :)

PMGroupingProxy sounds like an allergy drug though. :D
Playlist::GroupingProxy would be a fine name.

> The play, nextTrack, and previousTrack functions are now part of
> PlaylistActions.  Functions that modify the contents of the playlist (insert,
> remove, and move tracks; undo and redo) are now part of PlaylistController.

Playlist::Actions/Playlist::Controller are poorly name. Controller
sounds like it should do everything Actions does as well. If they had
clear names to make their purposes clear I would be OK probably with
the Model/Controller/Actions trinity. As it is, I don't get why
Actions and Controller are separate really.

> If you want to find out about the contents of the playlist, you still use
> PlaylistModel, though you are forbidden from modifying it.  I encourage you to
> take a look at the headers for each of these classes to find out what functions
> are available.
>
> PMGroupingProxy is described in the next section, and should not be used for
> anything other than playlist views.
>
> The signals and slots for the PlaylistModel have changed substantially;
> PlaylistModel now behaves much more like a QAbstractItem model when it is
> modified.  I have also added a position-independent accessing system for
> situations where you care what is in the playlist, but not what order the
> playlist is in (eg, Random Track mode).

> The Model::trackNumberLessThan() function has moved to Meta::Track::lessThan()
>
> == Changes in the playlist subsystem ==
>
> The src/playlist/ directory has been rearranged.
>
> Along with the PlaylistModel changes, the PlaylistGraphicsView has been
> deprecated, though it has not yet removed from the source tree and will
> probably still work with all the other changes I've made.  Its replacement is a
> new PrettyListView, which is a subclass of QListView.

You say that it'd be possible to bring back animation, but I don't see
how. If PrettyListView isn't a QGraphicsView we've lost the ability to
animate and loads of functionality in that regard. Delegates and
animation don't mix well together.

> The reworking of
> PlaylistModel to behave more like QAbstractItemModel should vastly simplify
> writing new playlist views.  Even the base QListView adequately displays the
> content of the playlist and responds to model changes.
>
> The grouping functionality which used to be in PlaylistModel has now be pulled
> out into PMGroupingProxy, and the grouping algorithm has been rewritten so that
> it stays properly synchronized with the playlist contents.  PrettyListView and
> PlaylitGraphicsView both use this proxy model.

But this is good to hear. :D

PMGroupingProxy shouldn't be in the listview folder.

> --------
>
> If you have any feedback before I commit the patch, please feel free to email
> me.  If there aren't any major objections, I'll commit this in a day or two,
> and then we can start bug hunting.

I don't like use of 'using namespace', especially with the simple
naming scheme we have for the Playlist. It was intended for 'using' to
never be used.

Also be sure that you used svn move for all the renamed files so that
file histories are intact. Or that you are using git-svn, which tracks
moves automatically.

This is my 'static' analysis. I'll try out the patch tonight hopefully.

Ian