Respect completion boundaries in completion-in-region

[AmaiKinono]

Simple test:

• M-x shell
• Type cd /some/path
• Call completion-in-region
• The path is expanded correctly. In previous versions, /some/path will be deleted.

@clemera offerd a lot of help and suggestions in #84 (Thanks!). I'd like to explain some of my decisions, and discuss on those I'm not sure about.

• @clemera suggested using choose-completion-string. It actually doesn't fix the bug. I looked into it, it uses completion-boundaries on minibuffer-completion-table and minibuffer-completion-predicate, but even I let bind those variables, it doesn't work. So I fixed it by directly using completion-boundaries.

  Also, based on the code of simple.el, seems choose-completion-string is used when choose a candidate in the *Completion* buffer.

• We mentioned base-size. I found the relevant docstrings: see completion-base-size and completion-base-position. From the docstrings, they are used only for the *Completion* buffer (should be some face related things), plus handling it doesn't fix the bug. So I didn't use it.

• @clemera suggested using completion--done to do the things after completion (e.g. calling exit-function). I didn't use it, but I don't oppose it. From the code, seems it deals with some message relevant things, which I don't understand well for now.

• @clemera suggested always using the exact status in exit-function, since it "leads to less problems". When completing the path in shell-mode, with this patch, sometimes a space is inserted after completion, and sometimes doesn't. And with the default UI, it never inserts the space. I don't oppose this change, but I feel the reason is insufficient.

When hacking this, my feeling is it seems Emacs strongly assumes the *Completion* buffer is the only UI to use, so relevant code is everywhere and in every abstraction level, which contradicts the pluggable completion-in-region-function design. Due to this reason, I would like to avoid using these built-in functions in selectrum-completion-in-region.

[clemera]

When hacking this, my feeling is it seems Emacs strongly assumes the Completion buffer is the only UI to use, so relevant code is everywhere and in every abstraction level, which contradicts the pluggable completion-in-region-function design. Due to this reason, I would like to avoid using these built-in functions in selectrum-completion-in-region.

That is an important point and I agree with your conclusions. Your solution is probably the right way to adapt to these observations.

When completing the path in shell-mode, with this patch, sometimes a space is inserted after completion, and sometimes doesn't.

I think this bug is super annoying. I have looked into how file name completion is handled in shell-mode and found a simple fix to avoid inserting the space afterwards is by unsetting pcomplete-termination-string. For comint-mode we would need to unset comint-completion-addsuffix. Setting both of these when activating selectrum-mode fixes this bug.

I'm can not think of any downsides setting these in combination with selectrum-mode would have but maybe there are use cases I have overlooked?. (Note that let binding those vars would work for pcomplete-termination-string but not for comint-completion-addsuffix because its value is taken into account before selectrum-completion-in-region runs).

[AmaiKinono]

I think a cleaner fix is to detect the completion category in metadata, and if it's file, we don't run the exit-function, or run it with the exact status.

[clemera]

That is a good idea but I think using the meta data would probably affect more use cases, we probably don't want to affect or block the exit function for all file completions (there might be use cases we haven't thought of).

[clemera]

Thinking about it the cleanest way would probably be to use status exact in cases where we have both: derived-mode-p 'comint-mode and category file. This way the fix would be pretty much exactly narrowed to the use case we care about and we don't have to deal with any variables. What do you think?

[AmaiKinono]

I agree, that's the best we can do for now. I'll add a note in the comment to explain this workaround, and hope someone read it could find the real solution.

[AmaiKinono]

I have another thought. When completing the filename, the user would want to type some characters, hit tab, type more, hit tab again, until the whole path is inserted. This actually is how things work with the default completion UI. Right now, when completing the filename, we can only choose file/directories in one level. Why not use read-file-name to do the completion when the category is file?

[clemera]

I think in general this is would be a great feature. I wrote my own completion function added to completion-at-point-functions to achieve something similar back when I was using ivy. Having that behaviour integrated into completion-in-region by default sounds awesome! But what if the completion table does something special? Maybe there would be cases where you don't want to delegate to read-file-name or where this completion behaviour would be wrong?

[AmaiKinono]

This is also what I'm worried about. My opinion is we can do this as long as completion-file-name-table doesn't do something special.

[clemera]

I'm not sure I understand that. Also if I understand correctly this is only the default completion table but what about custom completion tables which return category file?

[AmaiKinono]

You understood correctly. About custom completion tables, I'm just assuming people won't create their own ones for filename completion.

Maybe a better option is to implement a selectrum-read-file like UI that uses the completion table passed to completion-in-region (i.e., press TAB to further complete). I didn't looked into selectrum-read-file, so I'm not sure if this is possible or easy to implement.

[clemera]

About custom completion tables, I'm just assuming people won't create their own ones for filename completion.

