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

Searching tracks and displaying similar tracks in a dialog box. [WIP] #475

Merged
merged 98 commits into from Aug 15, 2016

Conversation

@rahul-raturi
Copy link
Contributor

commented Jun 14, 2016

Part of my GSoC project. Main motive is to avoid browser lookup, by displaying the Tracks in a separate dialog box, and then loading them from there.

Accessing the dialog box

There are two ways:

  1. Add a file in Picard. Right click on it, either before or after automatic tagging (when the file moves to the right pane), and select Search similar tracks....
  2. Searching a track in the search box at top right corner of Picard: Enable the dialog search for this. I've created a checkbox for this in "Advanced -> User Interface -> Use builtin search". After that has been checked, select Track from drop down menu, and type in the query. Results will be displayed in the dialog from where a release can be loaded back.

Performing a search within a dialog

If the displayed results are not relevant, one can enhance the search by putting in a different query. There's also an option to enable/disable advance query syntax in the dialog. Even without the advance query syntax, the dialog can produce pretty accurate search results (same as that of Musicbrainz) because of the dismax query parser. It allows to search among multiple fields without specific syntax. Check the screenshot for demo.

Screenshot of dialog box

track_search_dialog

@rahul-raturi rahul-raturi force-pushed the rahul-raturi:embed_search branch 3 times, most recently from c8598c4 to 7d28d2c Jun 16, 2016

self.setLayout(layout)

def search(self):
text = self.search_edit.text()

This comment has been minimized.

Copy link
@zas

zas Jun 20, 2016

Collaborator

Useless temp var ;)


def __init__(self, parent=None):
QtGui.QTableWidget.__init__(self, 0, 7)
self.setHorizontalHeaderLabels([_("Name"), _("Length"),

This comment has been minimized.

Copy link
@zas

zas Jun 20, 2016

Collaborator

Since those are translatable, i would put one per line. It is simpler to review/diff/po update any change.

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

self.more_results_action = QtGui.QAction(_(u"Display more results"), self)
self.more_results_action.setStatusTip(_(u"Display more results"))

This comment has been minimized.

Copy link
@zas

zas Jun 20, 2016

Collaborator

Please do:

title = _(u"Display more results")
self.more_results_action = QtGui.QAction(title, self)
self.more_results_action.setStatusTip(title)

If this is intended to have the same translatable string for action and status tip.

This comment has been minimized.

Copy link
@rahul-raturi

rahul-raturi Jun 21, 2016

Author Contributor

No it's not intended to be the same (that'll not be a good status tip :D ). I had put it temporarily, so thanks for the reminder. I'll update it.

QtGui.QDialog.accept(self)
except AttributeError:
self.save_state(False)
QtGui.QDialog.accept(self)

This comment has been minimized.

Copy link
@zas

zas Jun 20, 2016

Collaborator

QtGui.QDialog.accept(self) is at the end of both try and except blocks, any particular reason to not move it outside ?

types_list.append(sec.secondary_type[0].text)
types = "+".join(types_list)

result = (rec_id, rel_id, rg_id, rec_title, artist, length,

This comment has been minimized.

Copy link
@zas

zas Jun 20, 2016

Collaborator

I would create a SearchResult object instead of using a long tuple.

@mwiencek

This comment has been minimized.

Copy link
Member

commented Jun 28, 2016

Here are some current issues I see, which you might be aware of:

  • You should also be able to see more results if a lookup fails to match anything.
  • "Display more results" should have an ellipsis at the end to indicate it'll open a dialog that needs input.
  • "Display more results" needs an icon next to it. I think we can just reuse the tango magnifying glass that the search uses.
  • The loading icon under "Fetching results..." needs transparency. At least on my machine, the window is gray and the icon background is white.
  • The search box in the dialog is empty when you first open it. It should contain the query that was used to perform the search, so the user can modify it if they want.
  • There's a lot of unnecessary padding all around the table (left and right sides aren't aligned with the search box).
  • The table columns aren't sortable when you click on the column headers.
  • The "Ok" button needs a more descriptive name. In your proposal you have "Load into Picard," which is a better name.
  • The "Ok" button doesn't do anything in d98490c other than close the dialog. I'm pretty sure it loaded the release before, so I guess this is a new bug.
@rahul-raturi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2016

@mwiencek, I've fixed the issues. Have a look whenever you find time. I've left sorting when clicked on column title for now.

@mwiencek

This comment has been minimized.

Copy link
Member

commented Jul 4, 2016

Looks a lot better! The search box in the dialog is still always empty for me, though. Another (minor) problem is that if you load a track that doesn't really match the current file metadata, the file will end up in "Unmatched Files" once the release loads. We should override that and always associate the file with the track that was selected.

The other issues I mentioned seem resolved. Being able to search for things in the unmatched cluster is really useful, I think.

rg_id = rg.id
types_list = []
if "primary_type" in rg.children:
types_list.append(rg.primary_type[0].text)

This comment has been minimized.

Copy link
@mwiencek

mwiencek Jul 4, 2016

Member

Those can be translated (same for secondary types below, but using release_group_secondary_type as the context).

from picard.i18n import ugettext_attr
# ...
types_list.append(ugettext_attr(rg.primary_type[0].text, 'release_group_primary_type'))

This comment has been minimized.

Copy link
@rahul-raturi

rahul-raturi Jul 4, 2016

Author Contributor

Can you explain this part a bit? Should I do the same for country and rec_title?

This comment has been minimized.

Copy link
@mwiencek

mwiencek Jul 4, 2016

Member

There's a fixed list of types that get extracted from the MB database and translated by people on transifex. The webservice always returns the English name (as it exists in the database), so we use that as the gettext key.

Yeah, for country we should use ugettext_countries. The recording title is a variable and arbitrary string, so not for rec_title.

release_type=types)
self.search_results.append((track, node))

