Now when the user enters recipients, suggestion are displayed by searching in the contact list. Also user can select by selecting the contact button on the recipient text field. For better usability this should be modified as below 1. When the user enters any number on the recipient list, search in contact as well as the unknown numbers (Numbers which dont have contact entries) in message list, and show it as suggestions. 2. When the user selects the add button on the recipients text field, it should show option to select either contacts or call logs, and user should be able to select recipient/(s) from call logs also.

Flagging Ayman, but I think these are sensible suggestions. Should probably be 2 bugs instead of one. I already thought of the first one, as this makes sense. Even if it's not that easy to do right now it should be doable. The second one would need major work in the call log. And I'm not exactly sure this is so useful.

Hey Julein As per the Android behaviour the first one is already implemented with them.

Julien The Android behavior is that the prediction list that comes up as a result of imputing numbers on the recipient placeholder = the list first shows all the numbers that are from the contact & appended to it is the list of contacts from the message history that are unsaved. What I thought of how to go about is that:- Whenever we are in the Window.hash.location == "#new" then we can save those contacts that are in the message history (unsaved ones) to the contacts app. As a result of these in the prediction we get the same functionality as in Android. And we may delete the contacts saved whenever Window.hash.location != "#new" is fulfield. What is your opinion (Julien) ? Because I could not find any other way to append to the prediction list except adding the unsaved contact from message history to the contact app. The only other way that could be possible to do this app (as I feel) :- is make a separate list of all the unsaved contact from message history & display it only when then is no prediction from the contact app. Here we are not saving it to contact app. we will maintain one separate list of unsaved contact. But if we do this way then it differs from the way Android do it. Requires feedback (Julien). thanks!

assign these bug to me! thanks

Maybe we need to wait until we move the Contacts data to a Datastore. Then we can also make the Messages app generate a DataStore from its unknown numbers. And this way all apps could easily use it as a suggestion. If we don't need that other apps (I'm thinking mainly of the Dialer) use suggestions from unknown numbers in SMS, then we can do your second solution. Now, we still need to wait for Ayman's feedback before doing anything.

Hi Julien, We have a proof of concept patch for option 1 in comment #0. For option 2, we were thinking about DataStore support in Dialer (for getting call history). But as far as I know its not planned for 1.4. So we have the option of exposing a pick activity in Dialer app for selecting entries from call log. For this we have Bug 963043. But as discussed there, we had a technical difficulty as Contact and Dialer are part of same application, but as separate entry points. This again will be solved with Bug 965152 (Patch available) So I feel that if we implement a pick activity in Dialer, we are all set. As you know this is a MADAI mandatory feature. So we need yours and UX team comments on these proposal to continue.

A pick activity a dialer would work for me ;) Please request review for option 1 when you're ready !

Hey Julien Please have a look at the patch. The patch enhances the suggestion list by including those contacts in the message history that are unsaved. It covers the part-1 of #comment-0 as pointed by sharaf. thanks!

Hi Jaime, Mike, Patch is already in progress here but we've been pending UX feedback for a while, can someone add some UX comments here so Julien's patch review would become more meaningful? Thanks

Assigning to partner contributor

I'll review part 1 probably today of tomorrow, but please understand this is of less priority than 1.3 work right now. I don't think we need more UX feedback than Ayman's. Just ping Ayman by mail if you need a feedback soon.

Thanks Julien, priority understood :) This bug actually addresses the #1 in the comment 0 only, bug 963043 addresses #2. #1 here is actually quite straight forward I am not sure if much UX input is required. However flagging UX in case they have any comments.

Hey Julien Please have a look at the further cleaned up code. These patch is for part#1 of comment#0. The patch enhances the suggestion list by including those contacts in the message history that are unsaved. How:- I have maintained an internal array which consists of all unsaved contact from message history. Then we are populating it to the prediction list based on the number pattern of what recipient number has been entered. I have also covered the case where probably if a number is first in unsaved message history & if the user puts the message app in the background & then saves the same contact (as was in unsaved message history) then it should it not be displayed twice in the recipient suggestion list. thanks!

looking into this. leaving ni? to me

I cannot test this at the moment because the dialler is not working on todays build: hamachi_light-ICS. user. master. B-60. Gecko-fdde61f. Gaia-1aa8e77 so cannot get unknown numbers in my call log leaving ni? for me

ok i have tested this now. from a UX PoV it needs just a little work: 1) It would be advisable to have some distinction in the auto suggestion list between contacts that come from the users contact list and unknown numbers. 2) the double presentation of unknown numbers in each suggestion module (once in the header and once for the number itself) is not an efficient use of space and makes the list not easy on the eye when read. As a quick win I would suggest that for suggestion modules that contact entires that are unknown numbers the header of the module contains the number with the relevant part highlighted and the content of the module contains the source of the number. so if the user has typed 123 and two results are returned, one from the contact list and one from the call log the two suggestion modules would be displayed as so: --------------------- Luzie Züll mobile | 123456 --------------------- 123456 call log --------------------- ni? me know if you would like a wireframe producing to clarify this. happy to do it.

