Allow to remove tracks by pressing Delete key #4330

ninomp · 2021-09-28T00:16:36Z

This PR is trying to fix a highly requested feature: ability to delete/remove tracks by pressing Delete key on the keyboard.

I have pointed this to 2.3, feel free to re-point to main if you think this should not go to stable branch.

This fixes:

Be-ing · 2021-09-28T00:25:14Z

This is a new feature. It should definitely go to the main branch, not the stable branch.

ronso0 · 2021-09-28T00:26:10Z

It's a new feature for 2.4 : )

ronso0 · 2021-09-28T00:26:30Z

: )

ninomp · 2021-09-28T00:27:16Z

OK I will rebase.

uklotzde

This seems to be a straightforward and easy extension. Unfortunately, we cannot add it without a command history and undo support.

Situation: You want to quickly start a new search or simply delete the existing search query. Therefore you press Ctrl+A followed by Del. Unfortunately, you didn't notice that the track table was focused instead of the search edit field. Now your carefully crafted playlist or crate is gone, forever. No chance to restore it if you don't have a database backup.

ronso0 · 2021-09-28T20:12:20Z

I think a confirm dialog could prevent that, incl. a checkbox [ ] Remember decision for this session.

foss- · 2021-09-28T21:07:36Z

Please keep in mind that for macOS laptop keyboards or compact external keyboards there is no del key. ⌘Backspace is what Apple uses in music app as keyboard shortcut to open the delete dialog.

And yes, a confirmation dialog is a requirement for any delete operation.

ronso0 · 2021-10-02T12:13:42Z

@uklotzde
Do you think a warning dialog is sufficient to prevent purging playlists?
Maybe if we don't add the [ ] Remember decision for this session checkbox?
UX would be

select tracks
hit Del
hit Enter (or Esc)

I think requiring Enter is okay because it wouldn't affect the remove workflow much since Del and Enter are close on the keyboard.
And the popup is entirely unexpected if you meant to clear the seach.

Besides: how do you imagine the Undo history to be implemented?

src/widget/wtracktableview.cpp

ronso0 · 2021-10-02T12:36:53Z

Idea to extend this feature:
in Tracks Del would hide tracks (incl. the same warning dialog I mentioned above), though I'd appreciate the "Remember decision.." checkbox then because hiding tracks can be reverted.

ronso0 · 2021-10-03T00:03:00Z

@ninomp Please take a look at 7920898 and 68c8941, that's an easy way to show the shortcut in the menu to make it more discoverable.

ronso0 · 2021-10-03T15:43:47Z

Also, if we want to avoid merge conflicts I could cherry-pick this commit into #4347 and take care of the dialogs and stuff.

ninomp · 2021-10-04T17:33:40Z

@ronso0 Sorry for delay. I'll try to finish this PR soon(ish), so it could be merged and then you can continue with your PR. Does this work for you?

ronso0 · 2021-10-04T19:15:57Z

Sure, I just noticed that now that I have everything sorted out it would be super easy to incorporate your changes into my PR, as well as taking care of the dialogs.
But if you're still motivated and have time to finish it soonish I won't interfere further, except reviewing, of course ; )

…to-remove-tracks

Co-authored-by: ronso0 <[email protected]>

This caused Delete key to always hide tracks.

ninomp · 2021-10-07T01:48:48Z

This is ready for review.

ronso0 · 2021-10-07T11:49:18Z

I'll leave some inline comments, but in general I'd like to know if @uklotzde considers this mergable with the confirmation dialogs instead of the undo history (which is way beyond the scope of this PR, of course).

src/util/defs.h

ronso0 · 2021-10-07T11:53:06Z

src/widget/wtracktableview.cpp

@@ -767,6 +767,30 @@ void WTrackTableView::keyPressEvent(QKeyEvent* event) {
            m_pTrackMenu->loadTrackModelIndices(indices);
            m_pTrackMenu->slotShowDlgTrackInfo();
        }
+    } else if (event->key() == kHideRemoveShortcutKey) {


ronso0 · 2021-10-07T11:57:17Z

src/widget/wtracktableview.cpp

+                if (plibraryTableModel) {
+                    // Hide tracks if this is the main library table
+                    // confirmation dialog
+                    pTrackModel->hideTracks(indices);


I think here we should also have a confirmation dialog, with the per-session checkbox I mentioned.
This would make the unhide function more discoverable.
What do you think?

ronso0 · 2021-10-07T11:58:30Z

src/widget/wtracktableview.cpp

+                    // confirmation dialog
+                    QMessageBox::StandardButton response;
+                    response = QMessageBox::question(this,
+                            tr("Confirm removal of track(s)"),


Suggested change

tr("Confirm removal of track(s)"),

tr("Confirm track removal"),

to avoid singular/plural complexity?

ronso0 · 2021-10-07T12:00:36Z

src/widget/wtracktableview.cpp

+                            tr("Are you sure you want to remove this/these "
+                               "track(s) from this crate/playlist?"));


Suggested change

tr("Are you sure you want to remove this/these "

"track(s) from this crate/playlist?"));

tr("Are you sure you want to remove all selected "

"tracks from this crate/playlist?"));

