Eshel Yaron writes: > One important point is that I'm a bit hesitant about adding the sexp > variant, rather then defining only completion-preview-insert-word, and > mentioning in the documentation that other variants are trivial to > define (and how). The reason is that I don't have a good idea of when > a completion candidate would span multiple sexps (if you have such an > example, please share it), so I'm not sure how much utility this > command would bring in practice. The use case that I have for the sexp variant is when completing eshell history. Both because: parts of shell commands such as file names can be considered sexp's, but also because eshell itself can interpret "full" elisp forms. On a similar note. Some similar tools additionally define a `forward-char` variant. This is not something that I've found a need for personally but would be willing to add for completeness. >> From 7fd70fb330e0623636729657b17a9cdac3841a3d Mon Sep 17 00:00:00 2001 >> From: Jules Tamagnan >> Date: Sat, 22 Jun 2024 00:45:01 -0700 >> Subject: [PATCH] Add new completion-preview-insert-{word,sexp} commands >> >> * lisp/completion-preview.el: Add new completion-preview-insert-word and >> completion-preview-insert-sexp commands. >> * test/lisp/completion-preview-tests.el: Add tests for new commands. > > It's best to single-quote symbols in the commit message, like 'this'. Thank you. Done >> +(defun completion-preview--insert-partial (command) > > This should be a public function (no --), to indicate that it's fine for > users to use it in their own command definitions. So either > completion-preview-insert-partial or completion-preview-partial-insert. > (I tend to prefer the latter, but both work.) Thank you. Done > Also, COMMAND should instead be FUN or FUNC, since the argument doesn't > have to be command, any motion function would do. Thank you. Done > Lastly this command should also take &rest args and pass them to the > function argument, to facilitate writing something like > (c-p-partial-insert #'forward-word 2) to complete two words. Thank you. Done Another idea would be to turn `c-p-partial-insert` into a macro that uses the `interactive-form` function to generate a sensible insert-partial function. I'm more than happy to take this tweak on as well. > The first line of the docstring should be a full sentence. We also want > to describe accurately enough what this function does for users to be > able to leverage it in their definitions. I suggest: > > (defun completion-preview-partial-insert (fun &rest args) > "Insert part of the current completion preview candidate. > This function calls FUN with arguments ARGS, after temporarily inserting > the entire current completion preview candidate. FUN should move point: > if it moves point forward into the completion text, this function > inserts the prefix of the completion candidate up to that point. > Beyond moving point, FUN should not modify the current buffer." Thank you. Done > Better strip text properties from AFT before inserting it here. Thank you. Done There were two ways of implementing this that I could think of. 1. Insert with properties, set `suf` to delete-and-extract-region to preserve the properties, use `(set-text-properties end (point) nil)` to remove the text properties from the deletion. 2. Insert without text properties, use `delete-region`, set `suf` to a substring of `aft` directly Both seem to work equally well, I've gone with option 2 because it seems more consistent with what you had suggested. > We should ensure that new-end isn't smaller then end, otherwise the > deletion below won't do the right thing. Thank you. Done > This is a nice use of delete-and-extract-region, but the insertion and > deletion must be inside an atomic-change-group, so we don't leave AFT > inserted in case the motion function signals an error. This is also > where we need to add an undo-amalgamate-change-group, to prevent undo > from seeing an unwanted intermediate step in case an undo-boundary is > created between the insertion and the deletion. So the structure should > be something like: > > (atomic-change-group > (let ((change-group (prepare-change-group))) > ;; Insert, > ;; Move, > ;; Delete. > (undo-amalgamate-change-group change-group))) Thank you. Done I'm glad to better understand how this works now. >> +(defun completion-preview-insert-word () >> + "Insert the next word of the completion candidate that the preview is showing." >> + (interactive) >> + (completion-preview--insert-partial #'forward-word)) > > This should handle an optional numeric argument, like forward-word does. Thank you. Done > Finally, we should document completion-preview-insert-word in the > Commentary section. Here's the text I'd like to add, you can include it > the patch (and change it as you see fit) or I'll add it later: > > ;; You can also insert only the first word of the completion candidate > ;; with the command `completion-preview-insert-word'. With a numeric > ;; prefix argument, it inserts that many words instead of just the one. > ;; This command is not bound by default, but you may want to bind it to > ;; M-f (or remap `forward-word') in `completion-preview-active-mode-map' > ;; since it's very much like a `forward-word' that also moves "into" the > ;; completion preview. Thank you. Done At the end of the paragraph I've additionally added a brief note about the `sexp` variant. > Best, > > Eshel Best, Jules