Ayman, just to clarify, the patch does _not_ look into the call log, but only in the Messages app's unknown numbers. NI you so that you see this comment ;)

(In reply to Julien Wajsberg [:julienw] from comment #17) > Ayman, just to clarify, the patch does _not_ look into the call log, but > only in the Messages app's unknown numbers. NI you so that you see this > comment ;) Oh dear, that's unfortunate as it kinda degrades the usefulness of this feature :( ...and makes me ask if it worth it in this case? ...Either way, I would still advise to strive to make the distinction between numbers that come from the Contact List and numbers within the messages app that are 'unknown' in order to keep the user orientated and remove the repetition i see in the patch. In the solution i propose in Comment 16 i would suggest to replace 'call log' with 'unknown contact' (or another suggestion if anyone has one). Remember that the visual treatment of these lists means that emphasis is on the first line of the module so 'unknown contact' will sit to the back in the Z plane (which is something i cannot replicate within these comments) so even though its a long string it should articulate itself as supporting, not dominating or competing with the number itself.

Hi Aymann As per your comments:- comment # 18 1.> For patch:- Please look at the file named Latest.patch (already uploaded) in which I have removed the repetitions. (https://bugzilla.mozilla.org/attachment.cgi?id=8371389&action=diff) Here the suggestion which is shown is this way:- --------------------- Luzie Züll mobile | 123456 --------------------- 123456 123456 --------------------- so there is already a distinction between those coming from contacts or from unsaved ones. For contacts which comes from saved ones they will always be of the form --------------------- Luzie Züll mobile | 123456 --------------------- & those from unsaved will of --------------------- 123456 123456 --------------------- so there is a clear distinction between the two. 2.> However, If you still feel that we need to change as you have said:- then we can have two scenarios:- a.>Name can be set as "call log":- Please have a look at folder 2.a (2.a-1 && 2.a-2). Number after selecting in the recipient field is shown as "call log". These is the issue. b.>Number can be set as "call log":- Please have a look at folder 2.b (2.b-1 && 2.b-2). Here if you see then the pattern is not underlined .i.e. when the user enters a number a number & it looks in the contacts (saved/unsaved) then there is a highlight in the prediction list for matching patterns which is not generated here because it comes from number & here in these case the number is "call log". I do not feel we should do : ---------------------- 123456 call log --------------------- thanks!!

(In reply to ayman maat :maat from comment #18) > (In reply to Julien Wajsberg [:julienw] from comment #17) > > Ayman, just to clarify, the patch does _not_ look into the call log, but > > only in the Messages app's unknown numbers. NI you so that you see this > > comment ;) > > Oh dear, that's unfortunate as it kinda degrades the usefulness of this > feature :( ...and makes me ask if it worth it in this case? This is actually a lot of work to make it work with the call log, because we miss some pieces, but I think we'd like to eventually do this.

Comment on attachment 8372075 [details] Pics.zip Superreview is for something else.

(In reply to ankit93040 from comment #19) > > b.>Number can be set as "call log":- Please have a look at folder 2.b > (2.b-1 && 2.b-2). Here if you see then the pattern is not underlined .i.e. > when the user enters a number a number & it looks in the contacts > (saved/unsaved) then there is a highlight in the prediction list for > matching patterns which is not generated here because it comes from number & > here in these case the number is "call log". > Hi, thanks for the quick response. Sorry but before i make a decision, for my clarity of understanding, with reference to ‘2.b-1png’ can you explain again why the first entry in the list (2344556) has a highlight but the second entry in the list (12334) does not. Thanks ni? ankit93040@gmail.com

Hi Ayman As per comment # 22 I am working on it. Even I don't have an idea right away. I ll probably be able to confirm you by tomorrow. thanks!

In reply to comment # 22:- Actually the result that is displayed is completely dynamic. It is sometimes showing highlighted prediction & sometimes not & even the prediction is not coming after the first successful prediction is done. The reason for the above behavior is in contact_render.js (render:). In render: function all comparisons are based on :- var details = Utils.getContactDetails(tels[0].value, contact, include); which in our case tels[0].value turns out be "call log". Therefore the result turns out to be completely dynamic. Just to remind you:- My initial patch was giving prediction in these way:- Here the suggestion which is shown is this way:- --------------------- Luzie Züll mobile | 123456 --------------------- 123456 123456 --------------------- Which I think even a novice user (customer) can understand that the first prediction is coming from saved contact & the other one is coming from unsaved contact.

Hi All Please find the attached patch. The Enhanced suggestion list is of the form:- --------------------- Luzie Züll mobile | 123456 --------------------- 123456 call log | 123456 --------------------- The first suggestion shows the contact type as :- mobile while the second one shows the type as call log. There by the user can differentiate between the two suggestion source. Ayman hope these meets your expectations. Thanks!

Ankit, note that you can make previous attachment "obsolete" so that we know which one to look at. Either you do this when you attach a new attachment (there is a check box to obsolete older attachments), or you can do it afterwards: click "Details" and then "Edit Details" (at the top of the page), then check "obsolete" and save. Because here I don't know what to do and what to look at with these 5 attachments. Thanks for cleaning this up!

Please review as mentioned in comment # 25.

(In reply to ankit93040 from comment #24) > Just to remind you:- My initial patch was giving prediction in these way:- > Here the suggestion which is shown is this way:- > --------------------- > Luzie Züll > mobile | 123456 > --------------------- > 123456 > 123456 > --------------------- > > Which I think even a novice user (customer) can understand that the first > prediction is coming from saved contact & the other one is coming from > unsaved contact. Hey, thanks for your feedback. Just to follow up on this comment before i test your patch so we are all on the same page. From a design PoV the emphasis of what I am prescribing is not solely on whether or not the user understands that the second entry is an unsaved contact. Its more than that. Firstly the point is to remove the repetition: > --------------------- > 123456 <- this number > 123456 <- is repeated here > --------------------- …because as stated in comment 16 repetition is not an efficient use of space and does not make the list easy on the eye to read. Secondly the point is to enforce to the user where this number originates from - so there is no ambiguity of its source and in doing this the information presented implicitly informs the user that this is an unsaved number. In doing this we remove ambiguity and maximise spatial efficiency, interpretability, understandability and therefore usability of this list when unknown numbers are presented. I will test your pact now and give you feedback :) leaving ni? to me until i have tested

(In reply to ankit93040 from comment #27) > Please review as mentioned in comment # 25. Hey there I tried to apply patch attachment 8373123 [details] [diff] [review] on the latest Gaia from master and I got: error: apps/sms/js/thread_ui.js: patch does not apply ...I guess the patch should be rebased. Am I right? removing ni? to me. Please ni? me again when patch is ready. Thanks :)

Comment on attachment 8373123 [details] [diff] [review] latest.patch Review of attachment 8373123 [details] [diff] [review]: ----------------------------------------------------------------- It's not really ready. I think most of this code should move to threads.js, with separate functions with good semantics. And unit tests of course ! Also note that Ayman does not do reviews, only feedbacks. ::: apps/sms/js/thread_list_ui.js @@ +143,5 @@ > src = ''; > + var duplicacy = ThreadListUI.message_history.indexOf(title.toString()); > + if (duplicacy == -1) > + ThreadListUI.message_history.push(title.toString()); > + } you should have a look to threads.js where we already register all messages. In registerMessage you can probably do something similar. ::: apps/sms/js/thread_ui.js @@ +2213,4 @@ > if (errorCode) { > this.showMessageError(errorCode, { > messageId: id, > + confirmHandler: function stateResetAndRetry() { This seems to be fixed in master already, you should rebase. @@ -2392,5 @@ > > this.container.textContent = ''; > - if (!contacts || !contacts.length) { > - return; > - } keep this (and see the next comments) @@ +2419,5 @@ > + var str = ThreadListUI.message_history[i].toString(); > + var patt = this.recipients.inputValue.toString(); > + var patt1 = new RegExp(patt); > + var restrict = str; > + if ((patt1.test(str)) == true) I think it's better to do this in searchContact. and I think we want to do a separate function for this. Maybe it's better to add this function in threads.js. Also, I think we need something better than iterating like this. But first you need to define a function in threads.js and define correctly its semantics. Maybe do unit tests first, so that the semantics are really well-defined, and then we'll try to find a way to do it efficiently. @@ +2455,5 @@ > + skip: this.recipients.numbers > + }); > + restrict = 0; > + } > + }, this); I don't see the point of these two loops.