except AttributeError:

This comment has been minimized.

Copy link
@mwiencek

mwiencek Jul 4, 2016

Member

Handling this via exception makes the control flow harder to follow (where is the exception thrown?) and seems strange since we know when it should happen (not (hasattr(node, 'release_list') and node.release_list) right?).

artist=artist,
length=length,
title=rec_title,
release="(Standalone Recording)")

This comment has been minimized.

Copy link
@mwiencek

mwiencek Jul 4, 2016

Member

If that's displayed to the user, _("(Standalone Recording)").

for name, value in kwargs.items():
if name == 'limit':
if name in ('limit'):

This comment has been minimized.

Copy link
@mwiencek

mwiencek Jul 4, 2016

Member

I think the intention here was to construct a tuple, which would be ('limit',). It still works currently because 'limit' in 'limit' is True.

@mwiencek

This comment has been minimized.

Copy link
Member

commented Jul 5, 2016

The search box doesn't seem to handle having advanced query syntax enabled. It should show the actual lucene query in that case (to allow the user to modify the lucene query directly), not just the title.

@mwiencek

This comment has been minimized.

Copy link
Member

commented Jul 5, 2016

It would be nice if, when the search request fails (due to being rate limited or something), there'd be a "Retry" button within the dialog, so that you don't have to close and re-open it. Maybe something to work on after everything else is finished.

from picard.i18n import ugettext_attr


class Track(object):

This comment has been minimized.

Copy link
@mwiencek

mwiencek Jul 5, 2016

Member

Hmm. Any reason to not just use dicts since this is just a wrapper around kwargs?

This comment has been minimized.

Copy link
@rahul-raturi

rahul-raturi Jul 6, 2016

Author Contributor

No specific reason. I thought I might be adding some methods to it and it seem more apt then using a long tuple at that time. But I guess dict would be more suitable for this purpose (for now).

else:
date = None
if "country" in release.children:
country = release.country[0].text

This comment has been minimized.

Copy link
@mwiencek

mwiencek Jul 5, 2016

Member

I think this should be ugettext_countries(release.country[0].text) (let me know if the gettext stuff is still confusing).

This comment has been minimized.

Copy link
@rahul-raturi

rahul-raturi Jul 5, 2016

Author Contributor

Wouldn't passing countries through context in ugettext_attr work (as there's no ugettext_countries in i18n.py)? Like ugettext_attr(country, "countries").

(let me know if the gettext stuff is still confusing).

Yeah, it is a bit confusing. Can you provide the list of attributes that get translated. Also, how context affects the translation. Thanks. :)

This comment has been minimized.

Copy link
@mwiencek

mwiencek Jul 5, 2016

Member

Wouldn't passing countries through context in ugettext_attr work (as there's no ugettext_countries in i18n.py)? Like ugettext_attr(country, "countries").

According to the code ugettext_countries is a builtin, so you don't need to import anything to use it.

You can't use ugettext_attr for countries, since it's not the context that differs. They're actually in completely different domains (i.e. sets of strings). po/attributes/attributes.pot and po/countries/countries.pot are the templates for those domains (you can open those files in a text editor to see all the strings that get translated).

Context is mainly used when a word can produce different translations depending on how it's used. For example, in musicbrainz-server we disambiguate between l('Series', 'singular') and l('Series', 'plural'), since those words are the same in English but not in other languages.

This comment has been minimized.

Copy link
@rahul-raturi

rahul-raturi Jul 6, 2016

Author Contributor

One more thing, as only country code is displayed in the dialog, would it help translating that (I hadn't thought of it when I first asked about translating country names).

I noticed that country name could be extraced from release_event_list element. So, maybe translated country name can be displayed as a tooltip. Any thoughts about that? Also, is the country element in release generated from name in release_event -> area?

This comment has been minimized.

Copy link
@mwiencek

mwiencek Jul 6, 2016

Member

Oh, good point. The country code shouldn't be translated, but I like the tooltip idea. I also didn't pay attention to which XML element you were using. The country element only exists for backwards-compatibility, since releases used to only have one date/country pair. So ideally we'd show a comma-separated list of all the country codes/names.

This comment has been minimized.

Copy link
@rahul-raturi

rahul-raturi Jul 7, 2016

Author Contributor

So, I should be using the iso-3166-1-code element, right? And how to handle the case when there are multiple release-event elements in release-event-list. Should I display the first one only (I guess country element is fetched from that too).

I couldn't find a recording with multiple release events. Do you know of one?

This comment has been minimized.

Copy link
@mwiencek

mwiencek Jul 11, 2016

Member

Yeah, use iso-3166-1-code for the country. You can see an example of a release with multiple events at http://musicbrainz.org/release-group/de208292-8db5-3aed-a14a-b37a84d8c521 (2014 mono remaster, barcode 5099963379815). On the website there, we just display each event on a separate line. We could do the same in Picard, or perhaps just display the earliest release date with the countries as a comma-separated list ("2014-09-08", "XE, GB, US"). I don't think the type of accuracy on the website really matters in this dialog. But if it's easy to display them all on separate lines, that would match the website better.

@mwiencek

This comment has been minimized.

Copy link
Member

commented Jul 5, 2016

It shows the lucene query now (good!) but you can't edit/search again with it, since it produces an invalid request (query=track:(track:(...). That string should always be passed through unchanged as the entire URL query parameter, even if advanced query syntax is disabled (since you can't search individual fields in that case anyway).

@rahul-raturi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2016

Should I move the advance query syntax check option to the dialog, alongside search button? As it is going to be used much frequently than earlier, and also useful for other two remaining dialogs. Thoughts?

@mwiencek

This comment has been minimized.

Copy link
Member

commented Jul 6, 2016

Should I move the advance query syntax check option to the dialog, alongside search button? As it is going to be used much frequently than earlier, and also useful for other two remaining dialogs. Thoughts?

That sounds useful. Maybe underneath the search box if there isn't enough space. And I would leave it in the main options too. Something like Use advanced query syntax (help) where "help" is a link to the documentation would be nice.

@rahul-raturi rahul-raturi changed the title Searching tracks and/or display similar tracks in Picard, in a dialog box. [WIP] Searching tracks and displaying similar tracks in a dialog box. [WIP] Jul 8, 2016

@rahul-raturi rahul-raturi force-pushed the rahul-raturi:embed_search branch 2 times, most recently from 8768d73 to d01b858 Jul 13, 2016

@mwiencek

This comment has been minimized.

Copy link
Member

commented Jul 15, 2016

Checkbox looks a bit weird for me:
Image of syntax help checkbox

Is there a way to space those better and add parentheses around "Syntax Help"?

If someone unchecks that, I think it should replace the contents of the input with the original title. And if someone checks/enables it, it should add all the additional lucene fields (using the current contents of the box as the title field).

@rahul-raturi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2016

Checkbox looks a bit weird for me:

