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 releases from within a Picard dialog #486

Merged
merged 18 commits into from Sep 5, 2016

Conversation

@rahul-raturi
Copy link
Contributor

commented Aug 15, 2016

This PR is a continuation of #475.

How to trigger the dialog?

Two ways:

  1. Directly type in the query in the search box at top right corner, selecting Album from drop down menu (assuming builtin search is enabled, from options -> user interface).
  2. Select a cluster, display additional options by right clicking on it, and choose Search for similar albums... from them.

Screenshot

screenshot from 2016-08-15 17-08-09

@rahul-raturi rahul-raturi force-pushed the rahul-raturi:embed_search_release branch 2 times, most recently from 28b6e68 to 55efe28 Aug 15, 2016

def label_info_from_node(node):
labels = []
catalog_numbers = []
if node.count != "0":
if len(node.children) != 0:

This comment has been minimized.

Copy link
@zas

zas Aug 15, 2016

Collaborator

if node.children is more pythonic

@@ -374,7 +387,7 @@ def release_to_metadata(node, m, album=None):
m['barcode'] = nodes[0].text
elif name == 'relation_list':
_relations_to_metadata(nodes, m)
elif name == 'label_info_list' and getattr(nodes[0], 'count', '0') != '0':
elif name == 'label_info_list' and len(nodes[0].children) != 0:

This comment has been minimized.

Copy link
@zas

zas Aug 15, 2016

Collaborator

and nodes[0].children

@rahul-raturi rahul-raturi force-pushed the rahul-raturi:embed_search_release branch from 18cf5e5 to 024a625 Aug 16, 2016

# Extracts iso_3166_1_codes for all countries in `release_event_list`
if "release_event_list" in node.children:
country = []
for re in node.release_event_list[0].release_event:

This comment has been minimized.

Copy link
@mwiencek

mwiencek Aug 17, 2016

Member

It's potentially confusing when you clobber module names (re is imported into this file). Just use release_event.

@@ -267,6 +267,7 @@ def contextMenuEvent(self, event):
if can_view_info:
menu.addAction(self.window.view_info_action)
menu.addAction(self.window.browser_lookup_action)
menu.addAction(self.window.albums_search_action)

This comment has been minimized.

Copy link
@mwiencek

mwiencek Aug 17, 2016

Member

albums here is plural but the class is named AlbumSearchDialog. I think it should be singular here too.

@@ -382,6 +382,10 @@ def create_actions(self):
self.browser_lookup_action.setEnabled(False)
self.browser_lookup_action.triggered.connect(self.browser_lookup)

self.albums_search_action = QtGui.QAction(icontheme.lookup('system-search'), _(u"Search similar albums..."), self)

This comment has been minimized.

Copy link
@mwiencek

mwiencek Aug 17, 2016

Member

I realized this sounds weird (affects the track one too). It should be Search for similar albums....

_("Date"),
_("Country"),
_("Label"),
_("Catalog#"),

This comment has been minimized.

Copy link
@mwiencek

mwiencek Aug 17, 2016

Member

In the cdlookup dialog we use Catalog #s.

_("Tracks"),
_("Date"),
_("Country"),
_("Label"),

This comment has been minimized.

Copy link
@mwiencek

mwiencek Aug 17, 2016

Member

_("Labels").

@mwiencek

This comment has been minimized.

Copy link
Member

commented Aug 17, 2016

If I use this on a cluster, it shows results, but the search input is empty.

Another weird thing is that it lets me use it on the "Unmatched Files" cluster. That causes an exception. I don't think it makes sense to allow it, anyway.

@rahul-raturi rahul-raturi force-pushed the rahul-raturi:embed_search_release branch 5 times, most recently from 1f5a702 to d9fae78 Aug 18, 2016

@mwiencek

This comment has been minimized.

Copy link
Member

commented Aug 20, 2016

A couple more issues:

Pressing enter in the search input doesn't trigger a search.

If a search returns zero results, it spins forever and I get this stack trace:

Traceback (most recent call last):
  File "./picard/webservice.py", line 317, in _process_reply
    self._handle_reply(reply, request, handler, xml, refresh)
  File "./picard/webservice.py", line 306, in _handle_reply
    handler(document, reply, error)
  File "./picard/ui/searchdialog.py", line 550, in handle_reply
    self.zero_results()
AttributeError: 'AlbumSearchDialog' object has no attribute 'zero_results'
@rahul-raturi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2016

What should be the title of these dialogs. Like, "Album Search Dialog" or "Album Search Results". For the track dialog, I've kept "Track Search Results", but it should be uniform amongst all.

@mwiencek

This comment has been minimized.

Copy link
Member

commented Aug 21, 2016

"Album Search Results" sounds fine to me.

self.search_results.append((track, node))
else:
# This handles the case when no release is associated with a track
# i.e. the track is a NAT
# i.e. the track is an NAT

This comment has been minimized.

Copy link
@mwiencek

mwiencek Aug 23, 2016

Member

'a NAT' was correct

This comment has been minimized.

Copy link
@rahul-raturi

