Closed
Bug 959696
Opened 11 years ago
Closed 11 years ago
[Messages][Drafts][Refactoring] Move draft saving on visibility change to ThreadUI, and only store drafts if new one is saved
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: evhan55, Assigned: evhan55)
Details
Attachments
(1 file)
https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/message_manager.js#L103-L119
Currently:
MessageManager handles visibility change events for the app, and saves a draft if the app is hidden
Refactor:
ThreadUI should handle this visibility change event as well, and handle the draft saving. Also, the Drafts.store() call should only happen when the draft is saved, so it should be moved inside the inner if statement.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → evelyn
Assignee | ||
Comment 1•11 years ago
|
||
- `message_manager.js`
- remove the visibility change draft-saving behavior
- `thread_ui.js`
- add a visibility change handler
- add draft-saving behavior to visibility change handler
- Tests
- `message_manager_test.js`
- remove visibility change tests
- `thread_ui_test.js`
- add visibility change tests to check that draft saves properly on app hiding
- manual tests
- start a new draft => hide app => show app => alter draft => click back => check that it asks to 'replace' draft and not 'save' a new draft
Attachment #8366123 -
Flags: review?(waldron.rick)
Attachment #8366123 -
Flags: feedback?(felash)
Comment 2•11 years ago
|
||
Comment on attachment 8366123 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15738
works fine but I think we should also move MessageManager.draft to ThreadUI.draft in this bug.
Attachment #8366123 -
Flags: feedback?(felash) → feedback+
Comment 3•11 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #2)
> Comment on attachment 8366123 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/15738
>
> works fine but I think we should also move MessageManager.draft to
> ThreadUI.draft in this bug.
Also I'd like to question the very existence of MessageManager.draft, but that is maybe for another time.
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #3)
> (In reply to Julien Wajsberg [:julienw] from comment #2)
> > Comment on attachment 8366123 [details] [review]
> > https://github.com/mozilla-b2g/gaia/pull/15738
> >
> > works fine but I think we should also move MessageManager.draft to
> > ThreadUI.draft in this bug.
>
> Also I'd like to question the very existence of MessageManager.draft, but
> that is maybe for another time.
Yeah! Wish I could be around for that chat.
Comment 5•11 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #3)
> (In reply to Julien Wajsberg [:julienw] from comment #2)
> > Comment on attachment 8366123 [details] [review]
> > https://github.com/mozilla-b2g/gaia/pull/15738
> >
> > works fine but I think we should also move MessageManager.draft to
> > ThreadUI.draft in this bug.
>
> Also I'd like to question the very existence of MessageManager.draft, but
> that is maybe for another time.
When you click on a threadless draft and arrive at the new message view, there is no way to know which draft to load in. I could've used the id as a parameter in the hash, like `#new&draft=1`, but that would've required changing a lot of extant code that expects `hash === '#new'` and I was under the impression that hashes were going away eventually. MessageManager.draft exists as a temporary holding station to solve this problem.
Comment 6•11 years ago
|
||
Another thought: this only exists because we support multiple threadless drafts.
Comment 7•11 years ago
|
||
(In reply to Rick Waldron [:rwaldron] from comment #5)
> (In reply to Julien Wajsberg [:julienw] from comment #3)
> > (In reply to Julien Wajsberg [:julienw] from comment #2)
> > > Comment on attachment 8366123 [details] [review]
> > > https://github.com/mozilla-b2g/gaia/pull/15738
> > >
> > > works fine but I think we should also move MessageManager.draft to
> > > ThreadUI.draft in this bug.
> >
> > Also I'd like to question the very existence of MessageManager.draft, but
> > that is maybe for another time.
>
> When you click on a threadless draft and arrive at the new message view,
> there is no way to know which draft to load in. I could've used the id as a
> parameter in the hash, like `#new&draft=1`, but that would've required
> changing a lot of extant code that expects `hash === '#new'` and I was under
> the impression that hashes were going away eventually. MessageManager.draft
> exists as a temporary holding station to solve this problem.
Oki, so after bug 881469 we can try to remove it. Thanks for the insight!
Comment 8•11 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #7)
> Oki, so after bug 881469 we can try to remove it. Thanks for the insight!
Yes, absolutely.
Comment 9•11 years ago
|
||
Comment on attachment 8366123 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15738
Good to go, r=me
Attachment #8366123 -
Flags: review?(waldron.rick) → review+
Assignee | ||
Comment 10•11 years ago
|
||
Landed and closing: https://github.com/mozilla-b2g/gaia/commit/6c9fac83b743711d02c4a32e45cd145d8a85f7eb
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 11•11 years ago
|
||
This was reverted:
https://github.com/mozilla-b2g/gaia/commit/75122a82fb557f6d349a09ef14c37c9e1dddf0dc
Presumably for the same test failures we were seeing on TBPL:
https://tbpl.mozilla.org/php/getParsedLog.php?id=33893843&tree=B2g-Inbound
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 12•11 years ago
|
||
3 green travis runs:
https://travis-ci.org/mozilla-b2g/gaia/builds/17997162
https://travis-ci.org/mozilla-b2g/gaia/builds/17997144
https://travis-ci.org/mozilla-b2g/gaia/builds/17997619
Landed https://github.com/mozilla-b2g/gaia/commit/6e232d655b5ad98dddeb4f1c42e206019a1b7137
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•