1135224 - Be consistent about which conversation is set on the outgoing message

Status: RESOLVED INVALID

(Instantbird Graveyard :: Conversation, defect)

Description

[arlolra]

Attachment: outgoing.patch

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.115 Safari/537.36




Expected results:

Use the ui conversation itself for both, in sendMsg.

Comment 1

[aleth [:aleth]]

What's the rationale for using the UIConv and not the target for both?

Comment 2

[arlolra]

Only that you can do uiConv.target to get the conversation.

Comment 3

[aleth [:aleth]]

I'm pretty sure we'll want to use the prplIConversation for both here, but let's see what Flo says to this API change when he returns.

Comment 4

[arlolra]

That's fine. It just feels like we should be consistent here with whichever we choose.

Comment 5

[aleth [:aleth]]

Yes, good catch!

Comment 6

[Florian Quèze [:florian]]

I don't understand what this is fixing / what's currently inconsistent.

Comment 7

[arlolra]

Sorry, I guess it isn't apparent in the diff.

See the `conversation` for the "preparing-message" and "sending-message". In one it's `this` and the other `this.target`.

https://github.com/mozilla/releases-comm-central/blob/master/chat/components/src/imConversations.js#L354-L372

Comment 8

[Florian Quèze [:florian]]

I still don't really see a bug here. Is this causing a problem when using these notifications? Should we just improve the documentation?

Both this and this.target implement the prplIConversation interface, so the code seems to work. When preparing the message, it doesn't seem impossible to me that an add-on would want to change the target of the message (so set conv.target to a different value) based on the message content. When sending the message, changing the target doesn't make sense, so I don't see any reason for an add-on to need the UI conv at that point.

Comment 9

[arlolra]

> I still don't really see a bug here. Is this causing a problem when using these notifications? Should we just improve the documentation?

You're right, there's no bug and it isn't really causing a problem. I just thought it was a potential source of confusion. Feel free to close as wontfix or invalid if you disagree.

> Both this and this.target implement the prplIConversation interface, so the code seems to work. When preparing the message, it doesn't seem impossible to me that an add-on would want to change the target of the message (so set conv.target to a different value) based on the message content.

Are you talking about the changing the `conversation` on the outgoing message? That's a readonly property. If you mean changing the `target` on the UIConv, wouldn't that also affect every other message in the conversation? I don't think the current API would support that scenario, without removing the readonly restriction and making a few subsequent steps use `om.conversation` instead of `this.target`, if that's even something we'd care to support.

Comment 10

[Florian Quèze [:florian]]

(In reply to arlolra from comment #9)
> If you mean changing the `target` on
> the UIConv, wouldn't that also affect every other message in the
> conversation?

Yes it would; and that wouldn't necessarily be a problem. If you are switching to a different conversation target because the current one doesn't support something you are trying to do, there's no intensive to switch back until you've got something to send that isn't supported by the new target.

Just to give a specific example, it used to be that MSN censored some strings and just silently dropped any message containing them. An add-on could detect these strings, and switch the conversation to GTalk before that string is sent.

Comment 11

[arlolra]

Ah, interesting. Ok, sounds like things are fine as is.