[PATCH] Handle case where `beg` and `end` are strings instead of markers

[PATCH] Handle case where `beg` and `end` are strings instead of markers

[James N. V. Cash]

With the changes to pass affixes to the completion functions, the
begin and end points passed to the completion function are now
sometimes strings instead of numbers or markers. This handles that
case by searching for said prefix and suffix.

Without this, completion functions that call `completion--replace` error
out -- for example, completing tags in org-mode.

---
 lisp/minibuffer.el | 69 +++++++++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 29 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index ef71b4e6be..d75b771044 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1147,35 +1147,46 @@ completion--replace
                               newtext)
     ;; Remove all text properties.
     (set-text-properties 0 (length newtext) nil newtext))
-  ;; Maybe this should be in subr.el.
-  ;; You'd think this is trivial to do, but details matter if you want
-  ;; to keep markers "at the right place" and be robust in the face of
-  ;; after-change-functions that may themselves modify the buffer.
-  (let ((prefix-len 0))
-    ;; Don't touch markers in the shared prefix (if any).
-    (while (and (< prefix-len (length newtext))
-                (< (+ beg prefix-len) end)
-                (eq (char-after (+ beg prefix-len))
-                    (aref newtext prefix-len)))
-      (setq prefix-len (1+ prefix-len)))
-    (unless (zerop prefix-len)
-      (setq beg (+ beg prefix-len))
-      (setq newtext (substring newtext prefix-len))))
-  (let ((suffix-len 0))
-    ;; Don't touch markers in the shared suffix (if any).
-    (while (and (< suffix-len (length newtext))
-                (< beg (- end suffix-len))
-                (eq (char-before (- end suffix-len))
-                    (aref newtext (- (length newtext) suffix-len 1))))
-      (setq suffix-len (1+ suffix-len)))
-    (unless (zerop suffix-len)
-      (setq end (- end suffix-len))
-      (setq newtext (substring newtext 0 (- suffix-len))))
-    (goto-char beg)
-    (let ((length (- end beg)))         ;Read `end' before we insert the text.
-      (insert-and-inherit newtext)
-      (delete-region (point) (+ (point) length)))
-    (forward-char suffix-len)))
+  (let ((beg (if (number-or-marker-p beg)
+                 beg
+               (save-excursion
+                 (goto-char (minibuffer-prompt-end))
+                 (+ (search-forward beg)
+                    (length beg)))))
+        (end (if (number-or-marker-p end)
+                 end
+               (save-excursion
+                 (goto-char (point-max))
+                 (search-backward end)))))
+    ;; Maybe this should be in subr.el.
+    ;; You'd think this is trivial to do, but details matter if you want
+    ;; to keep markers "at the right place" and be robust in the face of
+    ;; after-change-functions that may themselves modify the buffer.
+    (let ((prefix-len 0))
+      ;; Don't touch markers in the shared prefix (if any).
+      (while (and (< prefix-len (length newtext))
+                  (< (+ beg prefix-len) end)
+                  (eq (char-after (+ beg prefix-len))
+                      (aref newtext prefix-len)))
+        (setq prefix-len (1+ prefix-len)))
+      (unless (zerop prefix-len)
+        (setq beg (+ beg prefix-len))
+        (setq newtext (substring newtext prefix-len))))
+    (let ((suffix-len 0))
+      ;; Don't touch markers in the shared suffix (if any).
+      (while (and (< suffix-len (length newtext))
+                  (< beg (- end suffix-len))
+                  (eq (char-before (- end suffix-len))
+                      (aref newtext (- (length newtext) suffix-len 1))))
+        (setq suffix-len (1+ suffix-len)))
+      (unless (zerop suffix-len)
+        (setq end (- end suffix-len))
+        (setq newtext (substring newtext 0 (- suffix-len))))
+      (goto-char beg)
+      (let ((length (- end beg)))         ;Read `end' before we insert the text.
+        (insert-and-inherit newtext)
+        (delete-region (point) (+ (point) length)))
+      (forward-char suffix-len))))

 (defcustom completion-cycle-threshold nil
   "Number of completion candidates below which cycling is used.
--
2.25.1

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[James N. V. Cash]

"James N. V. Cash" <james.nvc@gmail.com> writes:

> With the changes to pass affixes to the completion functions, the
> begin and end points passed to the completion function are now
> sometimes strings instead of numbers or markers. This handles that
> case by searching for said prefix and suffix.
>
> Without this, completion functions that call `completion--replace` error
> out -- for example, completing tags in org-mode.

Hm, while my patch does fix the error I was seeing, it makes completing
multiple items with completing-read-multiple not properly insert
subsequent completions.

I also note that I only saw the aberrant behaviour while using functions
that used CRM, so perhaps there's a more underlying cause and/or better
place to fix this.

James

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[Juri Linkov]

> Hm, while my patch does fix the error I was seeing, it makes completing
> multiple items with completing-read-multiple not properly insert
> subsequent completions.
>
> I also note that I only saw the aberrant behaviour while using functions
> that used CRM, so perhaps there's a more underlying cause and/or better
> place to fix this.

Please describe how do you expect this should behave from a UI point of view.
I see that when using on a simple call:

  (completing-read-multiple "Tags: " '(("tag1") ("tag2") ("tag3")))

subsequent completions can be selected properly
after replacing these lines in your patch

+                 (+ (search-forward beg)
+                    (length beg)))))

