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.

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

Jump to bottom

Remove from disk: add to deck track menu, fix undeleted message #4560

Merged
merged 6 commits into from
Feb 21, 2022
Merged

Remove from disk: add to deck track menu, fix undeleted message #4560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b32addd
Remove from disk: enable for deck menus
ronso0 Dec 8, 2021
fda4ed1
Remove track from disk: eject from deck after deleting
ronso0 Dec 8, 2021
b9d5a4c
stop deck, eject track after confirmation, before deleting the file
ronso0 Dec 9, 2021
e89aedf
Remove from disk: use QStringLiteral, remove comments
ronso0 Feb 4, 2022
23b9581
Remove from disk: reword 'not deleted' messages
ronso0 Feb 4, 2022
3c6e33d
Remove from disk: use static ControlObject method to stop & eject track
ronso0 Feb 4, 2022
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
97 changes: 69 additions & 28 deletions src/widget/wtrackmenu.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,8 +535,7 @@ void WTrackMenu::setupActions() {
}
}

if (featureIsEnabled(Feature::RemoveFromDisk) &&
m_pTrackModel->hasCapabilities(TrackModel::Capability::RemoveFromDisk)) {
if (featureIsEnabled(Feature::RemoveFromDisk)) {
m_pRemoveFromDiskMenu->addAction(m_pRemoveFromDiskAct);
addMenu(m_pRemoveFromDiskMenu);
}
Expand Down Expand Up @@ -790,9 +789,11 @@ void WTrackMenu::updateMenus() {
}

if (featureIsEnabled(Feature::RemoveFromDisk)) {
bool locked = m_pTrackModel->hasCapabilities(TrackModel::Capability::Locked);
if (m_pTrackModel->hasCapabilities(TrackModel::Capability::RemoveFromDisk)) {
m_pRemoveFromDiskAct->setEnabled(!locked);
if (m_pTrackModel) {
uklotzde marked this conversation as resolved.
Show resolved Hide resolved
bool locked = m_pTrackModel->hasCapabilities(TrackModel::Capability::Locked);
if (m_pTrackModel->hasCapabilities(TrackModel::Capability::RemoveFromDisk)) {
m_pRemoveFromDiskAct->setEnabled(!locked);
}
}
}

Expand Down Expand Up @@ -1704,14 +1705,21 @@ class RemoveTrackFilesFromDiskTrackPointerOperation : public mixxx::TrackPointer
} // anonymous namespace

void WTrackMenu::slotRemoveFromDisk() {
const auto trackRefs = getTrackRefs();
QStringList locations;
locations.reserve(trackRefs.size());
for (const auto& trackRef : trackRefs) {
QString location = trackRef.getLocation();
if (m_pTrackModel) {
const auto trackRefs = getTrackRefs();
locations.reserve(trackRefs.size());
for (const auto& trackRef : trackRefs) {
QString location = trackRef.getLocation();
locations.append(location);
}
locations.removeDuplicates();
} else if (m_pTrack) {
QString location = m_pTrack->getLocation();
locations.append(location);
} else {
return;
}
locations.removeDuplicates();

{
// Prepare the delete confirmation dialog
Expand All @@ -1724,15 +1732,24 @@ void WTrackMenu::slotRemoveFromDisk() {
delListWidget->setFocusPolicy(Qt::ClickFocus);
delListWidget->addItems(locations);
mixxx::widgethelper::growListWidget(*delListWidget, *this);
// Warning text

QString delWarningText;
if (m_pTrackModel) {
delWarningText = tr("Permanently delete these files from disk?") +
QStringLiteral("<br><br><b>") +
tr("This can not be undone!") + QStringLiteral("</b>");
} else {
delWarningText =
Copy link
Member

Choose a reason for hiding this comment

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

On Linux you can delete the file while playing.
It is actually removed after the last reference to the file has been closed.
On Windows the file is locked until all file handles are closed.
I am not sure how it relates to caching. The file is kept in cache after rejecting.
We need to check if the lock is released than.

tr("Stop the deck and permanently delete this track file from disk?") +
QStringLiteral("<br><br><b>") +
tr("This can not be undone!") + QStringLiteral("</b>");
}
QLabel* delWarning = new QLabel();
delWarning->setText(tr("Permanently delete these files from disk?") +
QString("<br><br><b>") +
tr("This can not be undone!") + QString("</b>"));
delWarning->setText(delWarningText);
delWarning->setTextFormat(Qt::RichText);
delWarning->setSizePolicy(QSizePolicy(QSizePolicy::Minimum,
QSizePolicy::Minimum));
// Buttons

QDialogButtonBox* delButtons = new QDialogButtonBox();
QPushButton* cancelBtn = delButtons->addButton(
tr("Cancel"),
Expand Down Expand Up @@ -1762,6 +1779,13 @@ void WTrackMenu::slotRemoveFromDisk() {
}
}

// If the operation was initiated from a deck's track menu
Copy link
Contributor

Choose a reason for hiding this comment

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

This code drags dependencies into WTrackMenu that don't belong here. Another example of the spaghetti control flow in Mixxx, unmaintainable.

Is this side-effect even synchronous? Can we guarantee that the track is ejected immediately? Probably not, because decoding is done concurrently in a separate thread -> CachingReaderWorker.

I suppose this doesn't work as intended.

Copy link
Member Author

@ronso0 ronso0 Dec 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is only supposed to have a clean UX: it would be confusing if the deleted track was still loaded and could still be played.

Please clarify your technical concerns (for example the libmad issue) and give an estimation if this can be solved at all (by me, with guidance, of course). Because, if there are issues to be expected they apply to the delete and purge action in general, not just to the deck option, since we're not checking if a track queued for deletion is analyzed or loaded anywhere. If there are serious concerns it'd be sad they pop up just now because we'd then have wasted quite some time implementing the feature in the first place :\ (what I've learned along the way would be persist, but still..)

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't imagine an easy fix considering the current state of the code base. Merging the code for deleting files directly from within Mixxx without any chance to detect if those files are still accessed by Mixxx probably was a mistake. We didn't consider all the consequences.

I don't veto against merging this PR, but on the other hand I won't merge it myself. Someone else may decide.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, that's unfortunate.
I'm puzzled why you approve the changes even though you have serious technical objections.

I still don't overlook the potential consequences:
The worst case would be Mixxx crashing when deleting mp3 files due to the libmad / SoundSourceMP3 / mmap issue, right? In that case I wouldn't mind removing the entire feature.
Besides that, what could happen that harms the database or puts Mixxx into an inconsistent state?

Copy link
Contributor

Choose a reason for hiding this comment

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

I could only delete the entire review or keep the rejection. Both are not an option for me. "Disagree and merge" or "Disagree and let others merge" (for a second opinion).

I don't really care enough and I also don't have a better proposal. The code is just ugly with the entanglement of the control proxies for triggering a side-effect in another component directly. It works somehow, but probably not always.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, then thanks for your review!
I'll leave to others to judge the remove feature.

// we'll first stop the deck and eject the track.
if (m_pTrack) {
ControlObject::set(ConfigKey(m_deckGroup, "stop"), 1.0);
ControlObject::set(ConfigKey(m_deckGroup, "eject"), 1.0);
}

// Set up and initiate the track batch operation
const auto progressLabelText =
tr("Removing %1 track file(s) from disk...",
Expand All @@ -1782,14 +1806,25 @@ void WTrackMenu::slotRemoveFromDisk() {
// Show purge summary message
QMessageBox msgBoxPurgeTracks;
msgBoxPurgeTracks.setIcon(QMessageBox::Information);
msgBoxPurgeTracks.setWindowTitle(tr("Track Files Deleted"));
msgBoxPurgeTracks.setText(
tr("%1 track files were deleted from disk and purged "
"from the Mixxx database.")
.arg(QString::number(tracksToPurge.length())) +
QString("<br><br>") +
tr("Note: if you are in Browse or Recording you need to "
"click the current view again to see changes."));
QString msgTitle;
QString msgText;
if (m_pTrackModel) {
msgTitle = tr("Track Files Deleted");
msgText =
tr("%1 track files were deleted from disk and purged "
"from the Mixxx database.")
.arg(QString::number(tracksToPurge.length())) +
QStringLiteral("<br><br>") +
tr("Note: if you are in Browse or Recording you need to "
"click the current view again to see changes.");
} else {
msgTitle = tr("Track File Deleted");
msgText = tr(
"Track file was deleted from disk and purged "
"from the Mixxx database.");
}
msgBoxPurgeTracks.setWindowTitle(msgTitle);
msgBoxPurgeTracks.setText(msgText);
msgBoxPurgeTracks.setTextFormat(Qt::RichText);
msgBoxPurgeTracks.setStandardButtons(QMessageBox::Ok);
msgBoxPurgeTracks.exec();
Expand All @@ -1802,10 +1837,16 @@ void WTrackMenu::slotRemoveFromDisk() {
}
// Else show a message with a list of tracks that could not be deleted.
QLabel* notDeletedLabel = new QLabel;
notDeletedLabel->setText(
tr("The following %1 files could not be deleted from disk")
.arg(QString::number(
tracksToKeep.length())));
QString msgText;
if (m_pTrackModel) {
msgText =
tr("The following %1 file(s) could not be deleted from disk")
.arg(QString::number(
tracksToKeep.length()));
} else {
msgText = tr("This track file could not be deleted from disk");
}
notDeletedLabel->setText(msgText);
notDeletedLabel->setTextFormat(Qt::RichText);

QListWidget* notDeletedListWidget = new QListWidget;
Expand All @@ -1825,7 +1866,7 @@ void WTrackMenu::slotRemoveFromDisk() {

QDialog dlgNotDeleted;
dlgNotDeleted.setModal(true);
dlgNotDeleted.setWindowTitle(tr("Remaining Track Files"));
dlgNotDeleted.setWindowTitle(tr("Remaining Track File(s)"));
dlgNotDeleted.setLayout(notDeletedLayout);
// Required for being able to close the dialog
connect(closeBtn, &QPushButton::clicked, &dlgNotDeleted, &QDialog::close);
Expand Down
2 changes: 2 additions & 0 deletions src/widget/wtrackproperty.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ constexpr WTrackMenu::Features kTrackMenuFeatures =
WTrackMenu::Feature::Reset |
WTrackMenu::Feature::BPM |
WTrackMenu::Feature::Color |
WTrackMenu::Feature::RemoveFromDisk |
WTrackMenu::Feature::FileBrowser |
WTrackMenu::Feature::Properties |
WTrackMenu::Feature::UpdateReplayGain;
Expand Down Expand Up @@ -94,6 +95,7 @@ void WTrackProperty::mouseMoveEvent(QMouseEvent* event) {
DragAndDropHelper::dragTrack(m_pCurrentTrack, this, m_group);
}
}

void WTrackProperty::mouseDoubleClickEvent(QMouseEvent* event) {
Q_UNUSED(event);
if (m_pCurrentTrack) {
Expand Down