"J.P." <jp@neverwas.me> writes:

> diff --git a/lisp/erc/erc-goodies.el b/lisp/erc/erc-goodies.el
> index 05a21019042..0bf4bb9537c 100644
> --- a/lisp/erc/erc-goodies.el
> +++ b/lisp/erc/erc-goodies.el
> @@ -38,9 +38,10 @@ erc-controls-highlight-regexp
> [...]
>  (declare-function erc-buffer-list "erc" (&optional predicate proc))
>  (declare-function erc-error "erc" (&rest args))
> @@ -384,9 +385,11 @@ erc-get-fg-color-face
>  (define-erc-module irccontrols nil
>    "This mode enables the interpretation of IRC control chars."
>    ((add-hook 'erc-insert-modify-hook #'erc-controls-highlight)
> -   (add-hook 'erc-send-modify-hook #'erc-controls-highlight))
> +   (add-hook 'erc-send-modify-hook #'erc-controls-highlight)
> +   (define-key erc-mode-map "\C-c\C-c" #'erc-toggle-interpret-controls))
>    ((remove-hook 'erc-insert-modify-hook #'erc-controls-highlight)
> -   (remove-hook 'erc-send-modify-hook #'erc-controls-highlight)))
> +   (remove-hook 'erc-send-modify-hook #'erc-controls-highlight)
> +   (erc-compat-call define-key erc-mode-map "\C-c\C-c" nil t)))

Actually, messing with `erc-mode-map' like this is sure to break
configs. Snippets like

  (with-eval-after-load 'erc
    (define-key erc-mode-map (kbd "RET") nil)
    (define-key erc-mode-map (kbd "C-c C-c") #'erc-send-current-line))

are fairly common [1]. Of course, mutating `erc-mode-map' is prevalent
throughout ERC [2], but we probably shouldn't perpetuate the practice.
If this were a brand new module, we could just give it its own
minor-mode map. But doing so now might just serve to confuse.

Here are two possibilities that seem relatively benign:

  1. Leave the original binding in `erc-mode-map' but autoload its
     command, `erc-toggle-interpret-controls', and make it smart enough
     to enable `erc-irccontrols-mode' when necessary and maybe print a
     message explaining why.

  2. Mutate the local map when the module's loaded, but only do so
     conditionally, by first checking to see if the key is taken
     (locally or in `erc-mode-map'). Likewise, only remove the binding
     if it's set to `erc-toggle-interpret-controls' locally.

Door #1 is more conservative because it introduces zero churn, AFAICT.
But without any plan for deprecation, it's tantamount to kicking the can
down the road. Door #2 keeps things more compartmentalized and walled
off (ERC does tout itself as being modular, after all), but there's a
nonzero chance of churn in some cases, for example, when

  - someone sets a binding for `erc-mode-map' after the module is loaded
  - some map depends on `erc-mode-map' as a parent

Otherwise, conditionally updating the current local map seems to have
the same functional effect here as using a minor-mode map and
conditionally overriding that in all ERC buffers (since this is a global
module).

The main downside I see for #2 is that it adds yet another facet of
complexity to an already cluttered landscape, seeing as minor-mode maps
are likely to become more common with the introduction of local modules.
Perhaps of less concern is that there's no precedent for it (in ERC).
That said, if we were to go bold and convert all `erc-mode-map'
mutations across all ERC libraries, it would at least offer some sense
of consistency and predictability going forward. (I suspect there may be
other downsides I've not yet considered, though, so if you're privy to
any, please do share.)

Because #1 is pretty easy to visualize, I've gone ahead and provided a
POC of #2 (for the `irccontrols' module only) in the third patch below.
Anyone interested, please have a look.

Thanks.


[1] See (info "(erc) Sample Configuration").

[2] See `erc-button-setup' and the modules toggles for `erc-sound' and
    `erc-ring' and top-level forms in erc-match.el and erc-log.el.