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: Fri, 10 Nov 2023 15:05:53 +0200 Message-ID: <83msvlyczy.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> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="971"; mail-complaints-to="usenet@ciao.gmane.io" Cc: philipk@posteo.net, juri@linkov.net, dmitry@gutov.dev, stefankangas@gmail.com, 66948@debbugs.gnu.org, joaotavora@gmail.com To: Eshel Yaron Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Fri Nov 10 14:06:57 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 1r1RDr-000AWK-Na for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 10 Nov 2023 14:06:55 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1r1RDT-0003O7-Ly; Fri, 10 Nov 2023 08:06:36 -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 1r1RDK-0003M2-DS for bug-gnu-emacs@gnu.org; Fri, 10 Nov 2023 08:06:23 -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 1r1RDK-0006TF-5g for bug-gnu-emacs@gnu.org; Fri, 10 Nov 2023 08:06:22 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1r1RDx-0003CZ-LP for bug-gnu-emacs@gnu.org; Fri, 10 Nov 2023 08:07:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 10 Nov 2023 13:07:01 +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.169962161312293 (code B ref 66948); Fri, 10 Nov 2023 13:07:01 +0000 Original-Received: (at 66948) by debbugs.gnu.org; 10 Nov 2023 13:06:53 +0000 Original-Received: from localhost ([127.0.0.1]:49685 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1r1RDn-0003CC-UM for submit@debbugs.gnu.org; Fri, 10 Nov 2023 08:06:52 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:37066) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1r1RDi-0003Bo-LM for 66948@debbugs.gnu.org; Fri, 10 Nov 2023 08:06:50 -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 1r1RCy-0006Q3-EI; Fri, 10 Nov 2023 08:06:00 -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=0JWq/oA0encG/w2WjmC2koXe2h3Id1yVnGtdrVktT4w=; b=G/20G0nHFYqP KxTvyxokQEni5ve7WtFtM5ofcWWDAWBE3nmPrdxI/ldu0FeEZrKDLVy+aKLz96LIxROVUue/BplK3 HIviiDWsTCX4RINiO7BAcjMz22t9Q02BN433p3ub9Xpw1xBxblXjKaq1bwG4TjQKIelNvtgPHDTmt MBHH1LVegwaslpzDFRy7LXVt/uYfYL5c8kG3mR0qGDibxTn2BIZJumibhU4rOzvyP0JcnlBm8TN4f HYtGG/S5EJtmAR/xG0fhWJlFBLredJFgM6Hg1twfvRufyb5POmqNFRgH4yFKvOV4J5dfu1EhTud4S KoQdY6LAg3+ToXs1CEAHKg==; In-Reply-To: (message from Eshel Yaron on Fri, 10 Nov 2023 08:59:56 +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:274111 Archived-At: > From: Eshel Yaron > Cc: juri@linkov.net, dmitry@gutov.dev, philipk@posteo.net, > joaotavora@gmail.com, stefankangas@gmail.com, 66948@debbugs.gnu.org > Date: Fri, 10 Nov 2023 08:59:56 +0100 > > Eshel Yaron writes: > > > This is for Emacs core, I'm attaching the latest patch again. I think > > it mostly needs your OK. Notably, I'd appreciate it if you could check > > out the addition to the manual and see if you have any comments on that. > > And now with the actual patch attached... sorry about that. Thanks. If this is for core, I think there's still work to be done here, see the comments below. > --- a/doc/emacs/programs.texi > +++ b/doc/emacs/programs.texi > @@ -1701,6 +1701,90 @@ Symbol Completion > In Text mode and related modes, @kbd{M-@key{TAB}} completes words > based on the spell-checker's dictionary. @xref{Spelling}. > > +@cindex completion preview > +@cindex preview completion > +@cindex suggestion preview > +@cindex Completion Preview mode > +@findex completion-preview-mode > +@vindex completion-preview-mode > + 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. I don't think this minor mode warrants such a long and detailed description in the user manual. The section where you added that just mentions the various similar features, it doesn't describe them in such great detail, so I think we should do the same for this new mode. IOW, just mention it and what it does in a sentence or two, and move the rest of the description to the Commentary section of completion-preview.el and/or to the relevant doc strings. > +*** New minor mode 'completion-preview-mode'. > +This minor mode shows you symbol completion suggestions as you type, > +using an inline preview. New user options in the 'completion-preview' > +customization group control exactly when Emacs displays this preview. This fails to mention that (evidently) this mode is intended to be used only in descendants of prog-mode, i.e. that useful completions will be available only for editing program source. But see below. > +;; This library provides the Completion Preview mode. This minor mode > +;; displays the top completion candidate for the symbol at point in an > +;; overlay after point. If you want to enable Completion Preview mode > +;; in all programming modes, add the following to your Emacs init: > +;; > +;; (add-hook 'prog-mode-hook #'completion-preview-mode) I'm not sure why this is advertised for prog-mode. Are completions produced for descendants of Text mode, for example? If not, why not? I see other apps offering completion in this style for editing human-readable text, so why cannot we have that as well (for word at point)? > +(defcustom completion-preview-exact-match-only nil > + "Whether to show completion preview only when there is an exact match. > + > +If this option is non-nil, Completion Preview mode only shows the > +preview overlay when there is exactly one completion candidate ^^^^^^^ This is an implementation detail, better left out of the doc string. > +that matches the symbol at point, otherwise it shows the top > +candidate also when there are multiple matching candidates." What do you mean by "top candidate"? I can try guessing, but I think it is better to reword this. > + :type 'boolean) Every new defcustom should have a :version tag (this comment is for all the defcustoms in this file). > +(defcustom completion-preview-commands '(self-insert-command > + delete-backward-char > + backward-delete-char-untabify) I think you should add insert-char to this list. Also, did you test this minor mode when Overwrite mode is in effect? > +(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? > +(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. > +(defface completion-preview-exact > + '((t :underline t :inherit completion-preview)) The underline face is not universally supported, so this defface should have fallbacks. > +(defvar completion-preview--internal-commands > + '(completion-preview-next-candidate completion-preview-prev-candidate) > + "List of commands that manipulate the completion preview.") > + > +(defun completion-preview--internal-command-p () > + "Return non-nil if `this-command' manipulates the completion preview." > + (memq this-command completion-preview--internal-commands)) Should this be a defsubst? > +(defun completion-preview-require-certain-commands () > + "Check if `this-command' is one of `completion-preview-commands'." > + (or (completion-preview--internal-command-p) > + (memq this-command completion-preview-commands))) Likewise here. > +(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? > +(defun completion-preview--make-overlay (pos string) > + "Make a new completion preview overlay at POS showing STRING." > + (if completion-preview--overlay > + (move-overlay completion-preview--overlay pos pos) > + (setq completion-preview--overlay (make-overlay pos pos))) Should this overlay be specific to the selected window? IOW, do we really want to see the preview in all the windows showing this buffer? > + (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? > +(define-minor-mode completion-preview-active-mode > + "Mode for when the completion preview is active." ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ It is better to say "when the completion preview is shown". "Active" is ambiguous here. > +(defun completion-preview--exit-function (func) > + "Return an exit function that hides the completion preview and calls FUNC." > + (lambda (&rest args) > + (completion-preview-active-mode -1) > + (when func (apply func args)))) ^^^^^^^^^^ Perhaps "(when (functionp func) ..."? > +(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? > +(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? > +(defun completion-preview--post-command () > + "Create, update or delete completion preview post last command." > + (if (run-hook-with-args-until-failure 'completion-preview-hook) > + (or (completion-preview--internal-command-p) > + (completion-preview--show)) > + (completion-preview-active-mode -1))) This needs more comments to explain the non-trivial logic. > +(defun completion-preview--insert () > + "Completion at point function for inserting the current preview." The purpose of this function should be described either in the doc string or in some comment. > +(defun completion-preview-insert () > + "Insert the current completion preview." You cannot "insert the preview". Please reword the doc string. > + (interactive) > + (let ((completion-preview-insert-on-completion t)) > + (completion-at-point))) Why not just insert the string you show in the preview? > +(defun completion-preview-prev-candidate () > + "Cycle the preview to the previous completion suggestion." You are cycling the candidates, not the preview. > +(defun completion-preview-next-candidate (direction) > + "Cycle the preview to the next completion suggestion in DIRECTION. > + > +DIRECTION should be either 1 which means cycle forward, or -1 > +which means cycle backward. Interactively, DIRECTION is the > +prefix argument." ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^ "...and defaults to 1".