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

Revamp cover art providers UI #411

Closed
wants to merge 17 commits into from

Conversation

Projects
None yet
3 participants
@zas
Copy link
Collaborator

commented Jul 1, 2015

  • make cover art providers more modular
  • display all providers as tabs, which can be re-ordered to change provider's excution order
  • support for provider-specific options
  • clean up the API

capture du 2015-07-01 17 57 34

@dufferzafar

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2015

@zas This looks nice. 👍

I had to add these missing imports to get the CAA Select types working.

diff --git a/picard/coverart/providers/caa.py b/picard/coverart/providers/caa.py
index e06879a..fde6885 100644
--- a/picard/coverart/providers/caa.py
+++ b/picard/coverart/providers/caa.py
@@ -30,7 +30,10 @@ from picard import config, log
 from picard.const import CAA_HOST, CAA_PORT
 from picard.coverart.providers import CoverArtProvider, ProviderOptions
 from picard.coverart.image import CaaCoverArtImage, CaaThumbnailCoverArtImage
+from picard.coverart.utils import CAA_TYPES, translate_caa_type
 from picard.ui.ui_provider_options_caa import Ui_CaaOptions
+from picard.ui.util import StandardButton
+from picard.util import webbrowser2


 _CAA_THUMBNAIL_SIZE_MAP = {
@zas

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 2, 2015

I had to add these missing imports to get the CAA Select types working.

Thanks for the report, i forgot those when i splitted initial PR in two.
Fixed in af94ab0

@@ -38,17 +38,17 @@ class CoverArtProviderCaaReleaseGroup(CoverArtProviderCaa):
"""Use cover art from album release group"""

NAME = "CaaReleaseGroup"
TITLE = N_(u"CAA Release Group")

This comment has been minimized.

Copy link
@mineo

mineo Jul 2, 2015

Member

This should be "Cover Art Archive Release Group" because the CAA provider itself uses "Cover Art Archive".

This comment has been minimized.

Copy link
@zas

zas Jul 2, 2015

Author Collaborator

It was to make the tab title not too long

@mineo

This comment has been minimized.

Copy link
Member

commented Jul 2, 2015

I'm not sure reordering by left and right arrows is a good idea because that's also how you scroll through the tabs itself (I've also not seen it used in other software before, I can only think of "Move Up" and "Move Down" buttons in Firefox' language preferences and somewhere in Libreoffice). This might not be a problem if you're using Picard in english because it uses pretty short words, but for the german translation whitelist at the moment gets translated to "Seiten auf der Whitelist" and "Cover Art Archive Release Group" would be something like "Cover Art Archive Bild der Veröffentlichungsgruppe". Simply putting these strings into the python files for testing makes scrolling arrows for the tabs appear pretty quickly.

@zas

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 2, 2015

I'm not sure reordering by left and right arrows is a good idea because that's also how you scroll through the tabs itself (I've also not seen it used in other software before, I can only think of "Move Up" and "Move Down" buttons in Firefox' language preferences and somewhere in Libreoffice). This might not be a problem if you're using Picard in english because it uses pretty short words, but for the german translation whitelist at the moment gets translated to "Seiten auf der Whitelist" and "Cover Art Archive Release Group" would be something like "Cover Art Archive Bild der Veröffentlichungsgruppe". Simply putting these strings into the python files for testing makes scrolling arrows for the tabs appear pretty quickly.

I used left/right arrows because they were available with toolbuttons, but we can replace them by normal buttons with text, or use other icons.
About long text in tab's titles ... well i guess we can't do much about it, what do you propose ?

I don't think this blocks the PR as those issues can easily be addressed at anytime.

@mineo

This comment has been minimized.

Copy link
Member

commented Jul 2, 2015

I had another look at the changes and tried using this with no Picard.conf and with the default sizes of the options pages it's not even possible to see the button for the type selection or read all the text:
blabla

I'm not sure what a good way to solve this would be. Personally, I did like the checkboxes we had before and if we had moved the per-provider options (with local cover art that's at least 2) to subpages of the general cover art page, there would be enough space for "Move up" and "Move down" buttons (or up and down arrow ones) on the general page.

@zas

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 2, 2015

I had another look at the changes and tried using this with no Picard.conf and with the default sizes of the options pages it's not even possible to see the button for the type selection or read all the text:

The default size for options dialog is quite small, i think we could increase it, today's screens have much higher resolution they had when this dialog was first written (afaik the default size wasn't modified since ages).
What do you think about increasing initial size of this dialog ?

if we had moved the per-provider options (with local cover art that's at least 2) to subpages of the general cover art page

You mean adding the providers in the tree on the left ? I thought about it, and it is possible of course.
Not sure i have the time and the will to do it now though, basically this is just UI tuning, which is fairly easy now the UI of provider's options is separated (it is a widget we can easily place anywhere).
I propose to merge the PR as is, and improve that part later.

Does the core functionality is ok for you ? (re-ordering providers)

zas added a commit to zas/picard that referenced this pull request Jul 3, 2015

Rework the provider interface
As suggested by @mineo in metabrainz#411 (comment)

- add an option subpage per provider having options
- in the main cover art options page display a list of checkboxes with up/down arrows for re-ordering
- code is simplified
- more space is available for provider-specific options
- no drag'n'drop for re-ordering

zas added a commit to zas/picard that referenced this pull request Jul 3, 2015

Add provider-specific options, make providers re-orderable
As suggested by @mineo in metabrainz#411 (comment)

- add an option subpage per provider having options
- in the main cover art options page display a list of checkboxes with up/down arrows for re-ordering
- code is simplified
- more space is available for provider-specific options
- no drag'n'drop for re-ordering
@zas

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 3, 2015

Replaced by #413

@zas zas closed this Jul 3, 2015

@zas zas deleted the zas:revamp_cover_providers branch Mar 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.