Closed Bug 1649445 Opened 2 years ago Closed 2 years ago

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

Categories

(Chat Core :: IRC, defect)

defect

Tracking

(thunderbird_esr78 fixed, thunderbird79 fixed)

RESOLVED FIXED
Instantbird 80
Tracking Status
thunderbird_esr78 --- fixed
thunderbird79 --- fixed

People

(Reporter: jorgk-bmo, Assigned: clokep)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.

Summary: /me on a public channel is show twice for servers with echo-message support → /me on a public IRC channel is show twice for servers with echo-message support
Attached patch Patch v1 (obsolete) — Splinter Review

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.

Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attachment #9161698 - Flags: review?(mkmelin+mozilla)
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
Attachment #9161698 - Flags: review?(mkmelin+mozilla) → review+
Attached patch Patch v2Splinter Review

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

Attachment #9161698 - Attachment is obsolete: true
Attachment #9162268 - Flags: review+

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/b0c9985debc5
Unify code-paths for displaying incoming IRC messages. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 80

Please set the tracking flags accordingly and request uplift.

Flags: needinfo?(clokep)

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

Flags: needinfo?(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.
Attachment #9162268 - Flags: approval-comm-esr78?
Attachment #9162268 - Flags: approval-comm-beta?
Regressed by: 1601102
No longer regressed by: 1646049
Comment on attachment 9162268 [details] [diff] [review]
Patch v2

Approved for beta
Attachment #9162268 - Flags: approval-comm-beta? → approval-comm-beta+

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

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

Approved for esr78
Attachment #9162268 - Flags: approval-comm-esr78? → approval-comm-esr78+

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

Spoke too early, still broken in 78.1.1.

Summary: /me on a public IRC channel is show twice for servers with echo-message support → /me on a public IRC channel is shown twice for servers with echo-message support
Blocks: tb78found
Regressions: 1657703
You need to log in before you can comment on or make changes to this bug.