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

ronso0 · 2021-12-08T21:50:28Z

I was missing Remove From Disk in the deck menu when I was too lazy to look for the track in the library.
Here it is.

src/widget/wtrackmenu.cpp

ronso0 · 2021-12-08T22:02:12Z

src/widget/wtrackmenu.cpp

+            // Eject track from deck
+            ControlProxy* ejectTrackProxy = new ControlProxy(
+                    m_deckGroup, "eject", this);
+            ejectTrackProxy->set(1.0);


I'm not sure this is the best way to do it, but the first and simplest that came to mind.

Do we need to check if it's the deleted track that's loaded?
The progress dialog is modal so manual unloading is not possible in he GUI, but with keyboards and controllers.

Deleting an MP3 file while the audio stream is decoded might crash Mixxx: https://bugs.launchpad.net/mixxx/+bug/1452005

This happens because libmad/SoundSourceMP3 uses memory mapped files. You are not able to detect how many decoders are active. Checking if a track is loaded on a deck is not sufficient.

Better drop libmad/SoundSourceMP3 and use something more robust.

This might also happen with the existing delete & purge action, right? Like when the Analysis feature is busy.

How do you propose to proceed?

src/widget/wtrackmenu.cpp

ronso0 · 2021-12-09T02:05:19Z

I added another check and removed the redundant feature test.
I'll squash the fixup as soon as I get thumbs up.

uklotzde · 2021-12-09T09:49:55Z

On second thought we should not enable the option to delete the file of a track that is loaded on a deck. This option should never be enabled if m_pTrack is set.

ronso0 · 2021-12-09T16:20:50Z

I won't fight for this option, though I want to understand the reasons.
Because of the libmad bug you linked?
From my understanding any selected track can be analyzed (in the background) while the user deletes the file in Mixxx. So it wouldn't make a difference to unload the track after the user confirmed to delete it, would it?
If that is true we would need to disallow the track file deletion in general, right?

uklotzde · 2021-12-09T16:50:22Z

It is counterintuitive to delete the file of a track while it is still loaded. Even if it doesn't crash Mixxx.

Swiftb0y · 2021-12-09T17:09:24Z

It is counterintuitive to delete the file of a track while it is still loaded. Even if it doesn't crash Mixxx.

I disagree to be honest. Its counterintuitive if I want to delete a track but can't for no apparent reason. Maybe I just listened to the track and decided that it doesn't belong into my library, so I want to delete it but can't because its still loaded in a deck.

Swiftb0y · 2021-12-09T17:10:15Z

If we can't delete a track while its loaded for technical reason, I'd make that transparent so the user is confused (display the option, but grey it out and explain in a tooltip or something).

ronso0 · 2021-12-09T18:38:24Z

We can simply ~~unload~~ stop the deck and eject the track after the confirmation and before deleting the file.

Do you confirm to stop the deck and delete the loaded track file from disk?
This can not be undone!

ronso0 · 2021-12-09T22:24:30Z

Marked as draft because --in case it's decided to allow the operation for decks-- I'd squash the fixup commits.

This is the updated warning (for decks):

uklotzde · 2021-12-10T00:39:14Z

src/widget/wtrackmenu.cpp

@@ -1761,6 +1778,17 @@ void WTrackMenu::slotRemoveFromDisk() {
        }
    }

+    // If the operation was initiated from a deck's track menu


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.

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

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.

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?

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.

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

ronso0 · 2021-12-10T23:58:21Z

(I squashed the small fixup commits)

ronso0 · 2022-02-03T16:51:35Z

Rebased after #4639 has been merged.

daschuer

Thank you. I have some final comment:

src/widget/wtrackmenu.cpp

daschuer · 2022-02-04T07:13:12Z

src/widget/wtrackmenu.cpp

