bug#68401: 30.0.50; ERC 5.6-git: `erc-cmd-GMSG', `erc-cmd-AMSG', `erc-cmd-GME', `erc-cmd-AME'. 2nd attempt

[J.P.]

Emanuel Berg <incal@dataswamp.org> writes:

>> From fa8ae9dcc306d16cccdd6aa7c2bac242b90adbdb Mon Sep 17 00:00:00 2001
> From: Emanuel Berg <incal@dataswamp.org>
> Date: Sat, 13 Jan 2024 03:40:05 +0100
> Subject: [PATCH] Functions for ERC.

Emacs doesn't seem to be very picky about a commit's subject line, but
I'd prefer those for ERC that aren't mechanical or administrative in
nature to be somewhat unique and distinguishable at a glance, such as

  Make erc-cmd-AMSG session-local, add /GMSG /AME /GME

> `erc-cmd-GMSG', `erc-cmd-GME' and `erc-cmd-AME' was added.
> `erc-cmd-AMSG' was changed so that it does what the docstring says.
> * lisp/erc/erc.el: functions were added/modified to/in this file.
>
> (bug#68401)

I believe the guidelines call for a commit body to be formatted as a
change log entry, for example:

  * lisp/erc/erc.el (erc-cmd-AMSG): Make good on behavior described in
  the doc string by limiting damage to the current connection.
  (erc-cmd-GMSG, erc-cmd-GME, erc-cmd-AME): New functions, all IRC
  "slash commands".  (Bug#68401)

> ---
>  lisp/erc/erc.el | 38 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
> index 478683a77f5..aeb7722b563 100644
> --- a/lisp/erc/erc.el
> +++ b/lisp/erc/erc.el
> @@ -4016,16 +4016,44 @@ erc--split-string-shell-cmd
>  ;;                    Input commands handlers
>  ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
>  
> -(defun erc-cmd-AMSG (line)
> -  "Send LINE to all channels of the current server that you are on."
> -  (interactive "sSend to all channels you're on: ")
> +(defun erc-cmd-GMSG (line)
> +  "Send LINE to all channels on all networks you are on.
> +Interactively, prompt for the line of text to send."
> +  (interactive "sSend to all channels: ")

I question the wisdom of having new slash commands serve double duty as
interactive Emacs commands (at least those handling chat input). This
reservation has nothing to do with M-x erc-cmd-FOO <RET> being less
faithful (or whatever) to the traditional IRC experience than /FOO
<RET>. Rather, it stems from a need to prioritize consistent feedback
and promote maintainability by only having a single path for chat input
to reach the server (except under special circumstances).

Normally, when a user submits chat input at the prompt, ERC engages in a
series of validation checks before pushing a message out the door. These
steps are bypassed when someone invokes what's normally a slash command
via M-x. For example, if you /DISCONNECT and issue an /AMSG at the
prompt, you'll see "Process not running" in the echo area, and the input
will remain there for further editing, killing, etc. However, if you run
M-x erc-cmd-AMSG <RET>, the message will be inserted in all target
buffers, even though nothing is actually sent, which is misleading.
Obviously, we can't make `erc-cmd-AMSG' non-interactive because it's
been `commandp' forever. But new related commands don't have to follow
its (IMO flawed) example.

As far as counterarguments go, the only one that comes to mind for
making these `commandp' is that doing so also makes managing interactive
menus for modules like `bufbar', `nickbar', and `button' easier. For
example, at first glance, making `erc-cmd-KICK' interactive would appear
to streamline its inclusion in `erc-nick-popup-alist' and obviate the
need for an `erc-button-cmd-KICK'. However, if you look closely at this
arrangement, you'll see that even if `erc-cmd-KICK' were made a proper
Emacs command, a button-specific wrapper would still be necessary
because it makes special accommodations for the potential lack of a
channel context from which to draw membership rolls for completion. Such
a thing isn't necessary when issuing a /KICK <TAB> at the prompt because
the function `pcomplete/erc-mode/KICK' knows it's already running in a
channel.

>    (setq line (erc-trim-string line))

It might be nice to remove at most one space, for cases where a user
wants to send preformatted text. OTOH, normal /MSG doesn't do this, so
perhaps we shouldn't here either.

>    (erc-with-all-buffers-of-server nil
> -    (lambda ()
> -      (erc-channel-p (erc-default-target)))
> +    (lambda () (erc-channel-p (erc-default-target)))
> +    (erc-send-message line)))

Without first checking for connectivity, we run into another situation
in which messages may be inserted but not sent, similar to the bit about
commands being potentially "misleading," above. The most obvious way to
solve this is to check for "physical" connectivity with something like:

  (erc-with-all-buffers-of-server nil #'erc-server-process-alive
    (when (and erc--target (erc--current-buffer-joined-p))
      (erc-send-message line))))

Alternatively, you can check for "logical" connectivity, which is
probably more in keeping with traditional design principles:

  (erc-with-all-buffers-of-server nil nil
    (when (and erc-server-connected erc--target (erc--current-buffer-joined-p))
      (erc-send-message line))))

One minor downside of this second method is that IRC adjacent protocols
and aberrant proxy servers that happen to skip 376/422 and also provide
some (possibly &local) "control channel" won't be detected. (BTW, you
won't be needing the `erc--target' in either example if you rebase atop
the latest master.)

