Code review comment for lp:~mixxxdevelopers/mixxx/traktor_library

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

Thanks for your review Albert.

>
> - Fix indentation in CrateFeature

Done! Don't know why I always have indentation problems. My editor is set up correctly. Whenever I press <tab> it should create 4 spaces.

> - in CrateFeature::slotRenameCrate, we're probably resetting the selection
> model because we call m_crateListTableModel.select(). Can we instead tell the
> model that that particular row has changed so it tells the view to update it?
> Otherwise, I think when you rename a crate, you will lose your position in the
> sidebar. (This could be really annoying at some point if you're trying to
> rename subcrates.)

I've addressed this issue with some magic code as you noticed in your comment below. There's no reset in the sidebar model (the tree won't collapse) but you'll loose focus. I'll try to improve that behavior.

> - I see you did some magic with CrateFeature::constructChildModel() to deal
> with this problem in the case of inserting or deleting playlists though, which
> is awesome! :)
>
> - It's also awesome that you can import playlists into both crates and
> playlists.

The code was already there, I added only some improvements.

> - I have not tested the Traktor feature, but based on the code:
> - In TraktorFeature::getTraktorMusicDatabase(), should we look for registry
> entries for older versions of Traktor if we don't find the Traktor Pro entry?
> Do you know if the XML schema changed from Traktor 3 to Traktor Pro?
> - There's some more tabs in traktorfeature.h for indents instead of spaces!

Done.

> - Change the #ifndef at the top of treeitemmodel.h? (you'll see what I
> mean)

Done.

I've only tested with the latest 1.2.x series. I don't know where to get an older Traktor 3. But I've never seen Traktor 3 in German clubs. All DJs I know use either Serato or a one of the current Traktor 1.2.x releases.

The registry key to look up the library path should be the same for all Traktor 1.2.x products, i.e., Traktor LE, Traktor (Scratch) Duo, Traktor (Scratch) Pro.

> Great work overall though Tobias! There are some cool, polished features in
> here and the code is high quality.

« Back to merge proposal