Closed Bug 1010216 Opened 6 years ago Closed 5 years ago

[Messages][Drafts] Improve consistency of the draft code in ThreadUI and ThreadListUI

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S12 (15may)

People

(Reporter: julienw, Assigned: azasypkin)

References

Details

(Whiteboard: [sms-sprint-2.2S11][p=2])

Attachments

(1 file)

Once bug 881469 lands, we'll be able to:

* remove the use of ThreadUI.draft
* possibly use ThreadUI.handleDraft in ThreadUI.enterThread
* possibly get rid of ThreadListUI.draftLinks
* possibly use a "draftId" dataset instead of abusing "threadId" for drafts.
(maybe in another bug) possibly make thread-bound and thread-less drafts behave more alike.
See Also: → 1014226
We've just noticed that there is an inconsistency in how "ThreadListUI.draftLinks" map is handled. It's supposed that we store <a> references in this map [1], but "removeThread" method expects <li> reference here instead [2].

So in the scope of this bug we should revisit/fix this too.

[1] https://github.com/mozilla-b2g/gaia/blob/09ffd07964de1ad15d7ab34ab89c78bdf223605a/apps/sms/js/thread_list_ui.js#L654

[2] https://github.com/mozilla-b2g/gaia/blob/09ffd07964de1ad15d7ab34ab89c78bdf223605a/apps/sms/js/thread_list_ui.js#L294
Will take a look if I can improve something here without major rewrite. My main goal currently is to get rid of direct InboxView->ConversationView dependency via ConversationView.draft to unblock bug 1155509.
Blocks: sms-drafts
Flags: needinfo?(azasypkin)
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Summary: [Messages] Improve consistency of the draft code in ThreadUI and ThreadListUI → [Messages][Drafts] Improve consistency of the draft code in ThreadUI and ThreadListUI
Assignee: nobody → azasypkin
Flags: needinfo?(azasypkin)
Blocks: 1155509
Status: NEW → ASSIGNED
Whiteboard: [sms-sprint-2.2S11]
Target Milestone: --- → 2.2 S12 (15may)
Comment on attachment 8601454 [details] [review]
[gaia] azasypkin:bug-1010216-improve-drafts > mozilla-b2g:master

Hey Julien,

May I have your early feedback on the patch? I've left my doubts and questions at github.

Thanks!
Attachment #8601454 - Flags: feedback?(felash)
Whiteboard: [sms-sprint-2.2S11] → [sms-sprint-2.2S11][p=2]
Comment on attachment 8601454 [details] [review]
[gaia] azasypkin:bug-1010216-improve-drafts > mozilla-b2g:master

So there are good things in this patch. I'm quite sad that we don't change handleDraft but because this is mostly to unblock the split, let's do that and file a separate bug for the rest of the work.

Also in a quite short term (let's hope june) drafts will be stored along with threads directly in the same DB. So let's not spend too much time refactoring and thinking too much now as this would be wasted work anyway.
Attachment #8601454 - Flags: feedback?(felash) → feedback+
Comment on attachment 8601454 [details] [review]
[gaia] azasypkin:bug-1010216-improve-drafts > mozilla-b2g:master

(In reply to Julien Wajsberg [:julienw] (PTO May 8th -> May 17th) from comment #6)
> Comment on attachment 8601454 [details] [review]
> [gaia] azasypkin:bug-1010216-improve-drafts > mozilla-b2g:master
> 
> So there are good things in this patch. I'm quite sad that we don't change
> handleDraft but because this is mostly to unblock the split, let's do that
> and file a separate bug for the rest of the work.

Yep, generalizing handleDraft deserves separate patch especially in the light of future split of Composer and Conversation.

Hey Steve, since Julien is on PTO could you please review the patch?

Thanks!
Attachment #8601454 - Flags: review?(schung)
The patch looks great! I only some questions on github and nits. BTW the there are some weird failed unit test on treeherder, not sure why it happens.
Blocks: 1164431
Blocks: 1164435
(In reply to Steve Chung [:steveck] from comment #8)
> The patch looks great! I only some questions on github and nits. 

Thanks for review! Replied and nits are fixed in a separate commit.

> BTW the there are some weird failed unit test on treeherder, not sure why it happens.

It looks like Treeherder picks/groups other PRs that contain the same commits as target one - I have PR for bug 1010216 (with failing tests :) ) that is based on the same commits + WIP one.
Comment on attachment 8601454 [details] [review]
[gaia] azasypkin:bug-1010216-improve-drafts > mozilla-b2g:master

Thanks for the refactoring for drafts, now it looks clear and tidy :)
Attachment #8601454 - Flags: review?(schung) → review+
Thanks!
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
No longer depends on: 1166117
Duplicate of this bug: 1014226
Blocks: 1176976
No longer blocks: 1176976
You need to log in before you can comment on or make changes to this bug.