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

Unify cover art downloading #2812

Merged
merged 18 commits into from Mar 21, 2018
Merged

Unify cover art downloading #2812

merged 18 commits into from Mar 21, 2018

Conversation

@declension
Copy link
Member

@declension declension commented Mar 18, 2018

New Cover Art plugin (see #1279)

  • Various API tweaks to cover art plugin, including new ApiCoverSourcePlugin to encapsulate this once
  • Update all existing cover plugins to support search
  • Tidy Discogs, and optionally allow secondary images
  • Improve lastfm search: use higher quality images where possible (Fixes #2793)
  • Allow HTTP 3xx to succeed, sometimes redirects are used
  • Add search to Cover Manager, and use new signals to communicate
  • Improvements to existing cover plugins
  • Use GTK Flowbox for faster rendering
  • Resize previews live by using native pixbuf scaling.
  • Saves original (though feature exists to resample and export as JPEG) - needs UI.
declension added 16 commits Mar 1, 2018
 * Work in progress
 * New Cover Art plugin - very basic still but can display results now
 * Various API tweaks to cover art plugin, including new `ApiCoverSourcePlugin` to encapsulate this once
 * Update all existing cover plugins to support search
 * Tidy Discogs, and optionally allow secondary images
 * Improve lastfm search: use higher quality images where possible (Fixes #2793)
 * Allow HTTP 3xx to succeed, sometimes redirects are used
 * Add search to Cover Manager
 * Some basic testing
 * Download full image, but resize the pixbuf for display.
 * Use signals not callbacks in searching - we should probably do this in other http places too
 * And revert changes to http mixin
 * Add signal for `ResizeWebImage` to allow informing subscribers about the actual image size / dimensions
 * Use this to update the results in the UI with their details
 * Tidy some logging
 * Add spacing for FlowBox
 * Add progress bar for querying the various plugins, feels nicer UX
 * Move to using a Dialog (with headerbar support)
 * Real-time preview resizing now supported
 * Improve layout / padding a bit
 * Still no actual saving though
 * Use config proxies
 * Write out as many covers as we need (different directories etc)
 * Better configuration setup
 * Optionally save original image named appropriately (needs prefs UI)
 * If we don't, use a slightly lower JPEG quality
 * Pass on content-type info from downloader
 * Make multiple destination use cases _copy_ the image, rather than re-encode multiple times potentially
 * Fix resizing to adjust widget dimensions better
Was sometimes raising on shutdown.
# Conflicts:
#	quodlibet/quodlibet/ext/covers/discogs.py
#	quodlibet/quodlibet/ext/covers/lastfm.py
#	quodlibet/quodlibet/util/cover/http.py
...too noisy for little benefit
 * Be smarter about "artist" in API queries
 * Return grouped (by group key by plugin) songs from `CoverManager.search`
 * When nothing found, raise a dialog containing details of searches, then destroy.
 * Handle compilations better by using "various artists" etc
 * Add very basic test
@declension
Copy link
Member Author

@declension declension commented Mar 18, 2018

Seems that Ubuntu 16.04 has GTK < 3.18, 😞 - models with FlowBox need 3.18

 * This is [only supported on 3.18+](https://lazka.github.io/pgi-docs/Gtk-3.0/classes/FlowBox.html#Gtk.FlowBox.bind_model)
 * This doesn't seem to include Ubuntu 16.04, and probably other distros.
@declension declension requested a review from lazka Mar 18, 2018
@lazka
Copy link
Member

@lazka lazka commented Mar 18, 2018

16.04 has 3.18.9 oh you meant the method, never mind

@declension
Copy link
Member Author

@declension declension commented Mar 18, 2018

16.04 has 3.18.9

OK thanks. Hmm, but that's weird then:

E       AttributeError: 'FlowBox' object has no attribute 'bind_model'

from that 16.04 build.

@lazka
Copy link
Member

@lazka lazka commented Mar 18, 2018

Hm, right, confirmed in a VM. I guess something was fixed later: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=816461

@declension
Copy link
Member Author

@declension declension commented Mar 19, 2018

Thanks. I guess I'll leave it with the older-compatible code for now. It does feel quite fast the FlowBox though

 * Filter out small images (TODO: preferences for this, one day)
 * Only ask for a few results (faster to send, load, process). First few are normally the best anyway
 * Use the `cover_image` field directly, rather than a second network round-trip. Not sure why we weren't using this all along, especially as the one at `resouce_url` is unreliable (images listed with sizes but without URLs a _lot_)
 * Better logging for processing of images
@declension declension merged commit 62a4481 into master Mar 21, 2018
8 checks passed
8 checks passed
ci/circleci: job.fedora26 Your tests passed on CircleCI!
Details
ci/circleci: job.ubuntu16.04 Your tests passed on CircleCI!
Details
ci/circleci: job.ubuntu17.10 Your tests passed on CircleCI!
Details
ci/circleci: job.win32 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@declension declension deleted the unify-cover-art-downloading branch Mar 21, 2018
@lazka
Copy link
Member

@lazka lazka commented Mar 22, 2018

Nice work!

@declension
Copy link
Member Author

@declension declension commented Mar 22, 2018

Thanks @lazka 😄 ... took much longer than I was hoping but think it's good progress.

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

Successfully merging this pull request may close these issues.

None yet

2 participants