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

Allow mutiple operations on image file for fetchart plugin #4606

Closed
wants to merge 1 commit into from

Conversation

vicobarberan
Copy link

Description

Fixes #4452

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

This looks very nice; thanks for getting this started! I really like the design of breaking up the problematically-monolithic field check on the Candidate object into several independent flags. This seems to do the trick at first glance.

I added a couple of notes suggesting that we expand docstrings to explain the flow of data through these fields, which will make this code easier for humans in the future to deal with.

Could you also please add a quick changelog entry describing what changed?

Comment on lines 65 to 66
"""Determine whether the candidate artwork is valid based on
its dimensions (width and ratio).
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this method now has side effects. That is, it assigns to the self.{downscale,downsize,reformat} fields that otherwise default to false. This should probably be documented here so it's clear that there is a stateful update happening.


def validate(self, plugin):
self.check = self._validate(plugin)
return self.check

def resize(self, plugin):
if self.check == self.CANDIDATE_DOWNSCALE:
def adjust(self, plugin):
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, it would be really useful to add a docstring here that says that this method is sensitive to the various flags on the Candidate object.

@GrantMoyer
Copy link

Ideally, the plugin would do all the required conversions in one pass, so that it doesn't introduce quality loss for each re-encoding (and disregard the quality setting completely in some cases).

@wisp3rwind
Copy link
Member

Ideally, the plugin would do all the required conversions in one pass, so that it doesn't introduce quality loss for each re-encoding (and disregard the quality setting completely in some cases).

I agree. However, the current code is still a useful first step to fix the immediate issue; doing the single-pass conversion is a bigger project that requires adding this functionality in the ArtResizer first. So, I'd say it's still fine to merge this first.

@stale
Copy link

stale bot commented Jun 17, 2023

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 17, 2023
@wisp3rwind wisp3rwind removed the stale label Jun 21, 2023
Copy link

stale bot commented Dec 15, 2023

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 15, 2023
@sampsyo
Copy link
Member

sampsyo commented Dec 15, 2023

Just checking whether @vicobarberan might be interested in doing another iteration on this PR so we can land it!

@Dr-Blank
Copy link
Contributor

I was looking at open issues to work on and came across this. Idk how I missed it when I created a similar PR a month ago to address this issue.

It seems to have been inactive for over a year. I respect the work done by the author of the existing PR, and I am leaving this message here just to link both of them.

To the author of the existing PR, I hope you don't mind me submitting a new PR. If you plan to update your PR, please let me know and I'd be happy to close mine.

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.

fetchart cover_format does not seem to be working (PNG => JPEG)
6 participants