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

id3: Add option to combine id3v1/id3v2 tags during load #357

Merged
merged 7 commits into from Oct 30, 2018

Conversation

@frestr
Copy link
Member

frestr commented Oct 25, 2018

Fixes #351

Currently disabled by default. Having it enabled by default might lead to some surprises, but is perhaps more intuitive, especially given that translate is enabled by default already.

@lazka

This comment has been minimized.

Copy link
Member

lazka commented Oct 27, 2018

I think we can enable it by default as I can't think of any downsides. We don't support saving without updating id3v1 in some way so there is no chance of getting non-removable tags which get reloaded from id3v1 next time etc.

If we want to make it optional I think having a load_v1=True option which also affects the currently active no-v2-header fallback would be easier to understand.

self.assertEquals(tags["TPE1"].text, ["Anais Mitchell"])
with self.assertRaises(KeyError):
tags["TALB"]
tags["TDRC"]

This comment has been minimized.

Copy link
@lazka

lazka Oct 27, 2018

Member

This only tests if one of them raises KeyError, not both

@@ -171,6 +174,13 @@ class XMYF(Frame): ...
else:
self.update_to_v24()

if self._header and combine_v1v2:

This comment has been minimized.

Copy link
@lazka

lazka Oct 27, 2018

Member

Move this before translate

This comment has been minimized.

Copy link
@frestr

frestr Oct 27, 2018

Author Member

That will cause issues with the precedence, since if e.g. the ID3v2 header contains a TYER tag and the ID3v1 one a TDRC tag, both will get added and then TYER removed when updating (I'll add a test for this case). find_id3v1() already updates the ID3v1 tags, but to be explicit I guess we can run the translation twice (before and after adding the ID3v1 tags).

This comment has been minimized.

Copy link
@lazka

lazka Oct 27, 2018

Member

Ah, good catch. What if we make find_id3v1() take an optional v2_version=4 arg which if 3 makes it return TYER instead of TDRC?

This comment has been minimized.

Copy link
@frestr

frestr Oct 27, 2018

Author Member

Yeah, that sounds a bit cleaner

@frestr

This comment has been minimized.

Copy link
Member Author

frestr commented Oct 28, 2018

I think it should be okay now.

if frames is None:
raise

self.version = ID3Header._V11
for v in frames.values():
self.add(v)
for k, v in frames.items():

This comment has been minimized.

Copy link
@lazka

lazka Oct 28, 2018

Member

Some issues with this:

  • known_frames can map existing frame keys to different frame classes, so while the old code ignored known_frames this ignores the value part of it and always uses the builtin frame types anyway. I guess it's still an improvement though.
  • besides that, frames is a HashKey->Frame mapping while known_frames is a FrameID->Frame mapping, so this will throw away all COMM frames from ParseID3v1 for example.
  • If you keep the known_frames change can you add a small test?
frames, offset = find_id3v1(fileobj, v2_version)
if frames:
for k, v in frames.items():
if (len(self.getall(k)) == 0 and

This comment has been minimized.

Copy link
@lazka

lazka Oct 28, 2018

Member

same as above

@lazka

This comment has been minimized.

Copy link
Member

lazka commented Oct 28, 2018

looks good otherwise

Also add some additional tests.
@frestr

This comment has been minimized.

Copy link
Member Author

frestr commented Oct 29, 2018

Thanks for the feedback. I believe it does the right thing now.

@lazka
lazka approved these changes Oct 30, 2018
@lazka lazka merged commit 6507b1d into quodlibet:master Oct 30, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lazka

This comment has been minimized.

Copy link
Member

lazka commented Oct 30, 2018

Thanks!

@frestr frestr deleted the frestr:id3_combine branch Oct 30, 2018
lazka added a commit that referenced this pull request Oct 30, 2018
…. See #357

In case of no translation use the version of the tag we are loading.
lazka added a commit that referenced this pull request Oct 30, 2018
Otherwise we lose things like TCON index translation.
This changes one test result where a TYER frame in a v2.4 tag is
replaced by the v1 TDRC. Since that is off-spec I don't think that
will be a problem, and it will likely contain the same data anyway.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.