but he has the habit to always fix two things together :) my question is what do you think about his second change?
2009-11-01 30528, 2009
outsidecontext
how he handles the comment language
2009-11-01 30558, 2009
luks
it's a good idea, but it's not backwards compatible
2009-11-01 30523, 2009
luks
I think the fix for itunnorm isn't correct
2009-11-01 30530, 2009
luks
it should not delete anything by default
2009-11-01 30511, 2009
outsidecontext
yep, i was not sure why he did that at all
2009-11-01 30542, 2009
outsidecontext
without deleting, would it cause duplicate tags?
2009-11-01 30556, 2009
luks
only if the description or the language doesn't match
2009-11-01 30519, 2009
luks
there was a problem before, because picard was writting all COMM tags with language ' ' (three spaces)
2009-11-01 30527, 2009
luks
so it was writing a duplicate frame
2009-11-01 30533, 2009
luks
one for 'eng', one for ' '
2009-11-01 30555, 2009
outsidecontext
ok, now i see. so he originally needed the deletion, but with you language fix this got obsolete
2009-11-01 30501, 2009
luks
I think handling this is a good idea, because there shouldn't be more than one iTunNORM
2009-11-01 30514, 2009
luks
but it should be done only if we have a comment:iTunNORM entry in metadata
2009-11-01 30552, 2009
outsidecontext
sounds better
2009-11-01 30530, 2009
outsidecontext
what about "if desc.lower()[:4]=="itun":"? shouldn't it check for iTunNORM?
2009-11-01 30553, 2009
luks
I think there is one more special iTunes frame that should be handled similarly
2009-11-01 30558, 2009
luks
I can't remember what it is now
2009-11-01 30512, 2009
outsidecontext
ok
2009-11-01 30531, 2009
outsidecontext
i will change his fix to check for the tag present in metadata
2009-11-01 30539, 2009
outsidecontext
guess we must do a 0.12.1 then :(
2009-11-01 30526, 2009
luks
outsidecontext: I'd simply do it in the "if desc.lower()[:4]=="itun":" branch
2009-11-01 30555, 2009
outsidecontext
becuse there can be only one tag anyway, ok
2009-11-01 30509, 2009
luks
maybe just something like tag.delall('COMM:' + desc)
2009-11-01 30506, 2009
outsidecontext
about his language changes (which i won't merge right now): i told him to change the order to comment:des:lang, because it looked more backward compatible
2009-11-01 30529, 2009
luks
I'm not sure about that change
2009-11-01 30500, 2009
outsidecontext
me neither.
2009-11-01 30523, 2009
outsidecontext
one of my problems with many of his patches: they include one thing i like, and another i don't like :)
2009-11-01 30528, 2009
luks
comment:desc:lang would definitely be better
2009-11-01 30534, 2009
luks
but it should handle also the case of comment:desc
2009-11-01 30546, 2009
luks
because that's what existing plugins might write
2009-11-01 30500, 2009
outsidecontext
exactly. and we must check all other formats
2009-11-01 30541, 2009
outsidecontext
and a better default for lang is maybe 'eng' instead of None
2009-11-01 30525, 2009
luks
I'm not sure if None would actually work
2009-11-01 30501, 2009
outsidecontext
what was it set to before we explicitely added it?
2009-11-01 30503, 2009
luks
nothing
2009-11-01 30510, 2009
luks
so it used the default value
2009-11-01 30518, 2009
outsidecontext
which would be None in mutagen, and would result in 3 null bytes
2009-11-01 30558, 2009
outsidecontext
luks: ok, i added the fix for the COMM frames. i will make a0.12.1 then. damn
