Being in 'edit-mode', if we receive a SMS is not rendered properly.

If we receive a SMS while we are in EDIT MODE, the message is lost and the thread is not created. This it's due to during 'EDIT-MODE' we are neglecting this event, and we should append the message/thread properly (currently it's not working).

For me it should be blocking, we are missing SMSs and that's something we can not allow.

Loss of data should block. Borja - when do you expect to have a patch available for uplift?

Borja is on PTO, so stealing it

Stealing again due to I had a patch almost ready for it! Let me re-check and I will upload it.

Wow, this is a big patch... Wouldn't it be easier to go out of edit mode first then do the normal thing if we get a message-received while in edit mode? I mean for the tef+ milestone at least we should probably aim at the simplest solution.

Hi Etienne! The problem here is that there is no 'easy' solution for it... if every time you receive a SMS you go out of edit mode you are loosing all the info you are filling there! And further more, you are going to get a huge re-rendering of the whole list of threads... That's why we need a change in the paradigm (it's the same that we use in 'Thread-view'), so we are going to update the previously created list of threads, so 'edit-mode' it's going to work as expected and the increase in the performance it's huge.

I've added Julien and you as reviewers, because I dont know who of you could take this review asap! Thanks!

Comment on attachment 706390 [details] Pull Request My comments are inlined on the github pull request. It's definitely a good thing not to re-render the whole thread list for each new incoming sms like you're proposing in this patch. But the code should reflect this new behavior by having clear code paths for: - the initial rendering |renderThreads| (which append new threads with |appendThread|) - appending a new thread |appendThread| - updating the entry for an existing thread |updateThread| And not bundle all the create/update logic into the existing |appendThread|.

Ok! Im going to fix the patch with your suggestions. I will come back asap with these changes!

Patch working again! All tests passed, small issue fixed, comments added ;). Thanks!

One small thing regarding the 'create/update' methods for threads. My proposal it's based on the 'removing' old element paradigm and appending a new one in the right place, that's why everything it's in the same 'appendThread' method.

Comment on attachment 706390 [details] Pull Request As discussed on IRC: I still don't think this should be 1 method since we have very different cases. You could check if the thread exist in onmessagereceived, remove it if needed and then append (but the append code will be much smaller and simpler). Since this was the main comment on the previous patch, I'll wait for it to be addressed before doing another review.

All changes done! Ready for review ;)

Comment on attachment 706390 [details] Pull Request Since I'm on PTO tomorrow and this is an urgent matter I'm forwarding the review to Julien (he's aware of this :)).

Cool! Enjoy your vacations! ;).

Comment on attachment 706390 [details] Pull Request commented on the pull request. please ask the review again when fixed ! Thanks !

Added your suggestions & Tests passed! I've added some comments and explanations in the PR as well. Thanks for your review, I think that we have good & well-identified action points for getting a cleaner code, and for sure that Im gonna add them to the 'Optimization' PR.

Comment on attachment 706390 [details] Pull Request r=me with the latest nits addressed

Thanks a lot for your review Julien!!!

master: https://github.com/borjasalguero/gaia/commit/5ff68f304920ca8985ed48eaac465b43dccb398d

This commit does not apply cleanly to v1-train or v1.0.0. Instead of trying to merge the patch myself, as I don't know the code, could you please land this on v1-train? If you run: git cherry-pick 5ff68f304920ca8985ed48eaac465b43dccb398d you'll get a merge conflict. When you're done, please commit those changes to v1-train and then uplift those to v1.0.0.

Taking the uplift

Ok, the conflict comes from the fact that Bug 828476 was not uplifted yet (even if approval was given; processes are slowing us down). The conflict is easy to resolve that I'd prefer to wait for Bug 828476. John, what do you think ?

v1-train: 620c2975c15079cc5958c00cc94c6c612a376a16 v1.0.0: 6ef765aef0d6dbbc2936c09a511c69d737d6c7f0

(In reply to Julien Wajsberg [:julienw] from comment #26) > Ok, the conflict comes from the fact that Bug 828476 was not uplifted yet > (even if approval was given; processes are slowing us down). > > The conflict is easy to resolve that I'd prefer to wait for Bug 828476. > John, what do you think ? I think we waited for that bug to land, no merge conflict this time!

This issue does not reproduce on Unagi device, Master build ID 20130214032850 Dec 5th Kernel Gaia: remote="hgmozillaorg" revision="6ba3c0dfb2ee"/> Gecko: remote="hgmozillaorg" revision="aceeea086ccb"/> User is able to receive a sms while being in the "Edit" mode. However, it still reproduces on build 20130214070203 Dec 5th Kernel Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/d1288313218e Gaia: 6544fdb8dddc56f1aefe94482402488c89eeec49 User does not receive a SMS while being in EDIT MODE, the message is lost and thread is not created.

That does that mean exactly ? How are these builds (especally the second one) generated ? does that mean we have a very recent regression ? (and then we should file a new bug)

Julien, I've detected the same. It's not the same problem that we had in this bug, so I've created a new one https://bugzilla.mozilla.org/show_bug.cgi?id=833010

You mean Bug 841684 I suppose ? :)

Verified fixed of the following: If we receive a SMS while we are in EDIT MODE, the message is NOT lost and the thread is indeed created. This was tested on the following Unagi: Build ID: 20130313070202 Kernel Date: Dec 5 Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/e74dafa6b2d9 Gaia: b34e726147f8e671ad8c538b50900ccfbffcb084