Optionally reset metadata on reimport if file tags are missing #4873

uklotzde · 2022-07-30T13:42:50Z

Context:
https://mixxx.discourse.group/t/cannot-update-blank-tag-in-mixxx/25348
quodlibet/quodlibet#4065

The behavior is configurable, but the preference dialog has not been extended yet (I am not planning to do this). The default behavior is as before, i.e. missing fields in file tags are silently ignored and their metadata is preserved.

The only actual behavioral change affects the track properties dialog where missing fields in file tags are now reset unconditionally, independent of the new preference setting. The comment in the code explains why this behavior is suitable.

Context: https://mixxx.discourse.group/t/cannot-update-blank-tag-in-mixxx/25348 quodlibet/quodlibet#4065 The behavior is configurable, but the preference dialog has not been extended yet. The default behavior is as before, i.e. missing fields in file tags are silently ignored and their metadata is preserved. The only actual behavioral change affects the track properties dialog where missing fields in file tags are reset unconditionally, independent of the preference setting. The comment in the code explains why this behavior suitable.

Swiftb0y

Thanks. I have a couple questions.

Swiftb0y · 2022-07-30T14:25:17Z

src/track/taglib/trackmetadata_ape.cpp

@@ -146,6 +156,9 @@ void importTrackMetadataFromTag(TrackMetadata* pTrackMetadata, const TagLib::APE
                &discTotal);
        pTrackMetadata->refTrackInfo().setDiscNumber(discNumber);
        pTrackMetadata->refTrackInfo().setDiscTotal(discTotal);
+    } else if (resetMissingTagMetadata) {
+        pTrackMetadata->refTrackInfo().setDiscNumber(QString{});
+        pTrackMetadata->refTrackInfo().setDiscTotal(QString{});
    }
 #endif // __EXTRA_METADATA__



Just curious, below we also parse bpm, trackgain, etc. Why are we not resetting those?

Would have been extra work and for those the tags don't matter. You could reset them in Mixxx. Slightly inconsistent but acceptable.

Sure. Would you mind adding a that sentence as a comment above the imports you didn't touch?

A comment was already in place at the entry point. I have extended it: 5d1f73b

Sorry I seem to have read past that. Thanks.

Unfortunately, the code is a little bit scattered and the entry point is not obvious.

The boolean could be transformed into an enum for type safety, but the boolean was easier to handle while extending the implementation. That was a deliberate decision.

I have decided to add the missing reset of both bpm and replay gain: c848725

src/track/taglib/trackmetadata_ape.cpp

src/track/track_decl.h

Swiftb0y

LGTM. anyone else willing to take a look?

daschuer · 2022-07-31T09:50:54Z

I will have a look.

daschuer

I have not yet understand why we treat reading from the file differently if it is called from the track properties dialog or from the context menu. Sure, the users have control via "Apply", but how will they find out that the context menu behaves differently? I would prefer that both behave the same for a reliable and predictable behavior.

I think we have originally not reset the not existing tags, because we where not able to write the data back. But now we can reconsider this.

There is a an issue with BPM, When I read from metadata, the BPM value is rest, but when I press apply it reappears. This should not happen.

Maybe we should treat the mandatory metadata that is Mixxx using itself differently.
I have BPM/Key and ReplayGain in mind.
I think we import these value only if Mixxx does not have them and we never reimport them again.
The same should also apply to a not existing value in the file tags.

There is clearly a use case for empty textual tags in a third party tool and pull that in as described in the forum, but there is IMHO no use case in clearing BPM, Kay and ReplayGain, read from file tags and than expect that Mixxx is analyzing this again.

daschuer · 2022-07-31T09:11:41Z

src/library/browse/browsethread.cpp

