-
-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add vertico-crm-mode in separate package #74
Conversation
@bdarcus A lot of back and forth here - sorry about that. But now I have at least explored this from all sides. Please give it a test. |
No problem; one of the consequences of these decoupled packages! I think you're fixing a few bugs as I'm testing, but here's what I see so far:
|
|
@bdarcus There is one thing I am not 100% happy with - the behavior of RET vs TAB. Maybe RET should only submit the current candidate if no candidates are selected. If candidates are selected only these candidates should be submitted and the current candidate should simply be ignored. This brings us back to the |
I'm not totally following you on this. Maybe you can rephrase? The double-RET issue was me just thinking selecting a single candidate may be the common case, so if someone is doing that a lot, they will get annoyed by it, and maybe an abused pinky finger. |
I modified the RET behavior. If you select candidates with TAB and press RET, the current candidate position does not matter. RET only returns the current candidate when no candidates have been selected with TAB. In order to select three candidates you press TAB-TAB-TAB-RET. Before you had to press TAB-TAB-RET, which is one keypress less but also feels more confusing. If you select only a single candidate you can press RET only. Does this make sense? |
Yes, and it works well in my view. So 👍. |
241cb32
to
28462df
Compare
This is looking really good @minad. Are you convinced yet? BTW, doing |
Actually there are complications. In order to implement this correctly, completion boundaries must be taken into account. E.g., for file completion the currently completed candidate string (the file name) is only part of the full candidate string (the path). More detailed example: When completing a file path "~/.emacs.d/ini", the candidates are the files living in "~/.emacs.d/", e.g., "init.el". Selecting this file will add the path "~/.emacs.d/init.el" to the list of selected candidates. Then we would see the following UI, where we are completing file names and file paths at the same time. This is obviously incorrect.
Therefore the currently implemented UI is insufficient in the general scenario. Of course this does not matter for your use case, where your candidates are always full candidates. To implement the general case correctly, one probably ends up with an interface similar to (4) from our previous discussion. This means that all the logic must be baked into the UI directly (the vertico/vertico-crm split I tried here won't work out). At this point every completion (even
There are the two options:
However I just figured out that the default CRM implementation is also not fully compliant. Selecting multiple files separated by comma does not work and throws an error! This means that CRM by definition only allows tables without boundaries. (completing-read-multiple "Multiple files: " #'read-file-name-internal) So we are probably fine with the |
Interesting. It sounds like, regardless of the ultimate path you choose, it will depend on enhancements to Embark already discussed a bit with @oantolin? emacs-citar/citar#163 (comment) E.g. even the "simple" path requires some Embark enhancements. Would your more full-featured option require different, or more extensive, enhancements? |
No, crm does not require embark enhancements. I meant that if one wants to implement the multi selection feature correctly, one almost automatically ends up with a full featured UI, where multi selections are fundamentally baked in. Multi selections are valuable for two things: 1. crm, 2. embark actions on multiple candidates. Implementing this only for crm does not seem worth it, for Embark more so. The question is where to stop - as soon as one implements the correct thing (and not only a few special cases), one ends up with the full featured UI. Since all of this is such a significant deviation from the status quo which doesn't handle multi selections at all, it may be better to stop right away. Alternatively implement only these special cases (simple-crm, vertico-crm, consult-completing-read-multiple), which ultimately break down. I wonder - in bibtex-actions is |
I thought you said it did? To be clear, if I use CRM, and select three candidates, and then do If I do the same with So something's missing somewhere. I was just assuming from previous discussions it was in Embark.
Yes.
Or do the latter in the short-term, and see if requirements and demand for the former emerges in time, along with implementation ideas. But that has other problems, I know.
All of the functions derived from I think multi-selection make sense in a lot of cases here; creating a bibtex file out of filtered list, opening a list of notes or documents, inserting multi-cite citations (per below), etc. If you don't have multi-selection, you have to do them each in turn, of course. But not all of that requires CRM per se conceptually, in the sense a function that needs to operate on a list, rather than works on a list of items in sequence. Using An obvious exception is inserting a citation, where the result might be:
With CR only, one would instead insert three separate keys, invoking that command three separate times. If you're inserting dozens of citation and their keys in a document, that can be hassle. Or, per above, use There is also the capf option. So I guess the long-winded answer to your question of whether CRM is really needed is ... maybe. |
Regarding Embark - yes of course it needs support to support actions on multiple candidates. I meant that for CRM only (without actions) nothing is needed from the side of Embark. And even actions on single candidates work directly with Embark without special support. |
I have another idea regarding how CRM could be handled in general - going a route via Embark. I would like a solution which somehow fits into the whole picture. The idea is to add some ability to select candidates one by one, adding them to an
(defun embark-select ()
(interactive)
;; Here the candidates should actually go to an embark-collect-mode buffer
;; Unfortunately embark-collect-mode buffers only support types of a single candidate (generalize this?)
(let ((cand (cadr (embark--target)))
(window-min-height 1)
(window-resize-pixelwise t))
(with-current-buffer (get-buffer-create "*Embark Select*")
(goto-char (point-max))
(unless (= (point) (point-min))
(insert "\n"))
(insert cand))
(fit-window-to-buffer
(display-buffer "*Embark Select*"
`(display-buffer-in-side-window
(window-parameters (mode-line-format . none))
(window-height . 1)
(side . bottom)
(slot . -1))))))
(defun embark-select-setup ()
(lambda ()
(with-current-buffer (get-buffer-create "*Embark Select*")
(erase-buffer))))
(defun embark-completing-read-multiple (&rest args)
(let ((item (apply #'completing-read args)))
(or
;; If items have been selected with Embark, return them
(mapcar #'substring-no-properties
(split-string
(with-current-buffer (get-buffer-create "*Embark Select*")
(buffer-string))
"\n" 'omit-nulls))
;; Otherwise return the single item
(list item))))
(add-hook #'minibuffer-setup-hook #'embark-select-setup)
(define-key vertico-map [backtab] #'embark-select) |
As in, not specific to Vertico? Or are you thinking In any case, it seems an idea worth considering. The test would just be if the UI could be as elegant as this. But I guess what you're thinking is this could be a way to act on multiple candidates in Embark, even without CRM? |
Since we don't have to concept of multi selections anywhere in our package set and it matters most for Embark actions, yes, I think it is worth to consider implementing the multi action/selection support in Embark. What I mean is to put Embark first and then look how the design affects the rest. Implementing a general CRM in Vertico is a significant effort and complication as I described above and I don't feel comfortable adding a half baked solution here. I would be more comfortable with minad/consult#352 in a simplified form, which explicitly supports only a subset of scenarios (static completion tables, no completion boundaries). The function
So I guess I would be happy with either 1. or 4. here. Of course 4. would be more satisfactory if one finds a good design. 1. is just a poor man solution, a stop gap measure. Do you see my point? Does this make sense? |
Yes. So we need to see what @oantolin thinks then in any case. |
Yes, it would be great to hear @oantolin's opinion about acting/collecting multiple candidates, in particular the idea in #74 (comment). Independently we could still add minad/consult#352 to Consult. It would not hurt much and it may improve the status quo. But maybe you are spoiled now by the vertico-crm prototype in this PR? ;) |
Probably yes to both; it is an improvement over CRM, but this is of course better. EDIT: I forked your repo, so will maybe still personally use your branch code, even if you don't publish this package. In that scenario, I would still strongly recommend the Consult version on my README. |
BTW, I think the Embark angle is related to this question of mine and subsequent discussion from awhile back. See in particular: I think I was asking that question when pondering whether to use CR or CRM. I decided CRM made more sense, even if the UIs currently weren't ideal for it. It was, in effect, a bet on the future. |
This is very interesting! As you have probably guessed, I'm kind of swamped at work these days and haven't had that much time to work on Embark recently, but let me at least quickly write some disconnected thought about the Embark multiple-selection idea.
|
Thanks for weighing in @oantolin!
I was thinking the latter, if it's possible. The first allows support for the vastly wider array of CR commands, while the second those, like mine, which rely on CRM. I looked again at my list of commands. I think the only two that really
... since in both cases, you're basically running The others can just as well run the same action on the different candidates in sequence. |
I closed this in favor of the consult crm implementation, which is pretty much equivalent to the vertico-crm version, only UI agnostic. The consult version does not yet offer better keybindings (TAB/RET) as we discussed, but this is easy to add in the consult-vertico/selectrum integration code. Independent of that, I would love to see Embark support for actions on multiple candidates at once (calling an action in a loop for each candidate). As described before this should ideally even allow selecting multiple candidates in plain I think Embark multi actions/selections are orthogonal to the CRM discussion, therefore I went forward with |
I was just looking at this new (in the past day or so) code for https://code.orgmode.org/bzg/org-mode/src/wip-cite-new/lisp/oc-basic.el#L703 In his first draft, he used CRM. Now he's using CR. In that UI, you add the keys to the prompt, and then exit to complete the list and insert the citation. Just curious: WDYT of that alternative approach? |
This is much simpler but the downside is that you cannot remove keys anymore it seems. And you also need the double-RET. It seems to be pretty common in Org that they are reinventing the wheel somehow all over the place. It would be better if they just use |
To be fair, he started with CRM and completing the keys. When I and others pointed he needed to complete on the data and return the key (which necessarily means longer candidates), he went to this solution, almost surely because he ran into the same UI issues with CRM as I did. And yes, you can't edit the keys. |
What is the relation between the new org functionality and your bibtex-actions package btw? |
I'll just be able to plug in some of these functions into the standardized org infrastructure. So The new functionality is nicely modular. So out-of-box the user will have some baseline functionality, but if they do this, they get bibtex-actions instead. (setq org-cite-insert-processor 'bibtex-actions)
(setq org-cite-follow-processor 'bibtex-actions) That infrastructure also provides better contextual functionality. The insert functionality will behave differently depending on what part of the citation point is on, for example, without me having to write that. I started this project in part because I wanted to target this. |
Now I am experimenting with an enhanced vertico crm UI. This is a much more user friendly variant of
consult-completing-read-multiple
. Just turn onvertico-crm-mode
and be done.I think it is also more maintainable than
consult-completing-read-multiple
, which will be a hot mess if the implementation is scattered over Vertico, Embark and Consult. Downside is that we lose the nice portability aspect ofconsult-completing-read-multiple
. These are the advantages:By keeping the implementation strictly separate from vertico.el, I am not complicating Vertico. This was something I had been afraid of before. In my initial experiments I implemented crm in a much more intrusive fashion directly in vertico.el, which wasn't such a good idea and led me to explore the totally separate
consult-completing-read-multiple
implementation.