Bug 320129 - JJ: Tracks cannot be dragged around when playlist was shuffled
Summary: JJ: Tracks cannot be dragged around when playlist was shuffled
Status: RESOLVED FIXED
Alias: None
Product: amarok
Classification: Applications
Component: Playlist (show other bugs)
Version: 2.7-git
Platform: Other Linux
: NOR minor
Target Milestone: 2.8
Assignee: Amarok Developers
URL:
Keywords: junior-jobs
Depends on:
Blocks:
 
Reported: 2013-05-22 10:36 UTC by Matěj Laitl
Modified: 2013-05-31 12:23 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In: 2.8


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Matěj Laitl 2013-05-22 10:36:15 UTC
When you click Shuffle "sorting" in playlist sort widget, the tracks get shuffled, but you cannot move one track around in the playlist.

This is because the shuffle *action* is implementing as a *state*, which is IMO suboptimal. I suggest that we make the Shuffle an action instead. It would actually change track order of the bottom model of the playlist, not just *sorting* of the QSortFilterProxyModel.

Speaking about the ui, I think we can retain the Shuffle button in the playlist sorting panel, but it would just appear for a short interval upon clicking (0.5 a second? whatever looks good) and then disappear.

The action needs to be undoable using the playlist undo button.

Marking this as a junior job. Konrad, you've previously touched this code, perhaps you'd like to try this one, too?
Comment 1 Konrad Zemek 2013-05-22 11:41:19 UTC
Sure, I'll take a look at it once I have some free time (today maybe, friday at the latest).

Additionaly, a way to hint that the "Shuffle" entry works in a different way would be to place a separator between this entry and entries above... but that's just a minor detail. I'm a bit torn about the button appearing, though. I think it may very well not show at all, as the effects of shuffing should be immediately visible anyway.
Comment 2 Matěj Laitl 2013-05-22 11:48:17 UTC
(In reply to comment #1)
> Sure, I'll take a look at it once I have some free time (today maybe, friday
> at the latest).

No need to hurry. :)

> Additionaly, a way to hint that the "Shuffle" entry works in a different way
> would be to place a separator between this entry and entries above... but
> that's just a minor detail.

Good idea.

> I'm a bit torn about the button appearing,
> though. I think it may very well not show at all, as the effects of shuffing
> should be immediately visible anyway.

You're right. Just try it locally and then submit what works better for your eye.
Comment 3 Konrad Zemek 2013-05-27 00:20:50 UTC
https://git.reviewboard.kde.org/r/110658/
Comment 4 Matěj Laitl 2013-05-31 12:23:59 UTC
Git commit 5659f0f8ad73a2894ebe8d5da3f7925e75ce6901 by Matěj Laitl, on behalf of Konrad Zemek.
Committed on 31/05/2013 at 14:21.
Pushed by laitl into branch 'master'.

Playlist sort widget: reimplement Shuffle "sort" as an action.

Remove now-unnecessary Shuffle handlers in sort-related functions.
Additionally, PlaylistController::moveRows() has been modified to help
with big move actions (validation complexity reduced).

Playlist::BreadcrumbItemMenu introduced to deduplicate logic between
Playlist::BreadcrumbItem and Playlist::BreadcrumbAddMenuButton.

GUI: In the playlist sort widget, the Shuffle menu entry is now
separated from other entries. Activating the entry no longer results in
a "Shuffle" sort level being added.
FIXED-IN: 2.8
REVIEW: 110658
CCMAIL: amarok-promo@kde.org

M  +1    -0    ChangeLog
M  +21   -0    src/playlist/PlaylistActions.cpp
M  +5    -0    src/playlist/PlaylistActions.h
M  +60   -64   src/playlist/PlaylistBreadcrumbItem.cpp
M  +56   -26   src/playlist/PlaylistBreadcrumbItem.h
M  +32   -47   src/playlist/PlaylistBreadcrumbItemSortButton.cpp
M  +1    -2    src/playlist/PlaylistBreadcrumbItemSortButton.h
M  +2    -27   src/playlist/PlaylistBreadcrumbLevel.cpp
M  +0    -1    src/playlist/PlaylistBreadcrumbLevel.h
M  +9    -7    src/playlist/PlaylistController.cpp
M  +12   -2    src/playlist/PlaylistController.h
M  +0    -4    src/playlist/PlaylistDefines.h
M  +8    -4    src/playlist/PlaylistModel.cpp
M  +20   -15   src/playlist/PlaylistSortWidget.cpp
M  +6    -1    src/playlist/PlaylistSortWidget.h
M  +4    -69   src/playlist/proxymodels/SortAlgorithms.cpp
M  +0    -22   src/playlist/proxymodels/SortAlgorithms.h
M  +0    -2    src/playlist/proxymodels/SortScheme.cpp
M  +24   -24   tests/playlist/TestPlaylistModels.cpp
M  +3    -1    tests/playlist/TestPlaylistModels.h

http://commits.kde.org/amarok/5659f0f8ad73a2894ebe8d5da3f7925e75ce6901