Kyle Meyer writes: > Thanks for the patch. Looks like a nice improvement to me. > > akater 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.