Mixxx

Merge lp://qastaging/~mixxxdevelopers/mixxx/iTunes2 into lp://qastaging/~mixxxdevelopers/mixxx/trunk

  1. iTunes2
  2. Merge into trunk
Proposed by RAFFI TEA
Status: Merged
Merged at revision: 2615
Proposed branch: lp://qastaging/~mixxxdevelopers/mixxx/iTunes2
Merge into: lp://qastaging/~mixxxdevelopers/mixxx/trunk
Diff against target: 1687 lines (+776/-696)
9 files modified
mixxx/res/schema.xml (+27/-0)
mixxx/src/library/itunesfeature.cpp (+398/-33)
mixxx/src/library/itunesfeature.h (+18/-6)
mixxx/src/library/itunesplaylistmodel.cpp (+162/-233)
mixxx/src/library/itunesplaylistmodel.h (+29/-53)
mixxx/src/library/itunestrackmodel.cpp (+104/-302)
mixxx/src/library/itunestrackmodel.h (+36/-67)
mixxx/src/library/library.cpp (+1/-1)
mixxx/src/library/trackcollection.cpp (+1/-1)
To merge this branch: bzr merge lp://qastaging/~mixxxdevelopers/mixxx/iTunes2
Reviewer Review Type Date Requested Status
RJ Skerry-Ryan Approve
Review via email: mp+41225@code.qastaging.launchpad.net

Description of the change

NOTE: SINCE TRUNK HAS CHANGED, I HAVE CREATED THIS BRANCH WHICH RESOLVES MERGING CONFLICTS.

This is are rewrite of the itunes feature.
It is now sql-based. A Sax parser is used to put all tracks
into database tables.

The branch has been tested with itunes music collections from IRC user theresajayne and me.
A collection of over 70000 files will now consume no main memory anymore. Before it took ~2GB.

PLEASE REVIEW....

To post a comment you must log in.
Revision history for this message
RJ Skerry-Ryan (rryan) wrote :

Since features_library has merged to trunk revision 7 is live and should not be modified. In my local copy I added a revision 8 that only creates the itunes table.

Also, the indentation in a lot of these files seems to be 2-space indent, which doesn't match the rest of the files' 4-space indent.

After loading an iTunes XML sent to me by Tobias a long time ago, Mixxx hard-locked up after I clicked iTunes and said 'Ok' to the question of whether or not to load my library. Not sure what the problem is, looking at it now.

Revision history for this message
RJ Skerry-Ryan (rryan) wrote :

Turns out the while(1) loops were infinite looping. I changed them to while (!xml.atEnd()) and that seemed to fix it. For what it's worth, my XML file causes a "Premature end of document." error. There isn't any UI feedback when that happens, which would be nice.

I'll push my edits to this branch. (that updates the merge request automatically)

lp://qastaging/~mixxxdevelopers/mixxx/iTunes2 updated
2563. By RJ Skerry-Ryan

Adding updates. Split itunes SQL revision into revision 8 from 7 since 7 is now permanent. Add safety from infinite loops in parsing. Fix indentation to be 4-space indents.

Revision history for this message
RJ Skerry-Ryan (rryan) wrote :

OK I fixed what I mentioned and added a GUI warning when the file fails to parse. It looks good to me.

review: Approve
lp://qastaging/~mixxxdevelopers/mixxx/iTunes2 updated
2564. By RJ Skerry-Ryan

Adding a warning message when the iTunes library fails to load. Make failed parses of iTunes still select in the track model because it might have gotten partially through the file and showing some is better than showing none.

2565. By RJ Skerry-Ryan

Switch iTunes trackmodel and playlits model to use custom header states instead of the mixxx playlist one.

2566. By RJ Skerry-Ryan

Remove Traktor references.

Revision history for this message
RAFFI TEA (raffitea) wrote :

Tanks RJ for your review.

lp://qastaging/~mixxxdevelopers/mixxx/iTunes2 updated
2567. By RJ Skerry-Ryan

Oops, fix multi-line string.

Revision history for this message
RJ Skerry-Ryan (rryan) wrote :

I'll let you do the honors -- go ahead and merge it into trunk :)

Revision history for this message
RAFFI TEA (raffitea) wrote :

Before I merge I have another question:

Both itunestrackmodel and itunesplaylistmodel are subclassed from BaseSqlTableModel. In BaseSqlTableModel we handle 'trackChanged' signals. Now assume we've an iTunes track that has incorrect playtime information. If BaseSqlTableModel captures 'trackChanged' signals for iTunes playlistsmodels I'm sure we run into problems? Will this ever happen?

Revision history for this message
RJ Skerry-Ryan (rryan) wrote :

Hm, this is a good point. They shouldn't be inherited from BaseSQLTableModel
because BaseSqlTableModel assumes the track is in the Mixxx library. Maybe
we can split the Mixxx db specific stuff into a MixxxSqlTableModel that
inherits from BaseSqlTableModel ? Ugh, that seems like an ugly solution.
What do you think, Tobias?

RJ

On Sun, Nov 21, 2010 at 3:23 PM, RAFFI TEA <email address hidden>wrote:

> Before I merge I have another question:
>
> Both itunestrackmodel and itunesplaylistmodel are subclassed from
> BaseSqlTableModel. In BaseSqlTableModel we handle 'trackChanged' signals.
> Now assume we've an iTunes track that has incorrect playtime information. If
> BaseSqlTableModel captures 'trackChanged' signals for iTunes playlistsmodels
> I'm sure we run into problems? Will this ever happen?
> --
> https://code.launchpad.net/~mixxxdevelopers/mixxx/iTunes2/+merge/41225
> You are reviewing the proposed merge of lp:~mixxxdevelopers/mixxx/iTunes2
> into lp:mixxx.
>

Revision history for this message
RAFFI TEA (raffitea) wrote :

I think there are two options. (1) We follow RJ's proposal. However, we get a deeper class hierarchy, which is really not my favor. (2) We refactor BaseSqlTableModel. In particular, we move the connect statement in a separate method (e.g., setTrackChangeAwareness(bool) ). If the parameter is true, we connect the signal. This is probably the quickest way to realize.

Both suggestions are not the 'cleanest'. But I am more towards solution (2) because there's no real difference between native and non-native library features unless the latter are read-only models and trackChanged signals are unintended. This brings me to another idea: (3) We could disconnect the signal in BaseSqlTableModel::readOnlyFlags(QModelIndex ind). Basically, all non-native Mixxx library features return read-only model flags, e.g., iTunes models return BaseSqlTableModel::readOnlyFlags() in their flags() method.

What do you think RJ or Albert?

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