+                    QString("<br><br><b>") +
+                    tr("This can not be undone!") + QString("</b>");
+        } else {
+            delWarningText =


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.

src/widget/wtrackmenu.cpp

daschuer · 2022-02-21T22:38:59Z

LGTM, Thank you.

Sorry for the delay, this one has slipped through.

ronso0 · 2022-02-22T09:13:01Z

Never mind, this one wasn't urgent.
Did you have chance to test it on windows?

daschuer · 2022-02-22T12:57:22Z

Yes, at least on my Win 10 Virtual Machine it works without any issues.

github-actions bot added the ui label Dec 8, 2021

ronso0 marked this pull request as draft December 8, 2021 21:53

ronso0 force-pushed the remove-from-disk-fix-undeleted branch from d78d78f to 143c408 Compare December 8, 2021 22:00

ronso0 added this to the 2.4.0 milestone Dec 8, 2021

ronso0 commented Dec 8, 2021

View reviewed changes

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

ronso0 commented Dec 8, 2021

View reviewed changes

ronso0 force-pushed the remove-from-disk-fix-undeleted branch from 143c408 to 6d65df6 Compare December 8, 2021 22:05

ronso0 marked this pull request as ready for review December 8, 2021 22:05

uklotzde suggested changes Dec 8, 2021

View reviewed changes

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

src/widget/wtrackmenu.cpp Show resolved Hide resolved

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

ronso0 marked this pull request as draft December 9, 2021 22:13

ronso0 force-pushed the remove-from-disk-fix-undeleted branch from f86eb0f to 6a9aeda Compare December 9, 2021 22:19

uklotzde reviewed Dec 10, 2021

View reviewed changes

uklotzde approved these changes Dec 10, 2021

View reviewed changes

ronso0 force-pushed the remove-from-disk-fix-undeleted branch from 6a9aeda to 0bf2ce3 Compare December 10, 2021 23:57

ronso0 mentioned this pull request Dec 11, 2021

Updated CHANGELOG.md for 2.4 beta #4564

Merged

ronso0 added 3 commits February 3, 2022 17:49

Remove from disk: enable for deck menus

b32addd

Remove track from disk: eject from deck after deleting

fda4ed1

stop deck, eject track after confirmation, before deleting the file

b9d5a4c

ronso0 force-pushed the remove-from-disk-fix-undeleted branch from 0bf2ce3 to b9d5a4c Compare February 3, 2022 16:52

ronso0 marked this pull request as ready for review February 3, 2022 16:52

daschuer requested changes Feb 4, 2022

View reviewed changes

ronso0 added 3 commits February 4, 2022 22:34

Remove from disk: use QStringLiteral, remove comments

e89aedf

Remove from disk: reword 'not deleted' messages

23b9581

Remove from disk: use static ControlObject method to stop & eject track

3c6e33d

daschuer approved these changes Feb 21, 2022

View reviewed changes

daschuer merged commit 1b688f2 into mixxxdj:main Feb 21, 2022

ronso0 deleted the remove-from-disk-fix-undeleted branch February 22, 2022 09:10

ronso0 mentioned this pull request Sep 20, 2023

remove WTrackText class, use WTrackProperty or WLabel #12004

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ronso0 commented Dec 8, 2021

ronso0 Dec 8, 2021

ronso0 Dec 8, 2021

uklotzde Dec 8, 2021

ronso0 Dec 8, 2021

ronso0 commented Dec 9, 2021

uklotzde commented Dec 9, 2021

ronso0 commented Dec 9, 2021

uklotzde commented Dec 9, 2021

Swiftb0y commented Dec 9, 2021

Swiftb0y commented Dec 9, 2021

ronso0 commented Dec 9, 2021 •

edited

Loading

ronso0 commented Dec 9, 2021

uklotzde Dec 10, 2021

ronso0 Dec 10, 2021 •

edited

Loading

uklotzde Dec 10, 2021

ronso0 Dec 10, 2021

uklotzde Dec 10, 2021

ronso0 Dec 10, 2021

ronso0 commented Dec 10, 2021

ronso0 commented Feb 3, 2022

daschuer left a comment

daschuer Feb 4, 2022

daschuer commented Feb 21, 2022

ronso0 commented Feb 22, 2022

daschuer commented Feb 22, 2022

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

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

Conversation

ronso0 commented Dec 8, 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

ronso0 commented Dec 9, 2021

uklotzde commented Dec 9, 2021

ronso0 commented Dec 9, 2021

uklotzde commented Dec 9, 2021

Swiftb0y commented Dec 9, 2021

Swiftb0y commented Dec 9, 2021

ronso0 commented Dec 9, 2021 • edited Loading

ronso0 commented Dec 9, 2021

Choose a reason for hiding this comment

ronso0 Dec 10, 2021 • 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

Choose a reason for hiding this comment

ronso0 commented Dec 10, 2021

ronso0 commented Feb 3, 2022

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Feb 21, 2022

ronso0 commented Feb 22, 2022

daschuer commented Feb 22, 2022

ronso0 commented Dec 9, 2021 •

edited

Loading

ronso0 Dec 10, 2021 •

edited

Loading