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

Quickfix for upstream Mutagen compatibility #478

Closed
wants to merge 1 commit into from

Conversation

rahul-raturi
Copy link
Contributor

There have been some changes in Mutagen upstream which makes it
incompatible with Picard. For reference, commits in Mutagen
upstream that currently break Picard:

  1. quodlibet/mutagen@e6e3fbe
  2. quodlibet/mutagen@2cd8cfd

This patch takes care of things for now, but is not a permanent
solution. Also as Mutagen has dropped support for Python 2.6, but Picard
is continuing it, there may be more conflicts in future.

There have been some changes in Mutagen upstream which makes it
incompatible with Picard. For reference, SHAs of commits in Mutagen
upstream (https://github.com/quodlibet/mutagen) that currently break Picard:

1. 2cd8cfdaec88bf896afd46cd03b209d688d3740a
2. e6e3fbe34f5946c92be8fb9266e9d27b46d8a700

This patch takes care of things for now, but is not a permanent
solution. Also as Mutagen has dropped support for Python 2.6, but Picard
is continuing it, there may be more conflicts in future.
Sophist-UK referenced this pull request in quodlibet/mutagen Jul 27, 2016
Sophist-UK referenced this pull request in quodlibet/mutagen Jul 27, 2016
A rather large change as both can include nested frames and
we need to pass everything down to the specs and split out
parsing/saving from the main ID3 class.

Adds three new id3 specs:
CTOCFlagsSpec for the flags value, also comes with a new flags
type providing a nice repr. ID3FramesSpec for a list of id3 frames.
And Latin1TextListSpec for the chapter id list.

ID3 gets split up into ID3Tags and ID3, where the former only includes
the non-IO parts and can be reused for CHAP/CTOC.sub_frames.

update_do_v24() will recursively update sub frames and each ID3Tags
instance will hold a unknown_frames attr which will be written back as
is (given the version matches).
@@ -40,23 +40,23 @@
# Ugly, but... I need to save the text in ISO-8859-1 even if it contains
# unsupported characters and this better than encoding, decoding and
# again encoding.
def patched_EncodedTextSpec_write(self, frame, value):
Copy link
Member

Choose a reason for hiding this comment

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

This would break Picard for everyone using mutagen < 1.34, wouldn't it?

@Sophist-UK
Copy link
Contributor

It is not clear to me that this issues that this PR fixes are conflicts between Python versions (though I accept that if Picard is using 2.6 and Mutagen no longer supports it then issues could occur in future). Have the mutagen commits been tested with Picard on e.g. Python 2.7 to see? That said, Python 2.7 is now pretty mature, so I can see no real reason why Picard shouldn't choose 2.7+ as its version requirement.

Personally I am guessing that these are more likely to be backward compatibility issues introduced by these mutagen commits - and that maintaining backwards compatibility is a mutagen responsibility (hence my comments against the mutagen commits). It seems to me that if new mutagen functionality can only be provided by breaking backwards compatibility, then mutagen should create a new major version for the new functionality and continue to support the current major version with fixes for some time.)

@lazka
Copy link
Contributor

lazka commented Jul 27, 2016

I needed to rework the internal API to get the chapter extension working.

Re Python2.6: I'm happy to continue supporting it (afaik the latest release is compatible, I've just disabled testing on travis). I've asked on the mailinglist some weeks ago and got no response: https://groups.google.com/forum/#!topic/quod-libet-development/1ErExqiDNqY

I probably should have sent an extra mail.

@lazka
Copy link
Contributor

lazka commented Jul 27, 2016

I can look into removing all this monkey patching if wanted.

edit: I'm on it..

@lazka
Copy link
Contributor

lazka commented Jul 27, 2016

Opened #479

feedback welcome

@rahul-raturi
Copy link
Contributor Author

I'm closing this PR as it won't be required after #479. Follow that for related discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants