Closed
Bug 941213
Opened 11 years ago
Closed 10 years ago
[B2G][SMS] SMS "To:" field search results do not highlight phone numbers when both letters and numbers are searched
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect)
Tracking
(b2g18 affected, b2g-v1.2 affected)
RESOLVED
DUPLICATE
of bug 952554
People
(Reporter: bzumwalt, Assigned: patrick, Mentored)
Details
(Whiteboard: burirun4,)
Attachments
(2 files, 3 obsolete files)
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
Reporter | ||
Comment 1•11 years ago
|
||
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•11 years ago
|
||
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.
Whiteboard: burirun4 → [mentor=:julienw] burirun4,
Assignee | ||
Comment 3•11 years ago
|
||
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•11 years ago
|
||
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.
Assignee: nobody → patrick
Assignee | ||
Comment 5•11 years ago
|
||
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.
Assignee | ||
Comment 6•11 years ago
|
||
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•11 years ago
|
||
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
Assignee | ||
Comment 8•11 years ago
|
||
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!
Assignee | ||
Comment 9•11 years ago
|
||
Screenshot showing issue is fixed
Comment 10•11 years ago
|
||
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•11 years ago
|
||
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.
Assignee | ||
Comment 12•11 years ago
|
||
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 :)
Assignee | ||
Comment 13•11 years ago
|
||
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!
Attachment #8337435 -
Flags: review?(felash)
Comment 14•11 years ago
|
||
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 :)
Updated•11 years ago
|
Attachment #8337435 -
Attachment is patch: false
Attachment #8337435 -
Attachment mime type: text/plain → text/x-github-pull-request
Comment 15•11 years ago
|
||
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 :)
Attachment #8337435 -
Flags: review?(felash)
Assignee | ||
Comment 16•11 years ago
|
||
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! :)
Assignee | ||
Comment 17•11 years ago
|
||
Let me know if there is anything else I should do!
Attachment #8337435 -
Attachment is obsolete: true
Attachment #8340032 -
Flags: review?(felash)
Comment 18•11 years ago
|
||
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!
Attachment #8340032 -
Flags: review?(felash) → review-
Assignee | ||
Comment 19•11 years ago
|
||
Attachment #8336705 -
Attachment is obsolete: true
Attachment #8340032 -
Attachment is obsolete: true
Attachment #8349124 -
Flags: review?(waldron.rick)
Attachment #8349124 -
Flags: review?(felash)
Comment 20•11 years ago
|
||
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!
Attachment #8349124 -
Flags: review?(waldron.rick) → review-
Comment 21•11 years ago
|
||
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.
Attachment #8349124 -
Flags: review?(felash) → feedback+
Updated•10 years ago
|
Mentor: felash
Whiteboard: [mentor=:julienw] burirun4, → burirun4,
Comment 22•10 years ago
|
||
Hey Robert Patrick, I just wanted to check out if you still work on this! Thanks :)
Flags: needinfo?(patrick)
Comment 23•10 years ago
|
||
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
Updated•10 years ago
|
Flags: needinfo?(felash)
Comment 24•10 years ago
|
||
Hi Kumar, can you see this this issue still existed? Because I could not reproduce this on current master now...
Flags: needinfo?(rishav006)
Comment 25•10 years ago
|
||
Sorry !I didn't check before. Yeah steve.. this bug doesn't exist anymore. unable to reproduce. You can close it. Thanks
Flags: needinfo?(rishav006)
Flags: needinfo?(felash)
Comment 26•10 years ago
|
||
Right, it's been fixed in bug 952554. Thanks :)
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(patrick)
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•