Be consistent about which conversation is set on the outgoing message

RESOLVED INVALID

Status

Instantbird
Conversation
RESOLVED INVALID
4 years ago
3 years ago

People

(Reporter: arlolra, Unassigned)

Tracking

(Blocks: 1 bug)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
Created attachment 8567284 [details] [diff] [review]
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.
(Reporter)

Updated

4 years ago
Attachment #8567284 - Flags: review?(florian)
Attachment #8567284 - Flags: review?(clokep)
Attachment #8567284 - Flags: review?(aleth)
(Reporter)

Updated

4 years ago
Blocks: 954310

Comment 1

4 years ago
What's the rationale for using the UIConv and not the target for both?
(Reporter)

Comment 2

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

Comment 3

4 years ago
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.

Updated

4 years ago
Attachment #8567284 - Flags: review?(clokep)
Attachment #8567284 - Flags: review?(aleth)
Attachment #8567284 - Attachment is patch: true
Attachment #8567284 - Attachment mime type: text/x-patch → text/plain
(Reporter)

Comment 4

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

Comment 5

4 years ago
Yes, good catch!
I don't understand what this is fixing / what's currently inconsistent.
(Reporter)

Comment 7

3 years ago
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
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.
(Reporter)

Comment 9

3 years ago
> 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.
(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.
(Reporter)

Comment 11

3 years ago
Ah, interesting. Ok, sounds like things are fine as is.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → INVALID
(Reporter)

Updated

3 years ago
Attachment #8567284 - Flags: review?(florian)
You need to log in before you can comment on or make changes to this bug.