#musicbrainz-devel

Which pretty much means keeping the metadata class as-is.

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 '/')

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 /.

why is "AC/DC/Queen" a problem, other than being weird-looking?

I mean, I though we agreed we weren't going to try unflattening anything

s/though/thought/

Ok - perhaps not a counter-example. But suppose they were called DC/AC.

Sorry - meant to cancel that last line.

Yes - we agreed not to unflatten.

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.

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.

(P.S. There is no agreed standard for concatenating multi-values when saving id3v23 - some taggers use '/', some use '\' some '\\' some x'00'.)

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

My guess is that whatever character(s) you use, someone is going to have them in their Artist name.

'/' 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?

IMO we should leave the separator alone for now, and first fix the exact problem (new values should be flattened for comparison)

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.

nikki - are these actually two catalogue numbers concatenated or a single catalog number?

I don't know

Can you give me a MB URL so I can see the example?

probably the former but I'm just saying, even for something like catnos it won't always produce an unambiguous result

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.

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.

i.e.unflattening for comparison = small but significant probability of a false-negative

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 think what I suggested here is the cleanest: https://github.com/musicbrainz/picard/pull/105#... but I'm open to any idea that keeps the hacks (mostly) in a single place

I have (of course) considered bitmap's suggestion however...

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?

1. If we subclass metadata for all ID3 then we don't simplify anything - as the changes only apply to ID3v23 not to ID3v24.

Mineo: basically, yes

Mineo: though TagDiff.add compares just a single tag's old and new values

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).

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.

bitmap: ah, yes, the iteration is done in MetadataBox._update_tags

I don't understand (1), and I explained why (2) isn't an issue in the pull request :)

there is not need to "recast" anything, the getall() method returns the appropriate value based on the id3 setting

I don't understand (1) either because bitmaps suggested MP3Metadata is behaving the same... ^that

Hmmmm.

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.

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).

no, look at the getall method bitmap outlined

there's no need to do any casting because you can just check for the value of write_id3v23 when accessing the metadata

Yes - have read that code - but it is incorrect.

in which way?

Firstly, what we want to use for display and for comparison is different.

+1

I will see if I can explain with a counter-example:

hmmm.

Actually I think bitmap is right for individual files.

You can apply the same transformation for both Original and MB data and it will be consistent.

EXCEPT if the file is in the opposite ID3v2x format.

Or specifically if the file is in ID3v24 format and we are saving in ID3v23 format.

we shouldn't be transforming original values at all, only new values

In this case, the code as provided by bitmap will show a false positive match.

Ok - so we cast the Original values as the existing metadata class and the New values as the subclass.

I guess that will work.

yes, that's the code I posted for File.__init__ :)

it only assigns MP3Metadata to self.metadata, not self.orig_metadata

So you did. I apologise for not having understood before.

However, I still believe that the comparison code should be encapsulated in the metadata class.

So there is still room for improvement in the code.

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.

No - now I understand I can see that this is an elegant solution.

And it would seem to work where the user's existing album files are a mixture of ID3v23 and v24.

Ok. Thanks everyone for being so patient with me.

Especially bitmap!!

no, thank you for pushing this issue forward :)

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.

And create the subclass.

just one suggestion…it might be best to make these changes in smaller steps/separate pull requests, so that they are easier to review

Yes - I was planning to do that - once it has passed peer review we can collapse the changes.

okay, great

I am on the learning curve for GIT too <sigh>

there's plenty of knowledgable git people here if you have questions :)

Thanks for the offer.

On behalf of everyone else lol

Goodnight all

just reading backlog

"/" 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

then the tagging code can be reused in other open-source apps, and can be improved and maintained separately to picard

a pony would be nice too

^ +1

alternate response: thanks for volunteering :P

haha

Nah, think I'm too fat for a pony

Can it be a horse?

also, whatever changes are required to make id3v2.3 work should be done in compatid3.py, not the generic file/metadata classes

We can get a MetaBrainz horse for sharing :p

ianmcorvidae: tbh I might

anyway, I'm not sure anyone disagrees with that, it's just easier said than done

yeah

I've pretty much isolated the necessary code form picard already ;)

then see ianmcorvidae's response :P

nikki: I did, that's what I'm replying to :P

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

but I haven't talked about this in ages, so :P

yeah, the file saving and loading is very entangled with qt

I'm sure with some work it could be separated though

I wish the world would support id3v2.4 :P

you mean flac, surely

yeah, fuck id3 :P

everything that matters supports flac :P

android does, linux and windows do, so we're sorted

(not in itunes, since itunes is stupid, but still)

LordSputnik: it uses some Qt classes to queue batch file saves, but the individual file save methods aren't entangled

I've never used a mac, so I didn't know whether it worked there :P

bitmap: good, I didn't think so, I didn't think I wrote my own in Warp

that's a confusing name :P

I know ;)

I feel like I should just refer to your tagger as kuno

to be especially annoying :P

I'm not really developing it anymore :P

since beets does the same thing :P

and also it would be better to get automatic retagging in picard

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

sounds like a plan

I'm hoping to start helping out on picard when exams are over :)

I've got a load of tickets I'm watching atm