Closed Bug 1206727 Opened 4 years ago Closed 4 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: julienw, Assigned: julienw)

References

Details

(Whiteboard: [sms-papercuts][p=1])

Attachments

(1 file)

[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 on attachment 8663690 [details] [review]
[gaia] julienw:1206727-dont-reload-thread-at-change > mozilla-b2g:master

here is a not-working WIP.
Not a blocker but will be nice to have in 2.5 timeframe.
blocking-b2g: 2.5? → ---
Whiteboard: [sms-papercuts]
Whiteboard: [sms-papercuts] → [sms-papercuts][p=1]
Assignee: nobody → felash
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.
Attachment #8663690 - Flags: feedback?(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!
Attachment #8663690 - Flags: feedback?(azasypkin) → feedback+
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 ?
Attachment #8663690 - Flags: review?(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!
Attachment #8663690 - Flags: review?(azasypkin) → review+
Fixed the last comments and landed:

master: https://github.com/mozilla-b2g/gaia/commit/4b595b02f0767ed86cb5c13d2c961fc84108579b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
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
Attachment #8663690 - Flags: approval-gaia-v2.5?
Hey Mahe, how come this approval request was not handled for 1 month ?
Thanks
Flags: needinfo?(mpotharaju)
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)
Flags: needinfo?(mpotharaju)
Comment on attachment 8663690 [details] [review]
[gaia] julienw:1206727-dont-reload-thread-at-change > mozilla-b2g:master

Not needed for TV 2.5
Attachment #8663690 - Flags: approval-gaia-v2.5? → approval-gaia-v2.5-
You need to log in before you can comment on or make changes to this bug.