Ayman, the first chunk conflicts but it's useless, and you can still apply the 2 last chunks.

(In reply to Julien Wajsberg [:julienw] from comment #31) > Ayman, the first chunk conflicts but it's useless, and you can still apply > the 2 last chunks. Cool, could you provide me with a non conflicting patch please. I dont want to edit the code myself to remove the conflict as i don't know the wider picture this code sits in. Thanks :)

ni? julien regarding comment 32

sorted this out on IRC

Comment on attachment 8373123 [details] [diff] [review] latest.patch (In reply to ankit93040 from comment #27) > Please review as mentioned in comment # 25. Thanks for supplying the patch ankit, and thanks Juilen for assisting me in applying it. From a UX PoV we need to push this patch a little further: 1) we need to remove the duplication of the number within the same module. currently we show: ———————————————— 12345 call log | 12345 ———————————————— I would like the second instance of the number removing 2) as per Julien’s comment 20 we are not dealing with numbers from the call log (this was my mistake in interpreting the requirements). Therefore the text ‘call log’ should be replaced with ‘unknown contact’ as illustrated: ———————————————— 12345 unknown contact ———————————————— 3) For ease of interpretation I would like a structural distinction between numbers that come from the Contact List and Unknown numbers. Therefore: ———————————————— 12345 <- unknown number unknown contact ———————————————— Luzie Zull mobile | 12345 <- number from contact list ———————————————— 4) The highlighting behaviour of number correlation needs to behave the same wherever the correlation is within the returned string. Currently highlighting is only occurring if the correlating numbers are at the beginning of the returned string. Please refer to frame ‘2. Highlights Correct’ and ‘3. Highlights Incorrect’ of attachment ‘959018_feedback_20140211_V1.0’ for a clearer illustration 5) Unknown numbers are not as important to end users as numbers that come from the users Contacts List. Therefore they must appear at the bottom of the results list. They currently appear at the top pushing contact list entires under the keyboard and out of view. Please refer to frame ‘1. Entries from the contact list pushed too low’ of attachment ‘959018_feedback_20140211_V1.0’. Therefore the final list data structure should be presented as follows: ———————————————— Luzie Zull mobile | 12345 <- number from contact list ———————————————— 12345 <- unknown number unknown contact ————————————————

Attachment to accompany comment 35

Ayman, following is our understanding 1. Expected data structure. ———————————————— Luzie Zull mobile | 12345 <- number from contact list ———————————————— 12345 <- unknown number unknown contact ———————————————— 2. Scenario 2.1 All the matching contact enties with Name will be displayed as below. ———————————————— Luzie Zull mobile | 12345 <- number from contact list ———————————————— 2.2 All the matching contact entries with no Name will be displayed as below. ———————————————— 12345 <- unknown number unknown contact ———————————————— 2.3 All the matching non contact (message history only) will be displayed as below. ———————————————— 12345 <- unknown number unknown contact ———————————————— Note: 1) Scenario 2.2 is not covered in your comments. This is our assumption please confirm it. 2) 959018_feedback_20140211_V1.0.pdf we see "call log" string. We would like clarify that this patch provide suggestion list only from contacts and from message history and not from call log. There is one more extension to this feature as mentioned in comment #6 for accessing call logs.