rahul-raturi Aug 24, 2016

Author Contributor

I thought we pronounce it as "an-a-tee". Not the right way?

This comment has been minimized.

Copy link
@mwiencek

mwiencek Aug 24, 2016

Member

Haha, good point, there isn't a right or wrong way. I say gnat (if it was N.A.T. I'd pronounce it as an initialism). But since it's your writing you can keep it how you like.

track = Metadata()
recording_to_metadata(node, track)
track["album"] = _("Standalone Recording")
self.search_results.append((track, node))

def load_selection(self, row):
"""Loads album corresponding to selected track.
"""Load album corresponding to selected track.

This comment has been minimized.

Copy link
@mwiencek

mwiencek Aug 23, 2016

Member

While you're fixing the grammar, it should be "Load the album corresponding to the selected track."

@mwiencek

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

Pressing enter to perform a search still doesn't work. It looks great to me otherwise, but we should really fix that as a usability issue first.

@rahul-raturi rahul-raturi force-pushed the rahul-raturi:embed_search_release branch 2 times, most recently from e892a16 to 66a1524 Aug 24, 2016

@rahul-raturi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2016

@mwiencek, have a look at the branch now. I've included search on return.

@rahul-raturi rahul-raturi force-pushed the rahul-raturi:embed_search_release branch from bbcf58f to 90a5340 Aug 27, 2016

_("Cover")
]

_coverart_column = 12

This comment has been minimized.

Copy link
@zas

zas Aug 29, 2016

Collaborator

You should initialize this one using length of the table_headers list, in __init__() or even drop it and use the length of the list directly since it is used only twice.

else:
query_str = query["release"]

query["limit"] = 25

This comment has been minimized.

Copy link
@zas

zas Aug 29, 2016

Collaborator

I would put this 25 in a constant, at top of this file (if not used elsewhere).

@mwiencek

This comment has been minimized.

Copy link
Member

commented Aug 29, 2016

in the dialog works great now. :) It's a :shipit: from me if you can fix those nits from zas.

@@ -101,7 +101,7 @@ def _search(self, type_, query, adv=False):
if self.mbidLookup(query, type_):
return True
params = {
'limit': 25,
'limit': QUERY_LIMIT,

This comment has been minimized.

Copy link
@zas

zas Sep 3, 2016

Collaborator

This file is lacking from picard.const import QUERY_LIMIT at top, leading to an error

@zas

This comment has been minimized.

Copy link
Collaborator

commented Sep 3, 2016

When doing a search for an album, selected release is loaded in Picard only if i double click, but using the "Load in Picard" button has no effect.

rahul-raturi added 15 commits Aug 14, 2016
Don't use count element...
... for purpose of extracting label information. Reason being, `count`
element isn't present in `label_info_list` element of search xml.
Checking for length of children has same effect as with checking count
element.
Setup album search dialog
The dialog is somewhat analogous to track search dialog. Some noticeable
points are:

* Allow searching for albums from clusters, by right clicking.
* Move save and restore state/size logic to individual classes, from parent
class `SearchDialog`. Both dialogs have different number of columns, and
elements.
Minor fixes: Use named tuple in album dialog ...
... and replace `len(list)` with just `list` for testing whether it's
empty or not, as it's more pythonic.
Display query when searching cluster
Also, update comments to be more specific and concise.
Check for label element's children
In some nodes, empty `label` element may exist. In such cases, exception
is thrown when trying to access label name.
Update advanced syntax setting instantly...
... with checkbox state changes.Updating the state and reopening the dialog
for it to actually affect the query seems counter intuitive. Also, no need to
save state when dialog closes, as the setting will already be updated.
Update comments and remove unnecessary ones
Also add copyright information.
Signed-off-by: Rahul Raturi <rhlrtr44@gmail.com>
Trigger search action on pressing return
Some noticeable points:
* Disable search action when query field (search box) is empty.
* Return also triggers accept event. To avoid this, each time focus moves
out of table, disable the accept button. This would allow searching
without the dialog getting closed.
Table may not exist after reply is recieved
If that's the case, do nothing. This may happen if user starts another
request before existing one is finished.
@rahul-raturi

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2016

@zas, Thanks for the pointers. The reason the load button isn't working anymore is that it is disabled as soon as focus moves out of the table. This was to capture enter key and perform the search without the dialog getting closed (default behaivour is to trigger accept action on pressing enter, leading to dialog getting closed).
I'll try a different approach to avoid this.

@rahul-raturi rahul-raturi force-pushed the rahul-raturi:embed_search_release branch from 00c29ac to 3f114fd Sep 5, 2016

Define query limit as constant
25 is the standard limit for number of results returned.
Try capturing focus in event rather than out
The accept button needs to be disabled to allow searching on pressing
return. If not disabled, the dialog closes on return. In commit a44c375,
focus out event of the table was used to disable the load button. This
doesn't works as the button will be disabled when user moves the mouse
towards the button (focus moves out of the table).
To avoid this, capture focus in event of `search_edit` widget.

@zas zas merged commit 09df7f8 into metabrainz:master Sep 5, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.