From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#66948: [PATCH] Add Completion Preview mode Date: Sat, 11 Nov 2023 10:53:50 +0200 Message-ID: <83sf5cwu01.fsf@gnu.org> References: <87lebcqcof.fsf@posteo.net> <86a5rrcqd3.fsf@mail.linkov.net> <86zfzqeoix.fsf@mail.linkov.net> <86msvobtjm.fsf@mail.linkov.net> <86y1f7cs8l.fsf@mail.linkov.net> <83v8aaxdcy.fsf@gnu.org> <83msvlyczy.fsf@gnu.org> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="21086"; mail-complaints-to="usenet@ciao.gmane.io" Cc: philipk@posteo.net, juri@linkov.net, dmitry@gutov.dev, joaotavora@gmail.com, 66948@debbugs.gnu.org, stefankangas@gmail.com To: Eshel Yaron Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sat Nov 11 09:54:51 2023 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 1r1jlS-0005H5-Hn for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 11 Nov 2023 09:54:50 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1r1jl1-0001vd-Sd; Sat, 11 Nov 2023 03:54:23 -0500 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 1r1jl0-0001vP-Il for bug-gnu-emacs@gnu.org; Sat, 11 Nov 2023 03:54:22 -0500 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 1r1jl0-0008V6-AI for bug-gnu-emacs@gnu.org; Sat, 11 Nov 2023 03:54:22 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1r1jle-0001k0-99 for bug-gnu-emacs@gnu.org; Sat, 11 Nov 2023 03:55:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 11 Nov 2023 08:55:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 66948 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 66948-submit@debbugs.gnu.org id=B66948.16996928886665 (code B ref 66948); Sat, 11 Nov 2023 08:55:02 +0000 Original-Received: (at 66948) by debbugs.gnu.org; 11 Nov 2023 08:54:48 +0000 Original-Received: from localhost ([127.0.0.1]:51565 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1r1jlP-0001jQ-1E for submit@debbugs.gnu.org; Sat, 11 Nov 2023 03:54:47 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:40886) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1r1jlL-0001jC-SL for 66948@debbugs.gnu.org; Sat, 11 Nov 2023 03:54:45 -0500 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1r1jkY-0008SB-Vm; Sat, 11 Nov 2023 03:53:54 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=a++3k91PqxpJ2bEXwIxCcs6nv/Pej5sZG3c11GW/CyA=; b=Wfe/r2N3rt5R yNVFfOaF90lVVEztLVMN33OSaWSTzNKt0SXcNVY6GIvmNWlj6AtfLs0aVcBJspRgRzckKPaRCPtUf UpkvmxafpIWEyS0iA02YlEkyFlxum1DJ6GwkKbAJd1iThpDBBS8EkB8itfp9Yum+qbPD3XsY938Ru WPT/nI3W2jwokMHkqyQD/qKIJCyxxTm7PWT8yscFOKH1QeIaV/3giOMeJItA5/V5ut0k5YthOlXdm 34YfE1lurJBuQ/+HSKKlvMmbJ0HQV5JUX7ruSzVgr8F+rhMbbplNJYUgWXSRypj1Pcpbs+hZpJ8dZ yB9TEmwJBwNSVHz1FxZqOg==; In-Reply-To: (message from Eshel Yaron on Fri, 10 Nov 2023 17:23:12 +0100) 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:274145 Archived-At: > From: Eshel Yaron > Cc: philipk@posteo.net, juri@linkov.net, dmitry@gutov.dev, > stefankangas@gmail.com, 66948@debbugs.gnu.org, joaotavora@gmail.com > Date: Fri, 10 Nov 2023 17:23:12 +0100 > > Thanks for reviewing. I'm attaching an updated patch (v4) following > your comments. Thanks. > + Completion Preview mode is a minor mode that shows you symbol > +completion suggestions as type. When you enable Completion Preview > +mode in a buffer (with @kbd{M-x completion-preview-mode}), Emacs > +examines the text around point after certain commands you invoke and > +automatically suggests a possible completion. Emacs displays this > +suggestion with an inline preview right after point, so you see in > +advance exactly how the text will look if you accept the completion > +suggestion---that's why it's called a preview. This is still too wordy, IMO. I suggest Completion Preview mode is a minor mode that shows completion suggestions as you type. When you enable this mode (with @kbd{M-x completion-preview-mode}), Emacs automatically displays the suggested completion for text around point as an in-line preview right after point; type @key{TAB} to accept the suggestion. Also, one of these two index entries is redundant: > +@findex completion-preview-mode > +@vindex completion-preview-mode Since the ELisp manual has just one Index node, it is enough to have the @findex entry alone. > > Are completions produced for descendants of Text mode, for example? > > Sure. I'm running with `completion-preview-mode` in `text-mode-hook` myself. What is/are the backend(s) that provide(s) the completions in the text-mode case? Is that just a word completion, or are we capable of suggesting phrases as well? > > Also, did you test this minor mode when Overwrite mode is in effect? > > Yes, no surprises there AFAICT, works well. What does it do in Overwrite mode if one accepts a completion? If it overwrites the following text, I'm not sure it should. > >> +(defcustom completion-preview-minimum-symbol-length 3 > >> + "Minimum length of the symbol at point for showing completion preview." > >> + :type 'natnum) > > > > Why do we need this defcustom? IOW, why not show the completion after > > a single character? > > Basically, a single character often has many completion candidates, and > most of them are not what you want. After three characters, the preview > is much more likely to show you a useful candidate. So you can think of > this option as an adjustable threshold for how much information we > require the completion backend to have before we consider its > suggestions any good. I'm open to changing the default value, but I > think that three characters is a very sane default. The advantage of 1 character is that we don't need this defcustom at all, and it is basically up to the user when to type TAB, or even look at the preview. Alternatively, we could have a defcustom based on a different design: show preview only when there are fewer than N completion candidates, with N being customizable. That would make much more sense, IMO, since it replaces a largely "mechanical" limitation with one that is meaningful for users. > >> +(defcustom completion-preview-hook > >> + '(completion-preview-require-certain-commands > >> + completion-preview-require-minimum-symbol-length) > >> + "Hook for functions that determine whether to show preview completion. > >> + > >> +Completion Preview mode calls each of these functions in order > >> +after each command, and only displays the completion preview when > >> +all of the functions return non-nil." > > > > This feature sounds like over-engineering to me. > > I think this makes the mode nicely flexible, as it lets users and other > code add different conditions for when to show the preview, e.g. only in > or out of comments. And the added complexity is negligible, really. So > I guess we could do without this option, but I'd prefer to keep it > unless you feel strongly about that. I'd like to defer any extensibility features like this until we have some data to support the need for such extensibility. Defining those ahead of any real experience is a kind of premature optimization, IMO. > >> +(defface completion-preview-exact > >> + '((t :underline t :inherit completion-preview)) > > > > The underline face is not universally supported, so this defface > > should have fallbacks. > > The `underline` face in faces.el has `:underline t` in the fallback > clause too, so I figured that should be alright, no? If you are okay with seeing no effect at all when the terminal doesn't support the underline attribute, then yes. But I thought we want this face to stand out no matter what, don't we? > >> +(defun completion-preview-require-minimum-symbol-length () > >> + "Check if the length of symbol at point is at least above a certain threshold. > >> +`completion-preview-minimum-symbol-length' determines that threshold." > >> + (pcase (bounds-of-thing-at-point 'symbol) > >> + (`(,beg . ,end) > >> + (<= completion-preview-minimum-symbol-length (- end beg))))) > > > > Is usage of pcase really justified here, and if so, why? > > Since we're relying on `completion-at-point`, that already uses `pcase`, > I'm not sure what's the cost of using `pcase` here too. Readability. A person who isn't familiar with pcase will not need to go read the documentation to understand this code. > Furthermore, it does exactly what we want here, including correctly > handling the case where `bounds-of-thing-at-point` returns nil. So > yes, it's a good use of `pcase` IMO. But if you prefer a different > style I'm open to adjust this part, of course. I prefer not to use it where a simple if or cond will do, and where the data structures are as simple as a cons cell. This goes back to that long dispute on emacs-devel we are having, where many people agree -- in principle -- that over-using such advanced features with complex syntax is not a Good Thing. So I thought we should actually "practice what we preach". > >> + (add-text-properties 0 1 '(cursor 1) string) > >> + (overlay-put completion-preview--overlay 'after-string string) > > > > Sounds like you keep calling overlay-put and adding the same property > > to the string each time this function is called, even if the overlay > > already shows the same string? > > Even if the preview exists, this function is called with a different > `string` argument than the one already shown. That `string` is a > substring of a completion candidate, and it doesn't already have the > `cursor` property. So no, this is not redundant. There may be room for > an optimization here, but I don't think it'd be significant. What bothers me is consing (which leads to GC). Testing a string for equality is simple, and if that avoids extra consing, I think it's a good optimization. > >> +(defun completion-preview--update () > >> + "Update completion preview." > >> + (pcase (let ((completion-preview-insert-on-completion nil)) > >> + (run-hook-with-args-until-success 'completion-at-point-functions)) > >> + (`(,beg ,end ,table . ,plist) > > > > Why use pcase here and not seq-let? > > Because `seq-let` doesn't do the right thing (for our purposes here) > when the sequence that you pass it doesn't have the given shape. > Namely, here `pcase` correctly handles the case where the value is nil > for example, while `seq-let` would require another test in the body > (e.g. `(when beg ...)`) to see if we actually got what we expected. Is that single extra test so important to avoid? > >> +(defun completion-preview--show () > >> + "Show completion preview." > >> + (when completion-preview-active-mode > >> + (let* ((beg (completion-preview--get 'completion-preview-beg)) > >> + (cands (completion-preview--get 'completion-preview-cands)) > >> + (index (completion-preview--get 'completion-preview-index)) > >> + (cand (nth index cands)) > >> + (len (length cand)) > >> + (end (+ beg len)) > >> + (cur (point)) > >> + (face (get-text-property 0 'face (completion-preview--get 'after-string)))) > >> + (if (and (< beg cur end) (string-prefix-p (buffer-substring beg cur) cand)) > >> + (overlay-put (completion-preview--make-overlay > >> + cur (propertize (substring cand (- cur beg)) > >> + 'face face)) > >> + 'completion-preview-end cur) > >> + (completion-preview-active-mode -1)))) > >> + (while-no-input (completion-preview--update))) > > > > I'm puzzled by this function. What does it do, and why is it needed? > > I've added some comments in the updated patch. Thanks, but please also make the doc string more useful. It is now too terse, IMO. Also, the last part is still quite obscure: why do you turn off completion-preview-active-mode, and why the while-no-input loop that calls completion-preview--update? The comment which starts with "Reconsult" should be probably reworded to be more self-explanatory; e.g., the word "backend" is barely used, let alone explained, in the code or its comments. Btw, I noticed that your comments don't end in a period. They should, at least when a comment is a complete sentence. > >> + (interactive) > >> + (let ((completion-preview-insert-on-completion t)) > >> + (completion-at-point))) > > > > Why not just insert the string you show in the preview? > > This way we let `completion-at-point` to take care of things like > calling the `:exit-function`, instead of duplicating that code. Sorry, I still don't understand. What about :exit-function, and why inserting the completion needs it? Btw, did you consider an alternative design, where the completion is displayed from an idle timer, not from a post-command-hook? The latter means you make Emacs less responsive during fast typing (when the user won't normally need the preview), whereas doing it from a timer better suits the use case: a preview is shown when the user might be considering what to type next.