Skip to content

Remove from disk menu item #1737

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.

Sign up for GitHub

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

Closed
WaylonR wants to merge 5 commits into from
Closed

Remove from disk menu item #1737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
55a4d55
Remove from disk menu item and action/slot added.
WaylonR Jul 3, 2018
c50f5dd
Merge remote-tracking branch 'mixxx/master' into remove-from-disk
ronso0 Nov 18, 2019
a27e504
check 'Also hide' by default, remove blank lines
ronso0 Nov 28, 2019
7646257
Merge remote-tracking branch 'mixxx/master' into remove-from-disk
ronso0 Dec 24, 2019
b979bbe
Merge pull request #2 from ronso0/remove-from-disk
WaylonR Apr 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 36 additions & 1 deletion src/widget/wtracktableview.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,11 @@ void WTrackTableView::createActions() {
connect(m_pFileBrowserAct, SIGNAL(triggered()),
this, SLOT(slotOpenInFileBrowser()));

m_pAutoDJBottomAct = new QAction(tr("Add to Auto DJ Queue (bottom)"), this);
m_pFileRemoveFromDiskAct = new QAction(tr("Remove from Disk"),this);
Copy link
Copy Markdown
Member

@daschuer daschuer Jul 3, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@WaylonR WaylonR Dec 18, 2018

Choose a reason for hiding this comment

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

Good point.

why not just 'Delete File'?

Copy link
Copy Markdown
Member

@daschuer daschuer Dec 18, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@Be-ing Be-ing Dec 26, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

@nwautier nwautier Apr 29, 2020

Choose a reason for hiding this comment

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

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

connect(m_pFileRemoveFromDiskAct,SIGNAL(triggered()),
this, SLOT(slotRemoveFromDisk()));

m_pAutoDJBottomAct = new QAction(tr("Add to Auto DJ Queue (Bottom)"), this);
connect(m_pAutoDJBottomAct, SIGNAL(triggered()),
this, SLOT(slotAddToAutoDJBottom()));

Expand Down Expand Up @@ -684,6 +688,36 @@ void WTrackTableView::slotOpenInFileBrowser() {
mixxx::DesktopHelper::openInFileBrowser(locations);
}

void WTrackTableView::slotRemoveFromDisk() {
TrackModel* trackModel = getTrackModel();
if (!trackModel) {
return;
}
QMessageBox msgBox (QMessageBox::Information, QObject::tr("Remove From Disk"), QObject::tr("Really delete the files from disk? (Permanent Deletion)"));
Copy link
Copy Markdown
Contributor

@uklotzde uklotzde Dec 18, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@Be-ing Be-ing Dec 26, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

@nwautier nwautier Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@WaylonR WaylonR Apr 29, 2020

Choose a reason for hiding this comment

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

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.

QAbstractButton* pYES = (QAbstractButton*)msgBox.addButton(QMessageBox::Yes);
QAbstractButton* pNO = (QAbstractButton*)msgBox.addButton(QMessageBox::No);
QCheckBox alsoHide(QObject::tr("Also hide track in library"), &msgBox);
alsoHide.blockSignals(true);
// Note ronso0 Set checked by default because that is what I usually need
alsoHide.setCheckState(Qt::Checked);
msgBox.addButton(&alsoHide, QMessageBox::ActionRole);

if (QMessageBox::Yes == msgBox.exec()) {
QModelIndexList indices = selectionModel()->selectedRows();

for (const QModelIndex& index : indices) {
Copy link
Copy Markdown
Member

@rryan rryan Jul 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

if (!index.isValid()) {
continue;
}
QString filenameWithPath = trackModel->getTrackLocation(index);
QFile file (filenameWithPath);
file.remove();
Copy link
Copy Markdown
Member

@rryan rryan Jul 3, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@WaylonR WaylonR Jul 3, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@ronso0 ronso0 Sep 24, 2019

Choose a reason for hiding this comment

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

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?

} if( alsoHide.checkState() == Qt::Checked ) {
trackModel->hideTracks(indices);
Copy link
Copy Markdown
Member

@rryan rryan Jul 3, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@daschuer daschuer Jul 3, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@WaylonR WaylonR Jul 3, 2018

Choose a reason for hiding this comment

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

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.

}
}
}

void WTrackTableView::slotHide() {
QModelIndexList indices = selectionModel()->selectedRows();
if (indices.size() > 0) {
Expand Down Expand Up @@ -1112,6 +1146,7 @@ void WTrackTableView::contextMenuEvent(QContextMenuEvent* event) {
m_pMenu->addAction(m_pPurgeAct);
}
m_pMenu->addAction(m_pFileBrowserAct);
m_pMenu->addAction(m_pFileRemoveFromDiskAct);
Copy link
Copy Markdown
Contributor

@uklotzde uklotzde Dec 18, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@ronso0 ronso0 Feb 23, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown

@nwautier nwautier Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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...


if (modelHasCapabilities(TrackModel::TRACKMODELCAPS_EDITMETADATA)) {
m_pMenu->addSeparator();
Expand Down
3 changes: 3 additions & 0 deletions src/widget/wtracktableview.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ class WTrackTableView : public WLibraryTableView {
void slotRemove();
void slotHide();
void slotOpenInFileBrowser();
void slotRemoveFromDisk();
void slotShowTrackInfo();
void slotShowDlgTagFetcher();
void slotNextTrackInfo();
Expand Down Expand Up @@ -187,6 +188,8 @@ class WTrackTableView : public WLibraryTableView {
QAction *m_pPropertiesAct;
QAction *m_pFileBrowserAct;

// Remove from disk action
QAction *m_pFileRemoveFromDiskAct;
// BPM feature
QAction *m_pBpmLockAction;
QAction *m_pBpmUnlockAction;
Expand Down