Automatically update track metadata from file tags if outdated #4218

uklotzde · 2021-08-17T15:43:07Z

TODO:

Access ConfigObjects by predefined constants instead of plain strings #4223
Finalize Serato roundtrip (https://github.com/mixxxdj/mixxx/pull/4218/files#r702642333). Do not re-import Serato metadata if export of Serato metadata is disabled. uklotzde@278ec4f
Test Serato roundtrip (optional)
Add more unit tests (optional)

When loading a track from the database check if the underlying file has been modified. If the time stamp is newer than the timestamp of the last synchronization then update track metadata from file tags. Please note that this only works for files that have been (re-)imported with the new sourceSynchronizedAt property populated. Existing tracks in the library are not updated to prevent overwriting of modified data.

Restriction: Only iff export of track metadata into file tags has been enabled by the user to reduce surprising side-effects to a minimum. Introducing a new configuration option for this feature has deliberately been avoided to reduce the number of possible configurations. Either you want to keep your file tags synchronized or not, no exceptions! Consequently I have re-phrased the preference options.

Migration

Existing files without a synchronization time stamp are unaffected. The following query could be used to enforce that those files are re-imported eventually:

UPDATE library SET source_synchronized_ms = 0 WHERE source_synchronized_ms IS NULL;

Currently, there is no user operation in Mixxx to trigger this. If we want to offer this option to users we need to be clear about the consequences! The next time when such a file/track is loaded its properties in Mixxx will be replaced unconditionally with values from the file tags. Beat grid and cue points are preserved, as well as properties that are missing in file tags. Out of scope of this PR.

The query is also useful for testing, if you are sure that your file tags are up-to-date or considered more recent than the information stored in Mixxx.

coveralls · 2021-08-17T16:36:38Z

Pull Request Test Coverage Report for Build 1154288098

51 of 82 (62.2%) changed or added relevant lines in 11 files are covered.
4 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.01%) to 25.98%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/dialog/dlgkeywheel.cpp	0	1	0.0%
src/library/dao/trackdao.cpp	7	8	87.5%
src/sources/metadatasource.cpp	5	6	83.33%
src/widget/wtracktableview.cpp	4	5	80.0%
src/library/library.cpp	0	2	0.0%
src/track/track.cpp	6	9	66.67%
src/sources/soundsourceproxy.cpp	12	22	54.55%
src/track/trackrecord.cpp	8	20	40.0%

Files with Coverage Reduction	New Missed Lines	%
src/engine/cachingreader/cachingreaderworker.cpp	2	63.48%
src/sources/soundsourceproxy.cpp	2	63.16%

Totals
Change from base Build 1153454756:	0.01%
Covered Lines:	20029
Relevant Lines:	77093

💛 - Coveralls

Holzhaus · 2021-08-18T11:21:36Z

Did you consider the special case where the user has metadata export enabled, but not Serato metadata export? Or should we exclude serato metadata completely because it's not completely lossless?

uklotzde · 2021-08-18T12:26:05Z

Did you consider the special case where the user has metadata export enabled, but not Serato metadata export? Or should we exclude serato metadata completely because it's not completely lossless?

I have reverted the preference changes regarding Serato metadata.

There should not be any distinction between an automatic or manual import/export, both should behave exactly the same.

I don't want to split the option into Export only (one-way sync) and "Export/Re-import" (two-way sync). Every new option increases the maintenance burden. Either all-in (= full sync) or all-out (= no sync), but no middle ground.

Please give an example where this doesn't work as expect for a file with Serato tags. Then we can create a test and try to find a solution for resolving the issues.

Holzhaus · 2021-08-18T12:55:34Z

I'm open to remove the separate preferences option for serato export, as there doesn't seem to be major issues with it (or nobody besides me tested it).

However, I'm worried about potential data loss. For example, saved loops can't have a color in serato, but they can in Mixxx.

What happens in the following case?

Add loops with colors in Mixxx
Export to tags (color is lost)
Edit file tags outside mixxx (e. g. to correct typo in artist name or something)
Mixxx detects file as modified and Reimport tags automatically => will loop colors be lost?

uklotzde · 2021-08-18T14:17:42Z

How to merge existing data with (re-)imported data from Serato tags must be solved in the Serato tag import code. This issue also occurs when re-importing metadata manually. We need to investigate that.

I have already experienced issues with the round-trip in the past and had to disable writing of Serato tags: https://bugs.launchpad.net/mixxx/+bug/1921646

