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.

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

Fix relocating of moved tracks in external collections #2404

Merged
merged 10 commits into from
Feb 11, 2020
Merged

Fix relocating of moved tracks in external collections #2404

merged 10 commits into from
Feb 11, 2020

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Dec 17, 2019

The update of moved tracks in external collections didn't work as expected. Only the aoide integration in #2282 was affected affected. The old id of the old track and the new location of the new track need to be merged into a new TrackRef that identifies the relocated track.

This bug was mainly caused by using a QPair instead of a custom class. The class RelocatedTrack now encapsulates all the information with appropriate accessor functions and lots of assertions to check consistency. I also added checks to verify that Track::getTrackAdded() is valid for tracks in the library. This was the first indicator for the wrong behavior.

Note 1: Relinking library directories in Mixxx still requires to rescan the library manually. This should be fixed independently. Another bug is that the track table view is not updated with the new location after rescanning. This might also affect BaseTrackCache, I didn't check.

Note 2: During testing duplicate tracks appeared in aoide. This is a different issue that I need to check. Not reproducible, might be fixed. Aoide v0.5.0 will prevent this actively.

@uklotzde uklotzde added this to the 2.3.0 milestone Dec 22, 2019
@uklotzde uklotzde changed the title Fix relocating moved tracks in external collections Fix relocating of moved tracks in external collections Dec 31, 2019
@uklotzde
Copy link
Contributor Author

Rebased on master since no comments received and only a single commit.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

I had expected that after using Relink in Library preferences all tracks will come back without scanning the library. This is not the case. Is this a bug?

  • create folder 1 with one track
  • start Mixxx add the folder in the library preferences
  • Play the track
  • quit Mixxx
  • rename the folder to 2
  • start Mixxx go to the library preferences
  • Relink folder 1 to 2
  • try to play the track
  • ... Missing
  • Rescan Library track is back

QString newLocation;
newLocation.reserve(newFolder.size() + oldSuffixLen);
newLocation.append(newFolder);
newLocation.append(oldLocation.right(oldSuffixLen));
Copy link
Member

Choose a reason for hiding this comment

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

I think due to Stringbuilder this can become:
QString newLocation = newFolder + oldLocation.right(oldSuffixLen);


// The new TrackRef of the relocated track after merging
// the missing with the newly added track.
TrackRef mergedTrackRef() const {
Copy link
Member

Choose a reason for hiding this comment

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

when reading the usage code it was not instantly clear what is merged here.
Can it become newTrackRef .. or something smarter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updatedTrackRef() might better reflect the naming at the call side?

@uklotzde
Copy link
Contributor Author

uklotzde commented Feb 4, 2020

I had expected that after using Relink in Library preferences all tracks will come back without scanning the library. This is not the case. Is this a bug?

* create folder 1 with one track

* start Mixxx add the folder in the library preferences

* Play the track

* quit Mixxx

* rename the folder to 2

* start Mixxx go to the library preferences

* Relink folder 1 to 2

* try to play the track

* ... Missing

* Rescan Library track is back

This is a known bug that I discovered while fixing another bug. I was sure that I have already reported it, but I haven't found it yet. Out of scope, will not be fixed in this PR.

@uklotzde
Copy link
Contributor Author

uklotzde commented Feb 5, 2020

I had expected that after using Relink in Library preferences all tracks will come back without scanning the library. This is not the case. Is this a bug?

* create folder 1 with one track

* start Mixxx add the folder in the library preferences

* Play the track

* quit Mixxx

* rename the folder to 2

* start Mixxx go to the library preferences

* Relink folder 1 to 2

* try to play the track

* ... Missing

* Rescan Library track is back

This is a known bug that I discovered while fixing another bug. I was sure that I have already reported it, but I haven't found it yet. Out of scope, will not be fixed in this PR.

Ups, I mentioned it right here in this PR, see above Note 1 🙈 We should create a bug report.

@uklotzde
Copy link
Contributor Author

uklotzde commented Feb 7, 2020

Rescan after relinking: https://bugs.launchpad.net/mixxx/+bug/1862396

@daschuer
Copy link
Member

LGTM, Thank you.

@daschuer daschuer merged commit 9ff16f1 into mixxxdj:master Feb 11, 2020
@uklotzde uklotzde deleted the dev_relocate_tracks branch February 11, 2020 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants