GNU bug report logs -
#66948
[PATCH] Add Completion Preview mode
Previous Next
Reported by: Eshel Yaron <me <at> eshelyaron.com>
Date: Sun, 5 Nov 2023 10:27:02 UTC
Severity: normal
Tags: patch
Done: Eli Zaretskii <eliz <at> gnu.org>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 66948 in the body.
You can then email your comments to 66948 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
joaotavora <at> gmail.com, dmitry <at> gutov.dev, bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Sun, 05 Nov 2023 10:27:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Eshel Yaron <me <at> eshelyaron.com>:
New bug report received and forwarded. Copy sent to
joaotavora <at> gmail.com, dmitry <at> gutov.dev, bug-gnu-emacs <at> gnu.org.
(Sun, 05 Nov 2023 10:27:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Tags: patch
Following the recent discussion on emacs-devel[0], I'm attaching a patch
that adds a new Completion Preview minor mode.
I've been testing this patch with different completion backends, and so
far I'm pretty happy with the results. I've made some tweaks compared
to the version I posted on emacs-devel to make the implementation more
robust, and in particular to accommodate for some peculiarities of
LSP-based completion, which I've tested mostly with `eglot` but also
with `lsp-mode`.
In terms of documentation, this patch extends "(emacs)Symbol Completion"
with a description of Completion Preview mode. That seemed like a good
place to discuss this mode, but let me know if somewhere else would be
preferable.
Another question is whether to have RET/TAB/both accept and insert the
completion suggestion. In VS Code, both RET and TAB do that AFAICT. If
we want to, we can easily do that too by binding RET/TAB in
`completion-preview-active-mode-map`. Alternatively, we can just
suggest doing so in the documentation. The current patch binds neither
RET nor TAB, so to accept the suggestion you complete with C-M-i.
Personally, I find that convenient enough, but I can imagine that many
users might expect TAB or RET to work too.
[0] https://lists.gnu.org/archive/html/emacs-devel/2023-10/msg00693.html
[0001-Add-Completion-Preview-mode.patch (text/patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Sun, 05 Nov 2023 18:28:02 GMT)
Full text and
rfc822 format available.
Message #8 received at 66948 <at> debbugs.gnu.org (full text, mbox):
Eshel Yaron <me <at> eshelyaron.com> writes:
> Tags: patch
>
> Following the recent discussion on emacs-devel[0], I'm attaching a patch
> that adds a new Completion Preview minor mode.
I didn't follow the discussion in detail, but is this related to the
capf-autosuggest package?
> I've been testing this patch with different completion backends, and so
> far I'm pretty happy with the results. I've made some tweaks compared
> to the version I posted on emacs-devel to make the implementation more
> robust, and in particular to accommodate for some peculiarities of
> LSP-based completion, which I've tested mostly with `eglot` but also
> with `lsp-mode`.
>
> In terms of documentation, this patch extends "(emacs)Symbol Completion"
> with a description of Completion Preview mode. That seemed like a good
> place to discuss this mode, but let me know if somewhere else would be
> preferable.
>
> Another question is whether to have RET/TAB/both accept and insert the
> completion suggestion. In VS Code, both RET and TAB do that AFAICT. If
> we want to, we can easily do that too by binding RET/TAB in
> `completion-preview-active-mode-map`. Alternatively, we can just
> suggest doing so in the documentation. The current patch binds neither
> RET nor TAB, so to accept the suggestion you complete with C-M-i.
> Personally, I find that convenient enough, but I can imagine that many
> users might expect TAB or RET to work too.
FWIW From a brief test, my main annoyances have been that DEL drops any
completion and that there is no option to cycle between inexact
completions.
>
> [0] https://lists.gnu.org/archive/html/emacs-devel/2023-10/msg00693.html
>
> + (pcase (bounds-of-thing-at-point 'symbol)
[...]
You can use `pcase-let' here.
> + (`(,beg . ,end)
> + (<= completion-preview-minimum-symbol-length (- end beg)))))
--
Philip Kaludercic
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Sun, 05 Nov 2023 19:44:02 GMT)
Full text and
rfc822 format available.
Message #11 received at 66948 <at> debbugs.gnu.org (full text, mbox):
Philip Kaludercic <philipk <at> posteo.net> writes:
> Eshel Yaron <me <at> eshelyaron.com> writes:
>
>> Tags: patch
>>
>> Following the recent discussion on emacs-devel[0], I'm attaching a patch
>> that adds a new Completion Preview minor mode.
>
> I didn't follow the discussion in detail, but is this related to the
> capf-autosuggest package?
`capf-autosuggest` didn't really come up so far. It is related to
Completion Preview, but `capf-autosuggest` only deals with `comint`-like
buffers, while Completion Preview works in all buffers. I don't know if
and how `capf-autosuggest` could be extended to work in arbitrary buffers.
> FWIW From a brief test, my main annoyances have been that DEL drops any
> completion
Thanks for testing. To have DEL not dismiss the completion preview, add
`delete-backward-char` to `completion-preview-commands`. Actually,
would it make sense to have that behavior by default?
> and that there is no option to cycle between inexact completions.
That's interesting, can you elaborate about the use case that you have
in mind? I thought about adding a command to do that, but I wasn't sure
there'd be a good use case for it since you can always dismiss the
preview (e.g. with `C-g`) and say `C-M-i` to pick another candidate.
>> [0] https://lists.gnu.org/archive/html/emacs-devel/2023-10/msg00693.html
>>
>> + (pcase (bounds-of-thing-at-point 'symbol)
>
> [...]
>
> You can use `pcase-let' here.
>
Good idea, I'll try it out.
Best,
Eshel
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Mon, 06 Nov 2023 07:33:02 GMT)
Full text and
rfc822 format available.
Message #14 received at 66948 <at> debbugs.gnu.org (full text, mbox):
>> and that there is no option to cycle between inexact completions.
>
> That's interesting, can you elaborate about the use case that you have
> in mind? I thought about adding a command to do that, but I wasn't sure
> there'd be a good use case for it since you can always dismiss the
> preview (e.g. with `C-g`) and say `C-M-i` to pick another candidate.
Is it possible to avoid typing `C-g` before typing `C-M-i` for the list
of completions? I mean that I'd prefer only TAB or RET to accept the
completion suggestion, but not `C-M-i`. Then when the proposed preview
is correct, TAB or RET will accept it, otherwise `C-M-i` will immediately
display a list of alternative completions.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Mon, 06 Nov 2023 07:34:01 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Mon, 06 Nov 2023 07:37:02 GMT)
Full text and
rfc822 format available.
Message #20 received at 66948 <at> debbugs.gnu.org (full text, mbox):
Eshel Yaron <me <at> eshelyaron.com> writes:
> Philip Kaludercic <philipk <at> posteo.net> writes:
>
>> Eshel Yaron <me <at> eshelyaron.com> writes:
>>
>>> Tags: patch
>>>
>>> Following the recent discussion on emacs-devel[0], I'm attaching a patch
>>> that adds a new Completion Preview minor mode.
>>
>> I didn't follow the discussion in detail, but is this related to the
>> capf-autosuggest package?
>
> `capf-autosuggest` didn't really come up so far. It is related to
> Completion Preview, but `capf-autosuggest` only deals with `comint`-like
> buffers, while Completion Preview works in all buffers. I don't know if
> and how `capf-autosuggest` could be extended to work in arbitrary buffers.
It should be possible, but from what I see the capf-autosuggest code has
a lot of additional integration into comint, that could probably be
separated, or perhaps even build on your completion-preview-mode.
>> FWIW From a brief test, my main annoyances have been that DEL drops any
>> completion
>
> Thanks for testing. To have DEL not dismiss the completion preview, add
> `delete-backward-char` to `completion-preview-commands`. Actually,
> would it make sense to have that behavior by default?
I think it would.
>> and that there is no option to cycle between inexact completions.
>
> That's interesting, can you elaborate about the use case that you have
> in mind? I thought about adding a command to do that, but I wasn't sure
> there'd be a good use case for it since you can always dismiss the
> preview (e.g. with `C-g`) and say `C-M-i` to pick another candidate.
Just to use M-n/M-p to cycle through what the different completion
options are, without having to request a window that might mess up my
frame layout.
>>> [0] https://lists.gnu.org/archive/html/emacs-devel/2023-10/msg00693.html
>>>
>>> + (pcase (bounds-of-thing-at-point 'symbol)
>>
>> [...]
>>
>> You can use `pcase-let' here.
>>
>
> Good idea, I'll try it out.
>
>
> Best,
>
> Eshel
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Mon, 06 Nov 2023 15:32:02 GMT)
Full text and
rfc822 format available.
Message #23 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Hi,
Juri Linkov <juri <at> linkov.net> writes:
>>> and that there is no option to cycle between inexact completions.
>>
>> That's interesting, can you elaborate about the use case that you have
>> in mind? I thought about adding a command to do that, but I wasn't sure
>> there'd be a good use case for it since you can always dismiss the
>> preview (e.g. with `C-g`) and say `C-M-i` to pick another candidate.
>
> Is it possible to avoid typing `C-g` before typing `C-M-i` for the list
> of completions? I mean that I'd prefer only TAB or RET to accept the
> completion suggestion, but not `C-M-i`. Then when the proposed preview
> is correct, TAB or RET will accept it, otherwise `C-M-i` will immediately
> display a list of alternative completions.
I'm attaching a new patch (v2) following Juri's and Philip's comments.
The differences from the previous patch are:
1. TAB now accepts the completion suggestions when the preview is visible.
RET remains untouched, as I find binding it by default too intrusive.
2. Completion Preview avoids altering the behavior of
`completion-at-point` by default, so if you say `C-M-i` while the
preview is visible you get the usual list of completion candidates.
New user option `completion-preview-insert-on-completion` allows users
to regain the behavior of my original patch, where `C-M-i` would insert
accept the completion suggestion when the preview is visible.
3. New commands for cycling the candidate that the preview suggests.
These are currently unbound by default, but I added a recommendation in
the manual to bind `completion-preview-next-candidate` and
`completion-preview-prev-candidate` to `M-n` and `M-p` in
`completion-preview-active-mode-map`. That works nicely.
If you try it out and have any other suggestions, please let me know.
Here's the updated patch:
[v2-0001-Add-Completion-Preview-mode.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Mon, 06 Nov 2023 15:32:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Mon, 06 Nov 2023 15:39:01 GMT)
Full text and
rfc822 format available.
Message #29 received at 66948 <at> debbugs.gnu.org (full text, mbox):
Hi Philip,
Philip Kaludercic <philipk <at> posteo.net> writes:
>> Thanks for testing. To have DEL not dismiss the completion preview, add
>> `delete-backward-char` to `completion-preview-commands`. Actually,
>> would it make sense to have that behavior by default?
>
> I think it would.
Done, in the updated patch I've sent in my other message (patch v2).
>>> and that there is no option to cycle between inexact completions.
>>
>> That's interesting, can you elaborate about the use case that you have
>> in mind? I thought about adding a command to do that, but I wasn't sure
>> there'd be a good use case for it since you can always dismiss the
>> preview (e.g. with `C-g`) and say `C-M-i` to pick another candidate.
>
> Just to use M-n/M-p to cycle through what the different completion
> options are, without having to request a window that might mess up my
> frame layout.
That makes sense, thanks. Done in the updated patch, expect I didn't
provide these bindings by default, in benefit for use `M-n`/`M-p` for
other stuff that may conflict.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Mon, 06 Nov 2023 18:19:02 GMT)
Full text and
rfc822 format available.
Message #32 received at 66948 <at> debbugs.gnu.org (full text, mbox):
> If you try it out and have any other suggestions, please let me know.
> Here's the updated patch:
Thanks, everything works nicely now.
Only one thing that I suggest is to add `backward-delete-char-untabify`
to `completion-preview-commands` since it's bound to DEL in Lisp buffers.
Or there is no way to generalize it for all modes? For example, in c-mode
DEL is bound to `c-electric-backspace`, etc.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Mon, 06 Nov 2023 19:49:02 GMT)
Full text and
rfc822 format available.
Message #35 received at 66948 <at> debbugs.gnu.org (full text, mbox):
Juri Linkov <juri <at> linkov.net> writes:
>> If you try it out and have any other suggestions, please let me know.
>> Here's the updated patch:
>
> Thanks, everything works nicely now.
>
Great.
> Only one thing that I suggest is to add `backward-delete-char-untabify`
> to `completion-preview-commands` since it's bound to DEL in Lisp buffers.
> Or there is no way to generalize it for all modes?
I too wonder if there's a good generalization we can make here. It
seems impractical and inelegant to track all backward-deleting commands
in `completion-preview-commands`. We could try to check if a command
was invoked by pressing DEL, regardless of what command it actually is,
but I don't think that's such a good solution because of course the DEL
key itself is not the point--we want to recognize `delete-backward-char`
even if it's bound to some other key, for example.
The current solution, of providing a minimal list of commands in
`completion-preview-commands`, basically delegates the decision of which
further commands should keep the preview alive to major mode authors and
to the users themselves. I think that's basically fine, but if we had
some way to recognize backward-deleting commands without hard-coding
them (perhaps a symbol property such as `delete-backward-command`) that
would be even better IMO.
> For example, in c-mode DEL is bound to `c-electric-backspace`, etc.
Yes, and there's also Org mode's `org-delete-backward-char` and
certainly a couple more...
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Tue, 07 Nov 2023 07:22:01 GMT)
Full text and
rfc822 format available.
Message #38 received at 66948 <at> debbugs.gnu.org (full text, mbox):
>> Only one thing that I suggest is to add `backward-delete-char-untabify`
>> to `completion-preview-commands` since it's bound to DEL in Lisp buffers.
>> Or there is no way to generalize it for all modes?
>
> I too wonder if there's a good generalization we can make here. It
> seems impractical and inelegant to track all backward-deleting commands
> in `completion-preview-commands`. We could try to check if a command
> was invoked by pressing DEL, regardless of what command it actually is,
> but I don't think that's such a good solution because of course the DEL
> key itself is not the point--we want to recognize `delete-backward-char`
> even if it's bound to some other key, for example.
We have exactly the same problem while developing a live update
of the *Completions* window while typing in the minibuffer.
Here too DEL needs to update the completions. But this works
as long as the users don't rebind the DEL key. Therefore
currently a list of commands is used as well.
> The current solution, of providing a minimal list of commands in
> `completion-preview-commands`, basically delegates the decision of which
> further commands should keep the preview alive to major mode authors and
> to the users themselves. I think that's basically fine, but if we had
> some way to recognize backward-deleting commands without hard-coding
> them (perhaps a symbol property such as `delete-backward-command`) that
> would be even better IMO.
Agreed, it's easy to customize commands deemed to be important for the user.
Until we will find a way to remove such a list altogether.
One possible solution is to rely on the hook:
(add-hook 'after-change-functions
(lambda (beg end len)
(when (and (= beg end) (= len 1))
...))
nil t)
But this should be developed separately,
then later integrated into completion-preview.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Wed, 08 Nov 2023 07:43:01 GMT)
Full text and
rfc822 format available.
Message #41 received at 66948 <at> debbugs.gnu.org (full text, mbox):
>>> If you try it out and have any other suggestions, please let me know.
>>> Here's the updated patch:
>>
>> Thanks, everything works nicely now.
>
> Great.
While testing more, I noticed one strange thing,
I don't know if it's intended to work this way.
Typing in *scratch* e.g.
(message-f TAB
completes to
(message-forward
Then typing
(message-f M-C-i
stops at
(message-fo
Then trying again the same as in the first test case
(message-f TAB
now stops at the same too
(message-fo
whereas the overlay suggests the complete name `message-forward`.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Wed, 08 Nov 2023 09:16:02 GMT)
Full text and
rfc822 format available.
Message #44 received at 66948 <at> debbugs.gnu.org (full text, mbox):
Hi Juri,
Juri Linkov <juri <at> linkov.net> writes:
> While testing more, I noticed one strange thing,
> I don't know if it's intended to work this way.
This is not intended, thanks for raising it! Essentially, this is a
problem with the default `completion-in-region-function` that I wasn't
aware of (:
> Typing in *scratch* e.g.
>
> (message-f TAB
>
> completes to
>
> (message-forward
>
> Then typing
>
> (message-f M-C-i
>
At this point, `completion-in-region-mode` is enabled. That's fine.
> stops at
>
> (message-fo
>
For some reason, `completion-in-region-mode` remains active at point!
This means that `completion-in-region-mode-map` stays in effect, and
that binds TAB to `completion-at-point`, overriding the our binding for
`completion-preview-insert` (and any other binding TAB might have, for
that matter).
> Then trying again the same as in the first test case
>
> (message-f TAB
>
> now stops at the same too
>
> (message-fo
>
> whereas the overlay suggests the complete name `message-forward`.
Since TAB is bound to `completion-at-point` at this point, you get the
same result as you did when you pressed `C-M-i` beforehand. If you
would instead say `M-x completion-preview-insert` explicitly, you should
get the expected `message-forward`.
This appears to be a general problem with `completion-in-region-mode`,
that it keeps TAB bound to `completion-at-point` after a partial
completion, so I don't think we should attempt to handle it in
`completion-preview` specifically.
Consider the following scenario, with no `completion-preview-mode` in
play:
--8<---------------cut here---------------start------------->8---
(defun foo ()
bar-!-
--8<---------------cut here---------------end--------------->8---
Here -!- designates point. If you type TAB right away, `bar` gets
indented as expected. But if you first say `C-M-i`, the text stays the
same but now TAB is bound to `completion-at-point`, so pressing TAB no
longer indents and just says "No match" instead :(
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Wed, 08 Nov 2023 15:46:02 GMT)
Full text and
rfc822 format available.
Message #47 received at 66948 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
I wrote:
> Hi Juri,
>
> Juri Linkov <juri <at> linkov.net> writes:
>
>> While testing more, I noticed one strange thing,
>> I don't know if it's intended to work this way.
[...]
> This appears to be a general problem with `completion-in-region-mode`,
> that it keeps TAB bound to `completion-at-point` after a partial
> completion, so I don't think we should attempt to handle it in
> `completion-preview` specifically.
Let me know if you think otherwise, that `completion-preview` should
handle this case specifically, although I'm pretty convinced that it's
the behavior of `completion-in-region-mode` that's not quite right here.
Either way, I'm attaching an new patch (v3) that includes
`backward-delete-char-untabify` in `completion-preview-commands` to
provide a nicer OOTB experience for Elisp (as you suggested), along with
a couple of minor optimizations and documentation improvements.
Best,
Eshel
[v3-0001-Add-Completion-Preview-mode.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Thu, 09 Nov 2023 07:39:01 GMT)
Full text and
rfc822 format available.
Message #50 received at 66948 <at> debbugs.gnu.org (full text, mbox):
>>> While testing more, I noticed one strange thing,
>>> I don't know if it's intended to work this way.
> [...]
>> This appears to be a general problem with `completion-in-region-mode`,
>> that it keeps TAB bound to `completion-at-point` after a partial
>> completion, so I don't think we should attempt to handle it in
>> `completion-preview` specifically.
>
> Let me know if you think otherwise, that `completion-preview` should
> handle this case specifically, although I'm pretty convinced that it's
> the behavior of `completion-in-region-mode` that's not quite right here.
This looks like a problem of `completion-in-region-mode` indeed.
> Either way, I'm attaching an new patch (v3) that includes
> `backward-delete-char-untabify` in `completion-preview-commands` to
> provide a nicer OOTB experience for Elisp (as you suggested), along with
> a couple of minor optimizations and documentation improvements.
Let's see what Eli decides to do with your new package.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Fri, 10 Nov 2023 07:11:01 GMT)
Full text and
rfc822 format available.
Message #53 received at 66948 <at> debbugs.gnu.org (full text, mbox):
Juri Linkov <juri <at> linkov.net> writes:
>>>> While testing more, I noticed one strange thing,
>>>> I don't know if it's intended to work this way.
>> [...]
>>> This appears to be a general problem with `completion-in-region-mode`,
>>> that it keeps TAB bound to `completion-at-point` after a partial
>>> completion, so I don't think we should attempt to handle it in
>>> `completion-preview` specifically.
>>
>> Let me know if you think otherwise, that `completion-preview` should
>> handle this case specifically, although I'm pretty convinced that it's
>> the behavior of `completion-in-region-mode` that's not quite right here.
>
> This looks like a problem of `completion-in-region-mode` indeed.
And now that's been taken care of in Bug#67001 :)
>> Either way, I'm attaching an new patch (v3) that includes
>> `backward-delete-char-untabify` in `completion-preview-commands` to
>> provide a nicer OOTB experience for Elisp (as you suggested), along with
>> a couple of minor optimizations and documentation improvements.
>
> Let's see what Eli decides to do with your new package.
Sure. Eli, Stefan, any further comments about this addition?
Thanks,
Eshel
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Fri, 10 Nov 2023 07:45:02 GMT)
Full text and
rfc822 format available.
Message #56 received at 66948 <at> debbugs.gnu.org (full text, mbox):
> From: Eshel Yaron <me <at> eshelyaron.com>
> Cc: Dmitry Gutov <dmitry <at> gutov.dev>, Philip Kaludercic
> <philipk <at> posteo.net>, João Távora
> <joaotavora <at> gmail.com>, Stefan Kangas
> <stefankangas <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
> 66948 <at> debbugs.gnu.org
> Date: Fri, 10 Nov 2023 08:09:40 +0100
>
> Juri Linkov <juri <at> linkov.net> writes:
>
> > Let's see what Eli decides to do with your new package.
>
> Sure. Eli, Stefan, any further comments about this addition?
I'm not sure I understand what is expected from me. Is this for GNU
ELPA or for Emacs core?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Fri, 10 Nov 2023 07:59:01 GMT)
Full text and
rfc822 format available.
Message #59 received at 66948 <at> debbugs.gnu.org (full text, mbox):
Hi Eli,
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Eshel Yaron <me <at> eshelyaron.com>
>> Cc: Dmitry Gutov <dmitry <at> gutov.dev>, Philip Kaludercic
>> <philipk <at> posteo.net>, João Távora
>> <joaotavora <at> gmail.com>, Stefan Kangas
>> <stefankangas <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
>> 66948 <at> debbugs.gnu.org
>> Date: Fri, 10 Nov 2023 08:09:40 +0100
>>
>> Juri Linkov <juri <at> linkov.net> writes:
>>
>> > Let's see what Eli decides to do with your new package.
>>
>> Sure. Eli, Stefan, any further comments about this addition?
>
> I'm not sure I understand what is expected from me. Is this for GNU
> ELPA or for Emacs core?
This is for Emacs core, I'm attaching the latest patch again. I think
it mostly needs your OK. Notably, I'd appreciate it if you could check
out the addition to the manual and see if you have any comments on that.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Fri, 10 Nov 2023 08:01:01 GMT)
Full text and
rfc822 format available.
Message #62 received at 66948 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Eshel Yaron <me <at> eshelyaron.com> writes:
> This is for Emacs core, I'm attaching the latest patch again. I think
> it mostly needs your OK. Notably, I'd appreciate it if you could check
> out the addition to the manual and see if you have any comments on that.
And now with the actual patch attached... sorry about that.
[v3-0001-Add-Completion-Preview-mode.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Fri, 10 Nov 2023 08:02:02 GMT)
Full text and
rfc822 format available.
Message #65 received at 66948 <at> debbugs.gnu.org (full text, mbox):
Eshel Yaron <me <at> eshelyaron.com> writes:
> Hi Eli,
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>>> From: Eshel Yaron <me <at> eshelyaron.com>
>>> Cc: Dmitry Gutov <dmitry <at> gutov.dev>, Philip Kaludercic
>>> <philipk <at> posteo.net>, João Távora
>>> <joaotavora <at> gmail.com>, Stefan Kangas
>>> <stefankangas <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
>>> 66948 <at> debbugs.gnu.org
>>> Date: Fri, 10 Nov 2023 08:09:40 +0100
>>>
>>> Juri Linkov <juri <at> linkov.net> writes:
>>>
>>> > Let's see what Eli decides to do with your new package.
>>>
>>> Sure. Eli, Stefan, any further comments about this addition?
>>
>> I'm not sure I understand what is expected from me. Is this for GNU
>> ELPA or for Emacs core?
>
> This is for Emacs core, I'm attaching the latest patch again. I think
> it mostly needs your OK. Notably, I'd appreciate it if you could check
> out the addition to the manual and see if you have any comments on that.
This message doesn't appear to have any patch attached.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Fri, 10 Nov 2023 13:07:01 GMT)
Full text and
rfc822 format available.
Message #68 received at 66948 <at> debbugs.gnu.org (full text, mbox):
> From: Eshel Yaron <me <at> eshelyaron.com>
> Cc: juri <at> linkov.net, dmitry <at> gutov.dev, philipk <at> posteo.net,
> joaotavora <at> gmail.com, stefankangas <at> gmail.com, 66948 <at> debbugs.gnu.org
> Date: Fri, 10 Nov 2023 08:59:56 +0100
>
> Eshel Yaron <me <at> eshelyaron.com> writes:
>
> > This is for Emacs core, I'm attaching the latest patch again. I think
> > it mostly needs your OK. Notably, I'd appreciate it if you could check
> > out the addition to the manual and see if you have any comments on that.
>
> And now with the actual patch attached... sorry about that.
Thanks. If this is for core, I think there's still work to be done
here, see the comments below.
> --- a/doc/emacs/programs.texi
> +++ b/doc/emacs/programs.texi
> @@ -1701,6 +1701,90 @@ Symbol Completion
> In Text mode and related modes, @kbd{M-@key{TAB}} completes words
> based on the spell-checker's dictionary. @xref{Spelling}.
>
> +@cindex completion preview
> +@cindex preview completion
> +@cindex suggestion preview
> +@cindex Completion Preview mode
> +@findex completion-preview-mode
> +@vindex completion-preview-mode
> + Completion Preview mode is a minor mode that shows you symbol
> +completion suggestions as type. When you enable Completion Preview
> +mode in a buffer (with @kbd{M-x completion-preview-mode}), Emacs
> +examines the text around point after certain commands you invoke and
> +automatically suggests a possible completion. Emacs displays this
> +suggestion with an inline preview right after point, so you see in
> +advance exactly how the text will look if you accept the completion
> +suggestion---that's why it's called a preview.
I don't think this minor mode warrants such a long and detailed
description in the user manual. The section where you added that just
mentions the various similar features, it doesn't describe them in
such great detail, so I think we should do the same for this new mode.
IOW, just mention it and what it does in a sentence or two, and move
the rest of the description to the Commentary section of
completion-preview.el and/or to the relevant doc strings.
> +*** New minor mode 'completion-preview-mode'.
> +This minor mode shows you symbol completion suggestions as you type,
> +using an inline preview. New user options in the 'completion-preview'
> +customization group control exactly when Emacs displays this preview.
This fails to mention that (evidently) this mode is intended to be
used only in descendants of prog-mode, i.e. that useful completions
will be available only for editing program source. But see below.
> +;; This library provides the Completion Preview mode. This minor mode
> +;; displays the top completion candidate for the symbol at point in an
> +;; overlay after point. If you want to enable Completion Preview mode
> +;; in all programming modes, add the following to your Emacs init:
> +;;
> +;; (add-hook 'prog-mode-hook #'completion-preview-mode)
I'm not sure why this is advertised for prog-mode. Are completions
produced for descendants of Text mode, for example? If not, why not?
I see other apps offering completion in this style for editing
human-readable text, so why cannot we have that as well (for word at
point)?
> +(defcustom completion-preview-exact-match-only nil
> + "Whether to show completion preview only when there is an exact match.
> +
> +If this option is non-nil, Completion Preview mode only shows the
> +preview overlay when there is exactly one completion candidate
^^^^^^^
This is an implementation detail, better left out of the doc string.
> +that matches the symbol at point, otherwise it shows the top
> +candidate also when there are multiple matching candidates."
What do you mean by "top candidate"? I can try guessing, but I think
it is better to reword this.
> + :type 'boolean)
Every new defcustom should have a :version tag (this comment is for
all the defcustoms in this file).
> +(defcustom completion-preview-commands '(self-insert-command
> + delete-backward-char
> + backward-delete-char-untabify)
I think you should add insert-char to this list.
Also, did you test this minor mode when Overwrite mode is in effect?
> +(defcustom completion-preview-minimum-symbol-length 3
> + "Minimum length of the symbol at point for showing completion preview."
> + :type 'natnum)
Why do we need this defcustom? IOW, why not show the completion after
a single character?
> +(defcustom completion-preview-hook
> + '(completion-preview-require-certain-commands
> + completion-preview-require-minimum-symbol-length)
> + "Hook for functions that determine whether to show preview completion.
> +
> +Completion Preview mode calls each of these functions in order
> +after each command, and only displays the completion preview when
> +all of the functions return non-nil."
This feature sounds like over-engineering to me.
> +(defface completion-preview-exact
> + '((t :underline t :inherit completion-preview))
The underline face is not universally supported, so this defface
should have fallbacks.
> +(defvar completion-preview--internal-commands
> + '(completion-preview-next-candidate completion-preview-prev-candidate)
> + "List of commands that manipulate the completion preview.")
> +
> +(defun completion-preview--internal-command-p ()
> + "Return non-nil if `this-command' manipulates the completion preview."
> + (memq this-command completion-preview--internal-commands))
Should this be a defsubst?
> +(defun completion-preview-require-certain-commands ()
> + "Check if `this-command' is one of `completion-preview-commands'."
> + (or (completion-preview--internal-command-p)
> + (memq this-command completion-preview-commands)))
Likewise here.
> +(defun completion-preview-require-minimum-symbol-length ()
> + "Check if the length of symbol at point is at least above a certain threshold.
> +`completion-preview-minimum-symbol-length' determines that threshold."
> + (pcase (bounds-of-thing-at-point 'symbol)
> + (`(,beg . ,end)
> + (<= completion-preview-minimum-symbol-length (- end beg)))))
Is usage of pcase really justified here, and if so, why?
> +(defun completion-preview--make-overlay (pos string)
> + "Make a new completion preview overlay at POS showing STRING."
> + (if completion-preview--overlay
> + (move-overlay completion-preview--overlay pos pos)
> + (setq completion-preview--overlay (make-overlay pos pos)))
Should this overlay be specific to the selected window? IOW, do we
really want to see the preview in all the windows showing this buffer?
> + (add-text-properties 0 1 '(cursor 1) string)
> + (overlay-put completion-preview--overlay 'after-string string)
Sounds like you keep calling overlay-put and adding the same property
to the string each time this function is called, even if the overlay
already shows the same string?
> +(define-minor-mode completion-preview-active-mode
> + "Mode for when the completion preview is active."
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
It is better to say "when the completion preview is shown". "Active"
is ambiguous here.
> +(defun completion-preview--exit-function (func)
> + "Return an exit function that hides the completion preview and calls FUNC."
> + (lambda (&rest args)
> + (completion-preview-active-mode -1)
> + (when func (apply func args))))
^^^^^^^^^^
Perhaps "(when (functionp func) ..."?
> +(defun completion-preview--update ()
> + "Update completion preview."
> + (pcase (let ((completion-preview-insert-on-completion nil))
> + (run-hook-with-args-until-success 'completion-at-point-functions))
> + (`(,beg ,end ,table . ,plist)
Why use pcase here and not seq-let?
> +(defun completion-preview--show ()
> + "Show completion preview."
> + (when completion-preview-active-mode
> + (let* ((beg (completion-preview--get 'completion-preview-beg))
> + (cands (completion-preview--get 'completion-preview-cands))
> + (index (completion-preview--get 'completion-preview-index))
> + (cand (nth index cands))
> + (len (length cand))
> + (end (+ beg len))
> + (cur (point))
> + (face (get-text-property 0 'face (completion-preview--get 'after-string))))
> + (if (and (< beg cur end) (string-prefix-p (buffer-substring beg cur) cand))
> + (overlay-put (completion-preview--make-overlay
> + cur (propertize (substring cand (- cur beg))
> + 'face face))
> + 'completion-preview-end cur)
> + (completion-preview-active-mode -1))))
> + (while-no-input (completion-preview--update)))
I'm puzzled by this function. What does it do, and why is it needed?
> +(defun completion-preview--post-command ()
> + "Create, update or delete completion preview post last command."
> + (if (run-hook-with-args-until-failure 'completion-preview-hook)
> + (or (completion-preview--internal-command-p)
> + (completion-preview--show))
> + (completion-preview-active-mode -1)))
This needs more comments to explain the non-trivial logic.
> +(defun completion-preview--insert ()
> + "Completion at point function for inserting the current preview."
The purpose of this function should be described either in the doc
string or in some comment.
> +(defun completion-preview-insert ()
> + "Insert the current completion preview."
You cannot "insert the preview". Please reword the doc string.
> + (interactive)
> + (let ((completion-preview-insert-on-completion t))
> + (completion-at-point)))
Why not just insert the string you show in the preview?
> +(defun completion-preview-prev-candidate ()
> + "Cycle the preview to the previous completion suggestion."
You are cycling the candidates, not the preview.
> +(defun completion-preview-next-candidate (direction)
> + "Cycle the preview to the next completion suggestion in DIRECTION.
> +
> +DIRECTION should be either 1 which means cycle forward, or -1
> +which means cycle backward. Interactively, DIRECTION is the
> +prefix argument." ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
^^^^^^^^^^^^^^^
"...and defaults to 1".
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Fri, 10 Nov 2023 16:25:02 GMT)
Full text and
rfc822 format available.
Message #71 received at 66948 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:
> Thanks. If this is for core, I think there's still work to be done
> here, see the comments below.
Thanks for reviewing. I'm attaching an updated patch (v4) following
your comments.
>> + Completion Preview mode is a minor mode that shows you symbol
>> +completion suggestions as type. When you enable Completion Preview
>> +mode in a buffer (with @kbd{M-x completion-preview-mode}), Emacs
>> +examines the text around point after certain commands you invoke and
>> +automatically suggests a possible completion. Emacs displays this
>> +suggestion with an inline preview right after point, so you see in
>> +advance exactly how the text will look if you accept the completion
>> +suggestion---that's why it's called a preview.
>
> I don't think this minor mode warrants such a long and detailed
> description in the user manual. The section where you added that just
> mentions the various similar features, it doesn't describe them in
> such great detail, so I think we should do the same for this new mode.
> IOW, just mention it and what it does in a sentence or two, and move
> the rest of the description to the Commentary section of
> completion-preview.el and/or to the relevant doc strings.
Alright, done. I left the first paragraph in the manual, and moved the
rest to the commentary section of completion-preview.el after some
adjustments.
>> +*** New minor mode 'completion-preview-mode'.
>> +This minor mode shows you symbol completion suggestions as you type,
>> +using an inline preview. New user options in the 'completion-preview'
>> +customization group control exactly when Emacs displays this preview.
>
> This fails to mention that (evidently) this mode is intended to be
> used only in descendants of prog-mode, i.e. that useful completions
> will be available only for editing program source. But see below.
It is not the case that Completion Preview is intended only for
`prog-mode` descendants, it works just as well in e.g. `text-mode`.
>> +;; This library provides the Completion Preview mode. This minor mode
>> +;; displays the top completion candidate for the symbol at point in an
>> +;; overlay after point. If you want to enable Completion Preview mode
>> +;; in all programming modes, add the following to your Emacs init:
>> +;;
>> +;; (add-hook 'prog-mode-hook #'completion-preview-mode)
>
> I'm not sure why this is advertised for prog-mode.
This is just an example. I've now removed it to avoid confusion.
> Are completions produced for descendants of Text mode, for example?
Sure. I'm running with `completion-preview-mode` in `text-mode-hook` myself.
>> +(defcustom completion-preview-exact-match-only nil
>> + "Whether to show completion preview only when there is an exact match.
>> +
>> +If this option is non-nil, Completion Preview mode only shows the
>> +preview overlay when there is exactly one completion candidate
> ^^^^^^^
> This is an implementation detail, better left out of the doc string.
>
Done.
>> +that matches the symbol at point, otherwise it shows the top
>> +candidate also when there are multiple matching candidates."
>
> What do you mean by "top candidate"? I can try guessing, but I think
> it is better to reword this.
Right, reworded.
>> + :type 'boolean)
>
> Every new defcustom should have a :version tag (this comment is for
> all the defcustoms in this file).
Done.
>> +(defcustom completion-preview-commands '(self-insert-command
>> + delete-backward-char
>> + backward-delete-char-untabify)
>
> I think you should add insert-char to this list.
Done.
> Also, did you test this minor mode when Overwrite mode is in effect?
Yes, no surprises there AFAICT, works well.
>> +(defcustom completion-preview-minimum-symbol-length 3
>> + "Minimum length of the symbol at point for showing completion preview."
>> + :type 'natnum)
>
> Why do we need this defcustom? IOW, why not show the completion after
> a single character?
Basically, a single character often has many completion candidates, and
most of them are not what you want. After three characters, the preview
is much more likely to show you a useful candidate. So you can think of
this option as an adjustable threshold for how much information we
require the completion backend to have before we consider its
suggestions any good. I'm open to changing the default value, but I
think that three characters is a very sane default.
>> +(defcustom completion-preview-hook
>> + '(completion-preview-require-certain-commands
>> + completion-preview-require-minimum-symbol-length)
>> + "Hook for functions that determine whether to show preview completion.
>> +
>> +Completion Preview mode calls each of these functions in order
>> +after each command, and only displays the completion preview when
>> +all of the functions return non-nil."
>
> This feature sounds like over-engineering to me.
I think this makes the mode nicely flexible, as it lets users and other
code add different conditions for when to show the preview, e.g. only in
or out of comments. And the added complexity is negligible, really. So
I guess we could do without this option, but I'd prefer to keep it
unless you feel strongly about that.
>> +(defface completion-preview-exact
>> + '((t :underline t :inherit completion-preview))
>
> The underline face is not universally supported, so this defface
> should have fallbacks.
The `underline` face in faces.el has `:underline t` in the fallback
clause too, so I figured that should be alright, no?
>> +(defvar completion-preview--internal-commands
>> + '(completion-preview-next-candidate completion-preview-prev-candidate)
>> + "List of commands that manipulate the completion preview.")
>> +
>> +(defun completion-preview--internal-command-p ()
>> + "Return non-nil if `this-command' manipulates the completion preview."
>> + (memq this-command completion-preview--internal-commands))
>
> Should this be a defsubst?
Probably, it is now :)
>> +(defun completion-preview-require-certain-commands ()
>> + "Check if `this-command' is one of `completion-preview-commands'."
>> + (or (completion-preview--internal-command-p)
>> + (memq this-command completion-preview-commands)))
>
> Likewise here.
Done.
>> +(defun completion-preview-require-minimum-symbol-length ()
>> + "Check if the length of symbol at point is at least above a certain threshold.
>> +`completion-preview-minimum-symbol-length' determines that threshold."
>> + (pcase (bounds-of-thing-at-point 'symbol)
>> + (`(,beg . ,end)
>> + (<= completion-preview-minimum-symbol-length (- end beg)))))
>
> Is usage of pcase really justified here, and if so, why?
Since we're relying on `completion-at-point`, that already uses `pcase`,
I'm not sure what's the cost of using `pcase` here too. Furthermore, it
does exactly what we want here, including correctly handling the case
where `bounds-of-thing-at-point` returns nil. So yes, it's a good use
of `pcase` IMO. But if you prefer a different style I'm open to adjust
this part, of course.
>> +(defun completion-preview--make-overlay (pos string)
>> + "Make a new completion preview overlay at POS showing STRING."
>> + (if completion-preview--overlay
>> + (move-overlay completion-preview--overlay pos pos)
>> + (setq completion-preview--overlay (make-overlay pos pos)))
>
> Should this overlay be specific to the selected window? IOW, do we
> really want to see the preview in all the windows showing this buffer?
Making the preview window-specific is a good idea, thanks. Done in the
updated patch.
>> + (add-text-properties 0 1 '(cursor 1) string)
>> + (overlay-put completion-preview--overlay 'after-string string)
>
> Sounds like you keep calling overlay-put and adding the same property
> to the string each time this function is called, even if the overlay
> already shows the same string?
Even if the preview exists, this function is called with a different
`string` argument than the one already shown. That `string` is a
substring of a completion candidate, and it doesn't already have the
`cursor` property. So no, this is not redundant. There may be room for
an optimization here, but I don't think it'd be significant.
>> +(define-minor-mode completion-preview-active-mode
>> + "Mode for when the completion preview is active."
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> It is better to say "when the completion preview is shown". "Active"
> is ambiguous here.
Makes sense, done.
>> +(defun completion-preview--exit-function (func)
>> + "Return an exit function that hides the completion preview and calls FUNC."
>> + (lambda (&rest args)
>> + (completion-preview-active-mode -1)
>> + (when func (apply func args))))
> ^^^^^^^^^^
> Perhaps "(when (functionp func) ..."?
We're only ever called with either a function or nil here, so this is
basically equivalent. Still done now to be more explicit.
>> +(defun completion-preview--update ()
>> + "Update completion preview."
>> + (pcase (let ((completion-preview-insert-on-completion nil))
>> + (run-hook-with-args-until-success 'completion-at-point-functions))
>> + (`(,beg ,end ,table . ,plist)
>
> Why use pcase here and not seq-let?
Because `seq-let` doesn't do the right thing (for our purposes here)
when the sequence that you pass it doesn't have the given shape.
Namely, here `pcase` correctly handles the case where the value is nil
for example, while `seq-let` would require another test in the body
(e.g. `(when beg ...)`) to see if we actually got what we expected.
Anyways, as I mentioned earlier, I'm fine with not using `pcase` here if
there's a reason for that.
>> +(defun completion-preview--show ()
>> + "Show completion preview."
>> + (when completion-preview-active-mode
>> + (let* ((beg (completion-preview--get 'completion-preview-beg))
>> + (cands (completion-preview--get 'completion-preview-cands))
>> + (index (completion-preview--get 'completion-preview-index))
>> + (cand (nth index cands))
>> + (len (length cand))
>> + (end (+ beg len))
>> + (cur (point))
>> + (face (get-text-property 0 'face (completion-preview--get 'after-string))))
>> + (if (and (< beg cur end) (string-prefix-p (buffer-substring beg cur) cand))
>> + (overlay-put (completion-preview--make-overlay
>> + cur (propertize (substring cand (- cur beg))
>> + 'face face))
>> + 'completion-preview-end cur)
>> + (completion-preview-active-mode -1))))
>> + (while-no-input (completion-preview--update)))
>
> I'm puzzled by this function. What does it do, and why is it needed?
I've added some comments in the updated patch. This function is called
in `post-command-hook` if we determined that we want to show the
preview. It first checks if there's already a preview. If there is,
then we need to update it before consulting
`completion-at-point-functions` for a new completion candidate, since
that might not be immediate and we never want to show a stale preview.
So we check if the candidate of the existing preview is still applicable
after the command that just run. If so we update it, otherwise we hide
it. Finally, we go on to consulting the completion backends inside
`while-no-input`.
>> +(defun completion-preview--post-command ()
>> + "Create, update or delete completion preview post last command."
>> + (if (run-hook-with-args-until-failure 'completion-preview-hook)
>> + (or (completion-preview--internal-command-p)
>> + (completion-preview--show))
>> + (completion-preview-active-mode -1)))
>
> This needs more comments to explain the non-trivial logic.
Done.
>> +(defun completion-preview--insert ()
>> + "Completion at point function for inserting the current preview."
>
> The purpose of this function should be described either in the doc
> string or in some comment.
Sure, I've extended its docstring.
>> +(defun completion-preview-insert ()
>> + "Insert the current completion preview."
>
> You cannot "insert the preview". Please reword the doc string.
Right, done.
>> + (interactive)
>> + (let ((completion-preview-insert-on-completion t))
>> + (completion-at-point)))
>
> Why not just insert the string you show in the preview?
This way we let `completion-at-point` to take care of things like
calling the `:exit-function`, instead of duplicating that code.
>> +(defun completion-preview-prev-candidate ()
>> + "Cycle the preview to the previous completion suggestion."
>
> You are cycling the candidates, not the preview.
Indeed, I rephrased that part in the updated patch.
>> +(defun completion-preview-next-candidate (direction)
>> + "Cycle the preview to the next completion suggestion in DIRECTION.
>> +
>> +DIRECTION should be either 1 which means cycle forward, or -1
>> +which means cycle backward. Interactively, DIRECTION is the
>> +prefix argument." ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ^^^^^^^^^^^^^^^
> "...and defaults to 1".
Done.
Here's the new patch:
[v4-0001-Add-Completion-Preview-mode.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Sat, 11 Nov 2023 08:55:02 GMT)
Full text and
rfc822 format available.
Message #74 received at 66948 <at> debbugs.gnu.org (full text, mbox):
> From: Eshel Yaron <me <at> eshelyaron.com>
> Cc: philipk <at> posteo.net, juri <at> linkov.net, dmitry <at> gutov.dev,
> stefankangas <at> gmail.com, 66948 <at> debbugs.gnu.org, joaotavora <at> gmail.com
> Date: Fri, 10 Nov 2023 17:23:12 +0100
>
> Thanks for reviewing. I'm attaching an updated patch (v4) following
> your comments.
Thanks.
> + Completion Preview mode is a minor mode that shows you symbol
> +completion suggestions as type. When you enable Completion Preview
> +mode in a buffer (with @kbd{M-x completion-preview-mode}), Emacs
> +examines the text around point after certain commands you invoke and
> +automatically suggests a possible completion. Emacs displays this
> +suggestion with an inline preview right after point, so you see in
> +advance exactly how the text will look if you accept the completion
> +suggestion---that's why it's called a preview.
This is still too wordy, IMO. I suggest
Completion Preview mode is a minor mode that shows completion
suggestions as you type. When you enable this mode (with @kbd{M-x
completion-preview-mode}), Emacs automatically displays the
suggested completion for text around point as an in-line preview
right after point; type @key{TAB} to accept the suggestion.
Also, one of these two index entries is redundant:
> +@findex completion-preview-mode
> +@vindex completion-preview-mode
Since the ELisp manual has just one Index node, it is enough to have
the @findex entry alone.
> > Are completions produced for descendants of Text mode, for example?
>
> Sure. I'm running with `completion-preview-mode` in `text-mode-hook` myself.
What is/are the backend(s) that provide(s) the completions in the
text-mode case? Is that just a word completion, or are we capable of
suggesting phrases as well?
> > Also, did you test this minor mode when Overwrite mode is in effect?
>
> Yes, no surprises there AFAICT, works well.
What does it do in Overwrite mode if one accepts a completion?
If it overwrites the following text, I'm not sure it should.
> >> +(defcustom completion-preview-minimum-symbol-length 3
> >> + "Minimum length of the symbol at point for showing completion preview."
> >> + :type 'natnum)
> >
> > Why do we need this defcustom? IOW, why not show the completion after
> > a single character?
>
> Basically, a single character often has many completion candidates, and
> most of them are not what you want. After three characters, the preview
> is much more likely to show you a useful candidate. So you can think of
> this option as an adjustable threshold for how much information we
> require the completion backend to have before we consider its
> suggestions any good. I'm open to changing the default value, but I
> think that three characters is a very sane default.
The advantage of 1 character is that we don't need this defcustom at
all, and it is basically up to the user when to type TAB, or even look
at the preview.
Alternatively, we could have a defcustom based on a different design:
show preview only when there are fewer than N completion candidates,
with N being customizable. That would make much more sense, IMO,
since it replaces a largely "mechanical" limitation with one that is
meaningful for users.
> >> +(defcustom completion-preview-hook
> >> + '(completion-preview-require-certain-commands
> >> + completion-preview-require-minimum-symbol-length)
> >> + "Hook for functions that determine whether to show preview completion.
> >> +
> >> +Completion Preview mode calls each of these functions in order
> >> +after each command, and only displays the completion preview when
> >> +all of the functions return non-nil."
> >
> > This feature sounds like over-engineering to me.
>
> I think this makes the mode nicely flexible, as it lets users and other
> code add different conditions for when to show the preview, e.g. only in
> or out of comments. And the added complexity is negligible, really. So
> I guess we could do without this option, but I'd prefer to keep it
> unless you feel strongly about that.
I'd like to defer any extensibility features like this until we have
some data to support the need for such extensibility. Defining those
ahead of any real experience is a kind of premature optimization, IMO.
> >> +(defface completion-preview-exact
> >> + '((t :underline t :inherit completion-preview))
> >
> > The underline face is not universally supported, so this defface
> > should have fallbacks.
>
> The `underline` face in faces.el has `:underline t` in the fallback
> clause too, so I figured that should be alright, no?
If you are okay with seeing no effect at all when the terminal doesn't
support the underline attribute, then yes. But I thought we want this
face to stand out no matter what, don't we?
> >> +(defun completion-preview-require-minimum-symbol-length ()
> >> + "Check if the length of symbol at point is at least above a certain threshold.
> >> +`completion-preview-minimum-symbol-length' determines that threshold."
> >> + (pcase (bounds-of-thing-at-point 'symbol)
> >> + (`(,beg . ,end)
> >> + (<= completion-preview-minimum-symbol-length (- end beg)))))
> >
> > Is usage of pcase really justified here, and if so, why?
>
> Since we're relying on `completion-at-point`, that already uses `pcase`,
> I'm not sure what's the cost of using `pcase` here too.
Readability. A person who isn't familiar with pcase will not need to
go read the documentation to understand this code.
> Furthermore, it does exactly what we want here, including correctly
> handling the case where `bounds-of-thing-at-point` returns nil. So
> yes, it's a good use of `pcase` IMO. But if you prefer a different
> style I'm open to adjust this part, of course.
I prefer not to use it where a simple if or cond will do, and where
the data structures are as simple as a cons cell.
This goes back to that long dispute on emacs-devel we are having,
where many people agree -- in principle -- that over-using such
advanced features with complex syntax is not a Good Thing. So I
thought we should actually "practice what we preach".
> >> + (add-text-properties 0 1 '(cursor 1) string)
> >> + (overlay-put completion-preview--overlay 'after-string string)
> >
> > Sounds like you keep calling overlay-put and adding the same property
> > to the string each time this function is called, even if the overlay
> > already shows the same string?
>
> Even if the preview exists, this function is called with a different
> `string` argument than the one already shown. That `string` is a
> substring of a completion candidate, and it doesn't already have the
> `cursor` property. So no, this is not redundant. There may be room for
> an optimization here, but I don't think it'd be significant.
What bothers me is consing (which leads to GC). Testing a string for
equality is simple, and if that avoids extra consing, I think it's a
good optimization.
> >> +(defun completion-preview--update ()
> >> + "Update completion preview."
> >> + (pcase (let ((completion-preview-insert-on-completion nil))
> >> + (run-hook-with-args-until-success 'completion-at-point-functions))
> >> + (`(,beg ,end ,table . ,plist)
> >
> > Why use pcase here and not seq-let?
>
> Because `seq-let` doesn't do the right thing (for our purposes here)
> when the sequence that you pass it doesn't have the given shape.
> Namely, here `pcase` correctly handles the case where the value is nil
> for example, while `seq-let` would require another test in the body
> (e.g. `(when beg ...)`) to see if we actually got what we expected.
Is that single extra test so important to avoid?
> >> +(defun completion-preview--show ()
> >> + "Show completion preview."
> >> + (when completion-preview-active-mode
> >> + (let* ((beg (completion-preview--get 'completion-preview-beg))
> >> + (cands (completion-preview--get 'completion-preview-cands))
> >> + (index (completion-preview--get 'completion-preview-index))
> >> + (cand (nth index cands))
> >> + (len (length cand))
> >> + (end (+ beg len))
> >> + (cur (point))
> >> + (face (get-text-property 0 'face (completion-preview--get 'after-string))))
> >> + (if (and (< beg cur end) (string-prefix-p (buffer-substring beg cur) cand))
> >> + (overlay-put (completion-preview--make-overlay
> >> + cur (propertize (substring cand (- cur beg))
> >> + 'face face))
> >> + 'completion-preview-end cur)
> >> + (completion-preview-active-mode -1))))
> >> + (while-no-input (completion-preview--update)))
> >
> > I'm puzzled by this function. What does it do, and why is it needed?
>
> I've added some comments in the updated patch.
Thanks, but please also make the doc string more useful. It is now
too terse, IMO.
Also, the last part is still quite obscure: why do you turn off
completion-preview-active-mode, and why the while-no-input loop that
calls completion-preview--update? The comment which starts with
"Reconsult" should be probably reworded to be more self-explanatory;
e.g., the word "backend" is barely used, let alone explained, in the
code or its comments.
Btw, I noticed that your comments don't end in a period. They should,
at least when a comment is a complete sentence.
> >> + (interactive)
> >> + (let ((completion-preview-insert-on-completion t))
> >> + (completion-at-point)))
> >
> > Why not just insert the string you show in the preview?
>
> This way we let `completion-at-point` to take care of things like
> calling the `:exit-function`, instead of duplicating that code.
Sorry, I still don't understand. What about :exit-function, and why
inserting the completion needs it?
Btw, did you consider an alternative design, where the completion is
displayed from an idle timer, not from a post-command-hook? The
latter means you make Emacs less responsive during fast typing (when
the user won't normally need the preview), whereas doing it from a
timer better suits the use case: a preview is shown when the user
might be considering what to type next.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Sat, 11 Nov 2023 12:03:02 GMT)
Full text and
rfc822 format available.
Message #77 received at 66948 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Eshel Yaron <me <at> eshelyaron.com>
>> Cc: philipk <at> posteo.net, juri <at> linkov.net, dmitry <at> gutov.dev,
>> stefankangas <at> gmail.com, 66948 <at> debbugs.gnu.org, joaotavora <at> gmail.com
>> Date: Fri, 10 Nov 2023 17:23:12 +0100
>>
>> Thanks for reviewing. I'm attaching an updated patch (v4) following
>> your comments.
>
> Thanks.
>
>> + Completion Preview mode is a minor mode that shows you symbol
>> +completion suggestions as type. When you enable Completion Preview
>> +mode in a buffer (with @kbd{M-x completion-preview-mode}), Emacs
>> +examines the text around point after certain commands you invoke and
>> +automatically suggests a possible completion. Emacs displays this
>> +suggestion with an inline preview right after point, so you see in
>> +advance exactly how the text will look if you accept the completion
>> +suggestion---that's why it's called a preview.
>
> This is still too wordy, IMO. I suggest
>
> Completion Preview mode is a minor mode that shows completion
> suggestions as you type. When you enable this mode (with @kbd{M-x
> completion-preview-mode}), Emacs automatically displays the
> suggested completion for text around point as an in-line preview
> right after point; type @key{TAB} to accept the suggestion.
LGTM, thanks. I'm attaching an updated patch (v5) below.
> Also, one of these two index entries is redundant:
>
>> +@findex completion-preview-mode
>> +@vindex completion-preview-mode
>
> Since the ELisp manual has just one Index node, it is enough to have
> the @findex entry alone.
Done.
>> > Are completions produced for descendants of Text mode, for example?
>>
>> Sure. I'm running with `completion-preview-mode` in `text-mode-hook` myself.
>
> What is/are the backend(s) that provide(s) the completions in the
> text-mode case? Is that just a word completion, or are we capable of
> suggesting phrases as well?
That's all up to what you put in your `completion-at-point-functions`.
If you have a completion function that suggests phrases, it'll work just
as well as word completion. The default value of
`completion-at-point-functions` is not that useful for `text-mode` I'm
afraid, but adding something like `dabbrev-capf` is easy enough and
makes it much more useful. This is not a concern for Completion
Preview, though. We just call the `completion-at-point-functions`.
>> > Also, did you test this minor mode when Overwrite mode is in effect?
>>
>> Yes, no surprises there AFAICT, works well.
>
> What does it do in Overwrite mode if one accepts a completion?
> If it overwrites the following text, I'm not sure it should.
It inserts the completion suggestion without overwriting the following
text, so that's fine.
>> >> +(defcustom completion-preview-minimum-symbol-length 3
>> >> + "Minimum length of the symbol at point for showing completion preview."
>> >> + :type 'natnum)
>> >
>> > Why do we need this defcustom? IOW, why not show the completion after
>> > a single character?
>>
>> Basically, a single character often has many completion candidates, and
>> most of them are not what you want. After three characters, the preview
>> is much more likely to show you a useful candidate. So you can think of
>> this option as an adjustable threshold for how much information we
>> require the completion backend to have before we consider its
>> suggestions any good. I'm open to changing the default value, but I
>> think that three characters is a very sane default.
>
> The advantage of 1 character is that we don't need this defcustom at
> all, and it is basically up to the user when to type TAB, or even look
> at the preview.
One character is not the same as removing this `defcustom`. Without
this `defcustom`, i.e. without checking the length of the symbol at
point, we would try to show the preview even after the user types a
bunch of space and there is nothing useful to complete at point at all.
> Alternatively, we could have a defcustom based on a different design:
> show preview only when there are fewer than N completion candidates,
> with N being customizable. That would make much more sense, IMO,
> since it replaces a largely "mechanical" limitation with one that is
> meaningful for users.
That would indeed be a nice solution, but it has a fatal flaw, sadly.
Computing the set of completion candidates is a costly operation,
especially with backends such as LSP, so we don't want to do that after
each command. We need some heuristic to decide when we're likely to
obtain a valuable completion suggestion, otherwise we butt out.
Checking the length of the symbol at point is cheap and it provides a
good heuristic that's easy to understand and control.
I'm open to changing the default to one character if you think that's
preferable. I do think the `defcustom` itself should stay, though.
>> >> +(defcustom completion-preview-hook
>> >> + '(completion-preview-require-certain-commands
>> >> + completion-preview-require-minimum-symbol-length)
>> >> + "Hook for functions that determine whether to show preview completion.
>> >> +
>> >> +Completion Preview mode calls each of these functions in order
>> >> +after each command, and only displays the completion preview when
>> >> +all of the functions return non-nil."
>> >
>> > This feature sounds like over-engineering to me.
>>
>> I think this makes the mode nicely flexible, as it lets users and other
>> code add different conditions for when to show the preview, e.g. only in
>> or out of comments. And the added complexity is negligible, really. So
>> I guess we could do without this option, but I'd prefer to keep it
>> unless you feel strongly about that.
>
> I'd like to defer any extensibility features like this until we have
> some data to support the need for such extensibility. Defining those
> ahead of any real experience is a kind of premature optimization, IMO.
Fine, removed.
>> >> +(defface completion-preview-exact
>> >> + '((t :underline t :inherit completion-preview))
>> >
>> > The underline face is not universally supported, so this defface
>> > should have fallbacks.
>>
>> The `underline` face in faces.el has `:underline t` in the fallback
>> clause too, so I figured that should be alright, no?
>
> If you are okay with seeing no effect at all when the terminal doesn't
> support the underline attribute, then yes. But I thought we want this
> face to stand out no matter what, don't we?
That's okay IMO, the underline just differentiates between when you have
a single candidate and when you have multiple candidates. I don't think
that's that crucial, but if you can suggest a universally supported
fallback I'd be glad to add it, of course.
>> >> +(defun completion-preview-require-minimum-symbol-length ()
>> >> + "Check if the length of symbol at point is at least above a certain threshold.
>> >> +`completion-preview-minimum-symbol-length' determines that threshold."
>> >> + (pcase (bounds-of-thing-at-point 'symbol)
>> >> + (`(,beg . ,end)
>> >> + (<= completion-preview-minimum-symbol-length (- end beg)))))
>> >
>> > Is usage of pcase really justified here, and if so, why?
>>
>> Since we're relying on `completion-at-point`, that already uses `pcase`,
>> I'm not sure what's the cost of using `pcase` here too.
>
> Readability. A person who isn't familiar with pcase will not need to
> go read the documentation to understand this code.
Alright, I've change this function and another one to avoid `pcase`.
FWIW I find `pcase` perfectly readable, but the alternative isn't too
bad either in this case.
>> >> + (add-text-properties 0 1 '(cursor 1) string)
>> >> + (overlay-put completion-preview--overlay 'after-string string)
>> >
>> > Sounds like you keep calling overlay-put and adding the same property
>> > to the string each time this function is called, even if the overlay
>> > already shows the same string?
>>
>> Even if the preview exists, this function is called with a different
>> `string` argument than the one already shown. That `string` is a
>> substring of a completion candidate, and it doesn't already have the
>> `cursor` property. So no, this is not redundant. There may be room for
>> an optimization here, but I don't think it'd be significant.
>
> What bothers me is consing (which leads to GC). Testing a string for
> equality is simple, and if that avoids extra consing, I think it's a
> good optimization.
Makes sense, I've added an optimization for when the string is the same
as the existing `after-string`. This could happen if the user adds a
command that doesn't change the buffer text to
`completion-preview-commands` and uses that command.
>> >> +(defun completion-preview--update ()
>> >> + "Update completion preview."
>> >> + (pcase (let ((completion-preview-insert-on-completion nil))
>> >> + (run-hook-with-args-until-success 'completion-at-point-functions))
>> >> + (`(,beg ,end ,table . ,plist)
>> >
>> > Why use pcase here and not seq-let?
>>
>> Because `seq-let` doesn't do the right thing (for our purposes here)
>> when the sequence that you pass it doesn't have the given shape.
>> Namely, here `pcase` correctly handles the case where the value is nil
>> for example, while `seq-let` would require another test in the body
>> (e.g. `(when beg ...)`) to see if we actually got what we expected.
>
> Is that single extra test so important to avoid?
It's not super important, but it's nice. Anyway I've now changed to
`seq-let` here to avoid `pcase` as mentioned above.
>> >> +(defun completion-preview--show ()
>> >> + "Show completion preview."
>> >> + (when completion-preview-active-mode
>> >> + (let* ((beg (completion-preview--get 'completion-preview-beg))
>> >> + (cands (completion-preview--get 'completion-preview-cands))
>> >> + (index (completion-preview--get 'completion-preview-index))
>> >> + (cand (nth index cands))
>> >> + (len (length cand))
>> >> + (end (+ beg len))
>> >> + (cur (point))
>> >> + (face (get-text-property 0 'face (completion-preview--get 'after-string))))
>> >> + (if (and (< beg cur end) (string-prefix-p (buffer-substring beg cur) cand))
>> >> + (overlay-put (completion-preview--make-overlay
>> >> + cur (propertize (substring cand (- cur beg))
>> >> + 'face face))
>> >> + 'completion-preview-end cur)
>> >> + (completion-preview-active-mode -1))))
>> >> + (while-no-input (completion-preview--update)))
>> >
>> > I'm puzzled by this function. What does it do, and why is it needed?
>>
>> I've added some comments in the updated patch.
>
> Thanks, but please also make the doc string more useful. It is now
> too terse, IMO.
Done.
> Also, the last part is still quite obscure: why do you turn off
> completion-preview-active-mode,
I've added another comment to clarify that.
> and why the while-no-input loop that calls completion-preview--update?
`completion-preview--update` is where we invoke
`completion-at-point-functions`, which might take some time depending on
which backends you're using. So we wrap that with `while-no-input` and
gracefully handle the case in which this function is interrupted, by
virtue of the fact that we've already updated the preview to an
acceptable state just now.
> The comment which starts with "Reconsult" should be probably reworded
> to be more self-explanatory; e.g., the word "backend" is barely used,
> let alone explained, in the code or its comments.
I've updated that comments to explicitly say
`completion-at-point-functions` instead of "backends".
> Btw, I noticed that your comments don't end in a period. They should,
> at least when a comment is a complete sentence.
Done, thanks.
>> >> + (interactive)
>> >> + (let ((completion-preview-insert-on-completion t))
>> >> + (completion-at-point)))
>> >
>> > Why not just insert the string you show in the preview?
>>
>> This way we let `completion-at-point` to take care of things like
>> calling the `:exit-function`, instead of duplicating that code.
>
> Sorry, I still don't understand. What about :exit-function, and why
> inserting the completion needs it?
Some completion functions (that go on `completion-at-point-functions`)
specify an `:exit-function` that should be called after a candidate they
provide is inserted. This takes care of things like inserting
parentheses after the inserted function name, and moving point to in
between them. We want this to happen also when you insert the candidate
from the preview.
> Btw, did you consider an alternative design, where the completion is
> displayed from an idle timer, not from a post-command-hook? The
> latter means you make Emacs less responsive during fast typing (when
> the user won't normally need the preview), whereas doing it from a
> timer better suits the use case: a preview is shown when the user
> might be considering what to type next.
Yes, I considered that approach and tried it as well. The current
implementation works well enough even when typing fast (let me know if
you find otherwise), and it has the benefit of more immediate feedback.
It's also nice to get the preview even you're typing fast, that's how
some other editors behave too.
Thanks for your comments, here's the new patch:
[v5-0001-Add-Completion-Preview-mode.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Wed, 15 Nov 2023 15:42:03 GMT)
Full text and
rfc822 format available.
Message #80 received at 66948 <at> debbugs.gnu.org (full text, mbox):
Hello Eshel,
On Sun 05 Nov 2023 at 11:26am +01, Eshel Yaron wrote:
> Following the recent discussion on emacs-devel[0], I'm attaching a
> patch that adds a new Completion Preview minor mode.
Could you explain how this is intended to be related to
icomplete-in-buffer? Is it meant to be just an alternative? A
significantly enhanced replacement?
Thanks.
--
Sean Whitton
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Wed, 15 Nov 2023 15:43:01 GMT)
Full text and
rfc822 format available.
Message #83 received at 66948 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:
>> > Alternatively, we could have a defcustom based on a different design:
>> > show preview only when there are fewer than N completion candidates,
>> > with N being customizable. That would make much more sense, IMO,
>> > since it replaces a largely "mechanical" limitation with one that is
>> > meaningful for users.
>>
>> That would indeed be a nice solution, but it has a fatal flaw, sadly.
>> Computing the set of completion candidates is a costly operation,
>> especially with backends such as LSP, so we don't want to do that after
>> each command.
>
> This seems to be an argument in favor of the timer-based design I
> suggested. But if you aren't inclined to do that, I guess we can
> install the feature as it is designed now.
That'd be great, thanks.
>> >> > The underline face is not universally supported, so this defface
>> >> > should have fallbacks.
>> >>
>> >> The `underline` face in faces.el has `:underline t` in the fallback
>> >> clause too, so I figured that should be alright, no?
>> >
>> > If you are okay with seeing no effect at all when the terminal doesn't
>> > support the underline attribute, then yes. But I thought we want this
>> > face to stand out no matter what, don't we?
>>
>> That's okay IMO, the underline just differentiates between when you have
>> a single candidate and when you have multiple candidates. I don't think
>> that's that crucial, but if you can suggest a universally supported
>> fallback I'd be glad to add it, of course.
>
> How about some color (foreground or background)?
Sure, I've added a fallback that uses `:weight bold` when that's
supported but `:underline` isn't, and another ultimate fallback that
uses a background color instead. Here's the updated patch (v6):
[v6-0001-Add-Completion-Preview-mode.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Wed, 15 Nov 2023 15:47:02 GMT)
Full text and
rfc822 format available.
Message #86 received at 66948 <at> debbugs.gnu.org (full text, mbox):
Sean Whitton <spwhitton <at> spwhitton.name> writes:
> Hello Eshel,
Hi Sean,
> Could you explain how this is intended to be related to
> icomplete-in-buffer?
Juri asked[0] a similar question in the original discussion on
emacs-devel. `icomplete-in-buffer` and Completion Preview are
independent feature, you can use them in tandem if you want.
Best,
Eshel
[0] https://yhetil.org/emacs/m1cywyr2q0.fsf <at> eshelyaron.com/
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Wed, 15 Nov 2023 16:08:01 GMT)
Full text and
rfc822 format available.
Message #89 received at 66948 <at> debbugs.gnu.org (full text, mbox):
> From: Eshel Yaron <me <at> eshelyaron.com>
> Cc: philipk <at> posteo.net, juri <at> linkov.net, dmitry <at> gutov.dev,
> stefankangas <at> gmail.com, 66948 <at> debbugs.gnu.org, joaotavora <at> gmail.com
> Date: Sat, 11 Nov 2023 13:01:27 +0100
>
> >> Basically, a single character often has many completion candidates, and
> >> most of them are not what you want. After three characters, the preview
> >> is much more likely to show you a useful candidate. So you can think of
> >> this option as an adjustable threshold for how much information we
> >> require the completion backend to have before we consider its
> >> suggestions any good. I'm open to changing the default value, but I
> >> think that three characters is a very sane default.
> >
> > The advantage of 1 character is that we don't need this defcustom at
> > all, and it is basically up to the user when to type TAB, or even look
> > at the preview.
>
> One character is not the same as removing this `defcustom`. Without
> this `defcustom`, i.e. without checking the length of the symbol at
> point, we would try to show the preview even after the user types a
> bunch of space and there is nothing useful to complete at point at all.
>
> > Alternatively, we could have a defcustom based on a different design:
> > show preview only when there are fewer than N completion candidates,
> > with N being customizable. That would make much more sense, IMO,
> > since it replaces a largely "mechanical" limitation with one that is
> > meaningful for users.
>
> That would indeed be a nice solution, but it has a fatal flaw, sadly.
> Computing the set of completion candidates is a costly operation,
> especially with backends such as LSP, so we don't want to do that after
> each command.
This seems to be an argument in favor of the timer-based design I
suggested. But if you aren't inclined to do that, I guess we can
install the feature as it is designed now.
> >> > The underline face is not universally supported, so this defface
> >> > should have fallbacks.
> >>
> >> The `underline` face in faces.el has `:underline t` in the fallback
> >> clause too, so I figured that should be alright, no?
> >
> > If you are okay with seeing no effect at all when the terminal doesn't
> > support the underline attribute, then yes. But I thought we want this
> > face to stand out no matter what, don't we?
>
> That's okay IMO, the underline just differentiates between when you have
> a single candidate and when you have multiple candidates. I don't think
> that's that crucial, but if you can suggest a universally supported
> fallback I'd be glad to add it, of course.
How about some color (foreground or background)?
Reply sent
to
Eli Zaretskii <eliz <at> gnu.org>:
You have taken responsibility.
(Wed, 15 Nov 2023 17:19:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
Eshel Yaron <me <at> eshelyaron.com>:
bug acknowledged by developer.
(Wed, 15 Nov 2023 17:19:02 GMT)
Full text and
rfc822 format available.
Message #94 received at 66948-done <at> debbugs.gnu.org (full text, mbox):
> From: Eshel Yaron <me <at> eshelyaron.com>
> Cc: philipk <at> posteo.net, juri <at> linkov.net, dmitry <at> gutov.dev,
> stefankangas <at> gmail.com, 66948 <at> debbugs.gnu.org, joaotavora <at> gmail.com
> Date: Wed, 15 Nov 2023 15:22:25 +0100
>
> > How about some color (foreground or background)?
>
> Sure, I've added a fallback that uses `:weight bold` when that's
> supported but `:underline` isn't, and another ultimate fallback that
> uses a background color instead. Here's the updated patch (v6):
Thanks, installed on master, and closing the bug.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66948; Package
emacs.
(Wed, 15 Nov 2023 19:03:01 GMT)
Full text and
rfc822 format available.
Message #97 received at 66948-done <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Eshel Yaron <me <at> eshelyaron.com>
>> Cc: philipk <at> posteo.net, juri <at> linkov.net, dmitry <at> gutov.dev,
>> stefankangas <at> gmail.com, 66948 <at> debbugs.gnu.org, joaotavora <at> gmail.com
>> Date: Wed, 15 Nov 2023 15:22:25 +0100
>>
>> > How about some color (foreground or background)?
>>
>> Sure, I've added a fallback that uses `:weight bold` when that's
>> supported but `:underline` isn't, and another ultimate fallback that
>> uses a background color instead. Here's the updated patch (v6):
>
> Thanks, installed on master, and closing the bug.
Great, thanks Eli, and thank you all for commenting and testing.
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Thu, 14 Dec 2023 12:24:12 GMT)
Full text and
rfc822 format available.
This bug report was last modified 1 year and 148 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.