WTrackMenu: add Remove From Disk action

[ronso0]

supersedes #1737 by @WaylonR

Remove From Disk track menu action in

• Tracks
• Hidden
• Crates
• Browse
• Recording (= Browse with fixed folder)

Browse & Rec don't automatically refresh after deletion, the view has to be activated again (click).
This is mentioned in the Delete Summary dialog.

Nice to have:

• for tracks in History?
• for tree items in Browse

Current simple implementation:

• modal, resizable Delete confirmation dialog with scrollable file list
• track files are deleted (incl. progress dialog)
• deleted or non-existant tracks are purged from internal & external collections
• message box with list of files that could not be deleted, track references are kept

Which workflows do we want to cover?
Delete file and ...

1. purge all associated information
2. keep track reference in all features for a replacement track¹
3. keep track reference in history only¹²
   ¹ delete waveform and replayGain only
   ² delete all cue types, rating, track color? etc.

ToDo

• show a resizable Delete? confirmation dialog with a scrollable file list
• facilitate double-checking the files with confirmation dialog adjusted to the longest file path
• put Delete action into File submenu introduced in Better file export #3572 to minimize accidents Delete Track Files > Delete Files From Disk

Nice to have

• add Delete button to feature controls bar in Tracks > Hidden
• automatically refresh Browse/Rec folder view after deletion Not worth the effort

[uklotzde]

I suggest to delete and purge tracks in lockstep one after another while displaying a progress dialog. Implemented like the other batch operations for tracks.

It might take longer but it reduces the temporary inconsistencies between the file system and the database if anything goes wrong.

[ronso0]

I suggest to delete and purge tracks in lockstep one after another while displaying a progress dialog. Implemented like the other batch operations for tracks.

It might take longer but it reduces the temporary inconsistencies between the file system and the database if anything goes wrong.

So you suggest to only delete the files in removeTracksFromDisk(), then simply pass the refs to purgeTracks()?

[ronso0]

didn't encounter a progress bar so far, we have it only for track export, right?

[ronso0]

so I'll fork the mechanics from src/library/export/

[uklotzde]

didn't encounter a progress bar so far, we have it only for track export, right?

All batch operations that affect individual track objects are using mixxx::TrackPointerOperation. I suggest to apply this pattern here, because it is not a single batch database operation. You need to obtain the file locations from the track objects anyway.

Applying the batch database and filesystem operations separately is inconsistent. Your code already reveals this mismatch.

[uklotzde]

so I'll fork the mechanics from src/library/export/

No, not necessary. Just take a look at WTrackMenu, everything is already there. It's ok to execute those actions in the UI thread if the user is able to abort the long running operation at any point, i.e. after each processed track.

[ronso0]

Just take a look at WTrackMenu, everything is already there.

Excellent!
Are you suggesting to move removeFromDisk() from TrackCollectionManager to a batch operation in WTrackMenu?
And then pass the successfully deleted ids to TrackCollectionManager::purgeTracks?

btw, ignore the current implementation, I'm only tweaking it to get a feeling for the UX, fromatting etc.
I'll rebase this frequently. Comments are welcome though.

[uklotzde]

Just take a look at WTrackMenu, everything is already there.

Excellent!
Are you suggesting to move removeFromDisk() from TrackCollectionManager to a batch operation in WTrackMenu?
And then pass the successfully deleted ids to TrackCollectionManager::purgeTracks?

btw, ignore the current implementation, I'm only tweaking it to get a feeling for the UX, fromatting etc.
I'll rebase this frequently. Comments are welcome though.

Purge a single track immediately after the corresponding file has been deleted successfully, i.e. pass a list with a single element.

[ronso0]

@uklotzde I need some more guidance here:
I'm about to move the deletion to a TrackPointerOperation. I'm a bit clueless what the ideal way is to implement the Purge action here. As far as I can tell this can be done in trackcollectionmanager only.
Should I pass a trackcollectionmanager pointer to the TrackPointerOperation?

Also it would be helpful to store all failed deletions and show that list to the user when the batch process has finished.

[ronso0]

and how would get a TrackRef from the TrackPointer? didn't find an easy way to get that.
would it be okay to compose that myself inside the TrackPointerOperation using pTrack->getId/canoncalLocation/location?

solved.

[ronso0]

I tried to understand the existing batch process structure in WTrackMenu and adapt it to what's needed for file deletion.
Though I'm not sure it's correct how pass the TrackCollectionmanager pointer around.

deletion and purging seems to work okay but I don't get a progress bar and it always crashes after like 4-8 tracks.
(though tracks are purged, just the tablemodel is not refreshed yet)

I do get a progress bar when I experimented with the disabled hide() command. But has a few other drawbacks, like the "hide from playlists?" dialog covering the delete progress dialog. Also hiding doesn't seem to succed for all track pointers.

