PICARD-2729: Allow disabling date sanitization for APE and Vorbis tags

[ShubhamBhut]

Summary

• This is a…
  • Bug fix
  • Feature addition
  • Refactoring
  • Minor / simple change (like a typo)
  • Other
• Describe this change in 1-2 sentences:

Problem

• JIRA ticket (optional): PICARD-2729

Solution

Added a new config setting which is checked before sanitizing date for Vorbis and APE types. Config setting can be changed by a checkbox added in Metadata section of the Options

Action

Similar as Solution

[zas]

Also tests have to be fixed.

[rdswift]

Also a reminder that the documentation will need to be updated once this is merged.

[rdswift]

Perhaps this is a dumb question, but would there ever be a case where you might want to only apply this to one of the formats? If so, would it make sense to have separate settings for APE and Vorbis?

[zas]

Perhaps this is a dumb question, but would there ever be a case where you might want to only apply this to one of the formats? If so, would it make sense to have separate settings for APE and Vorbis?

Actually we could have an option that isn't a boolean, but rather a list of formats, so we can apply this to any chosen format instead.

Something like if format in disable_sanitize_date

[rdswift]

Added PICARD-2862 to update the documentation.

[zas]

@ShubhamBhut what do you think about suggested approach?

[ShubhamBhut]

@ShubhamBhut what do you think about suggested approach?

I understood it, except that there are only 3 tags vorbis, mp3/id3 and APE which uses sanitize_date so I think it would be implemented for only these 3 tags ?

Also apologies for late update, it's just that I have been tightly occupied for last couple of days. I will implement it tomorrow.

[zas]

I understood it, except that there are only 3 tags vorbis, mp3/id3 and APE which uses sanitize_date so I think it would be implemented for only these 3 tags ?

I'd add a method to those classes that wraps util.sanitize_date() so we can test if this method exists and add the matching class to the selector.

from picard.util import sanitize_date

class APEv2File(File):

    sanitize_date = sanitize_date
....
                    if name == 'year':
                        name = 'date'
                        value = self.sanitize_date(value)

In formats/util.py we need to add a method returning all known format classes having this method:

def formats_with_sanitize_date():
    for fmt in _formats:
        if hasattr(fmt, 'sanitize_date'):
             yield fmt

In selector code, you can iterate those classes:

from picard.formats.util import formats_with_sanitize_date

for fmt in formats_with_sanitize_date():
    do_something_with(fmt)

Totally untested, but you get the idea I guess.

[ShubhamBhut]

Hey @zas, actually this is not complete yet. For some reason, the marked formats of multicombobox are not saved. I have yet to figure it out :(

[zas]

Hey @zas, actually this is not complete yet. For some reason, the marked formats of multicombobox are not saved. I have yet to figure it out :(

See ShubhamBhut#1

• I don't think we need 2 options, if the set is empty that's enough
• We don't usually add our own widgets classes to .ui, we just override a dummy widget or just insert it in the layout.
• take care of import order (see https://github.com/metabrainz/picard/blob/master/CONTRIBUTING.md#coding-style about isort and pre-commit hook.

[zas]

It looks much better.
BTW, you shouldn't rebase all the time,Rebasing is useful, but commit history is too.

You didn't merge my PR?

[zas]

This file has to be generated by running python setup.py build_ui

[ShubhamBhut]

# WARNING: Any manual changes made to this file will be lost when pyuic6 is
# run again.  Do not edit this file unless you know what you are doing.

is to be manually added, right ? running python setup.py build_ui doesn't seem to add that

[zas]

Nope, but there was a bug that was recently fixed, be sure to rebase your PR onto the latest master

[ShubhamBhut]

It looks much better. BTW, you shouldn't rebase all the time,Rebasing is useful, but commit history is too.

You didn't merge my PR?

Apologies, I will keep that in mind. Actually I was using jujutsu for version control and it doesn't have pre-commit hooks support yet. I will stick to vanilla git from now on.

[zas]

@ShubhamBhut what's the status of this PR?