833010 - [SMS] [Edit mode] Receiving a SMS while edit-mode it's not working properly

Status: VERIFIED FIXED

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

Description

[Borja Salguero [:borjasalguero]]

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

Comment 1

[Borja Salguero [:borjasalguero]]

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).

Comment 2

[Maria Angeles Oteo (:oteo)]

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

Comment 3

[Alex Keybl [:akeybl]]

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

Comment 4

[Fernando Campo (:fcampo)]

Borja is on PTO, so stealing it

Comment 5

[Fernando Campo (:fcampo)]

Attachment: link to https://github.com/mozilla-b2g/gaia/pull/7803

Comment 6

[Borja Salguero [:borjasalguero]]

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

Comment 7

[Borja Salguero [:borjasalguero]]

Attachment: Pull Request

Comment 8

[Etienne Segonzac (:etienne)]

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.

Comment 9

[Borja Salguero [:borjasalguero]]

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.

Comment 10

[Borja Salguero [:borjasalguero]]

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

Comment 11

[Etienne Segonzac (:etienne)]

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|.

Comment 12

[Borja Salguero [:borjasalguero]]

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

Comment 13

[Borja Salguero [:borjasalguero]]

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

Comment 14

[Borja Salguero [:borjasalguero]]

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 15

[Etienne Segonzac (:etienne)]

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.

Comment 16

[Borja Salguero [:borjasalguero]]

All changes done! Ready for review ;)

Comment 17

[Etienne Segonzac (:etienne)]

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 :)).

Comment 18

[Borja Salguero [:borjasalguero]]

Cool! Enjoy your vacations! ;).

Comment 19

[Julien Wajsberg [:julienw]]

Comment on attachment 706390 [details]
Pull Request

commented on the pull request.

please ask the review again when fixed !

Thanks !

Comment 20

[Borja Salguero [:borjasalguero]]

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 21

[Julien Wajsberg [:julienw]]

Comment on attachment 706390 [details]
Pull Request

r=me with the latest nits addressed

Comment 22

[Borja Salguero [:borjasalguero]]

Thanks a lot for your review Julien!!!

Comment 23

[Julien Wajsberg [:julienw]]

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

Comment 24

[John Ford [:jhford] CET/CEST Berlin Time]

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.

Comment 25

[Julien Wajsberg [:julienw]]

Taking the uplift

Comment 26

[Julien Wajsberg [:julienw]]

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 ?

Comment 27

[John Ford [:jhford] CET/CEST Berlin Time]

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

Comment 28

[John Ford [:jhford] CET/CEST Berlin Time]

(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!

Comment 29

[Mila Davydova]

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.

Comment 30

[Julien Wajsberg [:julienw]]

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)

Comment 31

[Borja Salguero [:borjasalguero]]

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

Comment 32

[Julien Wajsberg [:julienw]]

You mean Bug 841684 I suppose ? :)

Comment 33

[mlevin]

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