Closed
Bug 1206727
Opened 9 years ago
Closed 9 years ago
[Messages][Inbox] Don't reload the conversation node after saving draft
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: julienw, Assigned: julienw)
References
Details
(Whiteboard: [sms-papercuts][p=1])
Attachments
(1 file)
46 bytes,
text/x-github-pull-request
|
azasypkin
:
review+
azasypkin
:
feedback+
jocheng
:
approval-gaia-v2.5-
|
Details | Review |
[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•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
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•9 years ago
|
||
Not a blocker but will be nice to have in 2.5 timeframe.
blocking-b2g: 2.5? → ---
Assignee | ||
Updated•9 years ago
|
Whiteboard: [sms-papercuts]
Assignee | ||
Updated•9 years ago
|
Blocks: sms-sprint-FxOS-S9
Whiteboard: [sms-papercuts] → [sms-papercuts][p=1]
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → felash
Assignee | ||
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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+
Assignee | ||
Comment 6•9 years ago
|
||
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 7•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
Fixed the last comments and landed:
master: https://github.com/mozilla-b2g/gaia/commit/4b595b02f0767ed86cb5c13d2c961fc84108579b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 9•9 years ago
|
||
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?
Assignee | ||
Comment 10•9 years ago
|
||
Hey Mahe, how come this approval request was not handled for 1 month ?
Thanks
Flags: needinfo?(mpotharaju)
Comment 11•9 years ago
|
||
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 12•9 years ago
|
||
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.
Description
•