same here?
Also, is it feasible to set QString crateOrPlaylist above, depending on the trackmodel, and use it as argument for this message?

uklotzde · 2021-10-18T11:10:33Z

src/widget/wtrackmenu.cpp

        m_pRemoveAct = new QAction(tr("Remove"), this);
+        m_pRemoveAct->setShortcut(QKeySequence(


Could we declare a constant for this particular key sequence instead of repeating it literally 4 times?

uklotzde · 2021-10-18T11:18:54Z

src/widget/wtracktableview.cpp

+        QMessageBox::StandardButton response;
+        response = QMessageBox::question(this,
+                tr("Confirm track removal"),
+                tr("Are you sure you want to remove all selected tracks from %1?")


Does this work well for all languages? I suggest to use a separate i18n string for each message.

src/widget/wtracktableview.cpp

ninomp · 2021-10-19T16:20:50Z

This is ready.

ronso0 · 2021-10-19T18:56:09Z

LGTM
@uklotzde ?

ronso0 · 2021-10-19T20:32:32Z

Squash into one commit to keep the git history clean?

uklotzde · 2021-10-19T21:28:09Z

src/widget/wtrackmenu.cpp

@@ -170,6 +170,9 @@ void WTrackMenu::createMenus() {
 }

 void WTrackMenu::createActions() {
+    const QKeySequence hideRemoveKeySequence =


Unnecessary repetition of the type. Please use const auto.

uklotzde · 2021-10-19T21:31:09Z

src/widget/wtracktableview.cpp

+        // Hide tracks if this is the main library table
+        response = QMessageBox::question(this,
+                tr("Confirm track hide"),
+                tr("Are you sure you want to hide selected tracks?"));


You use the term "all selected" for the other messages but here only "selected". Why?

Hmm This is just an oversight. I agree we should be consistent. Which one do you think we should use?

How about "the" to complete the sentence? Because "all" might be confusing and should be reserved for other use cases.

Aha So you are suggesting that this should read "Are you sure you want to hide the selected tracks?" and the other messages "Are you sure you want to remove the selected tracks from ..."?

ninomp · 2021-10-19T22:10:11Z

This is again ready.

uklotzde

Thank you! LGTM

ronso0 · 2021-10-20T23:34:15Z

@ninomp Thanks for adding this shortcut!

Please add the shortcut to https://manual.mixxx.org/2.4/en/chapters/appendix/keyboard_mapping_table.html#keyboard-mapping-table

Later, I'll take care of updating the keyboard PDF/image (there's #4359 and merge conflicts in SVGs are a pita).

ninomp · 2021-10-20T23:54:02Z

@ronso0 OK Will do so.

foss- · 2021-10-21T09:30:22Z

Minor followup re confirmation dialog UI on macOS: https://bugs.launchpad.net/mixxx/+bug/1947992

ninomp · 2021-12-23T00:48:04Z

@foss- Could you test #4577 to check whether it solves the minor issue on macOS you reported?

github-actions bot added the ui label Sep 28, 2021

Be-ing changed the base branch from 2.3 to main September 28, 2021 00:24

Allow to remove tracks by pressing Delete key

490298d

ninomp force-pushed the del-key-to-remove-tracks branch from be02cbb to 490298d Compare September 28, 2021 00:28

Be-ing requested a review from uklotzde September 28, 2021 00:30

uklotzde suggested changes Sep 28, 2021

View reviewed changes

ronso0 reviewed Oct 2, 2021

View reviewed changes

src/widget/wtracktableview.cpp Outdated Show resolved Hide resolved

ronso0 added the library label Oct 3, 2021

ronso0 mentioned this pull request Oct 3, 2021

Library: add keyboard short to open track properties Ctrl+Enter #4347

Merged

ninomp and others added 6 commits October 5, 2021 02:00

Merge commit '867a4b9e4c3d878ce1e6be3642664ec98f87a83a' into del-key-…

f3a7f2a

…to-remove-tracks

Merge commit 'd7b5ad5fcadc49e70b4463b2eb4b155000df0c45' into del-key-…

a8ef4cd

…to-remove-tracks

Extend Delete key to hide tracks in main library table

9059d9c

Co-authored-by: ronso0 <[email protected]>

Fix incorrect cast when checking type of track model

6dd7d98

This caused Delete key to always hide tracks.

Display keyboard shortcut for hiding/removing tracks in WTrackMenu

a0d1940

Ask user for confirmation before removing track from crate/playlist

b947068

ronso0 requested changes Oct 7, 2021

View reviewed changes

uklotzde suggested changes Oct 18, 2021

View reviewed changes

ronso0 requested changes Oct 18, 2021

View reviewed changes

src/widget/wtracktableview.cpp Outdated Show resolved Hide resolved

src/widget/wtracktableview.cpp Outdated Show resolved Hide resolved

ninomp added 4 commits October 19, 2021 03:33

WTrackMenu: Extract hide/remove key sequence to a constant

712e8ad

WTrackTableView: Use separate i18n string for each track removal message

b2aeb08

WTrackTableView: Use curly braces for single-statement ifs

5a80cc8

WTrackTableView: Deduplicate local variable declaration

172decc

ronso0 approved these changes Oct 19, 2021

View reviewed changes

ronso0 mentioned this pull request Oct 19, 2021

Search: save queries across restarts, related tweaks & fixes #4458

Merged

uklotzde suggested changes Oct 19, 2021

View reviewed changes

ninomp added 2 commits October 19, 2021 23:50

WTrackMenu: Use auto instead of QKeySequence

768cb9e

WTrackTableView: Make hide/remove confirmation messages consistent

c511446

uklotzde approved these changes Oct 20, 2021

View reviewed changes

uklotzde merged commit 11fcfcd into mixxxdj:main Oct 20, 2021

ninomp deleted the del-key-to-remove-tracks branch October 20, 2021 23:00

ronso0 added a commit to ronso0/mixxx that referenced this pull request Oct 21, 2021

keyboard sheet: add Del/Cmd+Backspace 'remove/hide track' (mixxxdj#4330)

efd7fec

This was referenced Dec 21, 2021

Tweak remove/hide track confirm message to match confirm exit message #4577

Merged

Update keyboard mapping table mixxxdj/manual#462

Merged

This was referenced Aug 22, 2022

allow delete key in library aka keyboard shortcut to remove track from playlist / crate / auto DJ #7176

Closed

AutoDJ - DEL key doesn't remove track #9793

Closed

no function of the delete key #10537

Closed

mixxxbot mentioned this pull request Aug 24, 2022

dialog consistency of new remove track keyboard shortcut confirmation dialogs #10577

Closed

ronso0 mentioned this pull request Mar 28, 2023

delete key does not remove tracks from AutoDJ queue #8607

Closed

	tr("Confirm removal of track(s)"),
	tr("Confirm track removal"),

		tr("Are you sure you want to remove this/these "
		"track(s) from this crate/playlist?"));

		m_pRemoveAct = new QAction(tr("Remove"), this);
		m_pRemoveAct->setShortcut(QKeySequence(

Allow to remove tracks by pressing Delete key #4330

Allow to remove tracks by pressing Delete key #4330

Conversation

ninomp commented Sep 28, 2021

Be-ing commented Sep 28, 2021

ronso0 commented Sep 28, 2021

ronso0 commented Sep 28, 2021

ninomp commented Sep 28, 2021

uklotzde left a comment • edited Loading

Choose a reason for hiding this comment

ronso0 commented Sep 28, 2021 • edited Loading

foss- commented Sep 28, 2021

ronso0 commented Oct 2, 2021 • edited Loading

ronso0 commented Oct 2, 2021

ronso0 commented Oct 3, 2021 • edited Loading

ronso0 commented Oct 3, 2021

ninomp commented Oct 4, 2021

ronso0 commented Oct 4, 2021

ninomp commented Oct 7, 2021

ronso0 commented Oct 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ninomp commented Oct 19, 2021

ronso0 commented Oct 19, 2021

ronso0 commented Oct 19, 2021

Choose a reason for hiding this comment

uklotzde Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ninomp Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

ninomp commented Oct 19, 2021

uklotzde left a comment

Choose a reason for hiding this comment

ronso0 commented Oct 20, 2021

ninomp commented Oct 20, 2021

foss- commented Oct 21, 2021

ninomp commented Dec 23, 2021

uklotzde left a comment •

edited

Loading

ronso0 commented Sep 28, 2021 •

edited

Loading

ronso0 commented Oct 2, 2021 •

edited

Loading

ronso0 commented Oct 3, 2021 •

edited

Loading

uklotzde Oct 19, 2021 •

edited

Loading

ninomp Oct 19, 2021 •

edited

Loading