Remove from disk menu item #1737

WaylonR · 2018-07-03T03:09:55Z

Method to remove a track from disk, and optionally hides it from the library, moving it to 'hidden' tracks. Will go to 'missing' after a rescan.

daschuer · 2018-07-03T05:49:20Z

Thank you for the PR.
This is a modal message Box stopping the rest of Mixxx, right?
This should be replaced with one that is non-modal. Maybe you could reuse this.

mixxx/src/recording/recordingmanager.cpp

Line 297 in 1c29f2a

    
           ErrorDialogProperties* props = ErrorDialogHandler::instance()->newDialogProperties();

daschuer · 2018-07-03T05:52:33Z

src/widget/wtracktableview.cpp

@@ -448,6 +448,10 @@ void WTrackTableView::createActions() {
    connect(m_pFileBrowserAct, SIGNAL(triggered()),
            this, SLOT(slotOpenInFileBrowser()));

+    m_pFileRemoveFromDiskAct = new QAction(tr("Remove from Disk"),this);


The Remove word is already used. The file does not necessary stored on a disk.
How about "Delete from file system"

Good point.

why not just 'Delete File'?

This is an action the users should do only when it is REALY intended.
So we should make it as clear as possible. I am not sure if "File" is clear for all non technic nerds and after it is translated to various languages.

I'd prefer "Delete file". I'm more concerned about users not understanding what a "file system" or "disk" is than a "file".

I like "Delete File" or "Move To Trash"

daschuer · 2018-07-03T05:57:58Z

A "Also hide track in library" is hard to understand.
...

This gives the chance to optimize the feature for the underlying use cases.
Mine are these:

I have a duplicated track an I like to keep the better one
I like to delete the track because I hate it.
I like to delete the track, because it is broken, but I like to replace it later.

What are you use cases? Can we reflect that in the dialog?

daschuer · 2018-07-03T06:10:15Z

The message Box should show also the path from the file to delete.

WaylonR · 2018-07-03T06:51:41Z

I am unsure how to make "Also hide track" checkbox, easier to understand. Please, feel free to write me a message.

WaylonR · 2018-07-03T06:52:44Z

The 'remove from disk' can remove multiple files at a time.. so, each file should be in a list that gets displayed in the message?

daschuer · 2018-07-03T07:33:34Z

Yes.

daschuer · 2018-07-03T07:34:38Z

First we have to clarify the use cases.
Is my list complete?

WaylonR · 2018-07-03T07:39:10Z

Yes, I think you've got a complete list.

WaylonR · 2018-07-03T08:50:06Z

As for model, it does not stop mixxx. It prevents access to the UI, yes, but mixxx itself does not stop. Nor the UI, or the music. The only thing that the model dialog does, is prevents a mouse/keyboard event inside mixxx. And to be fair, it is asking an important question.

ronso0 · 2018-07-03T12:50:39Z

Thanks for picking this up!
@daschuer I think your list is complete.

The message Box should show also the path from the file to delete.

Please show the complete file name & path so users can be sure to remove the track from the right subfolder (.../music/AlbumXY-copy/...) and catch the file extension as well as individual appendixes like 'supertrack_preview.mp3'

rryan · 2018-07-03T14:34:18Z

src/widget/wtracktableview.cpp

+            }
+            QString  filenameWithPath = trackModel->getTrackLocation(index);
+            QFile file (filenameWithPath);
+            file.remove();


Let's handle failure to remove -- keep a list of filenames that we couldn't remove and let the user know somehow...

So, two lists, a to do list with the dialog, and a dialog that shows any error, with a list of files which did not get deleted.

Could we find out the reason deletion failed?
File doesn't exist? no write access/rights?

The 'Success/Error' window could show either "OK" or a list:

inexistant files

failed deletions (try again?)

I realize it might be a not-so-common use case and hard to accomplish, but in case of deletion fails (partly) and the error window was closed, could the track view select the 'failed' tracks?

rryan · 2018-07-03T14:34:51Z

src/widget/wtracktableview.cpp

