1175503 - [Messages][NG] Get rid of direct ConversationView dependencies from Compose component

Status: RESOLVED FIXED

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

Description

[Oleg Zasypkin [:azasypkin]]

In this bug we'd like to remove direct ConversationView dependencies from Compose component:

* ConversationView.draft;
* ConversationView.on('recipientschange', ...);
* ConversationView.recipients (+ Threads.active.participants).

This should make separation of Conversation and NewMessages views easier + narrow down ConversationView.draft usage (after this bug it will be used only inside ConversationView).

Comment 1

[Julien Wajsberg [:julienw]]

In my view, this was a goal of bug 1040144: by instanciating the compose view, we could pass the view as constructor (or factory) parameter.

Comment 2

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #1)
> In my view, this was a goal of bug 1040144: by instanciating the compose
> view, we could pass the view as constructor (or factory) parameter.

For this bug, I have a bit different idea in mind at the moment, will ask for feedback once I have something working :) Basically I want Composer to be entirely not aware of recipients.

Comment 3

[Julien Wajsberg [:julienw]]

OK, let me see once you have something :)

My original view was to have the same interface for recipients for both NewMessage view and ConversationView, so the composer could just call one polymorphic method and get done with it.

Comment 4

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL (wip)

Here is the first WIP PR for what I had in mind.

Hey Julien,

It's still "very" WIP and some cases don't work yet, but it would be great to have your early feedback to validate this idea or come up with better one. I've left a bit lengthy explanation at GitHub to be on the same page :)

Thanks!

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 8624224 [details] [review]
GitHub pull request URL (wip)

I didn't look really close but I don't mind using a modifier when we change the panel.

However I really think we need to provide value accessors to the Composer component, instead of the values themselves.

Comment 6

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8624224 [details] [review]
GitHub pull request URL (wip)

Hey Julien,

Does it look better for you now?

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 8624224 [details] [review]
GitHub pull request URL (wip)

yeah, this makes me happier :)

Comment 8

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8624224 [details] [review]
GitHub pull request URL (wip)

Hey Julien,

I've fixed your comments and added/updated unit tests for the affected parts. I could not find any issues during on-device-testing, so I need your fresh eyes on this :)

Could you please take a look?

Thanks!

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 8624224 [details] [review]
GitHub pull request URL (wip)

Let's do one more round :)

Comment 10

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8624224 [details] [review]
GitHub pull request URL (wip)

Added 3d commit :)

Please, tell if I missed something you wanted me to do!

Thanks!

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 8624224 [details] [review]
GitHub pull request URL (wip)

r=me with the indentation nit in the test

Comment 12

[Oleg Zasypkin [:azasypkin]]

Thanks!

Treeherder is green, landed.

Master: https://github.com/mozilla-b2g/gaia/commit/e8e047d02fd3528325a3b0cc05391cbdb2cbffa7