Re: master 47e313e9805: Unbind 'C-M-i' in Text mode

Re: master 47e313e9805: Unbind 'C-M-i' in Text mode

[Arash Esbati]

Eli Zaretskii <eliz@gnu.org> writes:

> branch: master
> commit 47e313e9805c527e590df4270062a9185ee9db78
> Author: Eshel Yaron <me@eshelyaron.com>
> Commit: Eli Zaretskii <eliz@gnu.org>
> [...]
> diff --git a/lisp/textmodes/text-mode.el b/lisp/textmodes/text-mode.el
> index ccba1b063ab..7f38a1c463d 100644
> --- a/lisp/textmodes/text-mode.el
> +++ b/lisp/textmodes/text-mode.el
> [...]
> @@ -131,7 +143,8 @@ Turning on Text mode runs the normal hook `text-mode-hook'."
>  
>    ;; Enable text conversion in this buffer.
>    (setq-local text-conversion-style t)
> -  (add-hook 'context-menu-functions 'text-mode-context-menu 10 t))
> +  (add-hook 'context-menu-functions 'text-mode-context-menu 10 t)
> +  (add-hook 'completion-at-point-functions #'ispell-completion-at-point 10 t))
>  
>  (define-derived-mode paragraph-indent-text-mode text-mode "Parindent"
>    "Major mode for editing text, with leading spaces starting a paragraph.

I think this part of the change now throws:

    ELC+ELN  ../lisp/textmodes/text-mode.elc
  In end of data:
  text-mode.el:147:46: Warning: the function ‘ispell-completion-at-point’
  is not known to be defined.

Maybe declare'ing the function helps?  This is with Emacs 30
(a811846879) on macOS.

Best, Arash

Re: master 47e313e9805: Unbind 'C-M-i' in Text mode

[Eli Zaretskii]

> From: Arash Esbati <arash@gnu.org>
> Cc: Eshel Yaron <me@eshelyaron.com>
> Date: Wed, 29 Nov 2023 20:11:15 +0100
> 
> >    ;; Enable text conversion in this buffer.
> >    (setq-local text-conversion-style t)
> > -  (add-hook 'context-menu-functions 'text-mode-context-menu 10 t))
> > +  (add-hook 'context-menu-functions 'text-mode-context-menu 10 t)
> > +  (add-hook 'completion-at-point-functions #'ispell-completion-at-point 10 t))
> >  
> >  (define-derived-mode paragraph-indent-text-mode text-mode "Parindent"
> >    "Major mode for editing text, with leading spaces starting a paragraph.
> 
> I think this part of the change now throws:
> 
>     ELC+ELN  ../lisp/textmodes/text-mode.elc
>   In end of data:
>   text-mode.el:147:46: Warning: the function ‘ispell-completion-at-point’
>   is not known to be defined.
> 
> Maybe declare'ing the function helps?  This is with Emacs 30
> (a811846879) on macOS.

It's a bogus warning, it happens because you build Emacs
incrementally, starting from the previous build.  To make sure, say

  touch lisp/textmodes/text-mode.el

and then say "make" again -- this time there will be no warning.

AFAICT, the problem is that the first time loaddefs.el still isn't
updated, or something like that.

Such temporary warnings appear very frequently when Emacs is built
after an update (as opposed to a "make bootstrap").

Re: master 47e313e9805: Unbind 'C-M-i' in Text mode

[Arash Esbati]

Eli Zaretskii <eliz@gnu.org> writes:

> It's a bogus warning, it happens because you build Emacs
> incrementally, starting from the previous build.

Actually, I use a small script which does:

  git clean -fdx --exclude=ChangeLog
  ./autogen.sh
  ./configure ...
  make

So I don't build incrementally.  And I thought the method above is
(almost?) equivalent to "make bootstrap"

> To make sure, say
>
>   touch lisp/textmodes/text-mode.el
>
> and then say "make" again -- this time there will be no warning.

True, that works.

> AFAICT, the problem is that the first time loaddefs.el still isn't
> updated, or something like that.
>
> Such temporary warnings appear very frequently when Emacs is built
> after an update (as opposed to a "make bootstrap").

I get the warnings (see other message) also with "make bootstrap".

Best, Arash

Re: master 47e313e9805: Unbind 'C-M-i' in Text mode

[Eli Zaretskii]

> From: Arash Esbati <arash@gnu.org>
> Cc: emacs-devel@gnu.org,  me@eshelyaron.com
> Date: Thu, 30 Nov 2023 08:40:15 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > It's a bogus warning, it happens because you build Emacs
> > incrementally, starting from the previous build.
> 
> Actually, I use a small script which does:
> 
>   git clean -fdx --exclude=ChangeLog
>   ./autogen.sh
>   ./configure ...
>   make
> 
> So I don't build incrementally.  And I thought the method above is
> (almost?) equivalent to "make bootstrap"
> 
> > To make sure, say
> >
> >   touch lisp/textmodes/text-mode.el
> >
> > and then say "make" again -- this time there will be no warning.
> 
> True, that works.
> 
> > AFAICT, the problem is that the first time loaddefs.el still isn't
> > updated, or something like that.
> >
> > Such temporary warnings appear very frequently when Emacs is built
> > after an update (as opposed to a "make bootstrap").
> 
> I get the warnings (see other message) also with "make bootstrap".

If so, then a declare-function might indeed be necessary.

Re: master 47e313e9805: Unbind 'C-M-i' in Text mode

[Arash Esbati]

Eli Zaretskii <eliz@gnu.org> writes:

> If so, then a declare-function might indeed be necessary.

I suggest we wait and see if others get the warnings as well;
declare'ing those 2 functions shouldn't be a big deal.

Best, Arash

Re: master 47e313e9805: Unbind 'C-M-i' in Text mode

[Juri Linkov]

>> If so, then a declare-function might indeed be necessary.
>
> I suggest we wait and see if others get the warnings as well;
> declare'ing those 2 functions shouldn't be a big deal.

I see this warning on bootstrap:

  In end of data:
  text-mode.el:147:46: Warning: the function `ispell-completion-at-point' is not known to be defined.

