Support the echo-message capability for IRC
Categories
(Chat Core :: IRC, enhancement)
Tracking
(Not tracked)
People
(Reporter: clokep, Assigned: clokep)
References
(Blocks 1 open bug, )
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
freaktechnik
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•5 years ago
|
||
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.
Assignee | ||
Comment 2•5 years ago
|
||
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.
Comment 3•5 years ago
|
||
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.
Assignee | ||
Comment 4•5 years ago
|
||
(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.
Assignee | ||
Updated•5 years ago
|
Comment 5•5 years ago
|
||
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)
Assignee | ||
Comment 6•5 years ago
|
||
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.
Comment 7•5 years ago
|
||
Assignee | ||
Comment 8•5 years ago
|
||
(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
Assignee | ||
Comment 10•5 years ago
|
||
I decided to not hold this back on supporting the znc version.
Comment 11•4 years ago
|
||
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.
Comment 12•4 years ago
|
||
Maybe we could just enable (disable) it behind a pref. For https://searchfox.org/comm-central/rev/a8444d358c7abb921d81ee97d73b6f6ba26c7c8a/chat/protocols/irc/ircEchoMessage.jsm#24 - but let's take that in bug 1646049.
Comment 13•4 years ago
|
||
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.
Comment 14•4 years ago
|
||
BTW, you'd pref off this line:
https://searchfox.org/comm-central/rev/a8444d358c7abb921d81ee97d73b6f6ba26c7c8a/chat/protocols/irc/irc.jsm#2325
Assignee | ||
Comment 15•4 years ago
|
||
I do not believe we need to back this out yet, can we finish investigating this on bug 1646049?
Comment 16•4 years ago
|
||
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.
Assignee | ||
Comment 17•4 years ago
|
||
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.
Updated•4 years ago
|
Comment 18•4 years ago
|
||
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.
Comment 19•4 years ago
|
||
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.
Description
•