When loading a track from the database check if the underlying file has been modified. If the time stamp is newer than the timestamp of the last synchronization then update track metadata from file tags. But only if export of track metadata into file tags has been enabled by the user to reduce surprising side-effects to a minimum. Introducing a new configuration option for this feature has deliberately been avoided to reduce the number of possible configurations. Either you want to keep your file tags synchronized or not, no exceptions! # Conflicts: # src/sources/metadatasourcetaglib.cpp

src/preferences/dialog/dlgpreflibrarydlg.ui

daschuer · 2021-09-06T07:04:36Z

src/preferences/dialog/dlgpreflibrarydlg.ui

        </property>
       </widget>
      </item>
      <item row="1" column="0">
       <widget class="QCheckBox" name="checkBox_SeratoMetadataExport">
        <property name="text">
-         <string>Write Serato Metadata to files (experimental)</string>
+         <string>Write Serato metadata to files (experimental)</string>


Is this now also in both directions?

Serato metadata is implicitly re-imported if available. Not sure what happens when new cue points/loops and color have been edited in Mixxx and the file is re-imported.

I have added a TODO.

@Holzhaus Can you test/explain the situation, that we can adjust the text in one or the other direction?

src/track/trackrecord.cpp

…tadata-sync

uklotzde · 2021-09-15T23:37:09Z

I'm open to remove the separate preferences option for serato export, as there doesn't seem to be major issues with it (or nobody besides me tested it).

However, I'm worried about potential data loss. For example, saved loops can't have a color in serato, but they can in Mixxx.

What happens in the following case?
1. Add loops with colors in Mixxx

2. Export to tags (color is lost)

3. Edit file tags outside mixxx (e. g. to correct typo in artist name or something)

4. Mixxx detects file as modified and Reimport tags automatically => will loop colors be lost?

I don't have the time to deal with these incompatibilities or special cases that may only affect a few users. The only option then is to introduce a new preference setting that explicitly enables re-import of updated metadata with a disclaimer.

uklotzde · 2021-09-16T11:58:58Z

Changing the current behavior of how Serato metadata is synchronized is out of scope. If merging of existing with imported metadata is desired then this has to be implemented separately, non-trivial. Currently metadata is simply replaced. The behavior is documented and the user is informed about it, see uklotzde@f315803.

daschuer · 2021-09-16T16:45:14Z

This is becoming mature. Is it still a draft? I think it required some manual tests. Please remove the draft state of ready for that.

…tadata-sync # Conflicts: # src/preferences/dialog/dlgpreflibrary.cpp

…tadata-sync

Be-ing · 2021-10-12T21:25:39Z

@Holzhaus will you have time to do a final review soon?

Holzhaus · 2021-10-12T22:00:49Z

@Holzhaus will you have time to do a final review soon?

Yes, I can do that. Note that @daschuer requested changes and needs to give his approval, too.

uklotzde · 2021-10-12T22:22:28Z

@Holzhaus will you have time to do a final review soon?

Yes, I can do that. Note that @daschuer requested changes and needs to give his approval, too.

I didn't rebase on purpose and resolved the recent conflicts by merging. No substantial changes expected.

The TODOs are just sit there as a reminder what would be desirable. Edit: Marked as optional, because I am unsure how much more time I am willing to spend on this.

uklotzde · 2021-10-12T22:26:39Z

Please note that I marked this PR as ready weeks ago. The open TODOs might be confusing.

…tadata-sync

daschuer

LGTM, Thank you.
Sorry for the delay, this has slipped through.

uklotzde added the library label Aug 17, 2021

github-actions bot added build code quality ui labels Aug 17, 2021

uklotzde changed the title ~~Automatically update track metadata from file tags if outdated~~ [WiP] Automatically update track metadata from file tags if outdated Aug 17, 2021

uklotzde marked this pull request as draft August 17, 2021 15:45

uklotzde mentioned this pull request Aug 18, 2021

Access ConfigObjects by predefined constants instead of plain strings #4223

Merged

uklotzde added 2 commits September 5, 2021 13:52

UI: Reword options for track metadata sync with file tags

3ad6fcd

daschuer requested changes Sep 6, 2021

View reviewed changes

src/preferences/dialog/dlgpreflibrarydlg.ui Outdated Show resolved Hide resolved

daschuer reviewed Sep 6, 2021

View reviewed changes

src/track/trackrecord.cpp Show resolved Hide resolved

daschuer reviewed Sep 6, 2021

View reviewed changes

src/track/trackrecord.cpp Show resolved Hide resolved

uklotzde added 2 commits September 6, 2021 11:41