But it goes away after manually using

  make -C lisp autoloads

Re: master 47e313e9805: Unbind 'C-M-i' in Text mode

[Dmitry Gutov]

Hi,

On 26/11/2023 12:39, Eli Zaretskii wrote:
> +;;;###autoload
> +(defun ispell-completion-at-point ()
> +  "Word completion function for use in `completion-at-point-functions'."
> +  (pcase (bounds-of-thing-at-point 'word)
> +    (`(,beg . ,end)
> +     (when (and (< beg (point)) (<= (point) end))
> +       (let* ((word (buffer-substring-no-properties beg end))
> +              (len (length word))
> +              (inhibit-message t)
> +              (all (cons word (ispell-lookup-words word)))
> +              (cur all))
> +         (while cur
> +           (unless (string-prefix-p word (car cur))
> +             (setcar cur (concat word (substring (car cur) len))))
> +           (while (when-let ((next (cadr cur)))
> +                    (not (string-prefix-p word next t)))
> +             (setcdr cur (cddr cur)))
> +           (setq cur (cdr cur)))
> +         (list beg end (cdr all)
> +               :annotation-function (lambda (_) " Dictionary word")

I have to say that "Dictionary word" is not a great annotation: it's 
long, longer than most of the completions coming from that backend, so 
these words, repeated, constitute most of the text inside the 
Completions buffer. And it's the same string for all completions.

Re: master 47e313e9805: Unbind 'C-M-i' in Text mode

[Eshel Yaron]

Hi Dmitry,

Dmitry Gutov <dmitry@gutov.dev> writes:

>> +(defun ispell-completion-at-point ()
>> +  "Word completion function for use in `completion-at-point-functions'."
>> [...]
>> +         (list beg end (cdr all)
>> +               :annotation-function (lambda (_) " Dictionary word")
>
> I have to say that "Dictionary word" is not a great annotation: it's
> long, longer than most of the completions coming from that backend, so
> these words, repeated, constitute most of the text inside the
> Completions buffer. And it's the same string for all completions.

I see what you mean.  How do you feel about the following alternatives?

- "Word"
- "Dict"
- "Spell"/"Ispell"
- "🕮"
- No annotation at all
- One of the above, and make it configurable


Thanks,

Eshel

Re: master 47e313e9805: Unbind 'C-M-i' in Text mode

[Dmitry Gutov]

On 06/12/2023 09:53, Eshel Yaron wrote:
> Hi Dmitry,
> 
> Dmitry Gutov <dmitry@gutov.dev> writes:
> 
>>> +(defun ispell-completion-at-point ()
>>> +  "Word completion function for use in `completion-at-point-functions'."
>>> [...]
>>> +         (list beg end (cdr all)
>>> +               :annotation-function (lambda (_) " Dictionary word")
>>
>> I have to say that "Dictionary word" is not a great annotation: it's
>> long, longer than most of the completions coming from that backend, so
>> these words, repeated, constitute most of the text inside the
>> Completions buffer. And it's the same string for all completions.
> 
> I see what you mean.  How do you feel about the following alternatives?
> 
> - "Word"
> - "Dict"
> - "Spell"/"Ispell"
> - "🕮"
> - No annotation at all
> - One of the above, and make it configurable

My preference would be "no annotation at all", if we're talking about 
simple solutions.

The fourth option was a newspaper emoji? I think the more comprehensive 
approach here would be to implement support for `:company-kind` in the 
default UI, and then return `text' in that function.

Re: master 47e313e9805: Unbind 'C-M-i' in Text mode

[Eshel Yaron]

[-- Attachment #1: Type: text/plain, Size: 1668 bytes --]

Dmitry Gutov <dmitry@gutov.dev> writes:

> On 06/12/2023 09:53, Eshel Yaron wrote:
>> Hi Dmitry,
>> Dmitry Gutov <dmitry@gutov.dev> writes:
>>
>>>> +(defun ispell-completion-at-point ()
>>>> +  "Word completion function for use in `completion-at-point-functions'."
>>>> [...]
>>>> +         (list beg end (cdr all)
>>>> +               :annotation-function (lambda (_) " Dictionary word")
>>>
>>> I have to say that "Dictionary word" is not a great annotation: it's
>>> long, longer than most of the completions coming from that backend, so
>>> these words, repeated, constitute most of the text inside the
>>> Completions buffer. And it's the same string for all completions.
>> I see what you mean.  How do you feel about the following
>> alternatives?
>> - "Word"
>> - "Dict"
>> - "Spell"/"Ispell"
>> - "🕮"
>> - No annotation at all
>> - One of the above, and make it configurable
>
> My preference would be "no annotation at all", if we're talking about
> simple solutions.

Fine by me.  I'm attaching a simple patch that does that.

> The fourth option was a newspaper emoji?

Well, it's supposed to be a book, you get it with `C-x 8 RET book RET`.

> I think the more comprehensive approach here would be to implement
> support for `:company-kind` in the default UI, and then return `text'
> in that function.

I agree, that would be nice in general and help here too.  For this case
I was also thinking about using the common part-of-speech of each word
as its annotation (e.g. "n" for nouns, "v" for verbs...) but we don't
get that information from the spelling dictionary, unfortunately.


Here's that patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Remove-long-annotation-for-word-completion-candidate.patch --]
[-- Type: text/x-patch, Size: 837 bytes --]

From 5055fd663b3e82d752ae7aa3d0cad5d0ea4f87a6 Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Wed, 6 Dec 2023 14:41:56 +0100
Subject: [PATCH] ; Remove long annotation for word completion candidates

* lisp/textmodes/ispell.el (ispell-completion-at-point): Remove
':annotation-function' from return value.
---
 lisp/textmodes/ispell.el | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lisp/textmodes/ispell.el b/lisp/textmodes/ispell.el
index 4c3b22281bd..2c387342026 100644
--- a/lisp/textmodes/ispell.el
+++ b/lisp/textmodes/ispell.el
@@ -3699,7 +3699,6 @@ ispell-completion-at-point
              (setcdr cur (cddr cur)))
            (setq cur (cdr cur)))
          (list beg end (cdr all)
-               :annotation-function (lambda (_) " Dictionary word")
                :exclusive 'no))))))
 
 
-- 
2.42.0

Re: master 47e313e9805: Unbind 'C-M-i' in Text mode

[Dmitry Gutov]

On 06/12/2023 15:56, Eshel Yaron wrote:

>> My preference would be "no annotation at all", if we're talking about
>> simple solutions.
> 
> Fine by me.  I'm attaching a simple patch that does that.

Thanks, pushed.

>> The fourth option was a newspaper emoji?
> 
> Well, it's supposed to be a book, you get it with `C-x 8 RET book RET`.

Cool.

>> I think the more comprehensive approach here would be to implement
>> support for `:company-kind` in the default UI, and then return `text'
>> in that function.
> 
> I agree, that would be nice in general and help here too.  For this case
> I was also thinking about using the common part-of-speech of each word
> as its annotation (e.g. "n" for nouns, "v" for verbs...) but we don't
> get that information from the spelling dictionary, unfortunately.

Note that this implementation approach conflates presentation with 
information. Symbolic values in company-kind with a pre-defined registry 
allow showing icons in GUI and prettily-formatted letters in the terminal.

Being able to choose narrower categories sometimes could be handy, though.