PICARD-1477: Option to never replace an image with a smaller one

[twodoorcoupe]

Summary

• This is a…
  • Bug fix
  • Feature addition
  • Refactoring
  • Minor / simple change (like a typo)
  • Other
• Describe this change in 1-2 sentences:
  Adds an option in the "Cover Art" options page to never replace embedded cover art with smaller images of the same type.

Problem

• JIRA ticket (optional): PICARD-1477

Solution

At first I tried to implement this as a filter, but I hit a wall. So I had to move the logic inside coverart/__init__.py. Now when an image is downloaded, it checks if there was an image of the same type embedded into tags and if it was bigger.

I'm not entirely convinced by this solution either, in fact I'm still not sure how multiple images of the same type should be handled. For example, if a file has multiple "booklet" type images embedded, what size should I compare the downloaded image with? Maybe the minimum among images of the same type?

Action

Additional actions required:

• Update Picard documentation (please include a reference to this PR)
• Other (please specify below)

[rdswift]

I'm not sure there is a "best" way to handle this with multiple images of the same type. With your suggestion, I assume you mean "Maybe the minimum among currently embedded images of the same type"? That's probably as good as any approach. I certainly can't think of anything better right now.

A bit off topic perhaps, but... What do you think of an option like "Never replace embedded images of this type:" to allow adding such an image if it doesn't exist, but not replace it (them) if an image of that type does exist? The selection interface could be similar to the one used for the "Cover Art Archive" -> "Download only cover art images matching selected types" (possibly even reusing the same one with a slightly different note at the bottom):

As a side note, I really should fix the size of that window to decrease the minimum width.

[twodoorcoupe]

With your suggestion, I assume you mean "Maybe the minimum among currently embedded images of the same type"?

Yes sorry, this is what I meant

What do you think of an option like "Never replace embedded images of this type:" to allow adding such an image if it doesn't exist, but not replace it (them) if an image of that type does exist?

I like the idea, this makes me think I should try to improve the possibilities for image filters.

If I pass also the coverartimage and album (or maybe just the previously embedded images to allow comparisons) to the filters, I would be able to frame both this pr and your proposal as image filters.

I think it would make it easier to add more features like these in the future too.

[twodoorcoupe]

With multiple images of same types dict values might be overwritten, not sure that's what we want.

Now I made the dict keep track only of the smallest image for each type.

The selection interface could be similar to the one used for the "Cover Art Archive" -> "Download only cover art images matching selected types" (possibly even reusing the same one with a slightly different note at the bottom):

For now I reused the same one, as I'm still trying to understand where it would be best to create a new one.

I was thinking to create a new class that inherits from CAATypesSelectorDialog and only changes the labels' text, but I don't know where it would be more appropriate to put this class.

Maybe inside the same file as the cover art options page?

[rdswift]

The selection interface could be similar to the one used for the "Cover Art Archive" -> "Download only cover art images matching selected types" (possibly even reusing the same one with a slightly different note at the bottom):

For now I reused the same one, as I'm still trying to understand where it would be best to create a new one.

I was thinking to create a new class that inherits from CAATypesSelectorDialog and only changes the labels' text, but I don't know where it would be more appropriate to put this class.

Maybe inside the same file as the cover art options page?

Creating a new class should work, but I'm wondering if it would be simpler to use the same class but just change the appropriate label text when loading it? In any case it may be appropriate to make some size adjustments to the existing dialog to decrease the width and allow expanding (vertically?) to accommodate text of different lengths. I'm not sure I took into account different string lengths due to different language translations when I initially designed it. I was pretty new to Python and the whole i18n stuff at the time. If you think there's any value in that, I should be able to take a quick look at making it more flexible (in a separate PR).

[zas]

I was thinking to create a new class that inherits from CAATypesSelectorDialog and only changes the labels' text, but I don't know where it would be more appropriate to put this class.

Which text do you want to change?
Actually that's the display() method which is called, so you may pass additional optional parameters to redefine stuff.

If you go for subclassing (which I'm ok with), you can do it directly where it is used.

[twodoorcoupe]

but I'm wondering if it would be simpler to use the same class but just change the appropriate label text when loading it?

Actually that's the display() method which is called, so you may pass additional optional parameters to redefine stuff.

Ah that makes sense, thank you so much to both of you!

In any case it may be appropriate to make some size adjustments to the existing dialog to decrease the width and allow expanding (vertically?) to accommodate text of different lengths. I'm not sure I took into account different string lengths due to different language translations when I initially designed it.

I just tested it and the dialog seems to expand vertically automatically based on the size of the text, so I think there's no need to worry about that.

[rdswift]

Actually that's the display() method which is called, so you may pass additional optional parameters to redefine stuff.

Ah that makes sense, thank you so much to both of you!

Have a look at #2522. This would allow you to specify the alternate instructions (both above and below the list boxes) in the call to the dialog. It might be simpler than adding a new class.

[zas]

Actually that's the display() method which is called, so you may pass additional optional parameters to redefine stuff.

Ah that makes sense, thank you so much to both of you!

Have a look at #2522. This would allow you to specify the alternate instructions (both above and below the list boxes) in the call to the dialog. It might be simpler than adding a new class.

@twodoorcoupe @rdswift 's PR was merged, you can update this one.

[twodoorcoupe]

Thank you @rdswift for the help!

This is what the new cover art options look like (I just added the two checkboxes under "Embed only a single front image"):

[zas]

LGTM

[rdswift]

Looks good to me, too. Nice work.

[phw]

Good work, LGTM