1649445 - /me on a public IRC channel is shown twice for servers with echo-message support

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[Jorg K (CEST = GMT+2)]

As the summary states:
Use /me on a public IRC channel on a server that supports echo-message and you get it twice.

Looks like another tweak to https://hg.mozilla.org/comm-central/rev/44e6b9b6759a is necessary.

Comment 1

[Patrick Cloke [:clokep]]

Attachment: Patch v1

There's two fixes here:

• Consolidates the code for displaying messages (this fixes the incoming action messages to work properly).
• Applies the same logic to displaying outgoing action messages as is used to display normal outgoing messages.

It seems to work OK in my testing.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9161698 [details] [diff] [review]
Patch v1

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

Looks reasonable to me. r=mkmelin with the below

::: chat/protocols/irc/ircCommands.jsm
@@ +139,5 @@
> +  // enabled with the echo-message capability. If this is not enabled, just
> +  // assume the message was received and immediately show it.
> +  if (!this._account._activeCAPs.has("echo-message")) {
> +    // Show the action on our conversation.
> +    conv.writeMessage(account._nickname, "/me " + om.message, {outgoing: true});

looks like this needs ../mach eslint --fix chat/

::: chat/protocols/irc/ircUtils.jsm
@@ +264,5 @@
>  
>    return true;
>  }
> +
> +/*

nit: a documentation comment starts with /**

/* is just uncommenting something

@@ +294,5 @@
> +      : aMessage.origin;
> +  }
> +  if (aIsNotification) {
> +    params.notification = true;
> +  }

this could be added above where params is declared

Comment 3

[Patrick Cloke [:clokep]]

Attachment: Patch v2

Oops, definitely forgot to re-run eslint. I made the requested changes. Thanks!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b0c9985debc5
Unify code-paths for displaying incoming IRC messages. r=mkmelin

Comment 5

[Jorg K (CEST = GMT+2)]

Please set the tracking flags accordingly and request uplift.

Comment 6

[Patrick Cloke [:clokep]]

Jorg -- I was planning to let this be checked in before requesting uplift.

Comment 7

[Patrick Cloke [:clokep]]

Comment on attachment 9162268 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Regression caused by (bug #): bug 1601102
User impact if declined: Users of IRC servers that have the "echo-message" capability enabled will see their own "action" messages twice in channels.
Testing completed (on c-c, etc.): None (chat on c-c is busted right now, so testing isn't going to happen there).
Risk to taking this patch (and alternatives if risky): I think the risk is fairly low. I tested this in a private chat, a channel, and talking to myself with this patch and everything seemed to work OK. This combines some code to share a code-path.

Comment 8

[Wayne Mery (:wsmwk)]

Comment on attachment 9162268 [details] [diff] [review]
Patch v2

Approved for beta

Comment 9

[Wayne Mery (:wsmwk)]

FWIW Someone also does smoktests of chat on beta - but AIUI chat is still broken there too.

Comment 10

[Wayne Mery (:wsmwk)]

Comment on attachment 9162268 [details] [diff] [review]
Patch v2

Approved for esr78

Comment 11

[Rob Lemley [:rjl]]

Thunderbird 79.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/c3a7b7aa31af

Comment 12

[Rob Lemley [:rjl]]

Thunderbird 78.0.1:
https://hg.mozilla.org/releases/comm-esr78/rev/c3c605e5447b

Comment 13

[Jorg K (CEST = GMT+2)]

Thanks for the fix. Working with ZNC in both public and private chats, with and without /me.

Comment 14

[Jorg K (CEST = GMT+2)]

Spoke too early, still broken in 78.1.1.