> +(put 'erc-cmd-GMSG 'do-not-parse-args t)
> +
> +(defun erc-cmd-AMSG (line)
> +  "Send LINE to all channels of the current network.
> +Interactively, prompt for the line of text to send."
> +  (interactive "sSend to all channels on this network: ")
> +  (setq line (erc-trim-string line))
> +  (erc-with-all-buffers-of-server erc-server-process
> +    (lambda () (erc-channel-p (erc-default-target)))

         ^ Indentation. This macro is declared "indent 2"

>      (erc-send-message line)))
>  (put 'erc-cmd-AMSG 'do-not-parse-args t)
>  
> +(defun erc-cmd-GME (line)
> +  "Send LINE as an action to all channels on all networks you are on.
> +Interactively, prompt for the line of text to send."
> +  (interactive "sSend action to all channels: ")

This command currently fails when invoked interactively. For example, if
I run M-x erc-cmd-GME <RET> hi <RET> from any ERC buffer belonging to a
connected session, nothing appears in the server logs or any ERC buffer.
This needs addressing if you're intent on keeping these interactive,
which I'm rather against for reasons previously noted.

> +  (erc-with-all-buffers-of-server nil
> +    (lambda () (erc-channel-p (erc-default-target)))
> +    (erc-cmd-ME line) ))

This currently suffers from the same problem as /GMSG regarding
disconnected buffers. However you address this, it's probably best to
use the same approach for fixing both functions.

> +(put 'erc-cmd-GME 'do-not-parse-args t)
> +
> +(defun erc-cmd-AME (line)
> +  "Send LINE as an action to all channels on the current network.
> +Interactively, prompt for the line of text to send."
> +  (interactive "sSend action to all channels on this network: ")

This command also appears do to nothing when invoked via M-x.

> +  (erc-with-all-buffers-of-server erc-server-process
> +    (lambda () (erc-channel-p (erc-default-target)))

         ^ Indentation again.

> +    (erc-cmd-ME line) ))
> +(put 'erc-cmd-AME 'do-not-parse-args t)
> +
>  (defun erc-cmd-SAY (line)
>    "Send LINE to the current query or channel as a message, not a command.
>  
> -- 
> 2.39.2

The attached patch is a unit test for all four commands. It doesn't
bother asserting M-x behavior (because see above). Please try to make it
pass without changing the test itself (unless there's a bug).

  $ git am /tmp/0002-5.x-Add...etc.patch

  $ make -C test lisp/erc/erc-scenarios-misc-commands.log \
    SELECTOR=erc-scenarios-misc-commands--AMSG-GMSG-AME-GME

0002-5.x-Add-tests-for-ERC-slash-commands-AMSG-GMSG-etc.patch
Description: Text Data