Skip to content
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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ShubhamBhut
Copy link
Contributor

Summary

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

Problem

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

Copy link
Collaborator

@zas zas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also tests have to be fixed.

picard/formats/apev2.py Outdated Show resolved Hide resolved
picard/formats/vorbis.py Outdated Show resolved Hide resolved
@ShubhamBhut ShubhamBhut force-pushed the PICARD-2729 branch 2 times, most recently from ae91763 to 4e75f53 Compare April 17, 2024 11:05
@zas zas changed the title PICARD-2729: Allow disabling date sanitazation for APE and Vorbis tags PICARD-2729: Allow disabling date sanitization for APE and Vorbis tags Apr 17, 2024
picard/formats/apev2.py Outdated Show resolved Hide resolved
picard/formats/vorbis.py Outdated Show resolved Hide resolved
@rdswift
Copy link
Collaborator

rdswift commented Apr 17, 2024

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

@rdswift
Copy link
Collaborator

rdswift commented Apr 17, 2024

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
Copy link
Collaborator

zas commented Apr 17, 2024

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

picard/ui/options/metadata.py Outdated Show resolved Hide resolved
@rdswift
Copy link
Collaborator

rdswift commented Apr 17, 2024

Added PICARD-2862 to update the documentation.

@zas
Copy link
Collaborator

zas commented Apr 19, 2024

@ShubhamBhut what do you think about suggested approach?

@ShubhamBhut
Copy link
Contributor Author

@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
Copy link
Collaborator

zas commented Apr 19, 2024

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.

picard/ui/ui_options_metadata.py Outdated Show resolved Hide resolved
picard/formats/util.py Outdated Show resolved Hide resolved
picard/formats/apev2.py Outdated Show resolved Hide resolved
picard/ui/options/metadata.py Outdated Show resolved Hide resolved
picard/ui/widgets/multicombobox.py Outdated Show resolved Hide resolved
picard/ui/widgets/multicombobox.py Show resolved Hide resolved
@ShubhamBhut ShubhamBhut force-pushed the PICARD-2729 branch 2 times, most recently from 51aaa38 to e82dc4c Compare April 23, 2024 17:44
@ShubhamBhut
Copy link
Contributor Author

ShubhamBhut commented Apr 23, 2024

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
Copy link
Collaborator

zas commented Apr 23, 2024

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

@ShubhamBhut ShubhamBhut force-pushed the PICARD-2729 branch 7 times, most recently from aa252cf to 5d9f57c Compare April 25, 2024 05:37
Copy link
Collaborator

@zas zas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

# Automatically generated - do not edit.
# Use `python setup.py build_ui` to update it.
# 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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# 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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ShubhamBhut
Copy link
Contributor Author

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
Copy link
Collaborator

zas commented Apr 30, 2024

@ShubhamBhut what's the status of this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants