857832 - [music] Search functionality doesn't find Mötley Crüe

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Music, defect)

Description

[Marcia Knous [:marcia]]

unagi, while running Mozilla RIL using:

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/d467369d1b0c
Gaia   06e0e5ce42bdfb62bdbe38271de6b5b2d9e40e75
BuildID 20130403070204
Version 18.0

STR:
1. Load some Mötley Crüe songs on your phone
2. Invoke the search feature and type "motley"

Expected: Search would find motley
Actual: Search doesn't find motley presumable due to umlauts?

Comment 1

[Jim Porter (:squib)]

As far as I'm aware, there's not an easy way to strip diacritics from strings to compare like this. Besides that, it's not always clear what the conversion should be. "Rock dots", since they're just for show, obviously map to the unaccented character, but a German would be surprised by that, since in German, "ö" == "oe".

Comment 2

[Jim Porter (:squib)]

Question: do we do anything like this in other search contexts, e.g. contacts?

Comment 3

[Jim Porter (:squib)]

(In reply to Jim Porter (:squib) from comment #2)
> Question: do we do anything like this in other search contexts, e.g.
> contacts?

To answer my own question: we do. Although I'm not sure how contacts does this yet.

Comment 4

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Jim, are you currently working on this? If not, I would like to take it -- I was thinking of a quick fix involving string normalization (i.e. "ö" -> "o").

Comment 5

[Jim Porter (:squib)]

Go for it. I recommend looking in the communications app, which already does this normalization. I was planning on adding some common code in /shared/js/ for this.

Comment 6

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

(In reply to Jim Porter (:squib) from comment #5)
> Go for it. I recommend looking in the communications app, which already does
> this normalization. I was planning on adding some common code in /shared/js/
> for this.

Thanks for the hints, I was checking the search implementation for the Contacts app and noticed this normalization approach.

Comment 7

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Attachment: Pull Request #9641 - Normalize search strings

When performing a search, treat commonly used accented chars as their ASCII equivalents (i.e. 'ö' -> 'o'), thus finding artists like 'Mötley Crüe' with ASCII type queries like 'motley'.

Comment 8

[Dominic Kuo [:dkuo]]

Comment on attachment 747371 [details]
Pull Request #9641 - Normalize search strings

Mihai,

I took a quick look on your patch, not in detail yet because I found you have implemented a different normalizer for Music, comparing to Contacts app. As Jim said in Comment 5, /shared/js will be a good place to put codes like the normalizer utility, so if your normalizer works well then we should put in it /shared/js for the other apps to use, and try to keep all the normalize logics the same since Music is not the only one which uses normalizer.

For fixing this issue, I think at least you should try:

1. Put your normalizer in /shared/js, use it in Music and also modify Contacts to use it(or file a bug for Contacts). or
2. Move the normalizer.js from Contacts to /shared/js and use it in both Contacts and Music.

It depends on which implementation is correct/better.

Please address these issues and I will review your revised patch, thanks.

Comment 9

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

(In reply to Dominic Kuo [:dkuo] from comment #8)
> Comment on attachment 747371 [details]
> Pull Request #9641 - Normalize search strings
> 
> Mihai,
> 
> I took a quick look on your patch, not in detail yet because I found you
> have implemented a different normalizer for Music, comparing to Contacts
> app. As Jim said in Comment 5, /shared/js will be a good place to put codes
> like the normalizer utility, so if your normalizer works well then we should
> put in it /shared/js for the other apps to use, and try to keep all the
> normalize logics the same since Music is not the only one which uses
> normalizer.
> 
> For fixing this issue, I think at least you should try:
> 
> 1. Put your normalizer in /shared/js, use it in Music and also modify
> Contacts to use it(or file a bug for Contacts). or
> 2. Move the normalizer.js from Contacts to /shared/js and use it in both
> Contacts and Music.
> 
> It depends on which implementation is correct/better.
> 
> Please address these issues and I will review your revised patch, thanks.

Hi Dominic, thanks for taking a look at my patch, I also found normalizer.js from Contacts, but it doesn't seem quite complete in terms of accented chars: for example, 'ț' and 'ș', specific for Romanian (v1.0.1 language), are not there and thus will not be normalized.

Moreover it defines an object of the form utils.text._ that also includes escapeHTML() as a function, which from what I noticed is implemented/copied in every app that uses it.

I'm thinking a good approach would be to combine normalizer.js with my implementation in shared/js, but I'm not sure how to go about naming: should I keep the utils.text._ structure or just name it Normalizer?

Regarding the use of the shared/js solution in Contacts, I will simply file a new bug about it once I get your input on this. Thanks!

Comment 10

[Dominic Kuo [:dkuo]]

(In reply to Mihai Cirlanaru [:mcirlanaru] from comment #9)
> Hi Dominic, thanks for taking a look at my patch, I also found normalizer.js
> from Contacts, but it doesn't seem quite complete in terms of accented
> chars: for example, 'ț' and 'ș', specific for Romanian (v1.0.1 language),
> are not there and thus will not be normalized.

Yes, so go ahead to implement a more complete normalizer.

> Moreover it defines an object of the form utils.text._ that also includes
> escapeHTML() as a function, which from what I noticed is implemented/copied
> in every app that uses it.

Actually if you assign your original text to an temporary element,
using element.textContent then gecko will escape for you so that
you don't have to write your own escapeHTML().

> I'm thinking a good approach would be to combine normalizer.js with my
> implementation in shared/js, but I'm not sure how to go about naming: should
> I keep the utils.text._ structure or just name it Normalizer?

I think normalizer(or text_normalizer) will be fine, the utils.text._ structure
is only used in communications apps, I am not sure why that happens because
window.utils seems not a standard(I cannot find any documents on MDN), so a
clear filename with easy-to-understand code comments will be enough.

Comment 11

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Comment on attachment 747371 [details]
Pull Request #9641 - Normalize search strings

Thanks for the suggestions, Dominic!

I refactored the code, extended the map of accented-form characters to be exhaustive (i.e. include all existing such chars), created the shared text_normalizer.js in shared/js, and included the new normalizer in the Music app search functionality.

Once I get your approval on these changes, I will file a bug to use the new normalizer in Contacts as well.

Comment 12

[Dominic Kuo [:dkuo]]

Mihai,

I found there is an latin equivalentChars mapping table which I am not familiar with, so I have invited Rudy to help on reviewing your patch cause he works on keyboard app and I guess the mapping is copied from it. Let's do the technical parts on github.

Comment 13

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

(In reply to Dominic Kuo [:dkuo] from comment #12)
> Mihai,
> 
> I found there is an latin equivalentChars mapping table which I am not
> familiar with, so I have invited Rudy to help on reviewing your patch cause
> he works on keyboard app and I guess the mapping is copied from it. Let's do
> the technical parts on github.

Hi Dominic, the equivalent chars map I took from predictions.js (https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/imes/latin/predictions.js#L211), since it seems to be the most complete list I could find.

Looking forward to your review.

Comment 14

[Dylan Oliver [:doliver]]

Dominic or Rudy, do you have time to take a look at this review?

Comment 15

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Hi Dylan,

Sure, sorry this has been in my queue for a while.
Will get to it in 1 or 2 hours.

Thanks for the heads-up.

Comment 16

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 747371 [details]
Pull Request #9641 - Normalize search strings

Looks good to me.
There are some minor points commented on the pull request, but not blocking ones.

Hi Mihai,
Thanks for your great work.

Comment 17

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Thanks for the review, Rudy!

Landed on master:
https://github.com/mozilla-b2g/gaia/commit/55b3f1629e0ea673b1d38b8b30274880264b7cf8

Comment 18

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 55b3f1629e0ea673b1d38b8b30274880264b7cf8 to:
v1-train: d5de42c41805281d45defaf6699165253821dff7

Comment 19

[Dominic Kuo [:dkuo]]

There is a performance follow up, please see bug 880186.