+
+        QModelIndexList indices = selectionModel()->selectedRows();
+
+        for (const QModelIndex& index : indices) {


The list could contain duplicate paths -- let's de-dupe first.

rryan · 2018-07-03T14:36:26Z

src/widget/wtracktableview.cpp

+        }
+        if( alsoHide.checkState() == Qt::Checked )
+        {
+            trackModel->hideTracks(indices);


shouldn't this be a purge? if the file is gone we should probably permanently remove the metadata too

That depends on the desired use case.
I will try to propose texts to distinguish it.

Tried to purge using trackModel->purgeTracks but.. it didn't work. not sure why. Even did hideTracks then purgeTracks. They stayed in hidden. So.. kinda stumped.

rryan · 2018-07-03T14:38:16Z

It'd be nice to add some unit tests for this feature, since it's quite dangerous, though WTrackTableView is difficult to test.

daschuer · 2018-07-03T15:12:07Z

Something like this:

[ ] Delete and replace crates and playlist entried with a successor track.
[ ] Purge, emove Track from Playlist crates and History.
[ ] Delete track and mark as missing.
[ ] Delete track and keep metadat as hidden track in history.

Not perfect, but an idea.

WaylonR · 2018-07-03T21:28:14Z

[ ] Delete and replace crates and playlist entried with a successor track.
What would this need to do?

[ ] Delete track and mark as missing.
is it possible for code to set a track as missing, via trackModel?

daschuer · 2018-07-04T06:50:04Z

We have "hide" this is original for hiding podcasts or other non music tracks. The track is moved to hidden tracks. This prevents the track from appearing again after library scan.

Than we have Purge, which removes the track from library as if it was never there.

The problematic thing here is the history.
The history should not be altered because of a changed library.

Looking at the wall of possible option, it can be anoying if a user just want to delete a bad track that was just added.

We can think about reverting the logic.
We may check if the track is member of any playlist and crate. And if not, we can delete the file and Purge the metadata, whithout asking anything else then a simple confirm.

If this is not the case we reject the delete:
"The track is used in .... "

daschuer · 2018-07-04T06:54:35Z

Alternative, for the case crate an playlist entried are found:

[ ] Just delete track file and keep references for a successor track.
[ ] Purge, remove Track from Playlist crates and History.
[ ] Hide, remove Track from Playlist crates and keep it in History.

uklotzde · 2018-12-18T07:29:00Z

src/widget/wtracktableview.cpp

+    if (!trackModel) {
+        return;
+    }
+    QMessageBox msgBox (QMessageBox::Information, QObject::tr("Remove From Disk"), QObject::tr("Really delete the files from disk? (Permanent Deletion)"));


This should be a simple and explicit question, no frills. The text is too complicated for confirming an irreversible action.

How about: "Are you sure you want to permanently delete this file?"/"Are you sure you want to permanently delete these files?" It would be nice if the code could distinguish between having a single file selected and multiple files selected to show the appropriate string. Also, I'd like to see the paths of files before deleting them.

How about "This will move one or more files to the Recycle Bin, are you sure you want to continue?"

I feel that some of the less computer literate think of the Recycle Bin as a keyword almost.

It is a permanent deletion, from the disk. It doesn't go to the recycle bin, it's simply unlinked? @uklotzde , do you have a suggestion as to wording? This is a pretty scary action.

uklotzde · 2018-12-18T07:35:16Z

src/widget/wtracktableview.cpp

@@ -1061,6 +1099,7 @@ void WTrackTableView::contextMenuEvent(QContextMenuEvent* event) {
        m_pMenu->addAction(m_pPurgeAct);
    }
    m_pMenu->addAction(m_pFileBrowserAct);
+    m_pMenu->addAction(m_pFileRemoveFromDiskAct);


I suppose this action is not used very often and at least not while performing. It doesn't deserve enlarging and cluttering the top level layer of the context menu.

What about a Hide / Delete submenu?
Could this hold items that distinguish between Hide, Delete but keep in history, Delete & Purge etc. or is it better to decide that per file in the dialog window?

I'm not too worried about cluttering context menu... How often do you really right-click while performing? I do most of my file selection with a hardware controller and never touch a mouse anyways. I would not want another dialog box for each file... If I selected a range, then hit purge, the software purge everything I selected...

Be-ing · 2018-12-26T21:16:38Z

src/widget/wtracktableview.cpp

+    if (QMessageBox::Yes == msgBox.exec())
+    {
+
+


Style nitpicks:

Delete extra blank lines.

Put brackets on same line as if/else statements:

if (condition == true) { foo(); } else { bar(); }

Be-ing · 2018-12-26T21:17:39Z

There is now a merge conflict with master.

ronso0 · 2019-01-22T12:32:20Z

This works well, thank you!

Any special cases that deserve further testing?

ronso0 · 2019-02-14T14:31:36Z

Is it really to late to merge this into 2.3?
It's improving the preparation workflow A LOT!

uklotzde · 2019-02-14T14:58:52Z

The requested changes have not been resolved, yet. This action performs a destructive task that must be save to use.

uklotzde · 2019-02-14T15:01:03Z

It is still unclear if the corresponding track of a deleted file should be purged from the library.

ronso0 · 2019-05-05T11:03:17Z

What defines a recording? how easy is it to identify one?

I'm not necessarily referring to Mixxx recordings. There might also be external files from a mobile recorder or another app, also files might be quite small like aborted Mixxx recordings, or field recording snippets.
When removing anyAudioFile.mp3/wav/... just look if there's an associated anyAudioFile.cue in the same folder and propose to delete it as well.

Conflicts resolved: src/widget/wtracktableview.cpp

ronso0 · 2019-11-19T16:32:52Z

merge conflicts are resolved here https://github.com/WaylonR/mixxx/pull/2

ronso0 · 2020-04-25T17:19:48Z

@WaylonR would you mind merging https://github.com/WaylonR/mixxx/pull/2 ?
After that, do you think you can also take care of the merge conflicts with master?
Recently the track menu was separated in #2612 and is now also available as right-click menu on track property labels in decks.

nwautier · 2020-04-29T04:26:18Z

Sorry if this isn't the way you like to hear it, but BUMP. This is a killer feature I'd love to have yesterday. I'm not really comfortable diving into the code on a project of this scale, but if you want to bounce UI ideas or test cases off of me, feel free.

Be-ing · 2020-04-29T04:36:13Z

It's not so intimidating to get into such a big project once you have an IDE set up to navigate the code easily. I felt intimidated for a while until I got KDevelop setup. Now that we are using CMake on the master branch, most C++ IDEs are easy to set up.

Remove from disk: merge master, fix conflicts

WaylonR · 2020-04-29T08:04:05Z

Looking to resolve this, I can't find the Hide action anymore? Only the Purge action.

ronso0 · 2020-04-30T22:11:51Z

Converted to Draft as there are a few open UI issues before we can implement it.

ronso0 · 2020-04-30T22:19:58Z

I'll pick this up soonish because I miss it in my personal builds. I'll only try to restore the current state, not sure if I find the time to add the dialogs we talked about.

ronso0 · 2020-09-18T15:49:05Z

I've ported this to 2.3 https://github.com/ronso0/mixxx/tree/remove-from-disk-2.3 to have it available in my 2.3 builds I use live. Shouldn't be too hard to apply it to master.
Respective PR to my 2.3 branch is ronso0#11
I added a simple file list to the message box:

Holzhaus · 2020-10-18T13:26:09Z

There are merge conflicts now.

ronso0 · 2020-10-23T18:00:32Z

@WaylonR
If you're still around, please spin back before merging b979bbe which is unrelated to this branch.

ronso0 · 2020-10-23T18:02:43Z

@WaylonR
If you're still around, please spin back before merging b979bbe which is unrelated to this branch.

Sorry fo rthe noise, I was confused by commit hover preview that links to #2 instead of my repo...

ronso0 · 2020-10-23T18:03:03Z

I'll take care of the conflicts. (seems I screwed it up earlier)

ronso0 · 2020-10-23T18:27:27Z

Looking closer I think it's way easier to start over. I recommend to close this.
The trackmenu was moved out the library to wtrackmenu, so let's add the Remove action there.

WaylonR · 2020-10-24T01:06:51Z

please @notify me when this is picked up in another PR. might get around to doing it again, myself. I've been using a artifact from an old main branch compile, and been missing the feature.

ronso0 · 2020-10-24T02:30:18Z

I just spent my time after dinner to rebuild this onto mas.. main.
wasn't too hard, works well in Tracks & Crates.
dirty branch is https://github.com/ronso0/mixxx/tree/remove-from-disk

I'll open a draft PR during the weekend and we can discuss where else it is needed, and how to tweak the user dialog.

ronso0 · 2020-10-24T02:34:00Z

I just spent my time after dinner to rebuild this onto mas.. main.

...because I missed it a lot since having rebuil my Live branch onto main.
Essential feature for my 'clean up the audio inbox' workflow!

ronso0 · 2020-10-24T23:48:41Z

@WaylonR Here you go #3212

Remove from disk menu item and action/slot added.

55a4d55

daschuer reviewed Jul 3, 2018

View reviewed changes

rryan reviewed Jul 3, 2018

View reviewed changes

Be-ing added this to the 2.3.0 milestone Jul 6, 2018

uklotzde suggested changes Dec 18, 2018

View reviewed changes

Be-ing reviewed Dec 26, 2018

View reviewed changes

Be-ing modified the milestones: 2.3.0, 2.4.0 Feb 7, 2019

Merge remote-tracking branch 'mixxx/master' into remove-from-disk

c50f5dd

Conflicts resolved: src/widget/wtracktableview.cpp

ronso0 added 2 commits November 28, 2019 16:51

check 'Also hide' by default, remove blank lines

a27e504

Merge remote-tracking branch 'mixxx/master' into remove-from-disk

7646257

Merge pull request #2 from ronso0/remove-from-disk

b979bbe

Remove from disk: merge master, fix conflicts

ronso0 marked this pull request as draft April 30, 2020 22:10

ronso0 mentioned this pull request Sep 18, 2020

WTrackMenu: add Delete File action ronso0/mixxx#11

Closed

2 tasks

Be-ing changed the base branch from master to main October 23, 2020 23:32

Be-ing closed this Oct 23, 2020

ronso0 mentioned this pull request Oct 24, 2020

WTrackMenu: add Remove From Disk action #3212

Merged

5 tasks


		QModelIndexList indices = selectionModel()->selectedRows();

		for (const QModelIndex& index : indices) {

Remove from disk menu item #1737

Remove from disk menu item #1737

Conversation

WaylonR commented Jul 3, 2018

daschuer commented Jul 3, 2018

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

daschuer commented Jul 3, 2018

daschuer commented Jul 3, 2018

WaylonR commented Jul 3, 2018

WaylonR commented Jul 3, 2018

daschuer commented Jul 3, 2018

daschuer commented Jul 3, 2018

WaylonR commented Jul 3, 2018

WaylonR commented Jul 3, 2018

ronso0 commented Jul 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rryan Jul 3, 2018 • edited Loading

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

rryan commented Jul 3, 2018 • edited Loading

daschuer commented Jul 3, 2018

WaylonR commented Jul 3, 2018 • edited Loading

daschuer commented Jul 4, 2018

daschuer commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nwautier Apr 29, 2020 • edited Loading

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

nwautier Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

Be-ing Dec 26, 2018 • edited Loading

Choose a reason for hiding this comment

Be-ing commented Dec 26, 2018

ronso0 commented Jan 22, 2019

ronso0 commented Feb 14, 2019

uklotzde commented Feb 14, 2019

uklotzde commented Feb 14, 2019

ronso0 commented May 5, 2019

ronso0 commented Nov 19, 2019

ronso0 commented Apr 25, 2020

nwautier commented Apr 29, 2020

Be-ing commented Apr 29, 2020

WaylonR commented Apr 29, 2020

ronso0 commented Apr 30, 2020

ronso0 commented Apr 30, 2020

ronso0 commented Sep 18, 2020

Holzhaus commented Oct 18, 2020

ronso0 commented Oct 23, 2020

ronso0 commented Oct 23, 2020 • edited Loading

ronso0 commented Oct 23, 2020 • edited Loading

ronso0 commented Oct 23, 2020

WaylonR commented Oct 24, 2020

ronso0 commented Oct 24, 2020

ronso0 commented Oct 24, 2020

ronso0 commented Oct 24, 2020

rryan Jul 3, 2018 •

edited

Loading

rryan commented Jul 3, 2018 •

edited

Loading

WaylonR commented Jul 3, 2018 •

edited

Loading

nwautier Apr 29, 2020 •

edited

Loading

nwautier Apr 29, 2020 •

edited

Loading

Be-ing Dec 26, 2018 •

edited

Loading

ronso0 commented Oct 23, 2020 •

edited

Loading

ronso0 commented Oct 23, 2020 •

edited

Loading