Closed Bug 1601102 Opened 5 years ago Closed 5 years ago

Support the echo-message capability for IRC

Categories

(Chat Core :: IRC, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Instantbird 73

People

(Reporter: clokep, Assigned: clokep)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 1 obsolete file)

The echo-message extension for IRC is fairly useful (https://ircv3.net/specs/extensions/echo-message-3.2):

  • It provides acknowledgement that the server received a sent message.
  • Is helpful for bouncers with multiple clients connected.

It is also pretty easy to implement.

Attached patch Patch v1 (obsolete) (deleted) — Splinter Review

This implements the additional capability, registers it as a CAP handler, and then modifies the sendMsg method to handle the echo capability.

This also modifies some of the other capability negotiations to be consistent between == and ===. I think this got copied and pasted around between a bunch of different files.

Attachment #9113342 - Flags: review?(martin)

Unfortunately none of the networks I use (moznet, freenode, oftc) actually support this, I tested this using testnet.inspircd.org and joined a channel and then sent a message. I then checked the protocol log and ensured that the message was echoed back to us.

From a first look this patch seems sane. I'm writing this from a machine where building this would take half a day, so I'll ask instead of check myself and be totally lazy: doesn't this lead to the echoed message being displayed as incoming instead of outgoing? I remember that being the "hard" part of implementing this. Sure, with the default TB message theme you can't really tell either way, but with bubbles or similar it's rather obvious.

(In reply to Martin Giger [:freaktechnik] from comment #3)

doesn't this lead to the echoed message being displayed as incoming instead of outgoing?

Ah! That's a good catch. I'll need to make some more changes.

Attachment #9113342 - Flags: review?(martin) → review-

Looking at my old notes, I've also seen me mentioning znc.in/self-message, but I'd argue that's too legacy to bother supporting now (and we could probably also drop the znc server-time one, but hey, maybe someone's still running znc 1.4 or something)

Attached patch Patch v2 (deleted) — Splinter Review

This fixes the incoming / outgoing issue that Martin pointed out.

I'll take a look at the ZNC version, it seems to be documented at https://wiki.znc.in/Query_buffers#Self_messages and https://defs.ircdocs.horse/info/selfmessages.html.

Attachment #9113342 - Attachment is obsolete: true
Comment on attachment 9113356 [details] [diff] [review] Patch v2 Review of attachment 9113356 [details] [diff] [review]: ----------------------------------------------------------------- Ah, I guess this would be a case where labelled-responses would let us map incoming messages to messages we sent and do the mapping for incoming messages with more confidence. So it wouldn't show messages sent by the same user but from another device connected to the bouncer as outgoing. But this is definitely a step in the right direction, next step is a "sending" indicator for high ping situations ;) Important: I have not tested this yet, but I don't think that should block an r+ on this. As mentioned, I'm pretty sure I've written essentially the same patch a couple years ago, and the main blocker for me was the issues I've mentioned so far. ::: chat/protocols/irc/ircBase.jsm @@ +37,5 @@ > + // mark it as outgoing. Otherwise, the message is incoming. > + if ( > + aAccount._activeCAPs.has("echo-message") && > + aAccount.normalizeNick(aMessage.origin) == > + aAccount.normalizeNick(aAccount._nickname) Isn't there an already normalized version somewhere on the account instance? Or was that on messages? I recall being really confused about normalizing at one point.
Attachment #9113356 - Flags: review+

(In reply to Martin Giger [:freaktechnik] from comment #7)

Ah, I guess this would be a case where labelled-responses would let us map
incoming messages to messages we sent and do the mapping for incoming
messages with more confidence. So it wouldn't show messages sent by the same
user but from another device connected to the bouncer as outgoing.

I think this will work for "most" situations, unless the nick is different on different devices, but frankly I'm unsure exactly how to deal with that.

next step is a "sending" indicator for high ping situations ;)

That would likely make sense for all of our protocols (and I think is even suggested in the echo-message spec!)

::: chat/protocols/irc/ircBase.jsm
@@ +37,5 @@

  • // mark it as outgoing. Otherwise, the message is incoming.
  • if (
  • aAccount._activeCAPs.has("echo-message") &&
  • aAccount.normalizeNick(aMessage.origin) ==
  •  aAccount.normalizeNick(aAccount._nickname)
    

Isn't there an already normalized version somewhere on the account instance?
Or was that on messages? I recall being really confused about normalizing at
one point.

I think you might be thinking of account buddies having a normalized name as part of the object. I checked other bits of our code and we use similar checks to this (e.g. in changeBuddyNick). As far as I can tell we don't store our own normalized nickname anywhere.

Messages also don't have a "normalized" version on them, although that would possibly make sense.

Pushed by clokep@gmail.com:
https://hg.mozilla.org/comm-central/rev/792c378e19bc
Support the echo-message capability for IRC. r=freaktechnik

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED

I decided to not hold this back on supporting the znc version.

Target Milestone: --- → Instantbird 73
Regressions: 1646049

We should back this out from beta since it's causing bug 1646049 which will break chat for a fair amount of users. Can someone please organise that.

Flags: needinfo?(vseerror)
Flags: needinfo?(rob)
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(vseerror)
Flags: needinfo?(rob)
Flags: needinfo?(mkmelin+mozilla)

You're enabling a broken feature behind a pref? That doesn't make any sense. Given the time constraints, this should be backed out from beta, you can leave it on trunk.

Flags: needinfo?(vseerror)
Flags: needinfo?(rob)

I do not believe we need to back this out yet, can we finish investigating this on bug 1646049?

Flags: needinfo?(vseerror)
Flags: needinfo?(rob)

This is really a release management issue. I suggested to pull it out from beta until a fix is found. You have two days left before it goes to ESR.

Flags: needinfo?(vseerror)
Flags: needinfo?(rob)

As I said on the other issue, this likely affects very few users as echo-message is not widely deployed yet.

I believe I already have a fix anyway, I'm just waiting for a build.

Flags: needinfo?(vseerror)
Flags: needinfo?(rob)

Wayne, it has been common practice to fix regressions on releases via backout as long as no other fix is available. Could you please consider that approach in this case.

Just to be clear: I suggest to back this out from beta only before it goes to ESR.

Flags: needinfo?(vseerror)

Turns out that a straight backout isn't all that easily possible due to various merge conflicts. Let's see what bug 1646049 brings. A one-line removal can temporarily disable the feature.

Flags: needinfo?(vseerror)
Regressions: 1649445
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: