1206727 - [Messages][Inbox] Don't reload the conversation node after saving draft

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

[Blocking Requested - why for this release]:

STR:
1. open a conversation.
2. type some text in the composer.
3. Press back.
4. before bug 1176976 lands, press 'save draft'. after bug 1176976 lands there will be no dialog.

Expected:
* The conversation node is refreshed with the new data.

Actual:
* The conversation node is completely reloaded, and we see the "contact" part blinking with the phone number.

It's there since forever but it's been a lot more visible after we landed bug 958105. So I think we should try to fix this in 2.5.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] julienw:1206727-dont-reload-thread-at-change > mozilla-b2g:master

Comment 2

[Julien Wajsberg [:julienw]]

Comment on attachment 8663690 [details] [review]
[gaia] julienw:1206727-dont-reload-thread-at-change > mozilla-b2g:master

here is a not-working WIP.

Comment 3

[Francisco Jordano [:arcturus] [:francisco]]

Not a blocker but will be nice to have in 2.5 timeframe.

Comment 4

[Julien Wajsberg [:julienw]]

Comment on attachment 8663690 [details] [review]
[gaia] julienw:1206727-dont-reload-thread-at-change > mozilla-b2g:master

Hey Oleg,

what do you think ?

I took the simplest path so that we can try to ask for an uplift.

The alternatives are that we keep a state of the rendered item:
* either as a JSON-encoded state in a dataset
* or keep it in memory

I think we'll eventually want to keep the rendered state in memory, but this is more work.

Also the current patch still has the issue of rerendering completely the node if we receive or send a message but the thread does not change its position in the inbox. I don't think we can fix this efficiently without using a in-memory state.

Comment 5

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8663690 [details] [review]
[gaia] julienw:1206727-dont-reload-thread-at-change > mozilla-b2g:master

Looks sane to me, just few nits and one issue with empty "number" section in the conversation node.

I think we can fix the rest of the cases later.

Thanks!

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 8663690 [details] [review]
[gaia] julienw:1206727-dont-reload-thread-at-change > mozilla-b2g:master

Hey Oleg, I fixed your comments and added unit tests. Do you think we need integration tests as well ?

Comment 7

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8663690 [details] [review]
[gaia] julienw:1206727-dont-reload-thread-at-change > mozilla-b2g:master

(In reply to Julien Wajsberg [:julienw] from comment #6)
> Comment on attachment 8663690 [details] [review]
> [gaia] julienw:1206727-dont-reload-thread-at-change > mozilla-b2g:master
> 
> Hey Oleg, I fixed your comments and added unit tests.

Looks good to me, just one question at Github (regarding unit test for the default contact title content).

> Do you think we need integration tests as well ?

I think no, but one of the existing is currently failing (seems we don't remove "has-draft" css class when draft is discarded), please take care of this before landing :)

Thanks!

Comment 8

[Julien Wajsberg [:julienw]]

Fixed the last comments and landed:

master: https://github.com/mozilla-b2g/gaia/commit/4b595b02f0767ed86cb5c13d2c961fc84108579b

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 8663690 [details] [review]
[gaia] julienw:1206727-dont-reload-thread-at-change > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Exists for a long time but was made more visible by bug 958105.
[User impact] if declined: In some situations (especially when changing a draft and going back to inbox), there is a very visible glitch while we reload the affected conversation.
[Testing completed]: yes, both manually and automatic.
[Risk to taking this patch] (and alternatives if risky): quite low, thanks to the unit and integration tests we have.
[String changes made]: none

Comment 10

[Julien Wajsberg [:julienw]]

Hey Mahe, how come this approval request was not handled for 1 month ?
Thanks

Comment 11

[Mahendra Potharaju [:mahe]]

Hey Julien, The reason being, we are shipping updates to users from the new b2g_ota branch which is a close copy of master. And 2.5 is being used for TV. Am approving only P1/P2 patches for 2.5. 

Currently am working through the bugs and updating this status. Apologies for the delay. (I was on PTO post Mozlando)

Comment 12

[Josh Cheng [:josh]]

Comment on attachment 8663690 [details] [review]
[gaia] julienw:1206727-dont-reload-thread-at-change > mozilla-b2g:master

Not needed for TV 2.5