[Messages][Drafts][Refactoring] Move draft saving on visibility change to ThreadUI, and only store drafts if new one is saved

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: evhan55, Assigned: evhan55)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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

5 years ago
Assignee: nobody → evelyn
(Assignee)

Comment 1

5 years ago
Created attachment 8366123 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15738

- `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 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+
(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

5 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.
(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.
Another thought: this only exists because we support multiple threadless drafts.
(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!
(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 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

5 years ago
Landed and closing: https://github.com/mozilla-b2g/gaia/commit/6c9fac83b743711d02c4a32e45cd145d8a85f7eb
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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 → ---
You need to log in before you can comment on or make changes to this bug.