Re: PATCH: isearch-yank-until-char

[Karl Fogel <kfogel@red-bean.com>]

On 24 Aug 2019, Drew Adams wrote:
>> I've left the code as-is; the current patch is attached here.
>> Do we have consensus to install it?
>
>Well, I don't agree, FWIW.  You seem to have ignored
>my message & patch (Aug 20).  Did you read or try it?
>(You were cc'd: "RE: PATCH: isearch-yank-until-match".)

No, I didn't.  I mean, I saw it, but didn't have time to try it.  Remember that you started out that new thread (on August 14th) with these words:

> This is similar to what Karl submitted today.
> Not a replacement for that; something different.

Since your followups were all in that thread, I assumed they were about something that was "not a replacement" for what I'd posted, and therefore wouldn't affect the question of whether what I posted should be installed.  (I rely pretty heavily on thread discipline, not just in Emacs Devel but generally.)

>Commands like `isearch-yank-until-char', which yank
>consecutive buffer text at the search point, are better
>if they can also work with backward search.  My patch
>implements that for this command and others.

It sounds like your patch does two conceptually distinct things:

1) Implement one or more new commands.

2) Make a bunch of isearch commands work with backward search.

If I were to install my patch (as it's currently written, though maybe with the keybinding changed), that doesn't really affect any of your new code.

However, I would still say your patch should be divided into two conceptually distinct patches, one for (1) and one for (2).  This is not a judgement about the technical merits or the UX merits of your patch.  I'm just saying that we should do one thing at a time.

>And I argued that `C-M-c' would be better used in
>Isearch for my command `isearch-yank-through-move',
>which initiates a recursive edit to allow arbitrary
>cursor movement.  In that case, `C-M-c' both starts
>and ends such movement (since globally it is
>`exit-recursive-edit').

*nod*  That's a separate question from the above.  I don't think it's very important that C-M-c be the binding for `isearch-yank-until-char'.  If we want to save C-M-c for this other potential use, that seems reasonable to me.

Your patch suggested "C-M-." for 'isearch-yank-until-char', which seems good.  Another possibility, to keep the "c" for "char" mnemonic, is to use M-c.

Right now that key seems to toggle case-sensitivity, but I'm not sure that's deliberate -- according to the `isearch-forward' documentation, "M-s c" is for that, while "M-c" isn't documented at all.  Given that the current action of M-c isn't documented, and given that another keybinding both does that action and is documented to do so, using "M-c" for `isearch-yank-until-char' might be okay.

The code and comments around "M-c" vs "M-s c" are a little more complex than I expected.  Here's the relevant block from the definition of `isearch-mode-map':

    ;; Some bindings you may want to put in your isearch-mode-hook.
    ;; Suggest some alternates...
    (define-key map "\M-c" 'isearch-toggle-case-fold)
    (define-key map "\M-r" 'isearch-toggle-regexp)
    (define-key map "\M-e" 'isearch-edit-string)

    (put 'isearch-toggle-case-fold :advertised-binding "\M-sc")
    (put 'isearch-toggle-regexp    :advertised-binding "\M-sr")
    (put 'isearch-edit-string      :advertised-binding "\M-se")

Both bindings actually work.  But you won't see "\M-sc" explicitly bound anywhere in the map, even though it's the advertised binding!  Instead, the actual binding happens elsewhere via a call to the macro `isearch-define-mode-toggle':

  (isearch-define-mode-toggle case-fold "c" nil
   ...)

The code above suggests that it is not important for M-c to remain redundantly bound to `isearch-toggle-case-fold', but I could be wrong.  If anyone knows more, please say.  If we can't figure out the answer, I guess I'd say let's go with "C-M-.", out of general conservatism.

Best regards,
-Karl