Add relocate button in the missing track library.

[louisld]

Add a relocate button on the toolbar of the missing library in case the filename of the track has changed. It opens a QFileDialog to choose the new track location on filesystem. The backend takes mixxx:FileInfo parameters and update the database with the new location.

See #8570

Before:

After:

[ronso0]

Thank you for this PR!
I left some first comments.

And the paperwork:
As a first-time contributor we need you to sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

[ronso0]

Turns out one of my comment got lost.
Why do we need to take the route via WTrackTableView and not use relocate in MissingTableModel directly?

[louisld]

Turns out one of my comment got lost. Why do we need to take the route via WTrackTableView and not use relocate in MissingTableModel directly?

I am not sure about the right pattern to use and I try to follow the one from the purge button. If you think going directly to MissingTableModel directly I can change it.

And the paperwork: As a first-time contributor we need you to sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

I already signed the contributor agreement for a previous PR.

[ronso0]

Okay, sorry, I saw the First-time contributor label (probably because your previous contributions are not merged, yet)

[ronso0]

Turns out one of my comment got lost. Why do we need to take the route via WTrackTableView and not use relocate in MissingTableModel directly?

I am not sure about the right pattern to use and I try to follow the one from the purge button. If you think going directly to MissingTableModel directly I can change it.

That's probably because WTrackTableView::slotPurge() is called from DlgHidden and DlgMissing.
Relocate is currently only called from Missing so I suggest to drop the WTrackTableView detour and relocate in the missing model.

[louisld]

That's probably because WTrackTableView::slotPurge() is called from DlgHidden and DlgMissing. Relocate is currently only called from Missing so I suggest to drop the WTrackTableView detour and relocate in the missing model.

I tried to move the implementation to the model but I need to get the selected rows from WTrackTableView.

[louisld]

Also I found that it was a bit hard to navigate in the code since there is no comment to describe what functions are doing. Is there any plan to do so ? And if yes, should I describe the functions I implemented ?

[ronso0]

I tried to move the implementation to the model but I need to get the selected rows from WTrackTableView.

Sure, you already get the rows and in case it's exactly one you can also get the selected index (or even TrackId?) from the table view, no?

I'm just aiming for a simple implementation. Disregard my comment if the hints are wrong, am currently not looking at the code.

[louisld]

I spent more time on understanding how the code works and I finally managed to go directly to the model by calling a slot in DlgMissing that get the index.

[ronso0]

Thank you!

I built this and unfortunately it doesn't work as expected.
Did it work for you with earlier commits?

How I tested:

• rename / move a track file test-track.mp3 -> test-track-new.mp3
• do a library rescan
  -> test-track.mp3 marked missing
  -> test-track-new.mp3 added as new track
• go to Missing, select test-track.mp3, click Relocate, select test-track-new.mp3, Okay
• now Mixxx tries to set the new location on the missing track which fails because Mixxx doesn't allow multiple tracks with the same location (UNIQUE constraint on the location column in track_locations table)

  warning [Main] FwdSqlQuery - Failed to execute "UPDATE track_locations SET location = :location,directory = :directory,filename = :filename,filesize = :filesize,fs_deleted = 0,needs_verification = 0 WHERE id=(SELECT location FROM library WHERE id =:trackId)" : QSqlError("2067", "Unable to fetch row", "UNIQUE constraint failed: track_locations.location")

I think we need to do it like in TrackDAO::detectMovedTracks:

• purge the new track
• update location of old track

[louisld]

I am able to relocate a track with the latest commit by using the same steps as you. However, I found that the relocation is effective only after a restart of the app.

Didn't you try to import the track with the new filename before relocating so it violates the unique constraint because a file with the same name already exists in the db ? If so, maybe a dialog with the error could be useful for the user.

[ronso0]

However, I found that the relocation is effective only after a restart of the app.

That indicates something's going wrong. The relocated track should be available instantly.
Are you sure it's the relocated track? verify by setting an obvious marker, eg. rating.
Do you have "Rescan library on start" enabled? Might be you're seeing the new track just after the scan.

---

Didn't you try to import the track with the new filename before relocating so it violates the unique constraint because a file with the same name already exists in the db ?

No. Just renamed the file and did a library scan as described above. (which does the "import" you're referring to)

The unique violation is obvious in TrackDAO::relocateTrack() because if you set the new hand-picked location (which has already been added as new track) on the missing track there'd be two track s with same location.

I can successfully relocate when I first delete the new discovered track from track_locations and then from library.
I verified it's the correct track+file by setting a rating on the missing track and double-checking the location after relocating.

[louisld]

Ok I understood why I wasn't able to reproduce the error. The track I was relocating was not in my library folder so it was not added to the library after renaming. I'll try to fix all of this.

Sorry for all the steps, I am a beginner with mixxx development.

[ronso0]

Ok I understood why I wasn't able to reproduce the error. The track I was relocating was not in my library folder so it was not added to the library after renaming.

Yeah, that makes sense since Mixxx only checks the library directories and single tracks outside those.
Will take a look at your fixup soonish.

[ronso0]

Thank you!

I left some more comments.

And regarding the term "Relocate": (sorry for noticing that late...)
I'm used to "Relocate" because I know the technical background.
But from a user's point of view we are not actually relocating anything (the user did, that's why the track file is missing)

I think "Relink" suits better.
This is also the term we use in Preferences -> Library for well.. relinking moved library directories, so it makes sense to use it here, too.

Will take a closer look at the TrackDAO changes asap.

[louisld]

I tried to fix the problem about file not found after a relink when trying to load it to a deck. I think it's linked to the cache and the track does not get updated if for the some reference to it exist.

For example it works fine if the track is never loaded in a deck but once it's loaded, even if it unloaded after, the app need a restart to update the location in the cache.

I tried a lot of different things to fix this issue but I still didn't find how to update the cache properly.

[ronso0]

I'll try to reproduce the cache issue you mentioned.

Maybe this is similar to challenge we faced when moving track files to trash with the library's track menu. The track menu finds all players where the track is loaded and tries to eject them from all. Then all units holding a pointer to that track should be released and the track can (somewhat) safely be purged from the library.

Btw why did you switch from bool to std::optional<RelocatedTrack>?
The caller (DlgMissing / MissingTrackModel) doesn't need it, simple bool is sufficient, no?

[louisld]

I changed the signature so that I can get a RelocatedTrack to pass to slotDatabaseTracksRelocated in my attempt to update the data in memory for the new location in TrackCollectionManager::relocateTrack.

[ronso0]

Oh, sorry, didn't spot that in the latest diff.

Will test this and take closer look in the next days / weeks.
Feel free to ping me in case I forget / you get impatient ; )

[ronso0]

The similarly named variables in this function make it a bit hard to understand the flow.
Let's rename for example
newTrackLocationId -> existingTrackLocationId
queryNewLocation -> queryExistingTrackLocation

and also add some comments:

Which cases it supposed to cover? (library scan already added a track with new location vs. new location not in db yet)
etc.

[louisld]

The else case covers the case where the new file is outside of the library folder.

[ronso0]

What's the reason the above code is in an extra scope?

[louisld]

I think the track does not update in the cache because there are still reference linked to it. This was an attempt to delete the local reference but maybe it's useless since references are somewhere else.

[ronso0]

Since every track in Missing should have a valid id, this can go up right after the line where we read the id.
And it can become

VERIFY_OR_DEBUG_ASSERT(trackId.isvalid()) {
    // optional qWarning()
    return;
}

[louisld]

I updated the code in relocateTrack and I think it is now way easier to understand what is happening. I am still struggling with the track location not being updated before a restart of the app but I think relocateTrack does what it should do.