941213 - [B2G][SMS] SMS "To:" field search results do not highlight phone numbers when both letters and numbers are searched

Status: RESOLVED DUPLICATE of bug 952554

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

Description

[Brogan Zumwalt [Inactive]]

Attachment: Screenshot

Description:
When user types letters belonging to the name of a contact along with numbers of that contact's phone number into "To:" field in new SMS message, the results panel only highlights the letters. The phone number remains unhighlighted.

Repro Steps:
1) Updated Buri to Build ID: 20131120004000
2) Open Messages app
3) Start a new sms txt message
4) Type letters and numbers belonging to a contact in the "To:" field

Actual:
When both letters of a name and numbers of a phone number are present in the "To:" search field only the letters of names are highlighted.

Expected:
When both letters of a name and numbers of a phone number are present in the "To:" search field all characters present are highlighted.

Environmental Variables
Device: Buri v 1.2 COM RIL
Build ID: 20131120004000
Gecko: http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/2d454e0de2ed
Gaia: 5ec2963fff60492c840707df8d8090f9908a5251
Platform Version: 26.0
RIL Version: 01.02.00.019.102 
Firmware Version: V1.2_US_20131115

Notes:
Repro frequency: 3/3, 100%
Link to failed test case: https://moztrap.mozilla.org/manage/case/7873/
See attached: screenshot

Comment 1

[Brogan Zumwalt [Inactive]]

Issue occurs in 1.1 as well

Result: When both letters of a name and numbers of a phone number are present in the "To:" search field only the letters of names are highlighted.

Environmental Variables
Device: (example:Leo v 1.1.0 Mozilla/ COM RIL)
Build ID: 20131119041201
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/7c3cfc0936ca
Gaia: b585b32441fafa67f2b4582db23be5f3a2afab21
Platform Version: 18.1
RIL Version: 01.01.00.019.281 
Firmware Version: D300f10a

Comment 2

[Julien Wajsberg [:julienw]]

Right, this is supposed to work.

Not a blocker at all though.

Marking as a mentoring bug until we have time to look at it.

Comment 3

[Robert Patrick Damery]

Is this bug still up for grabs? If it is would you mind pointing out what files we should be working with? Still coming to graps with firefox os

Comment 4

[Julien Wajsberg [:julienw]]

Hey,

yes it is!

Likely the bug is either in:
* apps/sms/js/contacts.js
* apps/sms/js/thread_ui.js in renderContact (this part is currently being rewritten in bug 934531, so if there is bug here, you'll want to apply the patch there before working on it)

My guts tell me the bug will be in apps/sms/js/contacts.js because we should not display the 2nd result we see in the screenshot.

I'd first dig in the unit tests in apps/sms/test/unit/contacts_test.js and add a test case that looks like this bug.

I think the issue is in the postprocess algorithm we do starting line 172. We can see the "fields" variable doesn't hold "tel". That said, we may have had a good reason for not adding it back at the time, but I don't remember it.

Good luck! And don't hesitate to ask if you have any issue. For this part, you can ask :rwaldron or me on IRC.

Comment 5

[Robert Patrick Damery]

I can't seem to be able to replicate the 2nd incorrect contact issue from the screenshot? 

Adding "tel" to "fields" seems to highlight either the number or the name, whichever is typed in first, but not both at the same time.

So it seems like this is heading on the right direction. I've got to ask though, I had tried earlier to add "tel" to "fields" and it broke, and after a while of going through the code to better understand it I noticed that a lot of times even if I added a comment or a simple alert("Hello World") it would break. After reloading the page a few times it would sometimes work. 

I am using nightly firefox to emulate firefox os on an ubuntu 12.04 laptop. Is there any reason why the emulator would break like this? It's kind of hard to know if I'm heading on the right path or if I broke things when I can't safely say if it's me or the emulator that's the problem.

Comment 6

[Robert Patrick Damery]

ahhh, think I know why tel was not in there in the first place, tel does not return a string like with givenName and familyName and thus breaks when you try running trim on it. extracting the value from tel might do the trick. An answer on the emulator issues would still be great though :)

