1621266 - "Add OTR Fingerprint" context menu shown for IRC chat rooms

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect, P2)

Description

[Magnus Melin [:mkmelin]]

The Add OTR Fingerprint context menu is shown for chat rooms, which doesn't make a lot of sense.

It is also shown for offline contacts, but do not work there. (Nothing happens). My test case is from GTalk.

https://searchfox.org/comm-central/rev/efb534c4a9bf90f090a40dadfae1eb928c6ea9b8
/chat/modules/OTRUI.jsm#149

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: 1621266-otr-context.diff

This should do the trick.
I remember there was a method which returned a boolean if the contact was a group or single, in order to prevent OTR features inside group chats, but I can't seem to find it.

Also, I'm gonna set this bug as a regression because I worked on this feature on Bug 1552256 and on Bug 1552227 and it was working properly, so something must have broken it.
Maybe the richlistitem de-xbl?

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9132447 [details] [diff] [review]
1621266-otr-context.diff

Review of attachment 9132447 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/modules/OTRUI.jsm
@@ +145,5 @@
>      let doc = win.document;
>      OTRUI.removeBuddyContextMenu(doc);
>    },
>  
> +  addBuddyContextMenu(buddyContextMenu, target, doc) {

Since you now do not use the target as such, it seems preferable to pass in the contact, like

addBuddyContextMenu(buddyContextMenu, doc, contact)

::: mail/components/im/content/chat-messenger.js
@@ +121,5 @@
>  
> +  // Prevent showing OTR related context menu items if:
> +  // * The OTR feature is currently not enabled.
> +  // * The target's status is currently offline (1) or unknown (0).
> +  // * The target can't send messages.

nit: usually hypens are used instead of stars for this

@@ +125,5 @@
> +  // * The target can't send messages.
> +  if (
> +    gOtrEnabled &&
> +    this.target.contact &&
> +    ![0, 1].includes(this.target.contact.statusType) &&

please use Ci.imIStatusInfo.STATUS_UNKNOWN and Ci.imIStatusInfo.STATUS_OFFLINE instead of the magic numbers

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: 1621266-otr-context.diff

Thanks for the review, I updated everything and changed a couple of things.

• I'm now calling the OTRUI.removeBuddyContextMenu(document); method earlier before any validation, as sometimes the context menu wasn't getting cleared when quickly right clicking between online and offline contacts.
• I'm using this.target.contact.statusType > Ci.imIStatusInfo.STATUS_OFFLINE to validate online statues.
• I updated the comment comment to match the direction of the condition, positive instead of negative.

I'm asking a quick feedback due to these changes.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9132648 [details] [diff] [review]
1621266-otr-context.diff

Review of attachment 9132648 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/im/content/chat-messenger.js
@@ +129,5 @@
> +  // - The target can send messages.
> +  if (
> +    gOtrEnabled &&
> +    this.target.contact &&
> +    this.target.contact.statusType > Ci.imIStatusInfo.STATUS_OFFLINE &&

I'm not sure it's a good idea to depend on the numeric ids of the statusType being in a specific order. Would be more clear to compare directly to the wanted statuses.

Comment 5

[Alessandro Castellani [:aleca]]

Attachment: 1621266-otr-context.diff

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4af2fc631df9
Avoid showing 'Add OTR Fingerprint' in the buddy context menu for unsupported conversations. r=mkmelin