Mixxx

Merge lp://qastaging/~mhaulo/mixxx/crate-and-playlist-lock into lp://qastaging/~mixxxdevelopers/mixxx/trunk

crate-and-playlist-lock
Merge into trunk

Proposed by Mika Haulo on 2011-01-26

Status:

Merged

Merged at revision:

2644

Proposed branch:

lp://qastaging/~mhaulo/mixxx/crate-and-playlist-lock

Merge into:

lp://qastaging/~mixxxdevelopers/mixxx/trunk

Diff against target:

1011 lines (+458/-79) (has conflicts)

20 files modified

mixxx/res/images/templates/ic_template_library_and_preferences.svg (+30/-7)
mixxx/res/mixxx.qrc (+1/-0)
mixxx/res/schema.xml (+9/-0)
mixxx/src/library/cratefeature.cpp (+96/-6)
mixxx/src/library/cratefeature.h (+2/-0)
mixxx/src/library/cratetablemodel.cpp (+41/-20)
mixxx/src/library/dao/cratedao.cpp (+45/-0)
mixxx/src/library/dao/cratedao.h (+2/-0)
mixxx/src/library/dao/playlistdao.cpp (+45/-0)
mixxx/src/library/dao/playlistdao.h (+4/-0)
mixxx/src/library/playlistfeature.cpp (+98/-6)
mixxx/src/library/playlistfeature.h (+4/-2)
mixxx/src/library/playlisttablemodel.cpp (+34/-21)
mixxx/src/library/sidebarmodel.cpp (+6/-2)
mixxx/src/library/trackcollection.cpp (+1/-1)
mixxx/src/library/trackmodel.h (+1/-0)
mixxx/src/library/treeitem.cpp (+12/-0)
mixxx/src/library/treeitem.h (+7/-0)
mixxx/src/library/treeitemmodel.cpp (+2/-2)
mixxx/src/widget/wtracktableview.cpp (+18/-12)

Text conflict in mixxx/src/library/cratefeature.cpp
Text conflict in mixxx/src/library/playlistfeature.cpp
Text conflict in mixxx/src/library/treeitem.h

To merge this branch:

bzr merge lp://qastaging/~mhaulo/mixxx/crate-and-playlist-lock

Wishlist

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
RJ Skerry-Ryan			Approve on 2011-02-20
Phillip Whelan	code review	2011-01-26	Needs Fixing on 2011-01-29
Review via email: mp+47554@code.qastaging.launchpad.net

Description of the change

An implementation for wishlist issue #661466 (https://bugs.launchpad.net/mixxx/+bug/661466).

Revision history for this message

Phillip Whelan (pwhelan) wrote on 2011-01-29:

Line 217 could be problematic. Different optimization levels and/or compilers can switch the order of boolean expressions.

review: Needs Fixing (code review)

lp://qastaging/~mhaulo/mixxx/crate-and-playlist-lock updated on 2011-01-30

2633. By Mika Haulo on 2011-01-30: A fix to prevent compiler switching the order of boolean expressions.

Revision history for this message

Mika Haulo (mhaulo) wrote on 2011-01-30:

Yep, you're right about that. r2633 should fix it.

Revision history for this message

RJ Skerry-Ryan (rryan) wrote on 2011-02-11:

@Phil The && operator isn't associative, so the compiler should never violate the evaluation order. Tons of code out there relies on short-circuiting for evaluation. Is this documented somewhere? I'd be pretty shocked if there was an optimizing compiler out there that would do that.

@Mika This all looks very good. I think we should keep the 'Remove' option in the context menu for the crate and playlist table models though and just make it not enabled. Ideally we could add a lock icon to the QAction so that there is some feedback that the option is locked. Also, as for drag-and-dropping onto the playlist or crate I think it would be more natural to just deny the drag-and-drop so it shows up as an 'X' as they hover over it instead of showing the "+" and then popping up a dialog box. What do you think?

Thanks!
RJ

Revision history for this message

RJ Skerry-Ryan (rryan) wrote on 2011-02-11:

Here are my thoughts on what needs fixing:

- Since you've worked on this (I'm guessing you based it on Mixxx 1.9) a feature to rename crates and playlists was added to trunk. The locking should affect renaming of the crates and playlists too.

- As I mentioned above, to reject drags to locked Playlists/Crates, just implement Crate/PlaylistFeature::dragMoveAcceptChild to return false if the crate/playlist is locked.

- I think the 'Remove' option for Crates and Playlists should be grayed out instead of omitted so that people don't wonder where it went.

- Similarly, the Remove option in the track-table view should be grayed out instead of omitted.

- Crates and playlists that are locked should be grayed out in the 'Add to Crate' and 'Add to Playlist' context menu's in the TrackTableView.

I'm totally up for helping fix these minor things if you're short on time. When it's ready we can merge it to trunk and it'll be part of the Mixxx 1.10 release. Thanks again for your work, Mika. Finally, sorry it took me so long to get a review done for this -- life changes were afoot :).

Cheers,
RJ

review: Needs Fixing

Revision history for this message

Mika Haulo (mhaulo) wrote on 2011-02-13:

On Friday 11 February 2011 05:02:54 RJ Ryan wrote:

> @Mika This all looks very good. I think we should keep the 'Remove' option
> in the context menu for the crate and playlist table models though and
> just make it not enabled. Ideally we could add a lock icon to the QAction
> so that there is some feedback that the option is locked. Also, as for
> drag-and-dropping onto the playlist or crate I think it would be more
> natural to just deny the drag-and-drop so it shows up as an 'X' as they
> hover over it instead of showing the "+" and then popping up a dialog box.
> What do you think?

Sure, that makes sense. I'll make some fixes soon.

> - Since you've worked on this (I'm guessing you based it on Mixxx 1.9) a
> feature to rename crates and playlists was added to trunk. The locking
> should affect renaming of the crates and playlists too.
>

There was some discussion if locking should affect renaming (and deleting) or
not. I decided to leave renaming enabled and wait for comments. But I guess it
makes more sense that locking really prevents all modifications, including
renaming.

--
MH

lp://qastaging/~mhaulo/mixxx/crate-and-playlist-lock updated on 2011-02-15

2634. By Mika Haulo on 2011-02-15: Review fixes

Revision history for this message

Mika Haulo (mhaulo) wrote on 2011-02-15:

On Feb 11, 2011, at 5:27 AM, RJ Ryan wrote:
>
> I'm totally up for helping fix these minor things if you're short on time.

Thanks RJ, I would actually appreciate some help at least on testing. I started in a new job on monday and the next couple of weeks feel a little bit hectic. I pushed some changes you proposed to my branch. Seems to be working on my machine, but if there's anything else to do or fix, go ahead and take control if you want.

--
MH

Revision history for this message

RJ Skerry-Ryan (rryan) wrote on 2011-02-20:

Hey Mika,

I tested everything out and merged your branch into the Mixxx trunk after fixing a couple minor issues.

Thanks so much for your work on this! I've credited you in the credits as "Mika Haulo". Please let me know if you'd like me to change it to something different. This code will see its first release in Mixxx 1.10.0.

RJ Ryan

review: Approve