885283 - [sms] receiving a SMS from an existing thread but on a new thread container is broken

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

STR:
* wake up in a new shiny day
* receive a SMS from a number you already have a thread for, but this must be your first SMS for the day

Expected:
* we receive a notification
* the thread is moved to a new thread container, with "today" as title

Actual:
* we receive a notification
* the thread is not moved, and there is a logcat error :

E/GeckoConsole( 2184): [JavaScript Error: "TypeError: threadsContainer is null" {file: "app://sms.gaiamobile.org/js/message_manager.js" line: 146}]

Restarting the app shows the thread correctly.

leo+ because this is a real bug that needs to be resolved.
qawanted to know if that happens on v1.1.

Comment 1

[Alex Keybl [:akeybl]]

(In reply to Julien Wajsberg [:julienw] from comment #0)
> Expected:
> * we receive a notification
> * the thread is moved to a new thread container, with "today" as title

To clarify, we believe Julien means that we want the new SMS to display below the Today header in an SMS window without a restart. 

If this is a regression, we'll block. Otherwise not.

Comment 2

[Julien Wajsberg [:julienw]]

Attachment: patch v1

* Refactored how threads are added when a new message is received
* This incidentally resolves the bug
* FixedHeader can't refresh the title too much, I was afraid when I saw it's
  being called when a thread is removed
* adds tests
---
 apps/sms/js/fixed_header.js               |   11 +-
 apps/sms/js/message_manager.js            |   55 +-------
 apps/sms/js/thread_list_ui.js             |   48 +++++++
 apps/sms/test/unit/mock_messages.js       |   10 +-
 apps/sms/test/unit/mock_utils.js          |    1 +
 apps/sms/test/unit/thread_list_ui_test.js |  193 +++++++++++++++++++++++++++++
 6 files changed, 261 insertions(+), 57 deletions(-)

Comment 3

[Julien Wajsberg [:julienw]]

Comment on attachment 766009 [details] [diff] [review]
patch v1

PR is https://github.com/mozilla-b2g/gaia/pull/10540

Comment 4

[Deepa Subramanian]

This defect still repros. The new SMS thread is not moved to display below the "Today" header.

Leo Build ID: 20130620070209
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/3e144cc723d3
Gaia: c507ff65b51194383da65f8927164f078a3e1f79
Platform Version: 18.0

Comment 5

[Julien Wajsberg [:julienw]]

qawanted: we need to know if that happens on 1.0.1 too.

Comment 6

[Corey Frang [:gnarf]]

Comment on attachment 766009 [details] [diff] [review]
patch v1

Review of attachment 766009 [details] [diff] [review]:
-----------------------------------------------------------------

There are merge conflicts, so I can't really test/review - can you rebase and ping me for review again?

::: apps/sms/js/fixed_header.js
@@ +29,5 @@
>      headings = view.querySelectorAll(selector);
>      // after setting up a new headings selection, check the scroll again
>      // in case we removed the header we have currently fixed.
> +    if (scrollingTimeout === null) {
> +      scrollingTimeout = setTimeout(scrolling);

Why are we forcing async here?  The old code path was calling it immediately?

::: apps/sms/js/thread_list_ui.js
@@ +409,5 @@
> +    // thread without requesting Gecko, so we increase the performance and we
> +    // reduce Gecko requests.
> +    return {
> +        id: message.threadId,
> +        participants: [message.sender],

I feel like this is going to need to proxy the message.recipients too?

I know you just moved this function over, so perhaps this gets solved in another bug?

Comment 7

[Natalya Kot [:nkot]]

does not reproduce on v1.0.1
 - after receiving a new for the day SMS, existing SMS thread is moved to the new thread under "Today" header 

Unagi build info:
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/9c62297d11b0
Gaia   93241eb6c5d6c110710fad8da3ccd4423312b0c9
Build  20130624070215
Version 18.0

Comment 8

[Julien Wajsberg [:julienw]]

So regression, so leo+ ?

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 766009 [details] [diff] [review]
patch v1

Review of attachment 766009 [details] [diff] [review]:
-----------------------------------------------------------------

a new patch is coming

::: apps/sms/js/fixed_header.js
@@ +29,5 @@
>      headings = view.querySelectorAll(selector);
>      // after setting up a new headings selection, check the scroll again
>      // in case we removed the header we have currently fixed.
> +    if (scrollingTimeout === null) {
> +      scrollingTimeout = setTimeout(scrolling);

refresh can be called several times

::: apps/sms/js/thread_list_ui.js
@@ +409,5 @@
> +    // thread without requesting Gecko, so we increase the performance and we
> +    // reduce Gecko requests.
> +    return {
> +        id: message.threadId,
> +        participants: [message.sender],

yep, that is bug 881213. Won't probably get solved for leo ;)

Comment 10

[Julien Wajsberg [:julienw]]

Attachment: patch v2

Diff with previous patch is mainly about FixedHeader changes. So requesting r from fcampo too, for that part.

Pull request is still https://github.com/mozilla-b2g/gaia/pull/10540, travis seems to be happy.

Full commit log:

* Refactored how threads are added when a new message is received
* This incidentally resolves the bug
* made FixedHeader's refresh and scroll handler so that it's being called once
  per tick only. Then we won't be afraid of calling refresh too many.
* remove some unused code in FixedHeader
* adds tests for FixedHeader
* Fix indentation of createThreadMockup
---
 apps/sms/js/fixed_header.js               |   23 ++--
 apps/sms/js/message_manager.js            |   55 +--------
 apps/sms/js/thread_list_ui.js             |   48 ++++++++
 apps/sms/test/unit/mock_messages.js       |   10 +-
 apps/sms/test/unit/mock_utils.js          |    1 +
 apps/sms/test/unit/thread_list_ui_test.js |  192 +++++++++++++++++++++++++++++
 6 files changed, 262 insertions(+), 67 deletions(-)

Comment 11

[Julien Wajsberg [:julienw]]

Attachment: patch v2 - second try

forgot the new test file in the previous patch...

Comment 12

[Corey Frang [:gnarf]]

Comment on attachment 768414 [details] [diff] [review]
patch v2 - second try

Review of attachment 768414 [details] [diff] [review]:
-----------------------------------------------------------------

r=me still :)

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 768414 [details] [diff] [review]
patch v2 - second try

borja, could you please have a look to the FixedHeader part ? It's already r+ by Corey but I'd like another look for the FixedHeader change.

PR is at https://github.com/mozilla-b2g/gaia/pull/10540

Thanks !

Comment 14

[Julien Wajsberg [:julienw]]

Attachment: patch v3

some renaming in FixedHeader to better reflect the current meanings.

still r=gnarf for the general SMS part, but requiring borja's review for fixed header stuff.

github PR is still at https://github.com/mozilla-b2g/gaia/pull/10540

Comment 15

[Julien Wajsberg [:julienw]]

master: 79fb5cd0996e91d1c47e9dfaf9ee00c79cea4da2

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 769760 [details] [diff] [review]
patch v3

carrying r=gnarf

Comment 17

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

Uplifted 79fb5cd0996e91d1c47e9dfaf9ee00c79cea4da2 to:
v1-train: 05e12f78f512ad2360ff03f389cd6c54dfa189ee

Comment 18

[sarsenyev]

Verified on Leo V1.1
A new notification is received and the SMS message moved to a new threat container with the new "Today" title

Environmental  Variables:
Build ID: 20130717070237
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/582e3a7018b0
Gaia: c506c50adaaebcf729ac3c27887ba2931ab79040
Platform Version: 18.1
RIL Version: 01.01.00.019.138

Comment 19

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

v1.1.0hd: 05e12f78f512ad2360ff03f389cd6c54dfa189ee