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

mid3v2: setting multiple values/frame; deleting and setting frames at the same time (with PATCH) #37

Closed
lazka opened this issue Jul 4, 2014 · 9 comments
Labels

Comments

@lazka
Copy link
Member

@lazka lazka commented Jul 4, 2014

Originally reported by: Christoph Reiter (Bitbucket: lazka, GitHub: lazka)


From hanse...@gmail.com on November 06, 2009 19:53:33

Hi there-

I'm using mid3v2 as a command line MP3 tag editor. There are a couple things
it does not do, which I have implemented and attach a patch. Could you
please review and (if okay) apply? This patch is against SVN as of today.

====

1.) "mid3v2 --delete-frames=...." and "mid3v2 --frame value" can not be
called at the same time

The parser structure checks whether we are setting/adding frames or whether
we are deleting frames, and it then executes delete_frames() or
write_files(). It does not call both, if both option categories are given.

The attached patch fixes it so that if we are both deleting and setting
frames, we are executing both operations serially. This is not optimal
(better would be to parse and write the file only once) but it _works_.

2.) "mid3v2 -g genre1 -g genre2" does not work

Inside write_file(), the variable edits will be a list of (frame,value)
pairs. If a frame recurs (i.e. I'm trying to set multiple genres), the last
pair will override all earlier choices.

I added a preprocessing stage for edits, where it will collapse all pairs
with identical frame name to have value lists instead of scalars.

===

This functionality has been tested and seems to work fine. However I'm
neither a python expert, nor did I want to change the overall structure of
mid3v2 to more organically support my use case. Feel free to rewrite my
patch. But some way - any way - to support my usecase would be great in
upstream mutagen.

Thank you!

Hans

Original issue: http://code.google.com/p/mutagen/issues/detail?id=37


@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 4, 2014

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


From hanse...@gmail.com on November 06, 2009 10:59:29

Sorry, got a mistake when I refreshed my diff against SVN, here is the corrected
version
@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 4, 2014

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


From hanse...@gmail.com on November 17, 2009 09:51:03

Okay, here is (as far as I can see) the final version of this patch.

I reversed the order of applying the "--delete-frame" and "--FRAME=value" operations.
It makes more sense to first apply the deletes, then the writes. This way a script can
first summarily delete all occurrences of certain frames, and then add the correct
values back in.

Attachment: mid3v2.diff

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 4, 2014

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


From joe.wreschnig@gmail.com on December 12, 2009 20:22:58

I am unclear where the atomic_types variable is supposed to come from. Can you change
that whole bit so the edits dict just always holds the right type, and we don't need
to postprocess the preprocess?
@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 4, 2014

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


From joe.wreschnig@gmail.com on November 24, 2009 16:02:36

Status: Accepted

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 4, 2014

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


From hanse...@gmail.com on December 28, 2009 09:52:59

Good point. How's the attached?

Note that the biggest part of the attached diff is just an indentation change: the block
between 

   values = value.split(":")

and

   encoding=3, text=value, lang=lang, desc=desc)

is now within a loop over vlist.

Attachment: mid3v2.diff

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 4, 2014

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


From hanse...@gmail.com on December 29, 2009 14:10:08

There was a problem with my patch. I'm sorry! I've never coded python before. Here is a fix:

Attachment: mid3v2.diff

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 4, 2014

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


From joe.wreschnig@gmail.com on December 28, 2009 10:35:03

This issue was closed by revision r69 .

Status: Fixed
Mergedinto: -

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 4, 2014

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


From hanse...@gmail.com on January 05, 2010 11:29:01

Don't know if Joe is looking at "Fixed" issues, so I made a new issue#50 out of it.
@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 4, 2014

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


From hanse...@gmail.com on December 29, 2009 14:15:16

Note that the bug I introduced with r69 only appeared when actually using the new
functionality, for example assigning multiple genres. In that case the bug is fatal, but at
least it is not a regression.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.