@@ -162,10 +162,12 @@ void BrowseThread::populateModel() {
                thisPath.token());
        {
            mixxx::TrackMetadata trackMetadata;
+            constexpr auto resetMissingTagMetadata = false; // no effect


I do not understand the comment. Can we remove that, or make it more verbose?

Comments are needed: a607187

src/library/dlgtrackinfo.cpp

daschuer · 2022-07-31T09:43:04Z

src/track/taglib/trackmetadata_common.cpp

-    bool isPeakValid = parseReplayGainPeak(&replayGain, strPeak);
-    if (isPeakValid) {
-        pTrackMetadata->refAlbumInfo().setReplayGain(replayGain);
+    ReplayGain* pReplayGain = pTrackMetadata->refAlbumInfo().ptrReplayGain();


This can be renamed to pAlbumReplayGain and the one above to pTrackReplayGain.

Did you consider to move the code into parseReplayGainPeak, to deduplicate it a bit?
The function is only used here.

uklotzde · 2022-07-31T12:55:44Z

There is clearly a use case for empty textual tags in a third party tool and pull that in as described in the forum, but there is IMHO no use case in clearing BPM, Kay and ReplayGain, read from file tags and than expect that Mixxx is analyzing this again.

These are all still regular tags. Introducing some special case handling for some of them will only cause confusion. The behavior should be predictable.

uklotzde · 2022-07-31T13:04:33Z

I think we have originally not reset the not existing tags, because we where not able to write the data back. But now we can reconsider this.

This PR allows to switch between 2 different modes of operation, i.e. lazy (traditional) or strict. I will not extend the scope.

uklotzde · 2022-07-31T13:07:52Z

There is a an issue with BPM, When I read from metadata, the BPM value is rest, but when I press apply it reappears. This should not happen.

This is probably an unrelated issue with the dialog and should be resolved separately.

uklotzde · 2022-07-31T13:21:35Z

I have not yet understand why we treat reading from the file differently if it is called from the track properties dialog or from the context menu. Sure, the users have control via "Apply", but how will they find out that the context menu behaves differently? I would prefer that both behave the same for a reliable and predictable behavior.

Restored the original behavior: de88868

daschuer

Thank you. LGTM.

I think the underlying idea of removing non existing metadata is correct. However ther is an existing Bug with the BPM value that demonstrates the issue with the scalar BPM tag. This also applies to Key and partially to ReplayGain. I think after this is solved we have a clear view how to deal with this new preferences option.

github-actions bot added code quality library labels Jul 30, 2022

Swiftb0y reviewed Jul 30, 2022

View reviewed changes

uklotzde added 2 commits July 30, 2022 18:04

Clarify comment about fields that are not reset if missing

5d1f73b

Use C++20 Designated Initializers

6f255b2

Swiftb0y approved these changes Jul 30, 2022

View reviewed changes

File tags: Reset missing/empty bpm and replay gain values on re-import

c848725

Swiftb0y approved these changes Jul 31, 2022

View reviewed changes

daschuer requested changes Jul 31, 2022

View reviewed changes

uklotzde added 3 commits July 31, 2022 15:00

Clarify comment

a607187

Deduplicate code

f377ded

Reword comment

0d17d9f

DlgTrackInfo: Re-import metadata according to preferences

de88868

github-actions bot added the ui label Jul 31, 2022

daschuer approved these changes Jul 31, 2022

View reviewed changes

daschuer merged commit 2fe0fc5 into mixxxdj:main Jul 31, 2022

uklotzde deleted the reset-missing-tag-metadata branch July 31, 2022 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optionally reset metadata on reimport if file tags are missing #4873

Optionally reset metadata on reimport if file tags are missing #4873

uklotzde commented Jul 30, 2022

Swiftb0y left a comment

Swiftb0y Jul 30, 2022

uklotzde Jul 30, 2022

Swiftb0y Jul 30, 2022

uklotzde Jul 30, 2022

Swiftb0y Jul 30, 2022

uklotzde Jul 30, 2022

uklotzde Jul 31, 2022

Swiftb0y left a comment

daschuer commented Jul 31, 2022

daschuer left a comment

daschuer Jul 31, 2022

uklotzde Jul 31, 2022

daschuer Jul 31, 2022

uklotzde Jul 31, 2022

uklotzde commented Jul 31, 2022

uklotzde commented Jul 31, 2022 •

edited

Loading

uklotzde commented Jul 31, 2022

uklotzde commented Jul 31, 2022

daschuer left a comment

Optionally reset metadata on reimport if file tags are missing #4873

Optionally reset metadata on reimport if file tags are missing #4873

Conversation

uklotzde commented Jul 30, 2022

Swiftb0y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y left a comment

Choose a reason for hiding this comment

daschuer commented Jul 31, 2022

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Jul 31, 2022

uklotzde commented Jul 31, 2022 • edited Loading

uklotzde commented Jul 31, 2022

uklotzde commented Jul 31, 2022

daschuer left a comment

Choose a reason for hiding this comment

uklotzde commented Jul 31, 2022 •

edited

Loading