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

Enable downscaling cover arts for 'fetchart' and 'embedart' plugins #64

Merged
merged 3 commits into from Nov 1, 2012
Merged

Enable downscaling cover arts for 'fetchart' and 'embedart' plugins #64

merged 3 commits into from Nov 1, 2012

Conversation

Kraymer
Copy link
Collaborator

@Kraymer Kraymer commented Oct 28, 2012

Add 'maxwidth' option to embedart and fetchart plugins.

artresizer.py instances an ArtResizer object that uses internally the PIL; ImageMagick or a web proxy service to perform the resizing operations.
Because embedart works on input images located on filesystem it requires PIL or ImageMagick, whereas fetchart is able to do the job with the fallback webproxy resizer.

artresizer.py instances an ArtResizer object that uses internally the PIL; ImageMagick
or a web proxy service to perform the resizing operations.
Because embedart works on input images located on filesystem it requires PIL or ImageMagick, whereas
fetchart is able to do the job with the fallback webproxy resizer.
@Kraymer
Copy link
Collaborator Author

Kraymer commented Oct 28, 2012

The Travis build failed

😱
Looks like I should have nosetested the whole thing before emitting the request.
One problem is that I use subprocess.check_output that has been introduced in Python 2.7

@sampsyo
Copy link
Member

sampsyo commented Oct 28, 2012

It also looks like we need to update some of the tests -- several of them are expecting the art-finding functions to return downloaded paths instead of URLs. Let me know if I can help clean up some of these.

@Kraymer
Copy link
Collaborator Author

Kraymer commented Oct 28, 2012

Removed the errors reported by nosestests (still 4 failures left) by fixing code
Tests should be rewritten but I'm not super at ease with the Mock* concept of test_art.py

@sampsyo
Copy link
Member

sampsyo commented Nov 1, 2012

Fantastic; thanks for looking into this! I'm going to start the merge process now and fix up the tests while I'm at it.

sampsyo added a commit that referenced this pull request Nov 1, 2012
sampsyo added a commit that referenced this pull request Nov 1, 2012
- Safer proxy resize. The URL parameters are now properly encoded. And a
  spurious additional request has been removed.
- Removed manual search of $PATH. Invoking "convert" without a path does this
  automatically.
- More pyflakes-friendly test import of PIL.
- Do not delete the NamedTemporaryFile -- doing so creates a race condition
  where the file might be created between the filename generation and the tool
  invocation.
sampsyo added a commit that referenced this pull request Nov 1, 2012
The previous method was to change self.__class__ dynamically to make __init__
instantiate different classes. This new way, which uses bare functions instead
of separate functor-like classes, instead just forwards the resize() call to
a module-global implementation based on self.method.

Additionally, the semantics of ArtResizer have changed. Clients now *always*
call resize() and proxy_url(), regardless of method. The method makes *one* of
these a no-op. This way, clients need not manually inspect which method is
being used.
sampsyo added a commit that referenced this pull request Nov 1, 2012
Fixed a number of issues with the changes to fetchart:
- Remove redundant fetches. This was making the Amazon source download every
  image twice even when art resizing was not enabled!
- Restore local_only switch in plugin hook, which got lost in the shuffle at
  some point.
- Don't replace the original image file in-place; use a temporary file instead.
  This would clobber the original source image on the filesystem with the
  downscaled version!
sampsyo added a commit that referenced this pull request Nov 1, 2012
The various source helper functions now return URLs instead of calling
_fetch_image themselves.
sampsyo added a commit that referenced this pull request Nov 1, 2012
An earlier commit broke the call to art_for_album here (too few arguments).
I've also now propagated the maxwidth setting for the command to match the
import hook.
@sampsyo sampsyo merged commit 447454a into beetbox:master Nov 1, 2012
sampsyo added a commit that referenced this pull request Nov 1, 2012
Searching for `convert` or PIL has non-negligible performance overhead, so it's
preferable to only do it when really necessary. This way, the search is only
performed when ArtResizer.shared is accessed for the first time.
sampsyo added a commit that referenced this pull request Nov 1, 2012
We currently just document the fact that convert.exe can interfere with finding
ImageMagick's convert binary. We can solve this with a config option easily once
confit is merged.

This also changes the line endings for fetchart.rst back to Unix.
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.

None yet

2 participants