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

Synchronize external track collections [aoide Prerequisite] #2285

Merged
merged 10 commits into from
Sep 28, 2019
Merged

Synchronize external track collections [aoide Prerequisite] #2285

merged 10 commits into from
Sep 28, 2019

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Sep 17, 2019

Synchronization from Mixxx to external track collections:

  • Implements only the export direction (one-way), i.e. no import. The internal track collection in Mixxx is the leader and the external track collection is the follower by receiving incremental updates.
  • New interface and base class ExternalTrackCollection
  • Library manages all external collections
  • Changed some parameters if required, e.g. from TrackId to TrackRef
  • TrackDAO::databaseTracksMoved has been replaced by TrackDAO::databaseTracksReplaced with slightly different and more useful parameters. Please don't start discussing the usage of QPair, that's just a negligible detail ;)
  • Moved the transactions from DirectoryDAO into TrackCollection.

Considered feature complete for the first iteration.

@uklotzde uklotzde changed the title [WiP - aoide Prerequisite] Extension points for external library sync [WiP - aoide Prerequisite] ExternalTrackCollection Sep 20, 2019
@daschuer
Copy link
Member

Did you also consider online Libraries like Spotify as ExteralTrackCollection?

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.

Some comments. Thank you for the work.

src/library/dao/directorydao.cpp Outdated Show resolved Hide resolved
for (const auto& movedTrackRef : movedTrackRefs) {
replacedTrackRefs += qMakePair(movedTrackRef, movedTrackRef);
}
m_trackDao.databaseTracksReplaced(std::move(replacedTrackRefs));
Copy link
Member

Choose a reason for hiding this comment

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

I thing this needs a comment why this needs to be called with two times movedTrackRef

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, this needs to be reconsidered. I just adapted the code technically to keep it working.

TrackCollection delegates back to TrackDAO not for updating the database but just to dispatch some notifications. It somehow works, but I refuse to touch this brittle design.

src/library/dao/trackdao.cpp Show resolved Hide resolved
if (pTrack->getId().isValid()) {
if (pTrack->isDirty()) {
for (const auto& externalTrackCollection : m_externalTrackCollections) {
externalTrackCollection->saveTrack(*pTrack, ExternalTrackCollection::ChangeHint::Modified);
Copy link
Member

Choose a reason for hiding this comment

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

How do you permit concurrent write to the same track file in case the external library also wants to save to metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only by convention. The external track collection is not supposed to touch any files that are used by Mixxx. Period.

src/library/library.cpp Outdated Show resolved Hide resolved
if (updatedTrackIds.isEmpty()) {
return;
}
if (!m_externalTrackCollections.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

This can also become an early exit.

@uklotzde
Copy link
Contributor Author

Did you also consider online Libraries like Spotify as ExteralTrackCollection?

ExternalTrackCollection only contains functions that propagate changes from Mixxx to the external collection, i.e. write-only. This is a different use case. Documentation is still missing, will follow eventually.

The interface is designed to cover only my use case and to keep the changes to a minimum. It is an adapter that serves as an anti-corruption layer between the new and the old world. I'm not able to repair the internal track management that is wildly scattered among Library, TrackCollection, TrackDAO, and the LibraryScanner without clear responsibilities and layers of abstractions. The goal was to make the integration work without breaking anything.

@uklotzde uklotzde changed the title [WiP - aoide Prerequisite] ExternalTrackCollection Synchronize external track collections [aoide Prerequisite] Sep 23, 2019
@daschuer
Copy link
Member

OK, Thank you. LGTM

@daschuer daschuer merged commit 63fc79c into mixxxdj:master Sep 28, 2019
@uklotzde uklotzde deleted the dev_external_library_sync branch September 28, 2019 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants