Re: [PATCH] org-agenda.el: Complete multiple todo keywords

[akater <nuclearspace@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5576 bytes --]

Kyle Meyer <kyle@kyleam.com> writes:

> Thanks for the patch.  Looks like a nice improvement to me.
>
> akater <nuclearspace@gmail.com> writes:
>
>> * lisp/org-agenda.el (org-todo-list): Use completing-read-multiple
>> instead of completing-read when selecting todo keywords to filter by
>> in Agenda.
>
> This and the rest of the lines were unwrapped.  Could you wrap them ~70
> characters?  (The Org repo's .dir-locals.el sets fill-column to 70.)

>> * lisp/org-agenda.el (org-todo-list): Fix a typo in the prompt.
>
> Thanks for spotting that typo.  I think it'd be more common to append
> this description to the entry above rather than adding another
> org-todo-list entry.

Done. New patch is attached.

>> There is minor UX cost to Helm users: while candidates list used to
>> appear immediately to Helm users, now Helm users have to hit TAB to
>> see the list.
>
> Just the opinion of one Helm user, but needing to hit tab for crm-based
> completion has never bothered me too much.  But if it did, Helm allows
> specifying that certain commands should go through the built-in
> completion.
>
> Out of curiosity I tried with the latest ivy (9e0803c), and I also
> needed to hit tab before seeing anything.
>
>> This inconsistency is not present in vanilla Emacs
>> completion.
>
> I'm confused by this.  When I try with no customization (Emacs 26.3), I
> need to hit tab to see any of the candidates.

Yes; my point is, in vanilla Emacs completing-read-multiple and
completing-read behave similarly while in Helm, singleton completion
does not require hitting TAB initially but multiple-candidates
completion does.  I remember initially thinking that I broke completion
altogether when I first introduced crm here.  My confusion didn't last
long but still, I found this a little unpleasant and tried my best to
make it go.

> Looks like you stuck with "|" as the separator, which seems like a good
> idea to me.
>
>> However, it is unfortunate that string patterns are strings themselves
>> and are thus indistinguishable from strings; it would be better if crm
>> exposed separator (the string) on its own in its interface.
>
> I'm not quite sure I follow what you're suggesting with the last bit.
> Could you rephrase the point in a way that is a bit more connected with
> the code change?  This patch sticks with the same separator, so aside
> from being able to complete multiple things, there's no change in
> behavior/added restriction here, right?

Well, this likely shouldn't be in the patch either.  I removed it and
it's fine to forget about it.  The rest of the message elaborates on
this.

What type is crm-separator of? According to crm.el, its default value is
that of the constant crm-default-separator which is regex, i.e. a string
pattern.  That means crm-separator is a string pattern.  It is employed
as a string pattern in (the function) completing-read-multiple to clean
some string from unneeded whitespace.

And yet, using string pattern modelled on the default one in my patch
would break things for Helm users (besides the fact that using it would
simply make the code unnecessary cluttered).  If you substitute said
direct equivalent, activate Helm, eval

(let ((crm-separator "[ \t]*\|[ \t]*"))
  (completing-read-multiple "kwds: "
    (mapcar #'list '("TODO" "DONE")) nil nil))

, press TAB C-SPC C-SPC RET | TAB C-SPC C-SPC RET, you'll get corrupted
result: TODO,DONE|TODO|DONE.  This is due to the fact that to make Helm
support non-standard crm-separator, Helm developer had to resort to a
fairly ugly hack trying to distinguish between a string pattern and an
actual separator, otherwise results like TODO,DONE|TODO|DONE would
appear in minibuffer.

The gist of the problem is, Helm needs a separator to insert into
buffer, and it now tries do detect whether crm-separator is regex or
not, only uses it as insertion material in the latter case and reverts
to the default comma otherwise.  Detection is based on string length:
https://github.com/emacs-helm/helm/commit/52e1d74f9ec6647c039012626f96596f0eb4140a
which is of course very unreliable.

This might be considered a low-quality decision in Helm but I'd say it
is natural for a user

- of multiple-candidates completion interface (such as Helm)

- that has to transform collections of candidates into strings on its
  own (this would be true for any Emacs application, I guess)

to need both the separator to search for and the separator to
intersperse a string with.

Some collection types, like this collection type “a collection of Org
todo keywords”, come bundled with specific separator-the-string.  It is
this string that I'm using in mapconcat here.  This string is a natural
part of the interface of many collections of candidates, due to
string-orientedness of everything that Emacs has to deal with, but this
separator-the-string is now inaccessible to users of crm because crm de
facto only exposes regex separator as part of its interface.

These two separators are different objects of different types: one of
them is a pattern, another is a string, and the former cannot be
reliably used in the role of the latter.  Maybe the former (regex, or
several such) can always be computed from the latter (the string), and
if true, crm.el could provide just separator-the-string for the
interface; relevant regexes may be constructed from it when needed.  But
I did not study all the use cases of crm-separator the regexp so this is
merely a guess.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Complete multiple todo keywords in org-agenda --]
[-- Type: text/x-diff, Size: 1019 bytes --]

org-agenda.el: Complete multiple todo keywords

* lisp/org-agenda.el (org-todo-list): Use completing-read-multiple
  instead of completing-read when selecting todo keywords to filter by
  in Agenda.  Fix a typo in the prompt.


--- a/lisp/org-agenda.el
+++ b/lisp/org-agenda.el
@@ -4790,8 +4790,12 @@
                                                 (nth (1- arg) kwds))))
       (when (equal arg '(4))
         (setq org-select-this-todo-keyword
-              (completing-read "Keyword (or KWD1|K2D2|...): "
-                               (mapcar #'list kwds) nil nil)))
+              (mapconcat #'identity
+                         (let ((crm-separator "|"))
+                           (completing-read-multiple
+                            "Keyword (or KWD1|KWD2|...): "
+                            (mapcar #'list kwds) nil nil))
+                         "|")))
       (and (equal 0 arg) (setq org-select-this-todo-keyword nil))
       (org-compile-prefix-format 'todo)
       (org-set-sorting-strategy 'todo)