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

Merged
merged 3 commits into from Nov 1, 2012

Conversation

Projects
None yet
2 participants
Collaborator

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.

Kraymer added some commits 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.
Collaborator

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

Owner

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.

Collaborator

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

Owner

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

misc. minor fixes for artresizer (#64)
- 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

artresizer (#64): helper functions, not classes
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

fetchart fixes for image resizing (#64)
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

fix art tests for new URL interface (#64)
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

fetchart: fix command & use maxwidth (#64)
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

1 check failed

default The Travis build failed
Details

sampsyo added a commit that referenced this pull request Nov 1, 2012

lazily initialize ArtResizer singleton (#64)
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

documentation for convert.exe problem (#64)
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