Purge issue fix: Lp1821514 #2065
Conversation
… is already used for missing entries, fixing lp1821514
This is required since they are not anymore part of the libary.
|
Not sure if this one fixes the pending drag and drop issue as well. There is a chance .. |
|
Messing up the identity of an existing object during its lifetime is the wrong approach. Why not mark both the database record as well as the track object as to-be-deleted? If it is rediscovered by the library scanner the track object keeps its ID and only the deletion flags need to be reset. If the track object is finally purged from the cache and the deletion flag is still set then it needs to be deleted from the database instead of updated. |
|
Only tracks that are not referenced by GlobalTrackCache are allowed to be physically deleted from the database when purged. Otherwise they would lose their ID when rediscovered. |
|
I have also considered a delete flag solution like you have proposed. I have actually started to implement it, but I have decided against, because of two reasons: The code involved becomes more complex because the additional case handling of a cached track and because the user expect that the track disappears instantly. Then I realized that the "messing up" is not a real problem, because we logical only undo the library "add". Mixxx can handle Tracks without an ID. The ID is set during the library addition. When purging a track we just undo this and go back to the original state. |
| @@ -698,6 +698,11 @@ void Track::initId(TrackId id) { | |||
| // generated by the Database itself. | |||
| } | |||
|
|
|||
| void Track::resetId() { | |||
| QMutexLocker lock(&m_qMutex); | |||
There was a problem hiding this comment.
This doesn't make sense, the id is not protected by the lock. I would recommend to adopt the same strategy as for initId() which is also invoked only from GlobalTrackCache. The ID of a track must only be modified by GlobalTrackCache while locked!
There was a problem hiding this comment.
I don't understand your point here. The call stacks of initId() and resetId() are very similar both private and both used the lock. We can of cause get rid of the lock in both cases because both are done from the GUI aka library thread and are int which is written atomic.
What could be improved here?
There was a problem hiding this comment.
Ups, sorry. I must have had an earlier state of the code in my mind. It's the dirty flag that doesn't need to be set. The locking seems to be correct!
src/track/track.h
Outdated
| @@ -327,6 +327,8 @@ class Track : public QObject { | |||
| // GlobalTrackCacheResolver! | |||
| void initId(TrackId id); // write-once | |||
There was a problem hiding this comment.
write-once is no longer true
initId() and resetId() and share the same restrictions mentioned in the comment above!
src/track/globaltrackcache.cpp
Outdated
| if (m_tracksById.end() != trackById) { | ||
| Track* track = trackById->second->getPlainPtr(); | ||
| track->resetId(); | ||
| track->setDateAdded(QDateTime()); |
There was a problem hiding this comment.
Resetting the date does definitely not belong here, completely wrong context and abstraction level.
There was a problem hiding this comment.
It is moved here, because normally we have no track object when purging a track. It is a database only action that operates with track IDs only. Only at this point if we have found the ID in the cache, we have access to the track object.
We can solve the abstraction issue by instantiate a track object before purging it, but that is just extra CPU and DB reads. Alternative, we may not reset The date added and remove the debug assert expecting a NULL date before adding.
What do you think?
There was a problem hiding this comment.
Then let's keep the date unmodified instead of violating abstraction rules. The Track object needs to be aware of the use case that it might be re-added. Please log a message on level INFO if that happens.
|
Done. |
|
LGTM. Thank you for handling this edge case! |
Fix issues with purged tracks.
Now purged tracks are kept in cache as usual. This is required to have only a single reference to a file.
This is relevant if the track is re-added, before the track is pushed from the cache.
To keep a consistent state, the track id and date added is removed.
https://bugs.launchpad.net/mixxx/+bug/1821514