There are also custom built-in ones like comint-completion-file-name-table or the one created by pcomplete-completions-at-point and maybe more which can be triggered on file completion.

Maybe a better option is to implement a selectrum-read-file like UI that uses the completion table passed to completion-in-region (i.e., press TAB to further complete). I didn't looked into selectrum-read-file, so I'm not sure if this is possible or easy to implement.

Yes I think that should work. We could pass a function to selectrum-read which calls the the original table with the input adjusted similar as in selectrum--completing-read-file-name.

[AmaiKinono]

I implemented it using selectrum--completing-read-file-name directly. Somethings are not handled:

• Annotation, docsig, and display-sort-func. I doubt if completing a file name really needs these.
• The exit status is always finished when completing file name. I think this behavior is actually reasonable, since even if the user exit with some non-existing path (like when completing after a touch command), conceptually it's still a complete file name, and it's expected that a space is added automatically, which requires the finished status.

If we are going to handle these things, we need to extend selectrum--completing-read-file-name or write a similar one. But I'm happy with the current implementation.

[clemera]

Cool, that you were able to reuse selectrum--completing-read-file-name is nice.

If we are going to handle these things, we need to extend selectrum--completing-read-file-name or write a similar one. But I'm happy with the current implementation.

There are some discussions about making the metadata available for regular completing-read sessions (#15, #82, #94). In the future selectrum--completing-read-file-name might be able to pass the metadata of the collection to selectrum-read.

[clemera]

I just tested it and it works great! I noticed another thing: Although it's not documented in the docstring of completion-in-region-function the predicate argument is optional (see completion--in-region which is the built-in default for completion-in-region-function) It seems this isn't relevant see completion-in-region.

The exit status is always finished when completing file name. I think this behavior is actually reasonable, since even if the user exit with some non-existing path (like when completing after a touch command), conceptually it's still a complete file name, and it's expected that a space is added automatically, which requires the finished status.

With the new behaviour I think using the finished status really makes sense, awesome work!

[clemera]

(when result
  (let* ((bound (car (completion-boundaries
                      input collection predicate "")))
         (start (pcase category
                  ('file start)
                  (_ (+ start bound)))))
    (delete-region start end)
    (insert (substring-no-properties result))))
(when exit-func
  (funcall exit-func result exit-status))

Are there cases where the exit function should run even if there is no completion result?

Another idea I had is that we could push the mark (inactively) before inserting the completion so it would be easy to pop back to the initial input after the completion got inserted. But this may violate expectations in some cases so I'm not sure but it could be convenient I guess.

[AmaiKinono]

Are there cases where the exit function should run even if there is no completion result?

I think no. I'll signal an error when the result is nil or an empty string, like the default behavior.

Edit: In the default UI, the bell does ring when there's no completions, but suprisingly the "No match" message is sent by completion--message. I don't know how it signals this error.

Another idea I had is that we could push the mark (inactively) before inserting the completion so it would be easy to pop back to the initial input after it was inserted.

I don't quite understand "pop back to the initial input". is this the same as undo?

[clemera]

I don't know how it signals this error.

This is done by completion--do-completion and apparently there is also a user option completion-fail-discreetly to control this. I did M-x debug-on-entry RET ding RET and invoked completion afterwards to find the caller in the backtrace.

I don't quite understand "pop back to the initial input". is this the same as undo?

Sorry that was poorly phrased. I meant you could jump back to the point where you started completion via C-u C-SPC or C-x C-x.

[AmaiKinono]

there is also a user option completion-fail-discreetly to control this.

Thanks! This is utilized now.

you could jump back to the point where you started completion via C-u C-SPC or C-x C-x.

I found this is actually the default behavior, but I didn't find a push-mark or set-mark call in minibuffer.el.

[clemera]

I found this is actually the default behavior, but I didn't find a push-mark or set-mark call in minibuffer.el.

Maybe this was by accident I can't see this behaviour in emacs -Q (v 26.3).

[AmaiKinono]

Maybe this was by accident

Yes. I found it's caused by evil.

[AmaiKinono]

@raxod502 This PR is ready. It does 2 things:

1. Respect completing boundaries. Here's a simple test:

   (defun my-collection (string pred flag)
     (pcase flag
       (`(boundaries . ,suffix) '(boundaries 1 . 0))
       (t '("aaa" "aa" "a"))
       (_ (error "FLAG is not handled"))))

   This is a completion function that only keeps the first char. In an empty line, type 123, then M-x eval-expression, and eval this:

   (selectrum-completion-in-region (line-beginning-position) (line-end-position) #'my-collection nil)

   You'll see "23" is deleted.

2. Use selectrum--completing-read-file-name for completing file names. You can completing some path in shell-mode to test it.

[raxod502]

Thanks!