Opened 6 years ago

Closed 6 years ago

#15090 closed defect (fixed)

freenode's new +q numeric (728) is unrecognized

Reported by: marienz Owned by: elb
Milestone: 2.10.4 Component: IRC
Version: 2.10.3 Keywords:
Cc:

Description

Handling a user complaint in my role as freenode staff member I noticed getting the channel quiets list ("/mode +q") results in no output to the channel tab, and this in the debug window (if you open it first):

irc: Unrecognized message: :leguin.freenode.net 728 dovemarienz ##marienz q $~a marienz!marienz@freenode/staff/marienz 1335862180

Please add support for this numeric. Alternatively, *please* consider just showing unrecognized numerics in their relevant tab. I'm assuming this has been suggested to you before and you rejected it, although I cannot find a ticket for it. Just in case it hasn't: the current behavior leads to confused users, frequently believing it's the *network* that is broken (if the ignored numeric is one telling them why they cannot join some channel or talk on it). It would be much appreciated if you either stopped doing this or included a notification in the UI telling people to use a different IRC client if Pidgin behaves oddly.

Attachments (2)

irc_unknown_numerics_patch.diff (1.4 KB) - added by EionRobb 6 years ago.
A different patch that has the same behaviour as elb's but without the nasty nasty goto's
purple_irc_unknown_numerics.diff (2.7 KB) - added by elb 6 years ago.
Print unknown numerics belonging to active conversations to their conversations

Download all attachments as: .zip

Change History (13)

comment:1 follow-up: Changed 6 years ago by elb

The problem is that, for this, as in all the other numerics that freenode just randomly made up, it's often hard to tell exactly where the message belongs. Even irssi prints this message to the server messages window, and doesn't know to put it in the channel. This is one price of stapling random crap onto the protocol with no documentation, rhyme, or reason. We have special cased several freenode numerics over the years, but they change -- +q alone has been handled at least three or four different ways since I have been using Freenode. (Though this might be its first wholly separate numeric, rather than changing mode letter or ban pattern, I'm not sure.)

As you may or may not know, Pidgin does not have a server messages window, as it does not really fit anywhere in the Pidgin UI. This causes a problem for us for trapping arbitrary messages to someplace the user can see them. The debug window is an unfortunate and poor catch-all, there's no question about that.

Pidgin is not, and likely will never be, a "power user" IRC client. Part of this is simply because its interface is geared toward IM-style messaging, not IRC-style messaging. We are comfortable with this limitation, although why try to mitigate its particularly wild excesses where possible.

The "right" solution to this, insomuch as there is a right solution, is probably to add an explicit 728 handler. We do not like to add handlers for made-up numerics, for two primary reasons: 1) it can be the case, and has been the case in the past, that different networks use the same numeric for different purposes, with no good way to differentiate; and 2) networks like to change their made-up numerics more often than their ops change underwear. However, we have certainly conceded this in the past, when a network mistakenly chooses to make up a numeric for a particularly important feature. I will consider it for +q. (As you mention, we have added explicit handlers for made-up critical info status numerics for Freenode in the past.)

As to whether it is in fact the client or the network that is broken, that is certainly a matter of perspective.

comment:2 in reply to: ↑ 1 Changed 6 years ago by marienz

Replying to elb:

The problem is that, for this, as in all the other numerics that freenode just randomly made up,

Our ircd is based on charybdis, and this new numeric was inherited from them. Any recent charybdis-based ircd (with quiets enabled) will use it too. I have asked them to clarify why they picked this, and was told this one should not collide with anything else currently in use, but is (so far) charybdis-specific. It's unfortunately not the same one OFTC uses, as their choice collides with an ircnet numeric.

it's often hard to tell exactly where the message belongs.

If the command following the prefix is three digits, and the second argument after that is a channel the client is on, you can be fairly certain displaying the message in that channel's window/tab would be reasonable. Clients really do make use of this (see for example the "default" branch in proto-irc.c:process_numeric in xchat).

Even irssi prints this message to the server messages window, and doesn't know to put it in the channel.

This is unfortunately correct for older irssis. Newer irssis use event_target_received for 728/729, which does what I describe above.

