Regarding issue #6468 (tagreader problems) #6903
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 19 commits
January 19, 2021 20:21
When using Apple style Mp4 tags the tmpo property which normally is translated to BPM is not fetched from audio file metadata. This adds this feature without fixing other problems, e.g. when changing tags within clementine existing but unchanged tags become deranged. I strongly suggest to change taglib interface in a way kid3 is using (explicit adding/removing tags). The minor change within utilities is just to avoid searching for a file within huge direcories as simply showing directory without positioning or at least highlighting is unuseable.
…dded to allow single tag editting while the original method SaveFile was overtwriting existing tags with invalid data. Therefor the original method now only effects basic attributes as required e.g. after ripping.
in single tag mode. This avoids to overwrite uneffected properties within audiofiles as it occured when using SaveFile interface. Quite sure not all names are used for tagging, only those marked edittable can be updated/inserted/removed. The names are taken from taglib translations when available (by convention written in uppercase allthough taglib name mapping is caseinsensitive). Main source: 3rdparty/taglib/.../id3v2frame translations. Names written lowercase miss a taglib equivalent and are used for internal informations. Note: When adding lines to column enumeration: Try to add mappable taglib properties.
When using Apple style Mp4 tags the tmpo property which normally is translated to BPM is not fetched from audio file metadata. This adds this feature without fixing other problems, e.g. when changing tags within clementine existing but unchanged tags become deranged. I strongly suggest to change taglib interface in a way kid3 is using (explicit adding/removing tags). The minor change within utilities is just to avoid searching for a file within huge direcories as simply showing directory without positioning or at least highlighting is unuseable.
…dded to allow single tag editting while the original method SaveFile was overtwriting existing tags with invalid data. Therefor the original method now only effects basic attributes as required e.g. after ripping.
in single tag mode. This avoids to overwrite uneffected properties within audiofiles as it occured when using SaveFile interface. Quite sure not all names are used for tagging, only those marked edittable can be updated/inserted/removed. The names are taken from taglib translations when available (by convention written in uppercase allthough taglib name mapping is caseinsensitive). Main source: 3rdparty/taglib/.../id3v2frame translations. Names written lowercase miss a taglib equivalent and are used for internal informations. Note: When adding lines to column enumeration: Try to add mappable taglib properties.
…gLog(Debug) removed" This reverts commit adc1fab.
mp4 tag 'tmpo'. Main change: Instead of posting whole song properties to external tagreader only single tag (aka the one just editted) is posted. That required a new tagreader interface. As a side effect the tag translation which differs depending on audio file type (mp3, aac, flac, ...) is delegated to propertymap in 3rdparty tagreader. On the other hand tagname are made available to playlist class (matching the columns enumeration) which should provide an easier way to add additional filepersistent tags.
|
You mentioned that you're new to git, so here are some quick squash instructions that I posted in a different PR recently: #6887 (comment) |
jbroadus
reviewed
Feb 7, 2021
| << "MOOD" | ||
| << "performer" | ||
| << "GROUPING" | ||
| << "ORIGINALDATE"; |
There was a problem hiding this comment.
A static GetTagName method with a case statement is probably less error prone.
There was a problem hiding this comment.
QStringList Song::kColumns was taken as example. QStringList might cause more runtime overhead but readability is much better to name enumeration items.
jbroadus
reviewed
Feb 7, 2021
| if (ret) { | ||
| // Linux: inotify doesn't seem to notice the change to the file unless we | ||
| // change the timestamps as well. (this is what touch does) | ||
| utimensat(0, QFile::encodeName(filename).constData(), nullptr, 0); |
There was a problem hiding this comment.
Not sure why that would be. The Qt code that handles the inotify specifies the IN_MODIFY flag. And the modification of the file would be updating the timestamp as well. Not sure what games taglib would be playing to circumvent this.
There was a problem hiding this comment.
It's just a copy from former code. I would like to remove that.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
External tagreader adaptions
to enhance usage of playlist for tag editting including fix to get BPM from mp4 tag 'tmpo'.
Main change: Instead of posting whole song properties to external tagreader
only single tag (aka the one just editted) is posted. That required a new
tagreader interface and changes on whole path from ext to playlist.
As a side effect the tag translation which differs depending on
audio file type (mp3, aac, flac, ...) is delegated to propertymap in
3rdparty tagreader. On the other hand tagnames are made available to
playlist class (matching the columns enumeration) which should provide an
easier way to add additional filepersistence tags.
Sorry for huge commit list (some are simple reverts) but I'm new to git.