Fix detection of moved tracks #2197
Conversation
|
Thank you for fixing this, this is itching me for long. |
|
Yes, it works for both 2.2 and master. The fixes should be straight forward by inspecting the individual commits. The wrong id parameter might only affect tracks that have already been moved, because the id counters from both tables are in lockstep for newly added tracks (AUTOINCREMENT). Though I'm not sure how long this condition holds, it might break at any time! |
|
The code looks good, I will do some tests. |
|
In the dev_aoide branch I have even eliminated some redundant queries. But I won't backport these improvements. |
uklotzde
changed the title
|
With the last commit I have backported all (relevant) changes from my feature branch. We can remove it if it is too complex for a bug fix of a release version. |
uklotzde
changed the title
src/library/dao/trackdao.cpp
Outdated
| << newTrackLocation; | ||
| ++newTrackLocationCount; | ||
| newTrackId = TrackId(newTrackQuery.value( | ||
| newTrackQuery.record().indexOf("track_id"))); |
There was a problem hiding this comment.
Can we move the indexOf() part to the top?
I think it is const among all queries.
src/library/dao/trackdao.cpp
Outdated
| DEBUG_ASSERT(newTrackId.isValid()); | ||
| DEBUG_ASSERT(newTrackLocationId.isValid()); | ||
| kLogger.info() | ||
| << "Found moved track location:" |
There was a problem hiding this comment.
Does this potentially join two tracks?
If we have a duplicate track and remove one?
Do we need some extra handling in this case?
src/library/dao/trackdao.cpp
Outdated
| << newTrackLocation; | ||
| break; | ||
| default: | ||
| kLogger.warning() |
There was a problem hiding this comment.
This is the case we have tree duplicates and one disappears, or if we have duplicates and one is moved.
It would be nice to detect the move. In this case use the just added track.
There was a problem hiding this comment.
I don't understand this comment. All the new have just been added. If multiple of them are candidates for one missing track, we cannot make a decision.
There was a problem hiding this comment.
Is this a possible case:
- User has the same track in two folders.
- He moved to a new library location.
- He expects that the tracks are still OK
What will happen? I have not tested it yet.
I assume currently both tracks remain missing after the move and pointing to the old location. I think it is better to assign both duplicates to the first of the two moved tracks than break a playlist.
| // table which corresponds to the track in the new location. We need | ||
| // to remove that so we don't end up with two rows in the library | ||
| // table for the same track. | ||
| { |
There was a problem hiding this comment.
What is if we found a duplicate and the track was already part of playlists?
There was a problem hiding this comment.
The newly added track can safely be deleted, because it is not referenced in any playlist. The track that was marked as missing will become visible again after referencing the new, existing location.
There was a problem hiding this comment.
But if we delete it here and something goes wrong below, the track is lost until the directory hash changes.
| const int filenameColumn = queryRecord.indexOf("filename"); | ||
| const int durationColumn = queryRecord.indexOf("duration"); | ||
| QSqlQuery newTrackQuery(m_database); | ||
| newTrackQuery.prepare(QString( |
There was a problem hiding this comment.
We need to decide if this shall only detect newly added tracks or not.
There was a problem hiding this comment.
No, we don't. At least not now. This is how the function was designed and I will not modify it.
src/library/dao/trackdao.cpp
Outdated
| // Should not happen! | ||
| VERIFY_OR_DEBUG_ASSERT(query.exec()) { | ||
| LOG_FAILED_QUERY(query); | ||
| // Last chance to skip this entry |
There was a problem hiding this comment.
Really? We have already deleted the new track above.
|
Nice solution :-) |
|
If addedTracks is empty the algorithm might consider all tracks in the library instead of just returning. But I don't want to mess around with it now. This should be done as a separate task when adding such a new library maintenance feature. |
|
LGTM, I just build it for a final test. |
|
Backported for 2.2. Both AppVeyour builds timed out, the Ubuntu build got stuck after ~27 min. |
Hmm This PR is (still) targeting master branch. 😕 |
|
Switched the target branch. Unfortunately that doesn't trigger a rebuild. |
uklotzde
changed the title
|
Still builds and works good. Thank you. |
|
@uklotzde Can you merge 2.2 into master there is a conflict with the tests. Thanks. |
While working on the aoide integration I found multiple bugs in the algorithm and database queries for detecting moved tracks.
Please don't ask for additional tests (the current test apparently didn't reveal those bugs) or further refactoring. I will only provide this minimal set of changes and will not dedicate more time for this backport.