I'm not sure why it's happening. I've set the spacing (https://github.com/metabrainz/picard/pull/475/files#diff-ed9fa5f3b6d9a30e990c5ad2a1279974R79) properly I guess. And it is looking normal to me (aside from brackets) (have a look at screenshot in first comment).

self.adv_syntax_help = QtGui.QLabel(self.adv_opt_row_widget)
self.adv_syntax_help.setOpenExternalLinks(True)
self.adv_syntax_help.setText(_(
"(<a href='https://musicbrainz.org/doc/Indexed_Search_Syntax'>"

This comment has been minimized.

Copy link
@mwiencek

mwiencek Jul 20, 2016

Member

The spacing issue I mentioned is fixed if I add &#160; before the opening parentheses.

@mwiencek

This comment has been minimized.

Copy link
Member

commented Jul 22, 2016

This seems really close to being mergeable...anyone from the @metabrainz/picard team care to look? :)

def handle_reply(self, document, http, error):
if error:
error_msg = _("Some network error occurred. Check debug logs for more details.<br>"
"Hit `Retry` or try a different query."

This comment has been minimized.

Copy link
@mwiencek

mwiencek Jul 22, 2016

Member

This should be using either ASCII double quotes ("Retry") or Unicode ones (“Retry”).

@rahul-raturi rahul-raturi force-pushed the rahul-raturi:embed_search branch from cf61ce8 to d1f5162 Aug 1, 2016

Remove unnecessary import
`artist_credit_from_node` is not required as `recording_to_metadata`
uses it internally.
self.adv_opt_row_layout.setContentsMargins(1, 1, 1, 1)
self.adv_opt_row_layout.setSpacing(1)
self.use_adv_search_syntax = QtGui.QCheckBox(self.adv_opt_row_widget)
self.use_adv_search_syntax.setText(_("Use advance query syntax"))

This comment has been minimized.

Copy link
@mineo

mineo Aug 4, 2016

Member

"advanced".

self.parent.search(self.search_edit.text())

def restore_checkbox_state(self):
self.use_adv_search_syntax.setChecked(config.setting["use_adv_search_syntax"])

This comment has been minimized.

Copy link
@mineo

mineo Aug 4, 2016

Member

There's no need to shorten "advanced" in the option name.

This comment has been minimized.

Copy link
@mineo

mineo Aug 4, 2016

Member

Whoops, sorry, I didn't realize this is an already existing options (I think I've never used one of the search functions other than Lookup in Picard ;-) ). Ignore the above comment

@@ -674,7 +679,13 @@ def search(self):
"""Search for album, artist or track on the MusicBrainz website."""
text = self.search_edit.text()
type = self.search_combo.itemData(self.search_combo.currentIndex())
self.tagger.search(text, type, config.setting["use_adv_search_syntax"])
if config.setting["builtin_search"]:
if type == "track":

This comment has been minimized.

Copy link
@mineo

mineo Aug 4, 2016

Member

This means that searching for anything other than a track via the search bar in the top-right corner does nothing at all. The other searches should either fall back on opening the browser and the option indicate this ("when searching for tracks") or be greyed out.

This comment has been minimized.

Copy link
@rahul-raturi

rahul-raturi Aug 5, 2016

Author Contributor

Please ignore this for now. I'll open two more PRs just after this one's merged, for album and artist dialog. All types would be covered after those. (Should've mentioned this in a comment.)

}
if config.setting["use_adv_search_syntax"]:
# Display the query in advance syntax format.
query_str = ' '.join(['%s:(%s)' % (item, value) for item, value in query.iteritems() if value])

This comment has been minimized.

Copy link
@mineo

mineo Aug 4, 2016

Member

value needs to be passed through _escape_lucene_query from picard.webservice here. Otherwise, the query string that's set in the search box could contain unescaped special characters.