Comment 7

[Julien Wajsberg [:julienw]]

So here are a little more information.

Among the SMS app developers, we usually load the app inside Firefox with a special profile generated in "debug, no desktop shim" mode. You can do that like this, from the Gaia subdirectory:

  PROFILE_FOLDER=profile-noshims DEBUG=1 DESKTOP=0 make

And then you can run Firefox Nightly like this:

  <path to firefox nightly>/firefox --no-remote -profile ./profile-noshims http://sms.gaiamobile.org:8080

Incidentally, we also use the same setup for running the unit tests.

This setup gives a quite plain Firefox, and in this case, we load a mock of the mozContacts API and of the Mobile Messaging API. You can see them in the apps/sms/js/desktop/ directory, and you're encouraged to change them if you need it, especially desktop/contacts.js.
The problem with this is that, since it's a mock, we can have a different behavior compared to the real stuff, but this should be good enough.

What other Gaia developers are generally doing is using a "debug, with shims" profile, which mocks or enables some API using extensions. With this we can't simulate the data we want (yet), but we can simulate more things (eg: activities).

So both setups have their pros and cons.

Usually, I use the first setup in Firefox, and a real device when I do more involved things.

Another setup that works quite well these days is using the Firefox 1.3 Simulator: in Nightly, you can open the App Manager, and from here install and connect and debug apps that run inside the Simulator. This works very well these days. See [1] for more information. I don't know how to access the profile used there though, you might need to search by yourself. Don't hesitate to add information on MDN if you find some things that are not already on it.

Last but not least, you can use an emulator, but you need to build it yourself, see [2]. An emulator is like a real device, but is quite slow.

So, now, why did your code break? I'd say you were using the mock for contacts (first setup) and maybe there is something sufficiently different between the mock and the real thing. Looking to firefox' console might help. If there is something in the console, you can paste it here or on a pasting service and I'll have a look (leave me a few hours since I'll go to sleep soon ;) ).

And in the end, what works well too, is creating a unit test that reproduces the bug, check that it fails, and then you can fix the bug without even going try and reload all the time.

So, that's it for now. Tell me if you need anything else!

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Using_the_App_Manager
[2] https://developer.mozilla.org/en-US/Firefox_OS/Preparing_for_your_first_B2G_build#Configuring_the_B2G_build_for_an_emulator

Comment 8

[Robert Patrick Damery]

Ok so I fixed the bug and everything seems to be working well, problem was that the RegExp call for the number was returning the entire string, so it was never found in the individual Strings. I had to edit thread_ui.js to fix this though, and you mentioned earlier that I should apply a patch to it. Is there anyway I could apply the patch without having to do everything manually?

Once I apply the patch I'll fix things up again and I should be good to go. If you can provide me with a link or a description of how to submit a patch that would be great as well btw :)

Thanks!

Comment 9

[Robert Patrick Damery]

Attachment: Screenshot from 2013-11-22 10:39:28.png

Screenshot showing issue is fixed

Comment 10

[Julien Wajsberg [:julienw]]

You can find all information about submitting a patch at [1] and [2]. [1] is more about Gecko and [2] more about Gaia, so better take [1] as general rules and [2] as specific rules. [2] wins if there are any contradictions ;)

Basically, you open a pull request on github, and add here a plain text attachment containing the pull request's URL (you can do this easily by clicking "paste text directly" when you create an attachment).

Don't forget to do/update some unit tests, see [3] for information about how to run them.

[1] https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch
[2] https://developer.mozilla.org/en-US/Firefox_OS/Platform/Gaia/Hacking#Submitting_a_patch.
[3] https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests

And if you want some more fun about Regexp, you can have a look at bug 941763 ;)

Comment 11

[Julien Wajsberg [:julienw]]

Note: I'm not working today so I may not answer quickly. If you have any general issue about generating a patch or running unit tests, you can go and ask on IRC.

Comment 12

[Robert Patrick Damery]

