bug#69133: [PATCH] Make key selection method configurable in EPA.

[Aleksandr Vityazev]

Thanks, added the fixes, attached the version 2 patch

On 2024-02-15 08:07, Eli Zaretskii wrote:

>> Date: Wed, 14 Feb 2024 22:47:08 +0300
>> From:  Aleksandr Vityazev via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> Currently in epa.el it is possible to select keys only through a
>> separate buffer, I think adding the option to select via minibuffer
>> would be a nice addition. The current behavior is set to default. Any
>> comments?
>
> Thanks, some comments below,
>
>> --- a/etc/NEWS
>> +++ b/etc/NEWS
>> @@ -1352,6 +1352,14 @@ characters, such as ½ (U+00BD VULGAR FRACTION ONE 
>> HALF), are also
>>  recognized as rational fractions.  They have been since 2004, but it
>>  looks like it was never mentioned in the NEWS, or even the manual.
>> 
>> +** EasyPG
>> +
>> +---
>> +*** New user option 'epa-keys-select-method'
>
> Heading lines in NEWS should end with a period.
>

Fixed

>> +This allows the user to customize the key selection method, a minibuffer
>> +or buffer option is available, which is set by default and was used in
>> +all earlier versions.
>
> This sentence is too complex.  Suggest to reword as two sentences:
>
>   This allows the user to customize the key selection method, which
>   can be either by using a pop-up buffer or from the minibuffer.  The
>   pop-up buffer method is the default, which preserves previous
>   behavior.
>

Applied

> Also, why did you decide not to document this in the manual?  epa.el
> has its own manual, so how about adding this option to it?

Added

>
>> +(defcustom epa-keys-select-method 'buffer
>> +  "Method used to select keys.
>> +It can have two values: buffer or minibuffer.
>> +Can have two values: buffer or minibuffer.  In the first case, the keys
>> +can be selected via a pop-up buffer. In the second case, the keys
>> +can be selected via a minibuffer and `completing-read-multiple' will be
>> +used."
>
> The doc string could also be simplified.  Like this, for example:
>
>     Method used to select keys in `epa-select-keys'.
>   If the value is \\='buffer, the default, keys are selected via a
>   pop-up buffer.  If the value is \\='minibuffer, keys are selected
>   via the minibuffer instead, using `completing-read-multiple'.
>
Applied

> This avoids explaining the values twice (first what they are, then
> what they do), and also mentions the default value.
>
> Please also make sure comments, doc strings, and commit log messages
> leave two spaces between sentences, per our conventions.  You had a
> couple of problems with that in the patch, which my suggested
> rephrasing fixed, but please keep this in mind in the future.
>
>> +  :type '(choice (const :tag "Read keys from buffer" buffer)
>                                  ^^^^^^^^^^^^^^^^^^^^^
> This should probably say "...from a pop-up buffer".

Fixed
>
>>  (defun epa-select-keys (context prompt &optional names secret)
>>    "Display a user's keyring and ask him to select keys.
>> @@ -459,7 +484,10 @@ epa-select-keys
>>  the keys are listed.
>>  If SECRET is non-nil, list secret keys instead of public keys."
>>    (let ((keys (epg-list-keys context names secret)))
>> -    (epa--select-keys prompt keys)))
>> +    (pcase epa-keys-select-method
>> +      ('buffer (epa--select-keys prompt keys))
>> +      ('minibuffer (epa--select-keys-in-minibuffer prompt keys))
>> +      (_ (error "Wrong method for key selection is specified")))))
>


> Hmm... are you assuming users know how to input multiple strings from
> the minibuffer, in particular what is the value of crm-separator?  the
> function epa--select-keys inserts some instructions into the pop-up
> buffer, but this new function you propose just shows the prompt and
> nothing else.  Should it somehow help the user with the job of typing
> the keys at the prompt?

The crm-separator is "[ \t]*,[ \t]*", so I added a "(comma separated)"
hint that will be in all prompts.

>
> Also, maybe instead of signaling an error for a value that's neither
> 'buffer' nor 'minibuffer', we should treat such values as 'buffer'?

Agree, fixed

0001-Make-key-selection-method-configurable-in-EPA.patch
Description: patch-v2

-- 
Best regards,
Aleksandr Vityazev