with

+                 (search-forward beg))))

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[James N. V. Cash]

Juri Linkov <juri@linkov.net> writes:

> Please describe how do you expect this should behave from a UI point of view.
> I see that when using on a simple call:
>
>   (completing-read-multiple "Tags: " '(("tag1") ("tag2") ("tag3")))
>
> subsequent completions can be selected properly
> after replacing these lines in your patch
>
> +                 (+ (search-forward beg)
> +                    (length beg)))))
>
> with
>
> +                 (search-forward beg))))

Ah, that seems to do it -- I was seeing subsequent completions only
partially inserting, but that change you suggest fixes that and works as
expected.

New patch as follows then:

---
 lisp/minibuffer.el | 68 ++++++++++++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 29 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index ef71b4e6be..08e2dc5a37 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1147,35 +1147,45 @@ completion--replace
                               newtext)
     ;; Remove all text properties.
     (set-text-properties 0 (length newtext) nil newtext))
-  ;; Maybe this should be in subr.el.
-  ;; You'd think this is trivial to do, but details matter if you want
-  ;; to keep markers "at the right place" and be robust in the face of
-  ;; after-change-functions that may themselves modify the buffer.
-  (let ((prefix-len 0))
-    ;; Don't touch markers in the shared prefix (if any).
-    (while (and (< prefix-len (length newtext))
-                (< (+ beg prefix-len) end)
-                (eq (char-after (+ beg prefix-len))
-                    (aref newtext prefix-len)))
-      (setq prefix-len (1+ prefix-len)))
-    (unless (zerop prefix-len)
-      (setq beg (+ beg prefix-len))
-      (setq newtext (substring newtext prefix-len))))
-  (let ((suffix-len 0))
-    ;; Don't touch markers in the shared suffix (if any).
-    (while (and (< suffix-len (length newtext))
-                (< beg (- end suffix-len))
-                (eq (char-before (- end suffix-len))
-                    (aref newtext (- (length newtext) suffix-len 1))))
-      (setq suffix-len (1+ suffix-len)))
-    (unless (zerop suffix-len)
-      (setq end (- end suffix-len))
-      (setq newtext (substring newtext 0 (- suffix-len))))
-    (goto-char beg)
-    (let ((length (- end beg)))         ;Read `end' before we insert the text.
-      (insert-and-inherit newtext)
-      (delete-region (point) (+ (point) length)))
-    (forward-char suffix-len)))
+  (let ((beg (if (number-or-marker-p beg)
+                 beg
+               (save-excursion
+                 (goto-char (minibuffer-prompt-end))
+                 (search-forward beg))))
+        (end (if (number-or-marker-p end)
+                 end
+               (save-excursion
+                 (goto-char (point-max))
+                 (search-backward end)))))
+    ;; Maybe this should be in subr.el.
+    ;; You'd think this is trivial to do, but details matter if you want
+    ;; to keep markers "at the right place" and be robust in the face of
+    ;; after-change-functions that may themselves modify the buffer.
+    (let ((prefix-len 0))
+      ;; Don't touch markers in the shared prefix (if any).
+      (while (and (< prefix-len (length newtext))
+                  (< (+ beg prefix-len) end)
+                  (eq (char-after (+ beg prefix-len))
+                      (aref newtext prefix-len)))
+        (setq prefix-len (1+ prefix-len)))
+      (unless (zerop prefix-len)
+        (setq beg (+ beg prefix-len))
+        (setq newtext (substring newtext prefix-len))))
+    (let ((suffix-len 0))
+      ;; Don't touch markers in the shared suffix (if any).
+      (while (and (< suffix-len (length newtext))
+                  (< beg (- end suffix-len))
+                  (eq (char-before (- end suffix-len))
+                      (aref newtext (- (length newtext) suffix-len 1))))
+        (setq suffix-len (1+ suffix-len)))
+      (unless (zerop suffix-len)
+        (setq end (- end suffix-len))
+        (setq newtext (substring newtext 0 (- suffix-len))))
+      (goto-char beg)
+      (let ((length (- end beg)))         ;Read `end' before we insert the text.
+        (insert-and-inherit newtext)
+        (delete-region (point) (+ (point) length)))
+      (forward-char suffix-len))))

 (defcustom completion-cycle-threshold nil
   "Number of completion candidates below which cycling is used.
--
2.25.1

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[Stefan Monnier]

> +  (let ((beg (if (number-or-marker-p beg)
> +                 beg
> +               (save-excursion
> +                 (goto-char (minibuffer-prompt-end))
> +                 (search-forward beg))))
> +        (end (if (number-or-marker-p end)
> +                 end
> +               (save-excursion
> +                 (goto-char (point-max))
> +                 (search-backward end)))))

What if there are several matches for `beg` and/or for `end` in
the buffer?  How do we know we got the intended one?


        Stefan

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[James N. V. Cash]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> +  (let ((beg (if (number-or-marker-p beg)
>> +                 beg
>> +               (save-excursion
>> +                 (goto-char (minibuffer-prompt-end))
>> +                 (search-forward beg))))
>> +        (end (if (number-or-marker-p end)
>> +                 end
>> +               (save-excursion
>> +                 (goto-char (point-max))
>> +                 (search-backward end)))))
>
> What if there are several matches for `beg` and/or for `end` in
> the buffer?  How do we know we got the intended one?

A good question; as far as I can tell though, `beg` and `end` are only
strings when called via `choose-completion` in `simple.el`, when it
passes in `base-affixes`. `base-affixes` is in turn set to
`completion-base-affixes`, which gets set in
`minibuffer-completion-help`. There, the value that becomes `beg` is
`(buffer-substring (minibuffer--completion-prompt-end) (+ start base-size))`
and `end` is `(buffer-substring (point) (point-max))`, so I think that changing
`minibuffer-prompt-end` in my patch to
`minibuffer--completion-prompt-end` would suffice to ensure that it is
always matching from the correct point to match the intended positions.

James

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[Stefan Monnier]

James N. V. Cash [2022-04-29 10:07:33] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>> +  (let ((beg (if (number-or-marker-p beg)
>>> +                 beg
>>> +               (save-excursion
>>> +                 (goto-char (minibuffer-prompt-end))
>>> +                 (search-forward beg))))
>>> +        (end (if (number-or-marker-p end)
>>> +                 end
>>> +               (save-excursion
>>> +                 (goto-char (point-max))
>>> +                 (search-backward end)))))
>>
>> What if there are several matches for `beg` and/or for `end` in
>> the buffer?  How do we know we got the intended one?
>
> A good question; as far as I can tell though, `beg` and `end` are only
> strings when called via `choose-completion` in `simple.el`, when it
> passes in `base-affixes`. `base-affixes` is in turn set to
> `completion-base-affixes`, which gets set in
> `minibuffer-completion-help`. There, the value that becomes `beg` is
> `(buffer-substring (minibuffer--completion-prompt-end) (+ start base-size))`
> and `end` is `(buffer-substring (point) (point-max))`, so I think that changing
> `minibuffer-prompt-end` in my patch to
> `minibuffer--completion-prompt-end` would suffice to ensure that it is
> always matching from the correct point to match the intended positions.

I think rather than (minibuffer--completion-prompt-end) and (point-max),
we should be using "the beginning of the completion area" and "the end
of the completion area".  In a typical minibuffer completion, that's
exactly the same, but in `completing-read-multiple` or
`complete-in-region` the difference can be significant.


        Stefan

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[James N. V. Cash]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> James N. V. Cash [2022-04-29 10:07:33] wrote:
>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>>> +  (let ((beg (if (number-or-marker-p beg)
>>>> +                 beg
>>>> +               (save-excursion
>>>> +                 (goto-char (minibuffer-prompt-end))
>>>> +                 (search-forward beg))))
>>>> +        (end (if (number-or-marker-p end)
>>>> +                 end
>>>> +               (save-excursion
>>>> +                 (goto-char (point-max))
>>>> +                 (search-backward end)))))
>>>
>>> What if there are several matches for `beg` and/or for `end` in
>>> the buffer?  How do we know we got the intended one?
>>
>> A good question; as far as I can tell though, `beg` and `end` are only
>> strings when called via `choose-completion` in `simple.el`, when it
>> passes in `base-affixes`. `base-affixes` is in turn set to
>> `completion-base-affixes`, which gets set in
>> `minibuffer-completion-help`. There, the value that becomes `beg` is
>> `(buffer-substring (minibuffer--completion-prompt-end) (+ start base-size))`
>> and `end` is `(buffer-substring (point) (point-max))`, so I think that changing
>> `minibuffer-prompt-end` in my patch to
>> `minibuffer--completion-prompt-end` would suffice to ensure that it is
>> always matching from the correct point to match the intended positions.
>
> I think rather than (minibuffer--completion-prompt-end) and (point-max),
> we should be using "the beginning of the completion area" and "the end
> of the completion area".  In a typical minibuffer completion, that's
> exactly the same, but in `completing-read-multiple` or
> `complete-in-region` the difference can be significant.

In the `completing-read-multiple` case, the `beg` is going to be the
previous completions, so I think this is exactly finding the completion
area (it also seems that `minibuffer--completion-prompt-end` is the same
value as `minibuffer-prompt-end,` it just also raises an error).

In my brief experimentation, it appears `complete-in-region` receives
`beg` and `end` as numbers, so it doesn't go through this searching
stuff at all.

James

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[Juri Linkov]

> I think rather than (minibuffer--completion-prompt-end) and (point-max),
> we should be using "the beginning of the completion area" and "the end
> of the completion area".  In a typical minibuffer completion, that's
> exactly the same, but in `completing-read-multiple` or
> `complete-in-region` the difference can be significant.

Is this what bug#48356 is intended to improve?

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[Stefan Monnier]

>> +  (let ((beg (if (number-or-marker-p beg)
>> +                 beg
>> +               (save-excursion
>> +                 (goto-char (minibuffer-prompt-end))
>> +                 (search-forward beg))))
>> +        (end (if (number-or-marker-p end)
>> +                 end
>> +               (save-excursion
>> +                 (goto-char (point-max))
>> +                 (search-backward end)))))
>
> What if there are several matches for `beg` and/or for `end` in
> the buffer?  How do we know we got the intended one?

IIUC part of the purpose of using strings rather than positions is to
account for the case where the text was modified between the moment the
*Completions* list was created and the moment the user selects one of
the options.  So another question is what to do if the search fails (the
above code signals an error in that case, which is probably not what we
want).


        Stefan

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[James N. V. Cash]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> +  (let ((beg (if (number-or-marker-p beg)
>>> +                 beg
>>> +               (save-excursion
>>> +                 (goto-char (minibuffer-prompt-end))
>>> +                 (search-forward beg))))
>>> +        (end (if (number-or-marker-p end)
>>> +                 end
>>> +               (save-excursion
>>> +                 (goto-char (point-max))
>>> +                 (search-backward end)))))
>>
>> What if there are several matches for `beg` and/or for `end` in
>> the buffer?  How do we know we got the intended one?
>
> IIUC part of the purpose of using strings rather than positions is to
> account for the case where the text was modified between the moment the
> *Completions* list was created and the moment the user selects one of
> the options.  So another question is what to do if the search fails (the
> above code signals an error in that case, which is probably not what we
> want).

That is a good question. That would happen, I guess, if you had selected
multiple candidates in a CRM, started completing, then deleted the
previously entered candidates, then selected one. I suppose I'd expect
it to add the completed candidate to the end of the remaining list?
So something like

  (let ((beg (if (number-or-marker-p beg)
                 beg
               (save-excursion
                 (goto-char (minibuffer-prompt-end))
                 (or (search-forward beg nil t)
                     (search-forward-regexp crm-separator nil t)
                     (minibuffer-prompt-end)))))

James

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[Stefan Monnier]

James N. V. Cash [2022-04-30 08:41:24] wrote:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>>> +  (let ((beg (if (number-or-marker-p beg)
>>>> +                 beg
>>>> +               (save-excursion
>>>> +                 (goto-char (minibuffer-prompt-end))
>>>> +                 (search-forward beg))))
>>>> +        (end (if (number-or-marker-p end)
>>>> +                 end
>>>> +               (save-excursion
>>>> +                 (goto-char (point-max))
>>>> +                 (search-backward end)))))
>>>
>>> What if there are several matches for `beg` and/or for `end` in
>>> the buffer?  How do we know we got the intended one?
>>
>> IIUC part of the purpose of using strings rather than positions is to
>> account for the case where the text was modified between the moment the
>> *Completions* list was created and the moment the user selects one of
>> the options.  So another question is what to do if the search fails (the
>> above code signals an error in that case, which is probably not what we
>> want).
>
> That is a good question. That would happen, I guess, if you had selected
> multiple candidates in a CRM, started completing, then deleted the
> previously entered candidates, then selected one. I suppose I'd expect
> it to add the completed candidate to the end of the remaining list?
> So something like
>
>   (let ((beg (if (number-or-marker-p beg)
>                  beg
>                (save-excursion
>                  (goto-char (minibuffer-prompt-end))
>                  (or (search-forward beg nil t)
>                      (search-forward-regexp crm-separator nil t)
>                      (minibuffer-prompt-end)))))

This makes it specific to CRM.  I think we should try and think more
locally, without paying too much attention about the various possible
external cases: what do we want the `beg` and `end` strings to *mean*?

Maybe instead of strings, they should be functions, so CRM can set them
to functions that first look for the separator and then search for some
leading/trailing string?


        Stefan

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[James N. V. Cash]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> This makes it specific to CRM.  I think we should try and think more
> locally, without paying too much attention about the various possible
> external cases: what do we want the `beg` and `end` strings to *mean*?
>
> Maybe instead of strings, they should be functions, so CRM can set them
> to functions that first look for the separator and then search for some
> leading/trailing string?

Upon further inspection, I see that `minibuffer-completion-help` is setting
`completion-list-insert-choice-function` to a function that checks if the `beg`
and `end` arguments are strings, in which case it just replaces the
minibuffer contents with "beg" + "choice" + "end". Indeed, when doing
`completing-read` instead of `completing-read-multiple`,
`completion--replace` doesn't get called at all in this case.

On one hand, that *might* mean that this particular code path
(completion--replace being called with `beg` and `end` strings)
necessarily means it's a completing-read-multiple, so searching for the
crm-separator is justifiable. On the other, a better solution than my
patch then might be to figure out why that function is being "lost" in
the completion-read-multiple case, so the "replace minibuffer contents
with beg + selection + end" behaviour happens there too (maybe,
arguably, in the CRM case in particular, deleting part of the prefix
then selecting a completion makes sense, but in the normal case, the
current behaviour (essentially, ignoring edits that have happened in the
prefix) seems like the only sensible thing to do).

James

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[Juri Linkov]

> Maybe instead of strings, they should be functions, so CRM can set them
> to functions that first look for the separator and then search for some
> leading/trailing string?

Then while creating the *Completions* list it could return a lambda
to be called later after selecting one of the options.

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[Juri Linkov]

> Upon further inspection, I see that `minibuffer-completion-help` is setting
> `completion-list-insert-choice-function` to a function that checks if the `beg`
> and `end` arguments are strings, in which case it just replaces the
> minibuffer contents with "beg" + "choice" + "end". Indeed, when doing
> `completing-read` instead of `completing-read-multiple`,
> `completion--replace` doesn't get called at all in this case.

completing-read-multiple already overrides choose-completion-string-functions
with own crm--choose-completion-string.  So it would also make sense to
override completion-list-insert-choice-function as well.

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[Stefan Monnier]

>> Maybe instead of strings, they should be functions, so CRM can set them
>> to functions that first look for the separator and then search for some
>> leading/trailing string?
> Then while creating the *Completions* list it could return a lambda
> to be called later after selecting one of the options.

That's the idea, yes.


        Stefan

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[James N. V. Cash]

Juri Linkov <juri@linkov.net> writes:

>> Upon further inspection, I see that `minibuffer-completion-help` is setting
>> `completion-list-insert-choice-function` to a function that checks if the `beg`
>> and `end` arguments are strings, in which case it just replaces the
>> minibuffer contents with "beg" + "choice" + "end". Indeed, when doing
>> `completing-read` instead of `completing-read-multiple`,
>> `completion--replace` doesn't get called at all in this case.
>
> completing-read-multiple already overrides choose-completion-string-functions
> with own crm--choose-completion-string.  So it would also make sense to
> override completion-list-insert-choice-function as well.

I think there are a few ways this could be done. The simplest would be
to do the same thing that minibuffer.el does (if `start` and `end` are
strings, insert `start`, `choice`, `end`; otherwise call
`completion--replace` as usual).

The other approach, which the below patch implements, is try to find the
bounds based on the strings, but if the contents been edited, find the
nearest CRM separator. This is kind of nice in that it lets you edit
other selections but then still select a candidate, but I don't know how
useful/expected that really is. The logic could also be made somewhat
more complex (count the number of separators in `start` and `end`, try
to guess how many we should skip over in each direction) but I don't
know if that's really worthwhile.

---
 lisp/emacs-lisp/crm.el | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/lisp/emacs-lisp/crm.el b/lisp/emacs-lisp/crm.el
index f3e1981732..8a5c3d3730 100644
--- a/lisp/emacs-lisp/crm.el
+++ b/lisp/emacs-lisp/crm.el
@@ -254,6 +254,23 @@ completing-read-multiple
                     'crm--choose-completion-string nil 'local)
           (setq-local minibuffer-completion-table #'crm--collection-fn)
           (setq-local minibuffer-completion-predicate predicate)
+          (setq-local completion-list-insert-choice-function
+                      (lambda (start end choice)
+                        (if (and (stringp start) (stringp end))
+                            (let* ((beg (save-excursion
+                                          (goto-char (minibuffer-prompt-end))
+                                          (or (search-forward start nil t)
+                                              (search-forward-regexp crm-separator nil t)
+                                              (minibuffer-prompt-end))))
+                                   (end (save-excursion
+                                          (goto-char (point-max))
+                                          (or (search-backward end nil t)
+                                              (progn
+                                                (goto-char beg)
+                                                (search-forward-regexp crm-separator nil t))
+                                              (point-max)))))
+                              (completion--replace beg end choice))
+                          (completion--replace start end choice))))
           ;; see completing_read in src/minibuf.c
           (setq-local minibuffer-completion-confirm
                       (unless (eq require-match t) require-match))
-- 
2.25.1

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[Juri Linkov]

> The other approach, which the below patch implements, is try to find the
> bounds based on the strings, but if the contents been edited, find the
> nearest CRM separator. This is kind of nice in that it lets you edit
> other selections but then still select a candidate, but I don't know how
> useful/expected that really is. The logic could also be made somewhat
> more complex (count the number of separators in `start` and `end`, try
> to guess how many we should skip over in each direction) but I don't
> know if that's really worthwhile.

I tried out this approach and it works nicely, except the case
when the CRM separator gets deleted by the user.  But OTOH,
the user might want to delete the separator intentionally,
to reduce the number of selections.  So it seems there is no need
to make the logic more complex.

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[James N. V. Cash]

Juri Linkov <juri@linkov.net> writes:

>> The other approach, which the below patch implements, is try to find the
>> bounds based on the strings, but if the contents been edited, find the
>> nearest CRM separator. This is kind of nice in that it lets you edit
>> other selections but then still select a candidate, but I don't know how
>> useful/expected that really is. The logic could also be made somewhat
>> more complex (count the number of separators in `start` and `end`, try
>> to guess how many we should skip over in each direction) but I don't
>> know if that's really worthwhile.
>
> I tried out this approach and it works nicely, except the case
> when the CRM separator gets deleted by the user.  But OTOH,
> the user might want to delete the separator intentionally,
> to reduce the number of selections.  So it seems there is no need
> to make the logic more complex.

Should I make a new thread with this patch then, or is the one here okay?

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[Juri Linkov]

>>> The other approach, which the below patch implements, is try to find the
>>> bounds based on the strings, but if the contents been edited, find the
>>> nearest CRM separator. This is kind of nice in that it lets you edit
>>> other selections but then still select a candidate, but I don't know how
>>> useful/expected that really is. The logic could also be made somewhat
>>> more complex (count the number of separators in `start` and `end`, try
>>> to guess how many we should skip over in each direction) but I don't
>>> know if that's really worthwhile.
>>
>> I tried out this approach and it works nicely, except the case
>> when the CRM separator gets deleted by the user.  But OTOH,
>> the user might want to delete the separator intentionally,
>> to reduce the number of selections.  So it seems there is no need
>> to make the logic more complex.
>
> Should I make a new thread with this patch then, or is the one here okay?

If you think your latest patch is safe enough to install and
you signed a copyright assignment for your changes, I could push it
without the need to create a bug report.

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[James N. V. Cash]

Juri Linkov <juri@linkov.net> writes:

>>>> The other approach, which the below patch implements, is try to find the
>>>> bounds based on the strings, but if the contents been edited, find the
>>>> nearest CRM separator. This is kind of nice in that it lets you edit
>>>> other selections but then still select a candidate, but I don't know how
>>>> useful/expected that really is. The logic could also be made somewhat
>>>> more complex (count the number of separators in `start` and `end`, try
>>>> to guess how many we should skip over in each direction) but I don't
>>>> know if that's really worthwhile.
>>>
>>> I tried out this approach and it works nicely, except the case
>>> when the CRM separator gets deleted by the user.  But OTOH,
>>> the user might want to delete the separator intentionally,
>>> to reduce the number of selections.  So it seems there is no need
>>> to make the logic more complex.
>>
>> Should I make a new thread with this patch then, or is the one here okay?
>
> If you think your latest patch is safe enough to install and
> you signed a copyright assignment for your changes, I could push it
> without the need to create a bug report.

It's been working okay for me for the past few days & I do indeed have a
copyright assignment on file. Thanks!

James

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[Juri Linkov]

> It's been working okay for me for the past few days & I do indeed have a
> copyright assignment on file. Thanks!

Thanks, pushed now.

Re: [PATCH] Handle case where `beg` and `end` are strings instead of markers

[Juri Linkov]

>>> Maybe instead of strings, they should be functions, so CRM can set them
>>> to functions that first look for the separator and then search for some
>>> leading/trailing string?
>> Then while creating the *Completions* list it could return a lambda
>> to be called later after selecting one of the options.
>
> That's the idea, yes.

I tried to do this, but stumbled upon the need to split this progn
to two lambdas: `(insert choice)` should go to the first lambda
that inserts the prefix, or to the second lambda that inserts the suffix?

  (setq-local completion-base-affixes
              (list base-prefix base-suffix))
  (setq-local completion-list-insert-choice-function
       (lambda (start end choice)
         (if (and (stringp start) (stringp end))
             (progn
               (delete-minibuffer-contents)
               (insert start)
               (insert choice)
               (insert end))
   ...

Exiting completion-in-region-mode (was: Handle case where `beg` and `end` are strings instead of markers)

[Juri Linkov]

BTW, there is a related problem: while using `M-down` in the minibuffer,
it unexpectedly closes the *Completions* window when using
completion-in-region mode in the minibuffer, such as with
`M-: (c TAB M-down` or `M-! c TAB M-down`.

The code responsible for automatic closing the *Completions* window
is found in `completion-at-point`:

              (completion-in-region-mode-predicate
               (lambda ()
                 ;; We're still in the same completion field.
                 (let ((newstart (car-safe (funcall hookfun))))
                   (and newstart (= newstart start)))))

And I'm completely lost how to adjust it to not exit completion-in-region-mode.

Re: Exiting completion-in-region-mode

[Juri Linkov]

> BTW, there is a related problem: while using `M-down` in the minibuffer,
> it unexpectedly closes the *Completions* window when using
> completion-in-region mode in the minibuffer, such as with
> `M-: (c TAB M-down` or `M-! c TAB M-down`.
>
> The code responsible for automatic closing the *Completions* window
> is found in `completion-at-point`:
>
>               (completion-in-region-mode-predicate
>                (lambda ()
>                  ;; We're still in the same completion field.
>                  (let ((newstart (car-safe (funcall hookfun))))
>                    (and newstart (= newstart start)))))
>
> And I'm completely lost how to adjust it to not exit completion-in-region-mode.

I realized now that inserting each candidate to the ordinary buffer
while navigating the list of in-buffer completions not only difficult to
implement, but also causes a lot of usability problems such as adding
a new entry to the undo list for every M-down/M-up.

OTOH, when forcing the value minibuffer-completion-auto-choose to nil
for in-buffer completions, it works surprisingly well: it navigates
the completions list without inserting candidates to the buffer,
then M-RET inserts the selected completion.

All this is achievable with a small patch:

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index f3003505f1..dfa7640ebe 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2624,7 +2624,10 @@ completion-in-region-mode-map
   ;; FIXME: Only works if completion-in-region-mode was activated via
   ;; completion-at-point called directly.
   "M-?" #'completion-help-at-point
-  "TAB" #'completion-at-point)
+  "TAB" #'completion-at-point
+  "M-<up>"    #'minibuffer-previous-completion
+  "M-<down>"  #'minibuffer-next-completion
+  "M-RET"     #'minibuffer-choose-completion)
 
 ;; It is difficult to know when to exit completion-in-region-mode (i.e. hide
 ;; the *Completions*).  Here's how previous packages did it:
@@ -2671,6 +2674,7 @@ completion-in-region-mode
     (cl-assert completion-in-region-mode-predicate)
     (setq completion-in-region-mode--predicate
 	  completion-in-region-mode-predicate)
+    (setq-local minibuffer-completion-auto-choose nil)
     (add-hook 'post-command-hook #'completion-in-region--postch)
     (push `(completion-in-region-mode . ,completion-in-region-mode-map)
           minor-mode-overriding-map-alist)))