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

Album art mods #2511

Merged
merged 4 commits into from Sep 22, 2017

Conversation

Projects
None yet
3 participants
@elbeardmorez
Contributor

elbeardmorez commented Aug 2, 2017

Hi,

Here I aim to improve the Album Art Downloading experience

This is achieved (certainly it is/was for me ..but hopefully this will affect others usage positively too) via this patch set which i split into two parts. Please read the notes having looked at the pictures for context in each instance.

1. Custom filename for output
-commit: allow configurable custom files names for downloaded art

image: pre custom filename
image: post custom filename
note: the main quodlibet prefs now allows the playbar's cover image to be selected based on a custom filename ..great ..but somewhat useless to me if i name all my 'covers' 'front', and yet i cannot download via the downloader to that chosen name, for me 'front.jpg'.

So to be clear.. with the changes, I can set custom filenames for the download target, without, I cannot. That is why I wish this to be included in quodlibet.

2. Search string clarity and custom search sizes
-commit: expose album art downloader's blackbox
-commit: expose album art downloader query results limit

image: search changes
note: the changes are in red. clearly, we would now have the ability to switch between 'raw' input search string, and the 'clean' (..ed via blackbox) string as per the original, pre-commit setup

image: original search example
note: here is the original search results for a 'American Pie: The Wedding' search.
note2: do try to ignore the correct top match in this pic as at the time I did this work the quodlibet Amazon service was down, do you (Lazka) remember the poke from declension about this last month? ..that was me poking declension ..and that's where this all started! thus ONLY Discogs results were returned, they were terrible/useless and limited in number and I wanted to know why!

image: new 'raw' search
note: the original 'blackbox' / 'cleaned' search is below this 'raw' search. it is now totally obvious as to why the original search failed ..the cleaning routine for the search (the blackbox) was overzelous. now we recover better matches from Discogs, and more of them if we want!

So to be clear.. with the changes, I can get the search results I want from both cover providers, without, I cannot. That is why I wish this to be included in quodlibet. Given quodlibet is (IMO) pitched at a slightly more advanced user, I don't think they'd baulk at exposure to this extra detail either.

Further implementation notes:
-Everywhere possible I've left defaults as they were and disabled the additional features.

Configs
-My current config mods, not required in any way, but as an example..

[plugins]
cover_filenames = front.jpg,back.jpg,cd.jpg,<artist>.jpg
cover_resultsmax = 10
cover_searchraw = False

@elbeardmorez

This comment has been minimized.

Contributor

elbeardmorez commented Aug 2, 2017

Well after struggling with a 'Gtk.init_check' issue for a while, which (miraculously) disappeared all of a sudden (I was initially blindly calling newly installed py.test with '-s' param and as it turns out 's' is not for 'script', so maybe it was that) ..the failing test ran without issue. 28 others failed but that's mainly due to my env (e.g. no pyflakes etc.)

The failing test only fails on the 'debian8-py2' bot so if anyone with that env available could shed more light that would be great.

@elbeardmorez

This comment has been minimized.

Contributor

elbeardmorez commented Aug 2, 2017

Further, the error in question (caused by Gtk.init_check() failure)..

'py.test tests/test_util_copool.py'
..
..
E AssertionError: None != True

..occurs for me at random if I rapidly repeat the test.. sometimes pass, sometimes fail. Could the same be occurring on the build bot?

@lazka

This comment has been minimized.

Member

lazka commented Aug 2, 2017

yeah, a flaky test, ignore

@lazka

This comment has been minimized.

Member

lazka commented Aug 8, 2017

What are the additional features and why do you think they should be added?

@elbeardmorez

This comment has been minimized.

Contributor

elbeardmorez commented Sep 2, 2017

Apologies that this has taken me so long to come back on. Sadly, my initial reaction was that I'd given sufficient detail, specifically with commit names like:

