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

Search for artists from dialog #487

Merged
merged 5 commits into from Sep 11, 2016

Conversation

@rahul-raturi
Copy link
Contributor

commented Aug 20, 2016

Allow searching for artists from a dialog. I'm not sure whether it will be super useful, but it's better for consistency with other two dialog which are Album (PR: #486) and Track (PR: #475). This dialog displays basic information about the artist, and provides an option to look him/her up in web browser.

Screenshot

screenshot from 2016-08-20 15-55-42

Note

This PR is based on rebase of #486. It would be helpful if review upto commit 851b581 be provided there.

@rahul-raturi rahul-raturi force-pushed the rahul-raturi:embed_search_artist branch from 1ccd445 to 363db2a Aug 23, 2016

@zas

This comment has been minimized.

Copy link
Collaborator

commented Sep 6, 2016

@rahul-raturi : can you rebase this branch ?

@rahul-raturi

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2016

@zas, Yes, working on it.

@rahul-raturi rahul-raturi force-pushed the rahul-raturi:embed_search_artist branch from 363db2a to 0253b71 Sep 7, 2016

@rahul-raturi rahul-raturi force-pushed the rahul-raturi:embed_search_artist branch from 0253b71 to 8d2934c Sep 7, 2016

Change accept event according to dialog
For example, for an artist dialog, loading into picard is not suitable.
Each child class of `SearchDialog` will provide an `accept_event` method,
which will call appropriate handler function for that event.
@rahul-raturi

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2016

Rebased.

self.tagger.xmlws.find_artists(self.handle_reply,
query=text,
search=True,
limit=25)

This comment has been minimized.

Copy link
@zas

zas Sep 8, 2016

Collaborator

Use QUERY_LIMIT here

This comment has been minimized.

Copy link
@rahul-raturi

rahul-raturi Sep 10, 2016

Author Contributor

Done, squashed into 939f44a.

@zas

This comment has been minimized.

Copy link
Collaborator

commented Sep 8, 2016

This PR looks fine to me.

I think we should implement paging in those dialogs, so the user can retrieved next 25 entries etc...
We can add offset keyword param to https://github.com/rahul-raturi/picard/blob/8d2934c063be3eeaa7032c6b663b836fed417a5b/picard/webservice.py#L457 (in another PR).

@zas

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2016

@rahul-raturi : i also noticed an annoying thing, search of mbids doesn't work anymore !

If you paste a text (or url) with mbid in Search field, it should either open the matching entity page in browser, or load the release in Picard (release mbids).
Related code doesn't seem to be executed anymore.
See

def mbidLookup(self, string, type_):
and following lines.

Can you fix this issue please ?

@rahul-raturi rahul-raturi force-pushed the rahul-raturi:embed_search_artist branch from 8d2934c to 939f44a Sep 10, 2016

Setup artist search dialog
The dialog displays basic information in search results, like gender,
type etc.  The artist can be looked up in browser after selection.
Skip search dialogs for mbid lookups
If mbid is of a release, directly load it into Picard, else open web
browser for more details.

@zas zas merged commit cb9b629 into metabrainz:master Sep 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rahul-raturi

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2016

I think we should implement paging in those dialogs, so the user can retrieved next 25 entries etc...

I'd discussed this with @mwiencek once. What I came to know is search results tend to be mostly irrelevant below this limit. The user may rather change the search query. But if it's necessary to show more results, we can either:

  1. Provide predefined limits from a combobox.
  2. Paging as you suggested. (I haven't checked whether it's possible with qt).

Suggestions?

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