Please give your comments # 37.

(In reply to ankit93040 from comment #37) > Ayman, following is our understanding > > 1. Expected data structure. > ———————————————— > Luzie Zull > mobile | 12345 <- number from contact list > ———————————————— > 12345 <- unknown number > unknown contact > ———————————————— correct > 2. Scenario > > 2.1 All the matching contact enties with Name will be displayed as below. > > ———————————————— > Luzie Zull > mobile | 12345 <- number from contact list > ———————————————— correct …with the addition that ‘mobile’ is whatever label the user has associated to the number in the Contacts list. If you wish to see a list of possible labels on the phone go to: 1) contacts 2) select new contact 3) select ‘mobile’ that sits to the left hand side of the ‘phone’ and ‘carrier’ fields. The options should be: - work - Personal - Fax Home - Fax Office - Fax Other - Other - Custom (which is a free text field) > 2.2 All the matching contact entries with no Name will be displayed as below. > > ———————————————— > 12345 <- unknown number > unknown contact > ———————————————— wrong. For this scenario the current implementation of: ———————————————— 12345 mobile | 12345 <- label the user has associated to the number in the Contacts list ———————————————— …does not reflect the information structure that was specified in the original wireframes (HTML5_SMS_20121212_R2S1_V8.0 page 10) which was : ———————————————— 12345 mobile <- label the user has associated to the number in the Contacts list ———————————————— You have two choices here. You can either leave it the way it is currently or you can change it to be the way it was specified in HTML5_SMS_20121212_R2S1_V8.0 page 10. If you have time I would like that we changed it to be presented as in HTML5_SMS_20121212_R2S1_V8.0 page 10, but this is entirely up to you as it is not the scope of this bug. > 2.3 All the matching non contact (message history only) will be displayed as > below. > > ———————————————— > 12345 <- unknown number > unknown contact > ———————————————— Correct > Note: > 2) 959018_feedback_20140211_V1.0.pdf we see "call log" string. We would like > clarify that > this patch provide suggestion list only from contacts and from message > history and not from call log. > There is one more extension to this feature as mentioned in comment #6 for > accessing call logs. Yes that is correct, we are only dealing with unknown contacts from message app, not from call log The only reason why 959018_feedback_20140211_V1.0.pdf contains the ‘call log’ string is that it is displaying screenshots taken whilst testing your patch contained in attachment 8373123 [details] [diff] [review]

Please find the attached patch for Enhancing the suggestion list. Note:- The suggestion list only consists of numbers from saved contacts (by default) & those contacts from message history which are unsaved.

Comment on attachment 8376120 [details] [diff] [review] f2.patch Its looking really good. Just one final point as per comment 35: The highlighting behaviour of number correlation needs to behave the same wherever the correlation is within the returned string. Currently highlighting is only occurring if the correlating numbers are at the beginning of the returned string. Please refer to frame ‘1. Highlights Correct’ and ‘2. Highlights Incorrect’ of attachment ‘959018_feedback_20140214_V2.0’ which shows screenshots of this patch in action for a clearer illustration

Reference comment 41

(In reply to ayman maat :maat from comment #41) > Comment on attachment 8376120 [details] [diff] [review] > f2.patch > > Its looking really good. > > Just one final point as per comment 35: The highlighting behaviour of number > correlation needs to behave the same wherever the correlation is within the > returned string. > > Currently highlighting is only occurring if the correlating numbers are at > the beginning of the returned string. Please refer to frame ‘1. Highlights > Correct’ and ‘2. Highlights Incorrect’ of attachment > ‘959018_feedback_20140214_V2.0’ which shows screenshots of this patch in > action for a clearer illustration We actually did this recently in another bug. We can revert this if this is incorrect (I don't know why I didn't ask you...) but anyway this is not part of this bug, so ankit, please do not focus on this here.

Comment on attachment 8376120 [details] [diff] [review] f2.patch Review of attachment 8376120 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the implementing this and it would be a nice feature in app, but we need more clear ux input here. How about we remove a non-contact thread? It seems no need to show the deleted number here, but your history didn't sync with the changes. And we also need unit test for verify this feature. ::: apps/sms/js/contact_renderer.js @@ +210,5 @@ > > var data = Utils.getDisplayObject(details.title, current); > + var index = ThreadListUI.message_history.indexOf(data.number); > + if ((index > -1) && data.number == data.name) { > + data.number = "unknown contact"; unknown contact need l10n string here. ::: apps/sms/js/thread_list_ui.js @@ +12,4 @@ > draftLinks: null, > draftRegistry: null, > DRAFT_SAVED_DURATION: 5000, > + message_history: new Array(), Please use camel case here and [] for array would be enough. @@ +144,5 @@ > src = ''; > + if (!isNaN(title)) { > + var duplicacy = ThreadListUI.message_history.indexOf(title.toString()); > + if (duplicacy == -1) { > + ThreadListUI.message_history.push(title.toString()); nit: '' + title ::: apps/sms/js/thread_ui.js @@ +2439,4 @@ > skip: this.recipients.numbers > }); > }, this); > + for (var i = 0; i < ThreadListUI.message_history.length; i++) This long part should be move to another function. @@ +2446,5 @@ > + var patt1 = new RegExp(patt); > + var restrict = str; > + if ((patt1.test(str)) == true) > + { > + if (!contacts || !contacts.length) { Please add comments for the each part. (And why we need to traverse contacts twice here?)

Hi Please find the Attached Patch! Ayman - It meets the UI specifications as commented in Comment # 39. Note:- These patch only looks for contacts in the message history (unsaved). Julien - As pointed by you in Comment # 30, i have moved the code in threads.js. I didn't find any relation of moving the pattern search to searchContact. Therefore i didn't made that changes. Please review and provide feedback on these!

ankit, for future patches, please choose one reviewer and keep it for the whole review process (unless there are problems like the reviewer goes in holiday or does not answer). This will save time for everyone.

Comment on attachment 8377151 [details] [diff] [review] Bug_959018_Suggestion_list.patch Review of attachment 8377151 [details] [diff] [review]: ----------------------------------------------------------------- Removing the r? request for Steve but I still have a question for you Steve. ::: apps/sms/index.html @@ +298,5 @@ > + data-type="${type}" data-separator="${separator}" data-carrier="${carrier}"> > + ${photoHTML} > + <p class="name">${numberHTML}</p> > + <p class="number"> > + <span data-l10n-id="${type}">Unknown Contact</span>${separator} I think we don't want the separator here ::: apps/sms/js/contact_renderer.js @@ +34,4 @@ > main: 'contact-suggestion-tmpl' > } > }, > + suggestionNew: { "suggestionUnknown" is probably a better name or even "suggestion-unknown" @@ +34,5 @@ > main: 'contact-suggestion-tmpl' > } > }, > + suggestionNew: { > + renderAll: true, I think renderAll is useless; this is used when we have several phone numbers for one contact, which is irrelevant here ::: apps/sms/js/thread_ui.js @@ +2430,4 @@ > > // Render each contact in the contacts results > var renderer = ContactRenderer.flavor('suggestion'); > + //New flavor added in Contact_render unnecessary comment @@ +2430,5 @@ > > // Render each contact in the contacts results > var renderer = ContactRenderer.flavor('suggestion'); > + //New flavor added in Contact_render > + var rendererNew = ContactRenderer.flavor('suggestionNew'); "unknownContactsRenderer" @@ +2473,5 @@ > + Threads.Message_Unsaved.splice(index, 1); > + i = i - 1; > + restrict = 0; > + } > + }, this); we have all the matching code in contacts.js so I think most of this should move there. (In the past I said "searchContacts" but now I think contacts.js is actually better fit). listContacts should not be changed too much, only to use one renderer or the other renderer depending on the "source" of the contacts. ::: apps/sms/js/threads.js @@ +31,5 @@ > + if (duplicacy == -1) { > + Message_Unsaved.push(this[key].toString()); > + } > + } > + } So, I thought a little more. We need to save only the unknown numbers. That means we need to write code in threadListUI.setContact, so that only numbers that are not found hwen looking up the contact are saved. A number needs to be removed when we get a "contactchange" event for this (there are examples in the code). I don't know where is the best place to store these numbers. Maybe in contacts.js? What do you think Steve?

NI Steve for the question in previous comment.

Ankit, also, we need a patch that is generated by "git format-patch" with a 8-line context diff. You can configure this in your ~/.gitconfig by adding this: [diff] context = 8

More thought: * I think the unknown number utility functions should go to contacts.js * but the code that merges the contacts and unknown number lists should be in ThreadUI.searchContacts.

(In reply to Julien Wajsberg [:julienw] from comment #50) > More thought: > * I think the unknown number utility functions should go to contacts.js > * but the code that merges the contacts and unknown number lists should be > in ThreadUI.searchContacts. I think it's ok to put unknown number utility functions in contacts, but maybe we could integrated existing contacts api and unknown number, like calling Contacts.findByString method will return both matched contact and unknown number. Otherwise we could just having a new unknown number manager.

(In reply to Steve Chung [:steveck] from comment #51) > (In reply to Julien Wajsberg [:julienw] from comment #50) > > More thought: > > * I think the unknown number utility functions should go to contacts.js > > * but the code that merges the contacts and unknown number lists should be > > in ThreadUI.searchContacts. > > I think it's ok to put unknown number utility functions in contacts, but > maybe we could integrated existing contacts api and unknown number, like > calling Contacts.findByString method will return both matched contact and > unknown number. That's exactly what I thought first, and then I thought it was not good. I think it's better to keep a method that accesses only mozContacts, because the suggestions list is the only place where we need to access both the unknown number list and the mozContacts list. Let's separate responsibilities :) > Otherwise we could just having a new unknown number manager. That could work for me too.

Please provide your feedback! thnaks!

Comment on attachment 8378887 [details] [diff] [review] new.patch Review of attachment 8378887 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/sms/js/contacts.js @@ +196,5 @@ > + var index = ThreadListUI.message_history.indexOf(number); > + if (index != -1) { > + ThreadListUI.message_history.splice(index, 1); > + } > + }, this); Please have a look to the "contactchange" handler in ThreadListUI. It's "updateContactsInfo". In this function, you just need to delete the array. So you can add a Contacts.clearUnknown() that would be called in updateContactsInfo(). ::: apps/sms/js/thread_list_ui.js @@ +13,4 @@ > draftLinks: null, > draftRegistry: null, > DRAFT_SAVED_DURATION: 5000, > + message_history: new Array(), move this to contacts.js @@ +143,4 @@ > } else { > title = number; > src = ''; > + if (!isNaN(title)) { you don't need this check, we already know that "number" is existing. @@ +146,5 @@ > + if (!isNaN(title)) { > + var duplicacy = ThreadListUI.message_history.indexOf(title); > + if (duplicacy == -1) { > + ThreadListUI.message_history.push(title); > + } move this to a Contacts.addUnknown() function ::: apps/sms/js/thread_ui.js @@ +2502,5 @@ > + restrict = 0; > + } > + }, this); > + } > + } Once you move the array to contacts.js, you can create a function "Contacts.findUnknownByString" to do the lookup with the same input/output than Contacts.findByString. ThreadUI.searchContacts can call these 2 functions. Contacts returned by findByString would have an additional property "source: 'contacts'" and contacts returned by findUnknownByString would have a property "source: 'unknown'". This way you can distinguish them in listContacts and choose the correct renderer. Another possibility is to rename findByString to findContactsByString, and have Contacts.findByString call findContactsByString and findUnknownByString.