Any help is appreciated.

[uklotzde]

debug [Main] GlobalTrackCache - Deleting Track(0x8aa8180)
critical [Main] Invalid database identifier value: QVariant(Invalid)
critical [Main] DEBUG ASSERT: "m_taskInfos.isEmpty()" in function virtual mixxx::TaskMonitor::~TaskMonitor() at ../src/util/taskmonitor.cpp:22

Purging tracks one by one doesn't seem to be possible, because we are not able to suppress the intermediate signals that invalidate the track model during the iteration.

Probably TrackDAO::forceModelUpdate() is the signal that causes the problematic behavior. I don't see any quick workaround other than waiting for a rewrite of the UI. The chaotic dependencies and runtime side-effects between different classes are unmaintainable.

[uklotzde]

Workaround: You already collect the TrackRefs upfront. Use these directly instead of invoking applyTrackPointerOperation(). You can reuse the existing framework if you implement a custom TrackPointerIterator that does not access m_pTrackModel during the iteration.

[ronso0]

Thanks for your feedback, appreciated.
I'll need to wrap my head around those iterators & stuff here.

[ronso0]

I agree. Do the file deletion first and thereby collect two distinct sets/lists of track references: File deletion either succeeded or failed. Then purge all the successful deleted tracks at once and report the failures. You need to use mutable members in the TrackPointerOperation.

This procedure is eventually consistent and safe. Tracks of already deleted files will be discovered as missing after a rescan in case anything goes wrong.

wooo, it works! (had to test in with a slow SD card.. just didn't even get a progressbar when purging from nvme)

[ronso0]

I need to move the de-duplication to the trackpointer operation, now only the Delete warning list is de-duplicated.
Also stuff got lost when I stashed away some debug output.

re: workflow & protecting users from mistakes
I suggest we move Hide and Delete into a submenu Hide / Delete where Hide would be the topmost item in order to have it somewhat accessible.

[ronso0]

I'll rewind to 394e338 and fix the dialog setup according to Uwe's review.

Then this is ready.

[ronso0]

I think I'm done here. Let's merge this.

[uklotzde]

Return by const-ref, even if implicitly shared.

[Swiftb0y]

Are you sure? I'm confused why one would return a value by const reference. Wouldn't that reference have a high chance of becoming dangling in the future?
As a C++ novice, I'm confused by this and I'd highly appreciate an explanation if you wouldn't mind.

[uklotzde]

No, common practice for getters. Return by value is only used for cheap copies and move (RVO).

Of course, the caller is responsible for taking lifetimes into account. Not checked as in Rust.

[Holzhaus]

No, common practice for getters. Return by value is only used for cheap copies and move (RVO).

So should we always return by const ref if we return an existing list/vector/map (i.e. not constructed on-demand)? I vaguely remember someone requested to change the const ref into a by-value return value on one of my PR some time ago, and that confused me but I assumed it was due to some c++ weirdness that I didn't know about.

[ronso0]

Am I getting it right: const QList<TrackRef>& getTracksToPurge() const {?

[uklotzde]

References to members are returned by const-ref, local variables by value.

Exception: Members of objects that are used by multiple threads where the reference would otherwise outlive the lock guard! And even if you don't need a guard (smart pointers & implicitly shared object) you should return by value for a taking a snapshot of the state.

If you are unsure about lifetimes then please stay away from C/C++ and better use Rust ;)

[Swiftb0y]

Thanks for the explanation. I was not aware that this was a getter function that just returns a member (my fault for not actually looking at the code), returning by const-ref makes sense then.

If you are unsure about lifetimes then please stay away from C/C++ and better use Rust ;)

True, but that doesn't mean I don't want to learn. I'd say I'm pretty comfortable with lifetimes (because I learned rust) but this tripped me up so I figured I'd ask so I can improve my understanding. Thanks for the explanation again.

[uklotzde]

@Swiftb0y You hopefully noticed that my sarcastic comment was not directed at you, Niko!

In C/C++ you have to do the work of the borrow checker in your mind, thereby reducing your mental capacity. Or you simply ignore it and your code will work by chance or 99.9999999% of the time. Managed, imperative languages have their own pitfalls.

[uklotzde]

Thank you for picking this up and finishing it! LGTM

[uklotzde]

This deserves a CHANGELOG entry. Forgot to ask.

[ronso0]

Woohoo, finally!
Thanks for your guidance, and your patience @uklotzde

[WaylonR]

Woohoo! thank you for picking this up, and getting it done!

…

On 24/11/2021 12:35 pm, ronso0 wrote: Woohoo, finally! Thanks for your guidance, and your patience @uklotzde <https://github.com/uklotzde> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3212 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEHF465PKWYHVTYXHZHIUJ3UNQQMDANCNFSM4S55MTXA>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.