Will definitely have a look at the next bug over the weekend. Just got back from college myself, I read over the links you sent me and I can't see anything about how to apply your patch? From what I understand because I had to change thread_ui.js to fix the bug I should apply your patch first before submitting mine right? How do I do this seeing as your patch is not yet merged to the main repository?

Maybe I'm getting things mixed up but isn't this your patch: https://github.com/mozilla-b2g/gaia/pull/13713 ?

I'll look into making the unit tests in the mean time :)

Comment 13

[Robert Patrick Damery]

Attachment: Fixed highlighting issue + added tel to fields for more accurate searches + updated test

I still didn't apply your patch, but I modified a test unit to provide automatic testing. Let me know if there is anything that needs to be changed besides adding your patch (Still need a bit of advice on that) If it's all good I'll move on to the bug you proposed!

Comment 14

[Julien Wajsberg [:julienw]]

The patch in bug 934531 just landed so you don't need to apply this patch manually now, you merely need to rebase your work. (I'm assuming here that you know the basic git workflows, but please tell me if any of these words are not clear to you.. or ask on IRC :) ).

For further reference, I was asking you to cherry-pick the commit in bug 934531 so that your new code would be based on it.

Will review your patch now :)

Comment 15

[Julien Wajsberg [:julienw]]

Comment on attachment 8337435 [details] [review]
Fixed highlighting issue + added tel to fields for more accurate searches + updated test

So Patrick, I feel quite sorry because I couldn't explain earlier how to cherry-pick my patch in the other bug. Because of that, a big part of your patch has conflicts now and you need to resolve them using the new ContactRenderer object. This shouldn't be too hard, as the interface looks similar to the old renderContact.

Please also install gjslint and jshint as they provide you good hints about your code.

You should also configure your code editor to use spaces instead of tabs for indentation. You can also probably integrate directly gjslint and jshint into your editor, this gives you immediate visual feedback about the code you're doing.

Thanks anyway for your patch, I'm looking forward your updated patch to fix this issue! :)

Please request review again once your pull request is updated! Please keep it in one commit for next review :)

Comment 16

[Robert Patrick Damery]

Yeah I just updated my master and I can see it's broken. I'll fix things up again no bother at all, I've been busy with college lately but will get around to doing this tomorrow, haven't forgotten about the other bug either! :)

Comment 17

[Robert Patrick Damery]

Attachment: Revised Patch for highlighting issue

Let me know if there is anything else I should do!

Comment 18

[Rick Waldron [:rwaldron]]

Comment on attachment 8340032 [details] [diff] [review]
Revised Patch for highlighting issue

I took this review to help move it along. Still have some formatting issues to address. Please r? Julien or I when you're ready—thanks!

Comment 19

[Robert Patrick Damery]

Attachment: Fixed indentation

Comment 20

[Rick Waldron [:rwaldron]]

Comment on attachment 8349124 [details] [review]
Fixed indentation

Patrick, looks like changes for two different bugs? I left some comment for minor nits to be addressed as well. Thanks!

Comment 21

[Julien Wajsberg [:julienw]]

Comment on attachment 8349124 [details] [review]
Fixed indentation

This looks good.

Please keep only the related commits for this bug, and fix the nits that we asked, and then ask review again when you're ready.

Note that we'll be away for 2 weeks, so don't worry if don't get answers from us before the January 6th.

Comment 22

[Julien Wajsberg [:julienw]]

Hey Robert Patrick, I just wanted to check out if you still work on this! Thanks :)

Comment 23

[kumar rishav (:rishav_)]

Hi Julien
Seems Robert is no longer working on this (it's long-long time, there is no update) .
If you will allow then i will update those :nits and do pull request. (then : one less bug for SMS ;) )
Thanks

Comment 24

[Steve Chung [:steveck]]

Hi Kumar, can you see this this issue still existed? Because I could not reproduce this on current master now...

Comment 25

[kumar rishav (:rishav_)]

Sorry !I didn't check before. 
Yeah steve.. this bug doesn't exist anymore.  unable to reproduce. You can close it.

Thanks

Comment 26

[Julien Wajsberg [:julienw]]

Right, it's been fixed in bug 952554.
Thanks :)