Hi Julien Kindly provide feedback for the patch! thanks!!

Comment on attachment 8379704 [details] [diff] [review] 0001-Bug_959018_Enhancement_Suggetion_list.patch Review of attachment 8379704 [details] [diff] [review]: ----------------------------------------------------------------- I can't review in detail now, but from a high level view this looks much much better! Don't you think? :) ::: apps/sms/js/contacts.js @@ +3,4 @@ > (function(exports) { > 'use strict'; > /*global fb */ > + var message_history = new Array(); var unknownNumbers = []; @@ +218,5 @@ > }, > findByString: function contacts_findBy(filterValue, callback) { > + var list = []; > + var self = this; > + var promise = new Promise(function(resolve, reject) { while I really like Promises, I think we should use them in all methods or not use them at all. So let's use "normal" callbacks for now if you don't mind, and we can convert these methods to use Promise in an upcoming patch. @@ +245,5 @@ > + for (var i = 0; i < message_history.length; i++) > + { > + var num = message_history[i]; > + var pattern = filterValue; > + var patterntest = new RegExp(pattern); you should use String.contains() instead of a regexp. But even this is not enough because we need to match "+33123456789" when the user types "0123456789". Utils.probablyMatches can help you but it's not good enough either, because it's meant to compare whole numbers, whereas here we want to compare part of numbers. For example, if the user types "01" do we want to match "+331" ? And this is a rule specific to France, other countries have other difficult rules, that all lead to a library we use in Gecko, but that is not completely available in Gaia. Maybe the best we can do is to trying to match the full num first, and then remove the first character if it's 0 and try to match the result. @@ +250,5 @@ > + if ((patterntest.test(num)) == true) { > + var obj = { > + name: [num], > + tel: [{value: num}], > + source: 'unknown contacts' I think we can just use "unknown" @@ +307,5 @@ > }); > + }, > + addUnknown: function addUnknown(number) { > + var index = message_history.indexOf(number); > + if (index == -1) nit: use braces and === @@ +311,5 @@ > + if (index == -1) > + message_history.push(number); > + }, > + clearUnknown: function clearUnknown() { > + message_history.splice(0, message_history.length); use "message_history.length = 0", same result and shorter

Hi Julien I have modified the patch as per your comments. Coming to the pattern recongnition part as you mentioned - There is a separate bug for the same https://bugzilla.mozilla.org/show_bug.cgi?id=909146. There are many cases for the same like it can start from 0,1,+91 (country codes) etc. There is a lot of work in order to accomplish the same. And these was never the scope of these bug. However if you suggest then I can mark a dependency on that bug. Kindly review the same. thanks!

Comment on attachment 8380586 [details] [diff] [review] bug959018.patch Review of attachment 8380586 [details] [diff] [review]: ----------------------------------------------------------------- I didn't try it on a device yet, I'll do it right now but maybe I'll report only tomorrow. In the mean time here are some comments. Mostly nits, this looks good. Thanks! ::: apps/sms/js/contact_renderer.js @@ +34,5 @@ > main: 'contact-suggestion-tmpl' > } > }, > + suggestionUnknown: { > + renderAll: true, just to mention I now understand why we need "renderAll: true" ::: apps/sms/js/contacts.js @@ +217,5 @@ > }; > }, > findByString: function contacts_findBy(filterValue, callback) { > + var list = []; > + var unknowncallback = function(contact) { nit: "unknownCallback" @@ +221,5 @@ > + var unknowncallback = function(contact) { > + list = list.concat(contact); > + self.findByUnknown(filterValue, function(contact) { > + list = list.concat(contact); > + callback(list); We can try something simpler here, with more meaningful names, like: * use "contacts" for the first "contact" argument * use "unknown" for the second "contact" argument and here: contacts = contacts || []; unknown = unknown || []; callback(contacts.contact(unknown)); @@ +223,5 @@ > + self.findByUnknown(filterValue, function(contact) { > + list = list.concat(contact); > + callback(list); > + }); > + }; use ".bind(this)" so that you don't need a "self" variable @@ +225,5 @@ > + callback(list); > + }); > + }; > + var self = this; > + self.findContactByString(filterValue, function(contact) { * use "this" * just pass "unknownCallback" as second parameter, you don't need the additional function @@ +228,5 @@ > + var self = this; > + self.findContactByString(filterValue, function(contact) { > + unknowncallback(contact); > + }); > + }, please fix the indentation issues @@ +236,5 @@ > filterBy: ['tel', 'givenName', 'familyName'], > filterOp: 'contains', > filterValue: filterValue > }, callback); > + }, please fix the indentation issue @@ +238,5 @@ > filterValue: filterValue > }, callback); > + }, > + findByUnknown: function findByUnknown(filterValue, callback) { > + var terms = filterValue; what is it for? @@ +240,5 @@ > + }, > + findByUnknown: function findByUnknown(filterValue, callback) { > + var terms = filterValue; > + var list = []; > + for (var i = 0; i < unknownNumbers.length; i++) That's the main point I don't like for performance reason but I don't know if we can do better except using a database or sorting the list and doing a binary search. Let's keep it like this for now and we can revisit the implementation in a follow-up patch. @@ +241,5 @@ > + findByUnknown: function findByUnknown(filterValue, callback) { > + var terms = filterValue; > + var list = []; > + for (var i = 0; i < unknownNumbers.length; i++) > + { nits: * put the brace on the previous line * use "l = unknownNumbers.length" after "i = 0" @@ +244,5 @@ > + for (var i = 0; i < unknownNumbers.length; i++) > + { > + var num = unknownNumbers[i]; > + var pattern = filterValue; > + if (num.contains(pattern)) { nit: just use "num.contains(filterValue)" @@ +250,5 @@ > + name: [num], > + tel: [{value: num}], > + source: 'unknown' > + }; > + list.push(obj); break when "list" has 3 items, we don't need more than that @@ +256,5 @@ > + } > + > + callback(list, { > + terms: terms > + }); I don't understand why you return this second parameter here? @@ +304,4 @@ > callback(results); > }); > }); > + }, nit: please add an empty line @@ +306,5 @@ > }); > + }, > + addUnknown: function addUnknown(number) { > + var index = unknownNumbers.indexOf(number); > + if (index === -1) nit: please add braces @@ +308,5 @@ > + addUnknown: function addUnknown(number) { > + var index = unknownNumbers.indexOf(number); > + if (index === -1) > + unknownNumbers.push(number); > + }, nit: please add an empty line ::: apps/sms/js/thread_ui.js @@ +2480,5 @@ > + input: fValue, > + target: ul, > + skip: this.recipients.numbers > + }); > + } nit: please fix indentation issues Please also create the argument in a previous variable. Like "var rendererArg = { contact: contact... }", and reuse it in the if/else block.

I think that bug 909146 is obsolete, but we can do a better match in a follow-up bug and keep it like this here.

Hi Julien Kindly review the attached patch updated with review comments. thanks!!

Comment on attachment 8382056 [details] [diff] [review] Suggestion-959018.patch Review of attachment 8382056 [details] [diff] [review]: ----------------------------------------------------------------- ok, this looks fine, but I can't give r+ without unit tests ! You need to create unit tests for your changes, and please create a github pull request to check everything is fine. The code itself looks ok except the following nits. Also I tried on the device with the biggest workload (2000 threads) and this does not slow down the device. Note that I tried on a Peak, so we might want to try on a Buri to be sure, but my Buri has been taken by someone this week so I can't test myself... So, I need unit tests now! Thanks a lot! ::: apps/sms/js/thread_ui.js @@ +2475,5 @@ > + }; > + if (contact.source == 'unknown') { > + unknownContactsRenderer.render(rendererArg); > + } > + else { nit: put else on the previous line

Hi Julien Please take a look at the Unit test cases. For me thread_ui_test.js & thread_list_ui_test.js were failing in the master (without my patch). thanks!

Hi Julien Please have a look the pull request. thanks!

Comment on attachment 8384486 [details] Pointer to Pull Request.html I added comments on the github pull request. The code is still fine, but the unit tests are not. Please request review again once you're ready. Sorry for the delay, as I explained to you already, we had (a lot of!) work with higher priority.

Hi Julien Kindly review the above pull request! And, Kindly merge the same to the master. thanks:)

Comment on attachment 8388975 [details] Pointer to Pull Request.html The unit tests are still wrong, I left a comment on github.

Hi Julien & Steve Steve - As julien is perhaps on leave. Will you kindly review the attached pull request. Julien gave some comments for Unit test cases which I have done with - https://github.com/mozilla-b2g/gaia/pull/17050 (old one). He gave feedback + for the code only Unit test case were pending. I am done with it. Please have a look at the attached file. Kindly merge the same to the master as in I really wish to see this landing on 1.4 branch. thanks a lot!

Comment on attachment 8393450 [details] Pointer to Pull Request.html Mostly are good, but I have a question about the test[1]. And don't forget to remove 'done' because we don't need it in these tests, thanks! [1] https://github.com/mozilla-b2g/gaia/pull/17322/files#r10804193

Oh, and don't forget to rebase to the latest master because the patch is still unable to merge on github.

Hi steve Kindly merge the same to the master when the tree is open. unknown list max length is 3 as per Julien's commnent in comment # 58. thanks a lot!

Comment on attachment 8394663 [details] Pointer to Pull Request.html I left some comments on github but it's good basically, so r+ = me with some nits addressed. Thanks for the great job!

Thanks Steve!!! I updated the previous pull request with your commnets. https://github.com/mozilla-b2g/gaia/pull/17424

Hey Steve, can you have a final look and merge the patch? (Otherwise I'll do it myself tomorrow)

Forgot to merge when tree reopened... Landed in master: 4f78fe58cceca1356009a6a03c65d10f677114ed

Hi Steve The "unknown contacts" work is not localized. So, uploading patch please review. If all's well then I'll upload pull request. I don't think you require any Unit test case changes for the same. Thanks

Comment on attachment 8486358 [details] [diff] [review] unknown.patch Review of attachment 8486358 [details] [diff] [review]: ----------------------------------------------------------------- The fixing is ok, but please refine the localization part, thanks. ::: apps/sms/locales/sms.en-US.properties @@ +317,5 @@ > conversion-warning-ok = OK > conversion-warning-title = Auto Conversion "TO" > conversion-warning-body = 'CC' and 'BCC' will be converted to "TO" automanically, if including them. > + > +unknown = Unknown Contact \ No newline at end of file please remove the original(but unused)key unknow-contact and replace with new one(maybe unknown-contact-title as key name would be better)

uploaded pull request for master. Please merge.. Thanks https://github.com/mozilla-b2g/gaia/pull/23883/files

(In reply to ankit93040 from comment #78) > uploaded pull request for master. Please merge.. Thanks > > https://github.com/mozilla-b2g/gaia/pull/23883/files Sorry for the late reply, I've updated on github

Can we move this to a separate bug? I think we'll need to uplift this too, so the bug needs to be late-l10n and we need to fix it very quick...

Raise a separate bug https://bugzilla.mozilla.org/show_bug.cgi?id=1069817. will upload there