-
-
Notifications
You must be signed in to change notification settings - Fork 105
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
Try add MusicBrainz tag to file #82
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delayed response, nice work so far..
Here's a few points. Cheers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was an error, I meant to request changes.
It seems all good now ! |
Yeah. I'll take a minute to apply some final touches on my end, formatting and all, and debug the logic to ensure it checks out. Also, yeah, we could have a Provided we can guarantee a high degree of accuracy, I'd prefer to embed the tags by default if it can find them. But leave the option available for users who'd rather skip them. |
For the default disabling of musicbrainz it's more of a personal preference but I think it's better to disable it by default since people who don't know it won't use it, and so it would reduce the number of calls to musicbrainz servers that aren't necessary (which is good since it's a totally free service) |
Valid point indeed, in that case, we'll do just that. Thanks for bringing it up. |
0601faa
to
e695eb2
Compare
Played around with MusicBrainz's API for a while, refactored the code After doing a lookup on MusicBrainz Picard, I noticed it filled in some missing fields. So, I thought why not replicate that. All the new tags:
So, it turns out that with AtomicParsley as-is, you actually can't include rDNS data whose name contains spaces. I've patched it on a local fork which works perfectly but then I stumbled on a memory error that leads to a segmentation fault on free. I'm currently investigating and as it turns out, long rDNS names like |
All's good on @Maxmystere, you have more expertise with C++, I suppose, if you have the time, could you equally investigate this on your end? https://github.com/wez/atomicparsley Opened an issue: wez/atomicparsley#44 |
Hello ! I'm back from the dead with another attempt at finding a solution to this tagging issue |
Hey man, welcome back. Can you run |
Thanks for that command I didn't know about this one to format files |
I think if we want a clean PR I should
But I'm kinda lacking knowledge on how to do it properly Is there a way to force crash on fail metadata tagging to ease development ? |
65893f7
to
d27c414
Compare
I tried to sync master and it seems like I didn't break anything (hopefully) |
Hello ! Resurrecting this PR to know what's left I should do to improve code before merging ? |
Closes #81
Did several research and managed to do most of tag gathering