Which pretty much means keeping the metadata class as-is.
2013-06-02 15330, 2013
Mineo does not understand the "new class of filemetadata to hold the Original and New metadata for files" part :/
2013-06-02 15320, 2013
bitmap
I still don't think (1) is necessary, and I'm not sure if (2) is a good idea (otherwise we're just causing what we want to solve, triggering unnecessary changes for everyone whose old tags all use '/')
2013-06-02 15331, 2013
Sophist-UK
Problem with using '/' as a separator is e.g. AlbumArtists = AC/DC and Queen - saved as AC/DC/Queen. Also some folksonomy tags include a /.
2013-06-02 15310, 2013
jessew joined the channel
2013-06-02 15325, 2013
bitmap
why is "AC/DC/Queen" a problem, other than being weird-looking?
2013-06-02 15326, 2013
Sophist-UK
Matching doesn't work now - so user is probably saving multiple times or spending a lot of time working out that saves are not needed.
2013-06-02 15302, 2013
bitmap
I mean, I though we agreed we weren't going to try unflattening anything
2013-06-02 15308, 2013
bitmap
s/though/thought/
2013-06-02 15310, 2013
Sophist-UK
Ok - perhaps not a counter-example. But suppose they were called DC/AC.
2013-06-02 15324, 2013
Sophist-UK
Sorry - meant to cancel that last line.
2013-06-02 15348, 2013
Sophist-UK
Yes - we agreed not to unflatten.
2013-06-02 15352, 2013
Sophist-UK
I guess it is just confusing and inconsistent to use a '/' - Picard uses '; ' as standard elsewhere. The method would probably still work if we use a '/' to concatenate - so long as we use a consistent character when we save and when we compare.
2013-06-02 15339, 2013
Sophist-UK
But if we are trying to improve consistency and simplicity of code, then using '; ' everywhere is more elegant and users will have a one-time save to make the matching work for them.
2013-06-02 15320, 2013
Sophist-UK
(P.S. There is no agreed standard for concatenating multi-values when saving id3v23 - some taggers use '/', some use '\' some '\\' some x'00'.)
2013-06-02 15314, 2013
bitmap
yeah, I don't know why '/' was used, I'm guessing because other software happened to use it as well…luks would probably know, but he's away I think
2013-06-02 15312, 2013
Sophist-UK
My guess is that whatever character(s) you use, someone is going to have them in their Artist name.
2013-06-02 15303, 2013
Sophist-UK
'/' is definitely used in some Catalog Numbers though - so it can definitely be confusing there. e.g. is 6754673/AN89564 one catalog number or two concatenated?
2013-06-02 15349, 2013
bitmap
IMO we should leave the separator alone for now, and first fix the exact problem (new values should be flattened for comparison)
2013-06-02 15356, 2013
nikki notes that we also have some catnos with '; ' (which are probably wrong, but still)
2013-06-02 15326, 2013
Sophist-UK
We could have an option about which character to use... there is already a similar option for folksonomy tags - so probably makes more sense for this to be global.
2013-06-02 15357, 2013
Sophist-UK
nikki - are these actually two catalogue numbers concatenated or a single catalog number?
2013-06-02 15308, 2013
nikki
I don't know
2013-06-02 15310, 2013
Sophist-UK
Can you give me a MB URL so I can see the example?
2013-06-02 15301, 2013
nikki
probably the former but I'm just saying, even for something like catnos it won't always produce an unambiguous result
2013-06-02 15301, 2013
Sophist-UK
This is wht luks, bitmap and I agreed that we would flatten the MB data to make a comparison with an ID3v23 file rather than try to unflatten the ID3v23 data.
2013-06-02 15327, 2013
Sophist-UK
If you try to unflatten, there will often be a counter-example - but if you flatten the multi-value data just for comparison, then it is pretty unlikely that you will get a false positive match.
2013-06-02 15330, 2013
andreypopp joined the channel
2013-06-02 15320, 2013
Sophist-UK
i.e.unflattening for comparison = small but significant probability of a false-negative
2013-06-02 15304, 2013
Sophist-UK
Anyway, I have agreed to refactor to improve the code quality whilst maintaining plugin compatibility. I am happy to spend the effort knowing that it will get peer review and may be turned down and need further work - but do want to get best input on how to make the best 2nd attempt.
I have (of course) considered bitmap's suggestion however...
2013-06-02 15349, 2013
Mineo
bitmap: for those of us who've never looked at the metadata comparison code (ME!) - it's basically iterating over the old and new metadata in TagDiff.add (in metadatabox.py) and comparing the values?
2013-06-02 15336, 2013
Sophist-UK
1. If we subclass metadata for all ID3 then we don't simplify anything - as the changes only apply to ID3v23 not to ID3v24.
2013-06-02 15345, 2013
bitmap
Mineo: basically, yes
2013-06-02 15334, 2013
bitmap
Mineo: though TagDiff.add compares just a single tag's old and new values
2013-06-02 15350, 2013
Sophist-UK
2. If we subclass metadata only for ID3v23 then we have an issue when we change the version the file is saved in from v23 to v24 or vice versa and have to recast the metadata to a different class (as opposed to initially choosing the class as suggested in the pull-request comment).
2013-06-02 15308, 2013
Sophist-UK
We definitely should put the tag comparison into the metadata class or similar and not have it doubled-up in both metadatabox.py (for the metadata widget at the bottom of the window) and file.py for determining whether a track needs to be resaved or not.
2013-06-02 15315, 2013
Mineo
bitmap: ah, yes, the iteration is done in MetadataBox._update_tags
2013-06-02 15345, 2013
bitmap
I don't understand (1), and I explained why (2) isn't an issue in the pull request :)
2013-06-02 15331, 2013
bitmap
there is not need to "recast" anything, the getall() method returns the appropriate value based on the id3 setting
2013-06-02 15340, 2013
Mineo
I don't understand (1) either because bitmaps suggested MP3Metadata is behaving the same... ^that
2013-06-02 15307, 2013
Sophist-UK
Hmmmm.
2013-06-02 15327, 2013
Sophist-UK
Maybe I didn't think it through well enough as I now see what you mean - however as I said, subclassing doesn't add any value over and above encapsulating the comparison inside the existing metadata class. Either way you still have to have alternative code depending on whether it is id3v23 or not.
2013-06-02 15355, 2013
Sophist-UK
The only way to simplify the code by using subclassing is to have a sub-class specifically for ID3v23 and then you introduce the complication of casting it back to the base metadata class if the file is saved in ID3v24 (or vice versa).
2013-06-02 15319, 2013
Mineo
no, look at the getall method bitmap outlined
2013-06-02 15302, 2013
Mineo
there's no need to do any casting because you can just check for the value of write_id3v23 when accessing the metadata
2013-06-02 15312, 2013
Sophist-UK
Yes - have read that code - but it is incorrect.
2013-06-02 15304, 2013
Mineo
in which way?
2013-06-02 15308, 2013
Sophist-UK
Firstly, what we want to use for display and for comparison is different.
2013-06-02 15303, 2013
bitmap disagrees…we should both display and compare with whatever will be written to the file
2013-06-02 15306, 2013
jessew joined the channel
2013-06-02 15308, 2013
Mineo
+1
2013-06-02 15357, 2013
Sophist-UK
I will see if I can explain with a counter-example:
2013-06-02 15330, 2013
Sophist-UK
hmmm.
2013-06-02 15342, 2013
Sophist-UK
Actually I think bitmap is right for individual files.
2013-06-02 15336, 2013
Sophist-UK
You can apply the same transformation for both Original and MB data and it will be consistent.
2013-06-02 15354, 2013
Sophist-UK
EXCEPT if the file is in the opposite ID3v2x format.
2013-06-02 15345, 2013
Sophist-UK
Or specifically if the file is in ID3v24 format and we are saving in ID3v23 format.
2013-06-02 15303, 2013
bitmap
we shouldn't be transforming original values at all, only new values
2013-06-02 15304, 2013
Sophist-UK
In this case, the code as provided by bitmap will show a false positive match.
2013-06-02 15352, 2013
Sophist-UK
Ok - so we cast the Original values as the existing metadata class and the New values as the subclass.
2013-06-02 15313, 2013
Sophist-UK
I guess that will work.
2013-06-02 15343, 2013
bitmap
yes, that's the code I posted for File.__init__ :)
2013-06-02 15359, 2013
bitmap
it only assigns MP3Metadata to self.metadata, not self.orig_metadata
2013-06-02 15324, 2013
Sophist-UK
So you did. I apologise for not having understood before.
2013-06-02 15312, 2013
Sophist-UK
However, I still believe that the comparison code should be encapsulated in the metadata class.
2013-06-02 15324, 2013
Sophist-UK
So there is still room for improvement in the code.
2013-06-02 15319, 2013
bitmap
well, the metadata class applies to all formats, not just ID3. so that was my reasoning for a subclass. having format-specific code in the general Metadata class doesn't seem right.
2013-06-02 15300, 2013
Sophist-UK
No - now I understand I can see that this is an elegant solution.
2013-06-02 15323, 2013
Sophist-UK
And it would seem to work where the user's existing album files are a mixture of ID3v23 and v24.
2013-06-02 15330, 2013
Sophist-UK
Ok. Thanks everyone for being so patient with me.
2013-06-02 15343, 2013
Sophist-UK
Especially bitmap!!
2013-06-02 15357, 2013
bitmap
no, thank you for pushing this issue forward :)
2013-06-02 15313, 2013
Sophist-UK
I will make the concatenation character an option in the Tags Options, and move the comparison stuff as additional methods in the metadata class and maybe it will have a chance of passing peer review.
2013-06-02 15334, 2013
Sophist-UK
And create the subclass.
2013-06-02 15313, 2013
bitmap
just one suggestion…it might be best to make these changes in smaller steps/separate pull requests, so that they are easier to review
2013-06-02 15354, 2013
Sophist-UK
Yes - I was planning to do that - once it has passed peer review we can collapse the changes.
2013-06-02 15309, 2013
bitmap
okay, great
2013-06-02 15330, 2013
Sophist-UK
I am on the learning curve for GIT too <sigh>
2013-06-02 15324, 2013
bitmap
there's plenty of knowledgable git people here if you have questions :)
2013-06-02 15355, 2013
Sophist-UK
Thanks for the offer.
2013-06-02 15302, 2013
Sophist-UK
On behalf of everyone else lol
2013-06-02 15336, 2013
Sophist-UK
Goodnight all
2013-06-02 15335, 2013
jessew joined the channel
2013-06-02 15311, 2013
LordSputnik
just reading backlog
2013-06-02 15328, 2013
LordSputnik
"/" is the separator specified in the id3 standard for v2.3
imo it would be good to split picard into a python module for tagging/saving files based on mbids and a graphical front-end which handles the user interaction
2013-06-02 15325, 2013
LordSputnik
then the tagging code can be reused in other open-source apps, and can be improved and maintained separately to picard
2013-06-02 15351, 2013
nikki
a pony would be nice too
2013-06-02 15307, 2013
LordSputnik
^ +1
2013-06-02 15318, 2013
ianmcorvidae
alternate response: thanks for volunteering :P
2013-06-02 15322, 2013
nikki
haha
2013-06-02 15323, 2013
reosarevok
Nah, think I'm too fat for a pony
2013-06-02 15328, 2013
reosarevok
Can it be a horse?
2013-06-02 15344, 2013
LordSputnik
also, whatever changes are required to make id3v2.3 work should be done in compatid3.py, not the generic file/metadata classes
2013-06-02 15344, 2013
reosarevok
We can get a MetaBrainz horse for sharing :p
2013-06-02 15349, 2013
LordSputnik
ianmcorvidae: tbh I might
2013-06-02 15351, 2013
nikki
anyway, I'm not sure anyone disagrees with that, it's just easier said than done
2013-06-02 15302, 2013
ianmcorvidae
yeah
2013-06-02 15307, 2013
LordSputnik
I've pretty much isolated the necessary code form picard already ;)
2013-06-02 15323, 2013
nikki
then see ianmcorvidae's response :P
2013-06-02 15343, 2013
LordSputnik
nikki: I did, that's what I'm replying to :P
2013-06-02 15348, 2013
ianmcorvidae
IIRC the biggest issue is that picard uses Qt-based objects for these sorts of things, so you want to split it out you possibly need to either split it out but require Qt (so, uh, no command-line apps I guess...) or rewrite the system of objects it uses
2013-06-02 15300, 2013
ianmcorvidae
but I haven't talked about this in ages, so :P
2013-06-02 15345, 2013
LordSputnik
yeah, the file saving and loading is very entangled with qt
2013-06-02 15303, 2013
LordSputnik
I'm sure with some work it could be separated though
2013-06-02 15318, 2013
LordSputnik
I wish the world would support id3v2.4 :P
2013-06-02 15329, 2013
reosarevok
you mean flac, surely
2013-06-02 15337, 2013
ianmcorvidae
yeah, fuck id3 :P
2013-06-02 15343, 2013
LordSputnik
everything that matters supports flac :P
2013-06-02 15300, 2013
LordSputnik
android does, linux and windows do, so we're sorted
2013-06-02 15330, 2013
nikki plays her flacs in osx :P
2013-06-02 15345, 2013
nikki
(not in itunes, since itunes is stupid, but still)
2013-06-02 15349, 2013
bitmap
LordSputnik: it uses some Qt classes to queue batch file saves, but the individual file save methods aren't entangled
2013-06-02 15349, 2013
LordSputnik
I've never used a mac, so I didn't know whether it worked there :P
2013-06-02 15312, 2013
LordSputnik
bitmap: good, I didn't think so, I didn't think I wrote my own in Warp
2013-06-02 15341, 2013
bitmap
that's a confusing name :P
2013-06-02 15346, 2013
LordSputnik
I know ;)
2013-06-02 15359, 2013
ianmcorvidae
I feel like I should just refer to your tagger as kuno
2013-06-02 15302, 2013
ianmcorvidae
to be especially annoying :P
2013-06-02 15311, 2013
LordSputnik
I'm not really developing it anymore :P
2013-06-02 15322, 2013
LordSputnik
since beets does the same thing :P
2013-06-02 15334, 2013
LordSputnik
and also it would be better to get automatic retagging in picard
2013-06-02 15359, 2013
bitmap
we could probably achieve the code separation in small steps - e.g. create a python package within the picard codebase that we slowly move stuff to, then factor it out into a separate package once it's done
2013-06-02 15347, 2013
LordSputnik
sounds like a plan
2013-06-02 15302, 2013
LordSputnik
I'm hoping to start helping out on picard when exams are over :)
