After landing https://bugzilla.mozilla.org/show_bug.cgi?id=833291, the ID for a thread has changed (previously was the international number, currently it's a regular ID), so given a new received message it's impossible to know the right thread to append it in the UI (due to the ID it's not exposed to the API). Gecko change it's tracked here https://bugzilla.mozilla.org/show_bug.cgi?id=850127, and we need to modify Gaia to take this new ID into account.

Ok, so there are only two options here, either we block on this one and fix bug 851032 and bug 850127 or we back-out 833291. Vicamo, you are the author of 850127 and 833291, what do you think is safest?

(In reply to Daniel Coloma:dcoloma from comment #1) > Ok, so there are only two options here, either we block on this one and fix > bug 851032 and bug 850127 or we back-out 833291. > > Vicamo, you are the author of 850127 and 833291, what do you think is safest? Of course land bug 850127. When we're supporting MMS in message app, the original way of fetching messages of a specific thread will be totally unusable because now we may have multiple recipients in one message. Please see also bug 849739, which will have another cursor-based GetThreads() API, which provides thread id, participants for MMS in v1.0. All these changes are based on previous discussions of Borja and me.

For this issue, I am only interested in v1.0.1 so the impact on MMS is irrelevant to me. Just to be sure, without taking into account MMS are you still suggesting with landing bug 850127 and bug 851032.

(In reply to Daniel Coloma:dcoloma from comment #3) > For this issue, I am only interested in v1.0.1 so the impact on MMS is > irrelevant to me. Just to be sure, without taking into account MMS are you > still suggesting with landing bug 850127 and bug 851032. Yes, that's why this patch contains only threadId in SmsMessage object, not including SmsFilter. A full solution for both SMS and MMS will be available in bug 849739 and bug 847760. Actually, I think we might also need threadId in SmsThreadListItem, the return data type of GetThreadList(). Borja, what do you think?

(Daniel, I differ the tef+ call to you since you nominated it yourself)

Since it's now a tef+, I'll do some more cleanups in the WIP patch and request for review asap.

Vicamo, could you tell us the name of the new value exposed? 'threadId'??? I would like to know asap for making all changes in Gaia while you are creating the patch in Gecko (I have to modify tests, mockups... with this new param). Regarding your question for sure! I need thread ID in the SMS and in the list of Threads. Thanks!

(In reply to Borja Salguero [:borjasalguero] from comment #7) > Vicamo, could you tell us the name of the new value exposed? 'threadId'??? I > would like to know asap for making all changes in Gaia while you are > creating the patch in Gecko (I have to modify tests, mockups... with this > new param). In SmsThreadListItem, its name is "id", in SmsMessage, it's "threadId". I'll have the same naming in both bug 850127 and bug 849739 (need updates). Thank you :)

One more thing. Are you gonna modify 'getMessages' for requesting ALL MESSAGES given a ThreadID? Because currently we are using an Object like: { senderOrReceiver: '436797', body: 'Sending :)', timestamp: new Date(Date.now() - 172800000), unreadCount: 0 } And I need 'senderOrReceiver' for creating the filter for getMessages. I would need this param in order to request 'getMessages' for keeping the already implemented Gaia code working. Something like: { id: 20123 senderOrReceiver: '436797', body: 'Sending :)', timestamp: new Date(Date.now() - 172800000), unreadCount: 0 } On the other hand, are we going to modify 'getMessages' for requesting ALL MESSAGES given a ThreadId? Is there any bug for making this change? Thanks!

Vicamo, I've created a patch for the 'ThreadID' issue. Could you test it with your Gecko patch? Thanks!

Comment on attachment 726690 [details] Pull Request Verified :)

Comment on attachment 726690 [details] Pull Request Julien, for testing it you have to generate Gecko with Vicamo Patch. Both patches will be landed at the same time once everything have R+. Thanks!

Comment on attachment 726690 [details] Pull Request Borja, I'm a little bit confused about the review flags. It seems you want Julien's review instead because you have r=julienw in the commit summary. I've placed a feedback+. Maybe it's enough. :)

(In reply to Borja Salguero [:borjasalguero] from comment #0) > After landing https://bugzilla.mozilla.org/show_bug.cgi?id=833291, the ID > for a thread has changed (previously was the international number, currently > it's a regular ID), so given a new received message it's impossible to know > the right thread to append it in the UI. Sorry, I don't understand why. I mean, I really like the idea of having a thread id, but I don't understand why it's broken in 1.0.1, and therefore why this needs to be tef+. Am I missing something ?

Comment on attachment 726690 [details] Pull Request comments on the PR.

Sorry Julien, I should add more info to this bug. The problem here is that, for analyzing if some received sms belongs to the current thread, we were using the 'normalized phone number'. However, before all latest changes in Gecko, we can't use this method due to we are not going to use this criteria anymore. Gecko will give us a 'thread_id' which will be the unique identifier for knowing if one new message belogs to which thread. The patch was landed in Gecko B2G-18, so if you send a SMS to a NON-normalized number (for example to 612123123) and, WITHOUT going back to thread list, you receive a response (from +34612123123 in this case), the message is not threaded properly. If you have any doubt dont hesitate to ask me or Vicamo. I will take a look to your comments. Thanks.

I'm perfectly happy with these changes in master or v1-train, what I don't like is having this in 1.0.1, and why it's needed.

Im gonna try to explain again. The change in Gecko it's **already** landed in B2g-18, what it means that SMS App it's **not** working properly in v1.0.1. That's why this patch it's needed, because currently it's not working (this is the reason of having TEF+). In fact this patch should be landed at the same time than Gecko patches.

Bug 833291 comment 93 implies that everything works correctly in 1.0.1 but I just verified and indeed the following STR doesn't work correctly : * send an SMS to a local number * stay on the newly-created thread while your correspondend replies * you get the reply, but it isn't shown in the current thread whereas it should. * you have to go to the thread list screen and show the thread again to display it.

Yep. Without both patches (Gecko & Gaia) this is the result. For testing it build your own Gecko with Vicamo's patch and use Gaia with my patch. Everything should work.

borja, you've seen that I already made a review a few days ago, right ? :)

Yep, I'll udpate the PR with your suggestions asap ;)

Hi all, bug 850127 is ready. I'll go ahead to prepare patches for b2g18_v1_0_1 :)

Im gonna take a look to Julien suggestions today!

I will test it again based on Julien's comment.

Vicamo, Julien, could you test it? @Julien: I've changed the proposal in order to get a cleaner code. Please take a look.

Comment on attachment 732346 [details] Pull Request Verified! Sending & receiving both international & national number messages results in only one thread and Gaia presents them well immediately.

Comment on attachment 732346 [details] Pull Request r=me let's land this together with Bug 850127.

master: 86a79ba3fdfb0eb6f593e34887fa89e3553d1a66

v1-train : 2f64311e0f23b85c29b20be9502cbdeaee8342ae v1.0.1 : f6a48343e3b5aaed19688ab50b90f1605a519365

should be backed out of all branches because Bug 850127 got backed out. Right now github seems busted, will do it later if nobody does it before.

backed out master: 3afa858a84f422b3a631e4e7b81cc7cad428ef2b backed out v1-train: 61aa777dba52a54d5e7c0040f1f4a4a27b4d4aa9 backed out v1.0.1: 28c144fa7e8061ee6bcbbb47ca911ef33b87814b I think we should land on master only when Bug 850127 lands on gecko b2g18 because that's the branch we usually use with master.

Julien: after the backout, what's the plan to fix this on v1.0.1? I still see the initial problem (incoming messages are not threaded correctly) in today's v1.0.1 build.

The plan is to do land and uplift now as Bug 850127 just landed on the branches ;) I'm on it now.

I'm testing this before relanding but I see a bug that I thought was fixed.

ok, no bug, it was my fault. Relanded all : master: 8292d09e18cf82364f07fe0948176245d68630cb v1-train: 8bab6b6803e046522a2b4c5ef9f81518895e1598 v1.0.1: f2546c5a6a6175aa9b661d5110f5ea00d19ef48a

This is how my messages view looks after getting the latest changes. I saw that this messages were some testing stuff (https://github.com/mozilla-b2g/gaia/pull/8716/files#L2R2) and I cannot receive messages anymore and setting one of message as read doesn't do anything (it is still unread after the next start of Messages). Didn't the patch break something?

Right, looks like you get the sms_mock's data. I'm not sure this bug is the culprit though, would you please file another bug, we'll take a look tomorrow.

Done, bug 860049.

Hi Bartosz. Are u testing in the device? or emulator?

Tested on ikura device and working fine with: Gecko Gaia 715de4016b63d417c8fdc20b1ae8b15b666e1afb BuildID 20130416092518 Version 18.0