* [PATCH] org-agenda.el: Complete multiple todo keywords
@ 2020-04-29 16:27 akater
2020-04-30 4:04 ` Kyle Meyer
0 siblings, 1 reply; 6+ messages in thread
From: akater @ 2020-04-29 16:27 UTC (permalink / raw)
To: emacs-orgmode
[-- Attachment #1: Type: text/plain, Size: 821 bytes --]
* lisp/org-agenda.el (org-todo-list): Use completing-read-multiple instead of completing-read when selecting todo keywords to filter by in Agenda.
* lisp/org-agenda.el (org-todo-list): Fix a typo in the prompt.
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. This inconsistency is not present in vanilla Emacs completion. The issue had been discussed with experienced Helm developer who insisted that current Helm behaviour should not change.
We opted to use custom separator that is more natural in context. 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.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Complete multiple todo keywords in org-agenda --]
[-- Type: text/x-diff, Size: 1660 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.
* lisp/org-agenda.el (org-todo-list): Fix a typo in the prompt.
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. This inconsistency is not present in vanilla Emacs completion. The issue had been discussed with experienced Helm developer who insisted that current Helm behaviour should not change.
We opted to use custom separator that is more natural in context. 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.
--- 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)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] org-agenda.el: Complete multiple todo keywords 2020-04-29 16:27 [PATCH] org-agenda.el: Complete multiple todo keywords akater @ 2020-04-30 4:04 ` Kyle Meyer 2020-04-30 6:50 ` akater 0 siblings, 1 reply; 6+ messages in thread From: Kyle Meyer @ 2020-04-30 4:04 UTC (permalink / raw) To: akater, emacs-orgmode 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. > 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. > The issue had been discussed with experienced Helm developer who > insisted that current Helm behaviour should not change. I think this bit can be pruned from the commit message. > We opted to use custom separator that is more natural in context. 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? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] org-agenda.el: Complete multiple todo keywords 2020-04-30 4:04 ` Kyle Meyer @ 2020-04-30 6:50 ` akater 2020-04-30 22:19 ` Kyle Meyer 0 siblings, 1 reply; 6+ messages in thread From: akater @ 2020-04-30 6:50 UTC (permalink / raw) To: Kyle Meyer, emacs-orgmode [-- 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) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] org-agenda.el: Complete multiple todo keywords 2020-04-30 6:50 ` akater @ 2020-04-30 22:19 ` Kyle Meyer 2020-05-01 20:58 ` akater 0 siblings, 1 reply; 6+ messages in thread From: Kyle Meyer @ 2020-04-30 22:19 UTC (permalink / raw) To: akater, emacs-orgmode akater <nuclearspace@gmail.com> writes: > Done. New patch is attached. Thanks for the update and the expanded explanations. The patch looks good to me. As a minor note for future patches, it's easier on the receiver if you include the non-patch commentary in one of the ways that git-am can automatically detect and trim out (see the discussion section in git-format-patch). This list is also okay with just attaching the output of git-format-patch directly. Assuming you haven't completed copy assignment paperwork with the FSF (see <https://orgmode.org/worg/org-contribute.html>), I'll add a TINYCHANGE cookie to the commit on apply. Is akater the name you prefer listed on that site and in the commit message? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] org-agenda.el: Complete multiple todo keywords 2020-04-30 22:19 ` Kyle Meyer @ 2020-05-01 20:58 ` akater 2020-05-02 3:37 ` Kyle Meyer 0 siblings, 1 reply; 6+ messages in thread From: akater @ 2020-05-01 20:58 UTC (permalink / raw) To: Kyle Meyer, emacs-orgmode [-- Attachment #1: Type: text/plain, Size: 212 bytes --] Kyle Meyer <kyle@kyleam.com> writes: > Is akater the name you prefer > listed on that site and in the commit message? Yes. I will use git-am complaint patch formatting next time; thank you for the guidelines. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 800 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] org-agenda.el: Complete multiple todo keywords 2020-05-01 20:58 ` akater @ 2020-05-02 3:37 ` Kyle Meyer 0 siblings, 0 replies; 6+ messages in thread From: Kyle Meyer @ 2020-05-02 3:37 UTC (permalink / raw) To: akater, emacs-orgmode Applied (d23bd9187). ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-02 3:37 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-29 16:27 [PATCH] org-agenda.el: Complete multiple todo keywords akater 2020-04-30 4:04 ` Kyle Meyer 2020-04-30 6:50 ` akater 2020-04-30 22:19 ` Kyle Meyer 2020-05-01 20:58 ` akater 2020-05-02 3:37 ` Kyle Meyer
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).