def network_error(self, reply, error):
error_msg = _("<strong>Following error occurred while fetching results:<br></strong>"
"Network request error for %s: %s (QT code %d, HTTP code %s)<br>" % (

This comment has been minimized.

Copy link
@mineo

mineo Aug 4, 2016

Member

Feel free to add some more newlines to this message. The URLs generated are very long and the errorString can potentially include the whole query sent to the server again if the query could not be parsed. Having all of that on one line with just a bit of wrapping depending on the window size looks a bit ugly.

retry_params[1] -- Search query information
-- Can be a text string, or a File object
"""
self.retry_params[0](self.retry_params[1])

This comment has been minimized.

Copy link
@mineo

mineo Aug 4, 2016

Member

Please use a namedtuple instead of magic tuples (we already do so in picard.script).


if self.file_:
# Sort the results by comparing them to original metadata tags
tmp = sorted((self.file_.orig_metadata.compare_to_track(

This comment has been minimized.

Copy link
@mineo

mineo Aug 4, 2016

Member

If you rename tmp to sorted_results, everyone knows what it is and you can get rid of the comment above.

I'd also format this a bit differently to make it clear which argument belongs to which function (although the first line now looks ugly):

            sorted_results = sorted(
                (self.file_.orig_metadata.compare_to_track(
                    track,
                    File.comparison_weights)
                 for track in tracks),
                reverse=True,
                key=itemgetter(0))


def main():
scriptdir = os.path.dirname(os.path.abspath(__file__))
topdir = os.path.abspath(os.path.join(scriptdir, ".."))
resourcesdir = os.path.join(topdir, "resources")
qrcfile = os.path.join(resourcesdir, "picard.qrc")
images = [i for i in find_files(resourcesdir, 'images', '*.png')]
images = [i for i in find_files(resourcesdir, 'images', ['*.png', '*.gif'])]

This comment has been minimized.

Copy link
@mineo

mineo Aug 4, 2016

Member

Please list gif before png to keep the list in ascending order.

self.show_error(error_msg)

def accept(self):
if getattr(self, "table", None):

This comment has been minimized.

Copy link
@mineo

mineo Aug 4, 2016

Member

To let you know which song is currently stuck in my head:

I need to tell you something
I really really really really really really

don't like these getattr calls ;-) Why not set table to None in __init__? It looks like the code needs to deal with table being None in any case because add_widget_to_center_layout can set it to that.

if self.file_:
# Search is performed for a file
# Have to move that file from its existing album to the new one
if type(self.file_.parent).__name__ == "Track":

This comment has been minimized.

Copy link
@mineo

mineo Aug 4, 2016

Member

You should use if isinstance(self.file_.parent, Track) (with the appropriate imports of course) instead.

@mineo

This comment has been minimized.

Copy link
Member

commented Aug 4, 2016

Some small things from my side, but other than making album and artist searches mysteriously not work, this looks good to me.

Improve code semantics and style
Some noticeable points:
* `save_state` can check whether table is loaded or not. No need to
check it in `accept` and `reject`. Also `table_loaded` isn't required
anymore.
* Keep file formats list in alphabatic order, in `makeqrc.py`.
* Import `Track` and use `isinstance` to check whether object belongs to
it.

@rahul-raturi rahul-raturi force-pushed the rahul-raturi:embed_search branch 2 times, most recently from 3cd214d to d9ebd73 Aug 6, 2016

rahul-raturi added 3 commits Aug 6, 2016
Fix multiple widgets appearing in center layout
This happens with displaying error. New widgets may appear without
existing being cleared. To avoid this:
1. Pass parent widget to `error_label` so it gets removed with parent.
2. Use `takeAt` rather than `itemAt` as it's more suitable for removing
widgets according to Qt docs. For reference:
http://doc.qt.io/qt-4.8/qlayout.html#itemAt and
http://doc.qt.io/qt-4.8/qlayout.html#takeAt
@mwiencek

This comment has been minimized.

Copy link
Member

commented Aug 12, 2016

I think this looks about ready to merge. Any objections from the @metabrainz/picard team to merging this this weekend so @rahul-raturi can submit the rest?

@zas

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2016

I think this looks about ready to merge. Any objections from the @metabrainz/picard team to merging this this weekend so @rahul-raturi can submit the rest

ok for me

@mwiencek mwiencek merged commit 31555ed into metabrainz:master Aug 15, 2016

1 check passed

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

This comment has been minimized.

Copy link
Member

commented Aug 15, 2016

1 aye and 0 nays then! I'm happy with how it turned out, it's really convenient. Good work @rahul-raturi

@zas

This comment has been minimized.

Copy link
Collaborator

commented on picard/ui/searchdialog.py in 022f7fe Sep 3, 2016

You should set a shortcut : QtGui.QPushButton(_("&Load into Picard")) (L as accelerator)

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