Library preferences: Reword file synchronization options

a33b9bb

Track source sync: Log possible reasons for runtime errors

4576f9b

uklotzde changed the title ~~[WiP] Automatically update track metadata from file tags if outdated~~ Automatically update track metadata from file tags if outdated Sep 6, 2021

uklotzde added 3 commits September 10, 2021 11:20

Merge branch 'main' of [email protected]:mixxxdj/mixxx.git into track-me…

77d3474

…tadata-sync

Merge branch 'main' of [email protected]:mixxxdj/mixxx.git into track-me…

d0f45fc

…tadata-sync

Only re-import Serato tags if Serato metadata export is enabled

278ec4f

Library Prefs: Reword and explain Serato metadata synchronization

f315803

uklotzde marked this pull request as ready for review September 16, 2021 20:14

Add missing const qualifiers in test classes

5dfc180

Be-ing requested a review from Holzhaus September 26, 2021 21:42

uklotzde added 2 commits October 12, 2021 21:50

Merge branch 'main' of [email protected]:mixxxdj/mixxx.git into track-me…

7ef3a12

…tadata-sync # Conflicts: # src/preferences/dialog/dlgpreflibrary.cpp

Merge branch 'main' of [email protected]:mixxxdj/mixxx.git into track-me…

8dfe5f6

…tadata-sync

uklotzde added 4 commits October 15, 2021 12:53

Merge branch 'main' of [email protected]:mixxxdj/mixxx.git into track-me…

c503082

…tadata-sync

Merge branch 'main' of [email protected]:mixxxdj/mixxx.git into track-me…

a024ef2

…tadata-sync

Merge branch 'main' of [email protected]:mixxxdj/mixxx.git into track-me…

645156d

…tadata-sync

Merge branch 'main' of [email protected]:mixxxdj/mixxx.git into track-me…

00ffdf4

…tadata-sync

daschuer approved these changes Oct 17, 2021

View reviewed changes

daschuer merged commit 581803a into mixxxdj:main Oct 17, 2021

uklotzde mentioned this pull request Oct 18, 2021

Add CHANGELOG entry for track metadata sync #4455

Merged

uklotzde deleted the track-metadata-sync branch October 23, 2021 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically update track metadata from file tags if outdated #4218

Automatically update track metadata from file tags if outdated #4218

uklotzde commented Aug 17, 2021 •

edited

Loading

coveralls commented Aug 17, 2021 •

edited

Loading

Holzhaus commented Aug 18, 2021

uklotzde commented Aug 18, 2021

Holzhaus commented Aug 18, 2021

uklotzde commented Aug 18, 2021 •

edited

Loading

daschuer Sep 6, 2021

uklotzde Sep 6, 2021

daschuer Sep 16, 2021

uklotzde Sep 16, 2021

uklotzde commented Sep 15, 2021

uklotzde commented Sep 16, 2021

daschuer commented Sep 16, 2021

Be-ing commented Oct 12, 2021

Holzhaus commented Oct 12, 2021

uklotzde commented Oct 12, 2021 •

edited

Loading

uklotzde commented Oct 12, 2021

daschuer left a comment

Automatically update track metadata from file tags if outdated #4218

Automatically update track metadata from file tags if outdated #4218

Conversation

uklotzde commented Aug 17, 2021 • edited Loading

Migration

coveralls commented Aug 17, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1154288098

💛 - Coveralls

Holzhaus commented Aug 18, 2021

uklotzde commented Aug 18, 2021

Holzhaus commented Aug 18, 2021

uklotzde commented Aug 18, 2021 • edited Loading

daschuer Sep 6, 2021

Choose a reason for hiding this comment

uklotzde Sep 6, 2021

Choose a reason for hiding this comment

daschuer Sep 16, 2021

Choose a reason for hiding this comment

uklotzde Sep 16, 2021

Choose a reason for hiding this comment

uklotzde commented Sep 15, 2021

uklotzde commented Sep 16, 2021

daschuer commented Sep 16, 2021

Be-ing commented Oct 12, 2021

Holzhaus commented Oct 12, 2021

uklotzde commented Oct 12, 2021 • edited Loading

uklotzde commented Oct 12, 2021

daschuer left a comment

Choose a reason for hiding this comment

uklotzde commented Aug 17, 2021 •

edited

Loading

coveralls commented Aug 17, 2021 •

edited

Loading

uklotzde commented Aug 18, 2021 •

edited

Loading

uklotzde commented Oct 12, 2021 •

edited

Loading