Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Playlists: move tracks with Alt + Up/Down/PageUp/PageDown/Home/End #13092

Merged
merged 2 commits into from
Apr 15, 2024

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Apr 11, 2024

Finally. Fixes #10826
This was rather easy only due the new Cut, Copy & Paste feature #12020. Thanks @m0dB !!
edit: Redone entirely by using the mechanics of table-internal drag'n'drop.

Also fixes an issue that dropping tracks at the bottom of a playlist wouldn't select the dropped tracks nor give a current index for navigation.

Also includes a small fix for pasteTracks() (same: current index for navi)

Applicable only in (unlocked) playlists.
(sort by # otherwise it's rather unpredictable)

  • continuous selections are moved right away
  • non-continuous selections are first consolidated at first/last row, then moved on next keypress may not be desired
  • non-continuous selections can be consolidated at the top/bottom with Up/PageUp and Down/PageDown respectively
  • wrap-around is suppressed to avoid undesirable overshoot with longpress (keyrepeat)

@github-actions github-actions bot added the ui label Apr 11, 2024
@ronso0 ronso0 force-pushed the track-move-with-alt branch 2 times, most recently from 351d6e5 to 0b0ecbd Compare April 12, 2024 09:56
@ronso0 ronso0 changed the title Playlists: move tracks Alt + Up/Down Playlists: move tracks with Alt + Up/Down/PageUp/PageDown/Home/End Apr 12, 2024
@ronso0
Copy link
Member Author

ronso0 commented Apr 12, 2024

FWIW I think the event->source() == this part of WTrackTableView::dropEvent() can be replaced with cut/paste.
@m0dB wdyt?

@ronso0 ronso0 marked this pull request as draft April 12, 2024 14:29
@ronso0
Copy link
Member Author

ronso0 commented Apr 12, 2024

Works, but draft again because I'll look into redoing moveTrack() -> moveTracks(QModelIndexList indices, int insertPos), moving all tracks at once.

edit I also briefly tried to (ab)use the dnd mechanics, i.e. construct a QDragMoveEvent but that's cumbersome and actually discouraged.

edit2
...but I'll check if a wrapper could work, i.e. a function that can be called by dragDropEvent() and moveTracks()

@nlw0
Copy link

nlw0 commented Apr 13, 2024

That's great, I've tried out and it seems to work. I noticed that 'del' seems to have stopped working for me, though. Could it be related to this patch?

What would be a good way to implement library commands for this now, using emitKeyEvent?

@ronso0
Copy link
Member Author

ronso0 commented Apr 13, 2024

...but I'll check if a wrapper could work, i.e. a function that can be called by dragDropEvent() and moveTracks()

Yup, that works!
Will pick that to get around the (presumably) costly cut/paste ops

@ronso0
Copy link
Member Author

ronso0 commented Apr 14, 2024

...but I'll check if a wrapper could work, i.e. a function that can be called by dragDropEvent() and moveTracks()

Yup, that works! Will pick that to get around the (presumably) costly cut/paste ops

Done. Works like a charm, espcially consolidating non-continuous selections.

@ronso0 ronso0 marked this pull request as ready for review April 14, 2024 22:22
@ronso0
Copy link
Member Author

ronso0 commented Apr 14, 2024

And it avoids running into bug #13100 (or limitation once that is fixed) with tracks whose files are currently missing.

@ronso0
Copy link
Member Author

ronso0 commented Apr 14, 2024

In case someone experiences issues, I can make a debu commit from the trace output I have in my stash.

@ronso0 ronso0 added this to the 2.5-beta milestone Apr 14, 2024
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some finding. A manual test is pending.

src/widget/wtracktableview.cpp Outdated Show resolved Hide resolved
src/widget/wtracktableview.cpp Outdated Show resolved Hide resolved
src/widget/wtracktableview.cpp Outdated Show resolved Hide resolved
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works like a charm. You may rebase it now. Thank you.

@ronso0
Copy link
Member Author

ronso0 commented Apr 15, 2024

Yeij, thanks for testing!

@daschuer
Copy link
Member

All green. Thank you.

@daschuer daschuer merged commit 59a4374 into mixxxdj:main Apr 15, 2024
13 checks passed
@ronso0 ronso0 deleted the track-move-with-alt branch April 15, 2024 21:17
daschuer added a commit to daschuer/mixxx that referenced this pull request Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify moving tracks in long playlists
3 participants