This is one price of stapling random crap onto the protocol with no documentation, rhyme, or reason. We have special cased several freenode numerics over the years, but they change -- +q alone has been handled at least three or four different ways since I have been using Freenode. (Though this might be its first wholly separate numeric, rather than changing mode letter or ban pattern, I'm not sure.)

The change was announced on http://blog.freenode.net/2011/09/ircd-upgrades/ , which we advertised a bunch (through wallops, possibly even globals) shortly before those upgrades happened. The charybdis NEWS file also mentions the change. We've never had much luck getting people to respond to our requests for testing, and we tend to not personally test all IRC clients (especially ones that fit our own usage patterns as poorly as things like Pidgin do). I don't see how to improve this situation. Perhaps we should gather some usage statistics and personally test the popular ones prior to upgrades, but I do not have high hopes of that happening given time constraints.

I believe the reason for the change was to make it easier for clients that request the +b and +q list at the same time to see which is which (the 367/368 numeric for the normal banlist does not include the mode character).

Also note that several client messages have a final component that is human-readable english text, for the benefit of clients that do not explicitly support this message.

As you may or may not know, Pidgin does not have a server messages window, as it does not really fit anywhere in the Pidgin UI. This causes a problem for us for trapping arbitrary messages to someplace the user can see them. The debug window is an unfortunate and poor catch-all, there's no question about that.

Right, but dealing with this by making a lot of information impossible to get at, even if requested by the client explicitly, makes for a poor user experience and is making you tremendously impopular with network staff (at least on freenode, but I doubt it's just us). We really do expect people to be able to see the MOTD somewhere, for example, at least when they ask for it explicitly (there are some notices in there that we need people to be able to read). "/motd" and "/quote motd" does not work ("Unrecognized command" for the former, the latter had no visible effect as Pidgin helpfully swallowed all MOTD content).

Pidgin is not, and likely will never be, a "power user" IRC client. Part of this is simply because its interface is geared toward IM-style messaging, not IRC-style messaging. We are comfortable with this limitation, although why try to mitigate its particularly wild excesses where possible.

You may be comfortable with it, but we get a fair number of people trying to actually use Pidgin as an IRC client, or use Pidgin as a channel op, and get very confused. The complete dropping of unrecognized messages makes this more confusing, not less. I'd argue that if you do not wish to support such things you should not implement commands like /mode (which can take any mode character) and /quote (obvious).

The "right" solution to this, insomuch as there is a right solution, is probably to add an explicit 728 handler. We do not like to add handlers for made-up numerics, for two primary reasons: 1) it can be the case, and has been the case in the past, that different networks use the same numeric for different purposes, with no good way to differentiate;

The numeric the charybdis developers picked for this should not collide with anything in popular use today (although obviously that's no guarantee for the future...)

and 2) networks like to change their made-up numerics more often than their ops change underwear.

freenode ircd upgrades (which introduce such numerics changes) occur infrequently (think one a year).

However, we have certainly conceded this in the past, when a network mistakenly chooses to make up a numeric for a particularly importantt feature. I will consider it for +q. (As you mention, we have added explicit handlers for made-up critical info status numerics for Freenode in the past.)

These are usually added or changed to make it possible for clients to tell certain messages apart, assuming that clients that do not recognize them will still display them. If you just show these messages somewhere (which can be as terrible as "the most recently active IRC channel or query tab/window", although I believe it'd be better to attempt to use the 2nd argument as a channel name as mentioned above) I would consider it a vast improvement.

As to whether it is in fact the client or the network that is broken, that is certainly a matter of perspective.

The only other client I am aware of that completely hides away unrecognized messages is empathy, and I'm about to file a bug there. I've asked around and have not been able to find any others. Make of that what you will.

Apart from getting this specific bug fixed I'm also trying to break the cycle of #freenode helpers telling people to use a "proper" client instead of Pidgin, and Pidgin users insisting Pidgin is an IRC client and should be usable by channel ops. If this is not a use case you care about (which given Pidgin being primarily an IM client would make sense, in my opinion) an explicit drop of some of your channel op features would make our life easier.

comment:3 Changed 6 years ago by elb

OK, a few specific points; I think much of the rest will be taken care of if/when I either get around to implementing this numeric, or someone else does.

First, the MOTD is stored by Pidgin, and accessible via Accounts -> (IRC account name) -> View MOTD. I don't see any particular reason we can't make it visible when the user types /motd, that should probably be a feature request.

Second, I understand and acknowledge everything you said up there about making up numerics -- there's always a reason, sometimes they conflict, sometimes they don't. It still makes it hard for clients. The fact that freenode only upgrades about once a year is irrelevant on all levels. Not only is it far from the only IRC network, Pidgin release lifecycles on many distributions (think Debian Stable) are even longer than that.

Third, that hack regarding unknown numerics is somewhat bletcherous, but seems safe. I'll make a note to modify the Pidgin unknown numeric handler to check the numeric and the possible "target" argument, then direct to a conversation rather than the debug log if possible.

Fourth, as I said before, we really don't recommend Pidgin for power IRC users, and we're quite frank with users about that when they come to us. You may feel free to tell your users the same thing. If they fight, send them to us. Pidgin IRC is intended for very casual IRC users (for example, users who clicked Help | Chat with Support type menu items). That said, we do know of quite a few "power" IRC users who use Pidgin. I can't fathom why. Our position (or at least, my position) on this is that there are many, far superior, IRC clients out there, and that IRC really doesn't "fit" the Pidgin interface very well anyway. That said, I'm not inclined to neuter it farther just because this might confuse some people. If people are happy with the level of functionality that Pidgin provides, we're happy for them to use it. If there are functionality improvements which can be put in without heroic efforts, we're happy to take patches.

comment:4 Changed 6 years ago by elb

I don't quite like this patch, but I'm going to put it here for now to keep track of it. I think maybe we need a new message flag to really do this "right". :-/

Changed 6 years ago by EionRobb

A different patch that has the same behaviour as elb's but without the nasty nasty goto's

comment:5 Changed 6 years ago by marienz

My apologies for missing the MOTD option, I had overlooked that in the UI. No need for a /motd, in my opinion, as a more GUI-oriented user than me would probably have found that.

I understand these changes happen quickly enough for them to be a pain for client developers (as you pointed out irssi doesn't handle the +q one great either, which is an annoyance to me), I just wanted to point out "networks like to change their made-up numerics more often than their ops change underwear" exaggerates slightly.

Thanks for the patch. I've tested it a bit and it is an improvement (and I'm assuming other libpurple-based clients will pick this up without needing further work on their end?). Two points though (on elb's patch, I noticed the second patch only after I'd started writing this comment, I'll go look at it now):

I'm fairly certain the null check in the loop should be against end, not cur. With the check as it is I can get pidgin to crash by doing "/quote info", which ends up calling g_strndup(cur, end - cur) with end null, and glib fails the resulting huge allocation. If I could get the server to send me an even shorter message the loop would call strchr on NULL + 1, which wouldn't be very healthy either.

I think it'd be slightly prettier to only report the arguments after the channel name to the user. That is: "tmp = purple_utf8_salvage(end + 1);" (which I believe is safe with the null check fixed, although it could theoretically log empty messages). This removes: the prefix (which should be the source server, which users will not care about), the numeric itself (which very few users will recognize), the 2nd argument (which is normally our own nick, as RFC1459 says this must be "the target" of the message), and the third argument, which we've just made sure is the name of the conversation the message appears in. So I believe those are all redundant.

A moderately visible unsupported numeric this exposes is 328 aka RPL_CHANNELURL: if you join #pidgin it'll now show you ":http://pidgin.im/".

It is unfortunate this still misses 435 aka "Cannot change nickname while banned on channel", but without a server/status window that is probably not fixable cleanly. Shall I file a fresh bug on that, as it's probably worth throwing a dialog up for similar to the "The nickname ... is already being used." one?

comment:6 Changed 6 years ago by marienz

The second patch has a typo that makes it not compile (PURPLE_MESSAGE_STYSTEM should be PURPLE_MESSAGE_SYSTEM) and a mistake that makes it not work: args[0] needs to be split into 5 pieces, not 4, as the last piece includes the entire remainder of the string. So positions[3] ends up being "##marienz q $~a marienz!marienz@freenode/staff/marienz 1335995600" which will never match an active conversation. It works with both occurrences of 4 changed to 5. Printing positions[4] instead of args[0] will then conveniently print a shorter message (see reasoning in the previous comment).

comment:7 Changed 6 years ago by elb

EionRobb?: You're right, it gets rid of the GOTO, but it also does a bunch of unnecessary allocation. I'm not convinced one of those poor behaviors is particularly superior to the other. ;-) But then again, GOTO doesn't really offend me... The numeric check is also lossy there.

marienz: You are absolutely correct, regarding the NULL check. The point about trimming the leading info is also good. However, we should probably keep the numeric itself. I'll make those changes.

Changed 6 years ago by elb

Print unknown numerics belonging to active conversations to their conversations

comment:8 Changed 6 years ago by elb

Please see the updated patch. It's less crashy and has the output you suggested.

comment:9 Changed 6 years ago by marienz

That looks pretty good.

It might be nice to skip over a leading ":" in the "remainder" printed: a ":" in that position is part of the irc protocol and just indicates everything after it is one argument, even if it has spaces.

I've not figured out a way to break this one. Also tried to join capitalized channels in case the conversation-finding is case sensitive, but this seems to come out right, at least on freenode's ircd (looks like the conversation name is taken from the server's JOIN message, not user input, and case used there matches case in the numerics).

Thanks for spending time improving this.

comment:10 Changed 6 years ago by elb

I thought about the : issue when I saw that 328. It's pretty easy to take care of the 328 case (exactly one escaped argument after the preamble arguments), but harder ("harder" -- it'd be easier if we did the mass bustup/allocation of EionRobb's patch, actually, then re-glued with a GString or something, but ... yuck extra allocation for an error handler) for the case where there are several remaining arguments. I'll add the quick : removal and apply this, with any luck (though I make no promises) it'll make it into 2.10.4.

We normalize channel and nicknames for joining, it's a bug in Pidgin if capitalization causes a match failure.

comment:11 Changed 6 years ago by elb@…

  • Milestone set to 2.10.4
  • Resolution set to fixed
  • Status changed from new to closed

(In 22d544ffc6215f22b81d1dc34931bad4c82775c2):
Print unknown IRC numerics to channels if we can associate them. Basically, if they are of the form:

<routing> <numeric> <...> <channel> <more>

... then we print <numeric>: <more> to <channel>. Thanks to marienz from Freenode for suggesting this.

Fixes #15090

Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!