GNU bug report logs - #67275
[PATCH] ; Improve and add tests for Completion Preview mode

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 67275 in the body.
You can then email your comments to 67275 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] ; Improve and add tests for Completion Preview mode
Date: Sun, 19 Nov 2023 11:25:55 +0100

[Message part 1 (text/plain, inline)]

Tags: patch

This patch makes Completion Preview mode more robust in face of
misbehaving `completion-at-point-functions`, and adds some tests.


Thanks,

Eshel

[0001-Improve-and-add-tests-for-Completion-Preview-mode.patch (text/patch, attachment)]

Message #8 received at 67275 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 67275 <at> debbugs.gnu.org
Subject: Re: bug#67275: [PATCH] ;
 Improve and add tests for Completion Preview mode
Date: Sun, 19 Nov 2023 12:58:01 +0200

> Date: Sun, 19 Nov 2023 11:25:55 +0100
> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> +(defun completion-preview--try-table (table beg end props)
> +  "Check TABLE for a completion matching the text between BEG and END.
> +
> +PROPS is a property list with additional information about TABLE.
> +See `completion-at-point-functions' for more details.
> +
> +When TABLE contains a matching completion, return a list
> +\(PREVIEW BEG END ALL EXIT-FN) where PREVIEW is the text to show
> +in the completion preview, ALL is the list of all matching
> +completion candidates, and EXIT-FN is either a function to call
> +after inserting PREVIEW or nil.  When TABLE does not contain
> +matching completions, or when there are multiple matching
> +completions and `completion-preview-exact-match-only' is non-nil,
> +return nil instead."

It is better to use "if" here where you use "when".  "When" can be
interpreted as a time-related condition, which is not what you want
here.

> +(defun completion-preview--capf-wrapper (capf)
> +  "Translate output of CAPF to properties for completion preview overlay.
> +
> +If CAPF returns a list (BEG END TABLE . PROPS), call

If CAPF _returns_ something, it is probably a function.  But then why
does the first sentence say "output of CAPF"? functions in ELisp don't
"output" stuff.

> +`completion-preview--try-table' to check TABLE for matching
> +completion candidates.  If `completion-preview--try-table'
> +returns a non-nil value, return that value.  Otherwise, return a
> +list with nil car which means that completion failed, unless
> +PROPS includes the property `:exclusive' with value `no', in
> +which case this function returns nil which means to try other
> +functions from `completion-at-point-functions'."

This basically tells in words what the code does.  But since the code
is quite simple, I wonder why we need this in the doc string.  The
disadvantage of having this in the doc string is that we'd need to
update it each time the code changes.

Instead, think if something in what the code does needs to be
explained _beyond_ what the code itself says, like if you need to
explain the reasons why the code does what it does, or why you access
this or that property, and explain that -- in comments, not in the doc
string.  The doc string should ideally be a higher-level description
of the function's purpose and the meaning of its return values.

Thanks.

Message #11 received at 67275 <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 67275 <at> debbugs.gnu.org
Subject: Re: bug#67275: [PATCH] ; Improve and add tests for Completion
 Preview mode
Date: Sun, 19 Nov 2023 12:23:16 +0100

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> +When TABLE contains a matching completion, return a list
>> +\(PREVIEW BEG END ALL EXIT-FN) where PREVIEW is the text to show
>> +in the completion preview, ALL is the list of all matching
>> +completion candidates, and EXIT-FN is either a function to call
>> +after inserting PREVIEW or nil.  When TABLE does not contain
>> +matching completions, or when there are multiple matching
>> +completions and `completion-preview-exact-match-only' is non-nil,
>> +return nil instead."
>
> It is better to use "if" here where you use "when".  "When" can be
> interpreted as a time-related condition, which is not what you want
> here.

Right, done in the updated patch (v2) below.

>> +(defun completion-preview--capf-wrapper (capf)
>> +  "Translate output of CAPF to properties for completion preview overlay.
>> +
>> +If CAPF returns a list (BEG END TABLE . PROPS), call
>
> If CAPF _returns_ something, it is probably a function.  But then why
> does the first sentence say "output of CAPF"? functions in ELisp don't
> "output" stuff.

Thanks, I've replaced "output" with "return value".

>> +`completion-preview--try-table' to check TABLE for matching
>> +completion candidates.  If `completion-preview--try-table'
>> +returns a non-nil value, return that value.  Otherwise, return a
>> +list with nil car which means that completion failed, unless
>> +PROPS includes the property `:exclusive' with value `no', in
>> +which case this function returns nil which means to try other
>> +functions from `completion-at-point-functions'."
>
> This basically tells in words what the code does.  But since the code
> is quite simple, I wonder why we need this in the doc string.  The
> disadvantage of having this in the doc string is that we'd need to
> update it each time the code changes.
>
> Instead, think if something in what the code does needs to be
> explained _beyond_ what the code itself says, like if you need to
> explain the reasons why the code does what it does, or why you access
> this or that property, and explain that -- in comments, not in the doc
> string.  The doc string should ideally be a higher-level description
> of the function's purpose and the meaning of its return values.

Makes sense, thanks.  I removed the lengthy description and added a
comment explaining the only non-obvious part.


Here's the updated patch:

[v2-0001-Improve-and-add-tests-for-Completion-Preview-mode.patch (text/x-patch, attachment)]

Message #14 received at 67275 <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: Eshel Yaron via "Bug reports for GNU Emacs, the Swiss army knife of text
 editors" <bug-gnu-emacs <at> gnu.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 67275 <at> debbugs.gnu.org
Subject: Re: bug#67275: [PATCH] ; Improve and add tests for Completion
 Preview mode
Date: Mon, 20 Nov 2023 13:26:45 +0100

[Message part 1 (text/plain, inline)]

Eshel Yaron writes:

> ...Here's the updated patch...

I'm attaching below another patch, which is meant to be applied on top
of the patch from my previous message.

This patch simplifies Completion Preview mode a bit and, crucially,
fixes an issue where certain `completion-at-point-functions`
configurations could prevent `completion-preview-insert` from actually
inserting the completion suggestion.

I could unify this patch with the previous one and submit them as a
single patch if that'd be preferable, although in terms of Git history
it might be nicer to commit them separately IMO.


Thanks,

Eshel

[0001-Avoid-completion-at-point-in-completion-preview-inse.patch (text/x-patch, attachment)]

Message #20 received at 67275 <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: 67275 <at> debbugs.gnu.org
Cc: Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#67275: [PATCH] ; Improve and add tests for Completion
 Preview mode
Date: Fri, 24 Nov 2023 08:32:39 +0100

>> ...Here's the updated patch...
>
> I'm attaching below another patch, which is meant to be applied on top
> of the patch from my previous message.

No further comments, it seems.  Can these two patches be applied?


Thanks,

Eshel

Message #23 received at 67275 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 67275 <at> debbugs.gnu.org
Subject: Re: bug#67275: [PATCH] ; Improve and add tests for Completion
 Preview mode
Date: Fri, 24 Nov 2023 09:56:01 +0200

> From: Eshel Yaron <me <at> eshelyaron.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>
> Date: Fri, 24 Nov 2023 08:32:39 +0100
> 
> >> ...Here's the updated patch...
> >
> > I'm attaching below another patch, which is meant to be applied on top
> > of the patch from my previous message.
> 
> No further comments, it seems.  Can these two patches be applied?

What, I only have 3 days to comment on patches? ;-)

Please be a bit more patient, especially during weekdays.  This patch
is still in my queue.

Message #26 received at 67275 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 67275 <at> debbugs.gnu.org
Subject: Re: bug#67275: [PATCH] ; Improve and add tests for Completion
 Preview mode
Date: Sat, 25 Nov 2023 12:11:03 +0200

> From: Eshel Yaron <me <at> eshelyaron.com>
> Cc: 67275 <at> debbugs.gnu.org
> Date: Sun, 19 Nov 2023 12:23:16 +0100
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> +When TABLE contains a matching completion, return a list
> >> +\(PREVIEW BEG END ALL EXIT-FN) where PREVIEW is the text to show
> >> +in the completion preview, ALL is the list of all matching
> >> +completion candidates, and EXIT-FN is either a function to call
> >> +after inserting PREVIEW or nil.  When TABLE does not contain
> >> +matching completions, or when there are multiple matching
> >> +completions and `completion-preview-exact-match-only' is non-nil,
> >> +return nil instead."
> >
> > It is better to use "if" here where you use "when".  "When" can be
> > interpreted as a time-related condition, which is not what you want
> > here.
> 
> Right, done in the updated patch (v2) below.
> 
> >> +(defun completion-preview--capf-wrapper (capf)
> >> +  "Translate output of CAPF to properties for completion preview overlay.
> >> +
> >> +If CAPF returns a list (BEG END TABLE . PROPS), call
> >
> > If CAPF _returns_ something, it is probably a function.  But then why
> > does the first sentence say "output of CAPF"? functions in ELisp don't
> > "output" stuff.
> 
> Thanks, I've replaced "output" with "return value".
> 
> >> +`completion-preview--try-table' to check TABLE for matching
> >> +completion candidates.  If `completion-preview--try-table'
> >> +returns a non-nil value, return that value.  Otherwise, return a
> >> +list with nil car which means that completion failed, unless
> >> +PROPS includes the property `:exclusive' with value `no', in
> >> +which case this function returns nil which means to try other
> >> +functions from `completion-at-point-functions'."
> >
> > This basically tells in words what the code does.  But since the code
> > is quite simple, I wonder why we need this in the doc string.  The
> > disadvantage of having this in the doc string is that we'd need to
> > update it each time the code changes.
> >
> > Instead, think if something in what the code does needs to be
> > explained _beyond_ what the code itself says, like if you need to
> > explain the reasons why the code does what it does, or why you access
> > this or that property, and explain that -- in comments, not in the doc
> > string.  The doc string should ideally be a higher-level description
> > of the function's purpose and the meaning of its return values.
> 
> Makes sense, thanks.  I removed the lengthy description and added a
> comment explaining the only non-obvious part.
> 
> 
> Here's the updated patch:

Thanks, now installed on master.

Message #29 received at 67275 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 67275 <at> debbugs.gnu.org
Subject: Re: bug#67275: [PATCH] ; Improve and add tests for Completion
 Preview mode
Date: Sat, 25 Nov 2023 12:12:49 +0200

> From: Eshel Yaron <me <at> eshelyaron.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>,  67275 <at> debbugs.gnu.org
> Date: Mon, 20 Nov 2023 13:26:45 +0100
> 
> Eshel Yaron writes:
> 
> > ...Here's the updated patch...
> 
> I'm attaching below another patch, which is meant to be applied on top
> of the patch from my previous message.
> 
> This patch simplifies Completion Preview mode a bit and, crucially,
> fixes an issue where certain `completion-at-point-functions`
> configurations could prevent `completion-preview-insert` from actually
> inserting the completion suggestion.
> 
> I could unify this patch with the previous one and submit them as a
> single patch if that'd be preferable, although in terms of Git history
> it might be nicer to commit them separately IMO.

Thanks, installed on master.

Should we now close this bug?

P.S. Please make a point of mentioning the bug number as part of the
commit log messages, whenever you know the number.  (The initial patch
that creates a bug report usually doesn't know the number, but
subsequent submissions do.)

Message #32 received at 67275 <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 67275 <at> debbugs.gnu.org
Subject: Re: bug#67275: [PATCH] ; Improve and add tests for Completion
 Preview mode
Date: Sat, 25 Nov 2023 12:15:43 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Eshel Yaron <me <at> eshelyaron.com>
>> Cc: Eli Zaretskii <eliz <at> gnu.org>,  67275 <at> debbugs.gnu.org
>> Date: Mon, 20 Nov 2023 13:26:45 +0100
>> 
>> Eshel Yaron writes:
>> 
>> > ...Here's the updated patch...
>> 
>> I'm attaching below another patch, which is meant to be applied on top
>> of the patch from my previous message.
>> 
>> This patch simplifies Completion Preview mode a bit and, crucially,
>> fixes an issue where certain `completion-at-point-functions`
>> configurations could prevent `completion-preview-insert` from actually
>> inserting the completion suggestion.
>> 
>> I could unify this patch with the previous one and submit them as a
>> single patch if that'd be preferable, although in terms of Git history
>> it might be nicer to commit them separately IMO.
>
> Thanks, installed on master.

Great, thanks.

> Should we now close this bug?

Yes, I think so.  I have another patch in the works (making Completion
Preview mode more convenient for mouse users), but it's probably best to
start a separate thread for that.

> P.S. Please make a point of mentioning the bug number as part of the
> commit log messages, whenever you know the number.  (The initial patch
> that creates a bug report usually doesn't know the number, but
> subsequent submissions do.)

Sure, sorry for omitting it here.


Eshel

Message #37 received at 67275-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 67275-done <at> debbugs.gnu.org
Subject: Re: bug#67275: [PATCH] ; Improve and add tests for Completion
 Preview mode
Date: Sat, 25 Nov 2023 13:18:46 +0200

> From: Eshel Yaron <me <at> eshelyaron.com>
> Cc: 67275 <at> debbugs.gnu.org
> Date: Sat, 25 Nov 2023 12:15:43 +0100
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > Should we now close this bug?
> 
> Yes, I think so.

Done, thanks.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.