-allow configurable custom files names for downloaded art
-move album art when renaming [#237]

but perhaps the final feature, implemented in:
-expose album art downloader's blackbox
-expose album art downloader query results limit
was a little less descriptive ..but for those there were/are commit messages too so.. ..and there was also a picture?!

So I've re-written the initial message and added many more pictures ..it does read much better than the initial attempt, and now, hopefully, even for someone who hasn't necessarily seen/used the 'Album Art Downloader' it should be enough to gauge exactly what I'm doing. The problem is, that this write up has taken me about as long it did to change the code ..and personally, that isn't a balance that I find workable.

Given that you (Lazka), are effectively a deity to me (even given my limited knowledge of what you do / have achieved in the OS community) could you humour me sufficiently to elaborate on what wasn't there originally? (lol ..difficult now I've deleted it, but maybe you recall?)

Cheers

@elbeardmorez elbeardmorez force-pushed the elbeardmorez:albumartmods branch 3 times, most recently from 4db718a to 0ebb302 Sep 2, 2017

@declension

This comment has been minimized.

Member

declension commented Sep 6, 2017

Excuse the jumping in here... I've re-read the changes and understand better now (thanks for the updates / pictures). Whilst I get reducing the number of PRs, AIUI there are definitely two separate things going on:

  • Album art plugin improvements (1 and 2 in the description ). These seem pretty reasonable to me, though a combobox and a few more defaults for the cover art name (e.g. front.jpg) might be easier actually...
  • Change of a core UI piece to allow moving art with tag renames - in both EF and QL. (item 3 in the description). There are a few UI suggestions I have and it also needs some tests.

Can I be annoying and ask you to split the second out to a separate PR? Then the first one could be merged easier, notwithstanding any comments from @lazka

@elbeardmorez

This comment has been minimized.

Contributor

elbeardmorez commented Sep 6, 2017

thanks for looking. i'll split this PR as suggested, not annoying at all, i can't argue against sound reasoning.

@elbeardmorez elbeardmorez force-pushed the elbeardmorez:albumartmods branch 2 times, most recently from ac3859d to 6d15d03 Sep 6, 2017

elbeardmorez added some commits Jul 31, 2017

expose album art downloader blackbox
the algorithm for construction of a final 'query to send'  based on user
input is imperfect. allow users to override this if the results are off
(e.g. missing words) whilst giving them insight into what's usually best.
expose album art downloader query results limit
-the current engine defaults of 5 and 3 for Amazon and Discogs respectively
 occassionally aren't enough to see the actual cover you want. this
 change allows for more results
-new overriding results limit takes the lower of current defaults - 3
-REQUEST_LIMIT_MAX is hardcoded as 15 so users can't do anything too crazy
-breaking change as it causes all engines to use the same user defined  limit

@elbeardmorez elbeardmorez force-pushed the elbeardmorez:albumartmods branch from 6d15d03 to 68452c5 Sep 19, 2017

@elbeardmorez

This comment has been minimized.

Contributor

elbeardmorez commented Sep 19, 2017

pretty sure that that current failure is nothing to do with me! i haven't changed anything since the last successful (passing) build, i only rebased the work over master HEAD thus i imagine it'll disappear the time around

@declension declension merged commit 37b62b8 into quodlibet:master Sep 22, 2017

8 checks passed

ci/circleci: job.debian8.py2 Your tests passed on CircleCI!
Details
ci/circleci: job.debian8.py3 Your tests passed on CircleCI!
Details
ci/circleci: job.ubuntu16.04.py2 Your tests passed on CircleCI!
Details
ci/circleci: job.ubuntu16.04.py3 Your tests passed on CircleCI!
Details
ci/circleci: job.ubuntu17.10.py3 Your tests passed on CircleCI!
Details
ci/circleci: job.win32.py2 Your tests passed on CircleCI!
Details
ci/circleci: job.win32.py3 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@declension

This comment has been minimized.

Member

declension commented Sep 22, 2017

Sorry about the delay. Thanks @elbeardmorez

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment