From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eshel Yaron via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#71716: [PATCH] Add new completion-preview-insert-{word, sexp} commands Date: Mon, 24 Jun 2024 14:43:56 +0200 Message-ID: References: <87a5jd8hqh.fsf@gmail.com> <877ceg9546.fsf@gmail.com> <874j9k8wpo.fsf@gmail.com> <87r0cn5n2s.fsf@gmail.com> Reply-To: Eshel Yaron Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="21057"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: 71716@debbugs.gnu.org To: Jules Tamagnan Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Mon Jun 24 14:45:30 2024 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1sLj4c-0005Ha-88 for geb-bug-gnu-emacs@m.gmane-mx.org; Mon, 24 Jun 2024 14:45:30 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1sLj4D-0007Sg-Jp; Mon, 24 Jun 2024 08:45:05 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1sLj49-0007S0-Pl for bug-gnu-emacs@gnu.org; Mon, 24 Jun 2024 08:45:01 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1sLj49-0005y7-Gf for bug-gnu-emacs@gnu.org; Mon, 24 Jun 2024 08:45:01 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1sLj4A-0001we-Az for bug-gnu-emacs@gnu.org; Mon, 24 Jun 2024 08:45:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eshel Yaron Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 24 Jun 2024 12:45:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 71716 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 71716-submit@debbugs.gnu.org id=B71716.17192330437343 (code B ref 71716); Mon, 24 Jun 2024 12:45:02 +0000 Original-Received: (at 71716) by debbugs.gnu.org; 24 Jun 2024 12:44:03 +0000 Original-Received: from localhost ([127.0.0.1]:55990 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1sLj3C-0001uL-R9 for submit@debbugs.gnu.org; Mon, 24 Jun 2024 08:44:03 -0400 Original-Received: from mail.eshelyaron.com ([107.175.124.16]:52388 helo=eshelyaron.com) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1sLj3A-0001tn-Oz for 71716@debbugs.gnu.org; Mon, 24 Jun 2024 08:44:01 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=eshelyaron.com; s=mail; t=1719233039; bh=66TYMPNVawPdFKOm0j+hTN7N783Q07s9yI+ucTBYwoU=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=dyJ3bG0dnc4eK09brA/wRkJGgIktgheYHLFMFT0OjfOqzOq3Hnrl+ayLMGUrPUuv5 1TGdKGN3lEH1n9hPbI0VzA9PbBw7SxH7RfvGXg/hq6oorikb5tk4nu0jjye0aOxwlM oUZZb8pxgzViPzDn59HaoD5YsdT8IIqk7AXqrt5OPNEBweNqT5vC8vNycQB8oLkwXQ STrFqPk8zHJXvc7/EoJ+64HiQG97rpqa3aDaWpGg9Kxm9UpF+FS3EkG6EBz1pPoQAX vXZQ1RHU/MtELuxf1gEWz75bcOj8LT+FZqjLqm+9iUXT77CrBweAGBHw7u9HKxGsZ3 PxxZoCO8ObnsA== In-Reply-To: <87r0cn5n2s.fsf@gmail.com> (Jules Tamagnan's message of "Sun, 23 Jun 2024 15:08:43 -0700") X-Hashcash: 1:20:240624:jtamagnan@gmail.com::xf7NIMI3OdzzIDOt:WJu X-Hashcash: 1:20:240624:71716@debbugs.gnu.org::OIc32FxbywapWX7e:7Woo X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:287837 Archived-At: Jules Tamagnan writes: > Hi Eshel, > > I just want to start off once again by saying thank you for the > thoughtful review, help, testing, and encouragement. Gladly, I appreciate your contribution and I'm happy to help with it. > The first patch > `completion-preview-partial-insertion-with-temp-buffer.patch` is the > same as the previous patch but with two critical changes: the revert, > and the addition of a new variable > `completion-preview-context-variables` which can be used to customize > the list of variables to copy into the temporary buffer. > > The second patch > `completion-preview-partial-insertion-with-region-delete.patch` is the > version of the change that uses in-buffer deletion. There's not much to > say here, it seems quite a bit more robust. > > I reckon the second patch is more in line with what you had in mind but > I wanted to bring the first approach to a more acceptable state. Thanks for that, the second patch is looking pretty good. I'm including some comments below, some of these are nits that I can fix myself later. 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. > 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'. > +(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.) Also, COMMAND should instead be FUN or FUNC, since the argument doesn't have to be command, any motion function would do. 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. > + "A helper function to insert part of the completion candidate that the > +preview is showing." 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: --8<---------------cut here---------------start------------->8--- (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." --8<---------------cut here---------------end--------------->8--- > + (if completion-preview-active-mode > + (let* ((beg (completion-preview--get 'completion-preview-beg)) > + (end (completion-preview--get 'completion-preview-end)) > + (efn (plist-get (completion-preview--get 'completion-preview-props) > + :exit-function)) > + (aft (completion-preview--get 'after-string)) > + (new-end) > + (full-end)) > + ;; Insert the new text > + (goto-char end) > + (insert aft) Better strip text properties from AFT before inserting it here. > + (setq full-end (point)) > + > + ;; Use the movement command to go to a new location in the buffer > + (goto-char end) > + (funcall command) > + (setq new-end (point)) We should ensure that new-end isn't smaller then end, otherwise the deletion below won't do the right thing. > + (if (< new-end full-end) > + ;; The movement command has not taken us to the end of the > + ;; initial insertion this means that a partial completion > + ;; occured. > + (progn > + (completion-preview--inhibit-update) > + ;; If we are not inserting a full completion update the preview > + (overlay-put (completion-preview--make-overlay > + new-end (propertize (delete-and-extract-region full-end new-end) > + 'mouse-face 'completion-preview-highlight > + 'keymap completion-preview--mouse-map)) > + 'completion-preview-end new-end)) > + ;; The movement command has taken us to the end of the > + ;; completion or past it which signifies a full completion. > + (goto-char full-end) > + (completion-preview-active-mode -1) > + (when (functionp efn) > + (funcall efn (concat (buffer-substring-no-properties beg end) aft) 'finished)))) > + (user-error "No current completion preview"))) 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: --8<---------------cut here---------------start------------->8--- (atomic-change-group (let ((change-group (prepare-change-group))) ;; Insert, ;; Move, ;; Delete. (undo-amalgamate-change-group change-group))) --8<---------------cut here---------------end--------------->8--- > +(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. 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: --8<---------------cut here---------------start------------->8--- ;; 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. --8<---------------cut here---------------end--------------->8--- Best, Eshel