From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.ciao.gmane.io!not-for-mail From: Mathias Dahl Newsgroups: gmane.emacs.devel Subject: Re: Abbrev suggestions - feedback appreciated Date: Tue, 12 May 2020 00:39:07 +0200 Message-ID: References: <871smeoalc.fsf@gnu.org> <87fuatmw71.fsf@gnu.org> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="0000000000007ee5e905a56704f4" Injection-Info: ciao.gmane.io; posting-host="ciao.gmane.io:159.69.161.202"; logging-data="1091"; mail-complaints-to="usenet@ciao.gmane.io" To: monnier@iro.umontreal.ca, emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Tue May 12 00:40:03 2020 Return-path: Envelope-to: ged-emacs-devel@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 1jYH5e-00006z-3X for ged-emacs-devel@m.gmane-mx.org; Tue, 12 May 2020 00:40:02 +0200 Original-Received: from localhost ([::1]:52496 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jYH5d-0006KP-5G for ged-emacs-devel@m.gmane-mx.org; Mon, 11 May 2020 18:40:01 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:51700) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jYH50-0005va-Gf for emacs-devel@gnu.org; Mon, 11 May 2020 18:39:22 -0400 Original-Received: from mail-ua1-x92e.google.com ([2607:f8b0:4864:20::92e]:43546) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jYH4y-0005he-BR for emacs-devel@gnu.org; Mon, 11 May 2020 18:39:22 -0400 Original-Received: by mail-ua1-x92e.google.com with SMTP id u12so4048364uau.10 for ; Mon, 11 May 2020 15:39:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=kldLKyYPK61A1+mPsxVjQEKnYz1oAtDh2T04GTH11N0=; b=c+qJ/hvdnq3LEfEdCvLS+SM03cfBQD889eQLfR1lLFN7d2kC2OoJcWrRV+uU4fxI4J zsP25KXwokMevYPOUiAS3v12leeCD1PbSzBlcDV6LNTOtCJ5E9Kcl4GENejBMJNIG5Ud GtvCXX5c8tJSmEwjR8o3qCcbUSgm6dQtBoHhI6EIrGM8EIDhQLZ3jxdSTSgnPYuxdd6Z kcSIuWd7rhAk1feCJni+me7CcYNUVt1oZl5JxUoDlmOijMRqNnVqxO7rnMMmPyvOG7R2 WlLoDxZ425ES6ih5RG/5+eVkVwHpHqF0XoUD7n2yD6XrFkFOT0ulTAPOP+WxRfCxuuo9 6SdA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=kldLKyYPK61A1+mPsxVjQEKnYz1oAtDh2T04GTH11N0=; b=SBhfvefFlnoGXivDEi8wq5ERESAmWZFPLwv+9M9JRr4DlVvXjTUtbAO1MY7pARMPQd sQV3QpC9QtSLO9oTdziKhskmZSXtFbT7F7p0iF55WWOggaCbvfR7hbp5jj6qy6xuaoz3 w89rhALbN4UwD5dvPMe3unpavmplJizrpsx2ez2o6e7F43immQEZM656V9WIT+RnAmCv S/fvbRWZgvVtBoSWV5gl1kJ6wEt7KGYKxGwFcFe0IbkkAtcGQ3+ykBPClVSHs31B8+d1 v1HiQa+FPn4rtidSfD0ywi1BRNUiD4nZtiPuH/jnxmacnzo0AnwGyKo4NsbJtnVWSg5z Mwjw== X-Gm-Message-State: AOAM530u5GW6zjXEIW2Rf90iH91yAp5yHvNy5BSyiZEA8hU0oEI7iBdh 1VyLkqC4Uor3w7KIsN41feveefY9xoxuc3LG6E8BXzip4qPx0Q== X-Google-Smtp-Source: ABdhPJyzVV0aKgXXD+N3Kns1H99InEt7efAxLxEUPKSrKPSIHmXTOQUO826wDuguLXzvaFZLX2hvAtx8R7wFmR2VZTc= X-Received: by 2002:a9f:2492:: with SMTP id 18mr378419uar.2.1589236759191; Mon, 11 May 2020 15:39:19 -0700 (PDT) In-Reply-To: Received-SPF: pass client-ip=2607:f8b0:4864:20::92e; envelope-from=mathias.dahl@gmail.com; helo=mail-ua1-x92e.google.com X-detected-operating-system: by eggs.gnu.org: No matching host in p0f cache. That's all we know. X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001 autolearn=_AUTOLEARN X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:249901 Archived-At: --0000000000007ee5e905a56704f4 Content-Type: text/plain; charset="UTF-8" Tried out the "why focus on words?" approach and it seems to work well. Here is the gist of that change: (defun abbrev-suggest-previous-chars (n) (buffer-substring (- (point) n) (point))) (defun abbrev-suggest-expansion-exist-at-point (expansion) (string= (car expansion) (abbrev-suggest-previous-chars (length (car expansion))))) (defun abbrev-suggest () "Suggest an abbrev to the user based on the text before point. Uses `abbrev-suggest-hint-threshold' to find out if the user should be informed about the existing abbrev." (let (abbrev-found) (dolist (expansion (abbrev-get-active-expansions)) (when (and (abbrev-suggest-above-threshold expansion) (abbrev-suggest-expansion-exist-at-point expansion)) (setq abbrev-found (abbrev-suggest-best-abbrev expansion abbrev-found)))) (when abbrev-found (abbrev-suggest-inform-user abbrev-found)))) The new abbrev-suggest function above also have that little optimization you suggested, checking the threshold early on to avoid looking around in the buffer. I have yet to make some of the other changes. /Mathias On Mon, May 11, 2020 at 11:37 PM Mathias Dahl wrote: > Hi Stefan, > > Thanks for your feedback. Now, one and a half year later I started working > on this again :) Still think it is a great idea to have this as part of the > abbrev concept. > > On Tue, Sep 18, 2018 at 4:06 AM Stefan Monnier > wrote: > >> [ Note: your patch seems to have its TABs converted into SPCs, messing >> up indentation. I tried to fix those when quoting your code, but >> I may have mishandled some parts. ] >> > > I'll have a look at that at my next attempt. Are you saying there should > be TABs in Emacs' .el files? > > > +(defcustom abbrev-suggest nil >> > + "Non-nil means we should suggest abbrevs to the user. >> > +By enabling this option, if abbrev mode is enabled and if the >> > +user has typed some text that exists as an abbrev, suggest to the >> > +user to use the abbrev instead." >> > + :type 'boolean >> > + :group 'abbrev-mode >> > + :group 'convenience) >> >> Does it really need to be in `convenience` as well? >> > > Looking at the other defcustoms in the same file, probably not. I guess I > added that when I first wrote absug.el, thinking it was a convenience > thing. To be honest I think abbrevs are a convenience thing, but I will not > fight for tagging it as such... > > >> What would be the disadvantages/risks of enabling it by default? >> > > Of the top of my head, some people might be annoyed by the constant > "nagging" about using abbrevs. They could of course turn it off, if we > enable it by default. Since I personally like it I would not bother keeping > it on and perhaps new users would like it too. The other disadvantage could > be performance, but we have discussed that already and perhaps it will > never be a problem, not sure. > > By the way, here are the two top defcustoms in abbrev.el: > > (defcustom abbrev-file-name > (locate-user-emacs-file "abbrev_defs" ".abbrev_defs") > "Default name of file from which to read abbrevs." > :initialize 'custom-initialize-delay > :type 'file) > > (defcustom only-global-abbrevs nil > "Non-nil means user plans to use global abbrevs only. > This makes the commands that normally define mode-specific abbrevs > define global abbrevs instead." > :type 'boolean > :group 'abbrev-mode > :group 'convenience) > > The first does not have a group set and the second has convenience as > group as well. I have never thought about if this is a normal or not. Is it? > > >> > -(defvar abbrev-expand-function #'abbrev--default-expand >> > - "Function that `expand-abbrev' uses to perform abbrev expansion. >> > +(defvar abbrev-expand-function #'abbrev--try-expand-maybe-suggest >> > + "Function that `expand-abbrev' uses to perform abbrev expansion. >> [...] >> > +(defun abbrev--try-expand-maybe-suggest () >> > + "Try to expand abbrev, look for suggestions if enabled. >> > +This is set as `abbrev-expand-function'. If no abbrev expansion >> > +is found by `abbrev--default-expand', see if there is an abbrev >> > +defined for the word before point, and suggest it to the user." >> > + (or (abbrev--default-expand) >> > + (if abbrev-suggest >> > + (abbrev-suggest-maybe-suggest) >> > + nil))) >> >> If you put the code directly into abbrev.el, then I think you may as >> well modify expand-abbrev rather than hooking into abbrev-expand-function. >> > > Yes, I was looking for other places to put this, but it felt better to me > to have the suggestions come at the end of the existing code, when abbrevs > was tried and not found. Also, I was worried to mess with what is said here: > > (defun abbrev--default-expand () > "Default function to use for `abbrev-expand-function'. > This also respects the obsolete wrapper hook `abbrev-expand-functions'. > \(See `with-wrapper-hook' for details about wrapper hooks.) > Calls `abbrev-insert' to insert any expansion, **and returns what it > does.**" <==== > (subr--with-wrapper-hook-no-warnings abbrev-expand-functions () > (pcase-let ((`(,sym ,name ,wordstart ,wordend) (abbrev--before-point))) > ... > > That is mentioned in other places too, that it should return what was > inserted, if any. It did not feel right to mess with that, but perhaps it > is okay... Here is an attempt: > > (defun expand-abbrev () > "Expand the abbrev before point, if there is an abbrev there. > Effective when explicitly called even when `abbrev-mode' is nil. > Before doing anything else, runs `pre-abbrev-expand-hook'. > Calls the value of `abbrev-expand-function' with no argument to do > the work, and returns whatever it does. (That return value should > be the abbrev symbol if expansion occurred, else nil.)" > (interactive) > (run-hooks 'pre-abbrev-expand-hook) > (or (funcall abbrev-expand-function) > (abbrev-suggest-maybe-suggest))) > > Is something like that what you had in mind? > > >> The rest of the code defines abbrev--suggest-maybe-suggest rather than >> abbrev-suggest-maybe-suggest. >> > > Seems to be a search and replace-mistake :) > > >> > +(defcustom abbrev-suggest-hint-threshold 3 >> > + "Threshold for when to inform the user that there is an abbrev. >> > +The threshold is the number of characters that differs between >> > +the length of the abbrev and the length of the expansion. The >> > +thinking is that if the expansion is only one or a few characters >> > +longer than the abbrev, the benefit of informing the user is not >> > +that big. If you always want to be informed, set this value to >> > +`0' or less." >> > + :type 'number >> > + :group 'abbrev-mode >> > + :group 'convenience) >> >> Do we really need to add it to `convenience`? >> Just as above, I'd recommend you just remove both :group keywords and >> let the default behavior kick in (which will put it into `abbrev-mode`). >> > > Sure, I'm fine with that. > > >> > +(defun abbrev--suggest-get-active-abbrev-expansions () >> > + "Return a list of all the active abbrev expansions. >> > +Includes expansions from parent abbrev tables." >> > + (let (expansions) >> > + (dolist (table >> (abbrev--suggest-get-active-tables-including-parents)) >> > + (mapatoms (lambda (e) >> > + (let ((value (symbol-value (abbrev--symbol e >> table)))) >> > + (when value >> >> I think you'd be better served defining a dolist-style macro or >> a mapc-style function to loop over all abbrevs without creating an >> intermediate list. >> > > Sorry, I don't understand what you are suggesting, and why (performance, > or saving memory?). > > >> > + (setq expansions >> > + (cons (cons value (symbol-name e)) >> > + expansions))))) >> >> Aka (push (cons value (symbol-name e)) expansions)))) >> > > You learn something new every day :) I'll try that out. > > >> > +(defun abbrev--suggest-count-words (expansion) >> > + "Return the number of words in EXPANSION. >> > +Expansion is a string of one or more words." >> > + (length (split-string expansion " " t))) >> >> Why does your code pay attention to words? >> > > I'll think about that... Are you thinking that only characters matter? I > think my focus is primarily using abbrevs for sentences. I also have some > abbrevs that are only short forms for longer words, but I think I feel that > I get more benefit from sentence-like abbrevs... Again, I will think about > this and see if words are what I want to pay attention to. > > > +(defun abbrev--suggest-inform-user (expansion) >> > + "Display a message to the user about the existing abbrev. >> > +EXPANSION is a cons cell where the `car' is the expansion and the >> > +`cdr' is the abbrev." >> > + (run-with-idle-timer >> > + 1 nil >> > + `(lambda () >> > + (message "You can write `%s' using the abbrev `%s'." >> > + ,(car expansion) ,(cdr expansion)))) >> >> Please don't quote your lambdas. `abbrev.el` uses lexical-binding, so >> you can just write >> >> (run-with-idle-timer >> 1 nil >> (lambda () >> (message "You can write `%s' using the abbrev `%s'." >> (car expansion) (cdr expansion)))) >> > > Got it! > > >> > + (setq abbrev--suggest-saved-recommendations >> > + (cons expansion abbrev--suggest-saved-recommendations))) >> >> Aka (push expansion abbrev--suggest-saved-recommendations)) >> > > Changed it. > > >> BTW, won't this list contain repetitions? >> > > Yes it will, and it should. Or, at least I am doing that now to show the > user a count of how many times an expansion was typed manually. Have a look > at `abbrev--suggest-get-totals'. > > >> Maybe you should use `add-to-list` or `cl-pushnew` instead? >> >> One more thing: I think it'd be even better to put abbrev objects (which >> are implemented as symbols) in there, so you have easy accesss to >> a more info than just the expansion. >> > > I have access to the expansion as well as the abbrev, and this is now > added to the optional report shown to the user. Not sure what I need more... > > > +(defun abbrev--suggest-shortest-abbrev (new current) >> > + "Return the shortest abbrev. >> > +NEW and CURRENT are cons cells where the `car' is the expansion >> > +and the `cdr' is the abbrev." >> > + (if (not current) >> > + new >> > + (if (< (length (cdr new)) >> > + (length (cdr current))) >> > + new >> > + current))) >> >> Maybe rather than the shortest abbrev, it would be better to choose the >> abbrev with the largest difference between abbrev and expansion. >> > > It sounds like an interesting idea. I will try it out. > > >> > +(defun abbrev--suggest-maybe-suggest () >> > + "Suggest an abbrev to the user based on the word(s) before point. >> > +Uses `abbrev-suggest-hint-threshold' to find out if the user should be >> > +informed about the existing abbrev." >> > + (let (words abbrev-found word-count) >> > + (dolist (expansion (abbrev--suggest-get-active-abbrev-expansions)) >> > + (setq word-count (abbrev--suggest-count-words (car expansion)) >> > + words (abbrev--suggest-get-previous-words word-count)) >> >> Why not use something like >> >> (buffer-substring (- (point) (length (car expansion))) (point))? >> >> and when it's equal to (car expansion), then maybe check that there's >> a word boundary? >> > > I'll experiment with not thinking about words. I think I cannot just > compare things like that because of newlines, but that can be handled since > I did that in my "word-based code". > > >> > + (let ((case-fold-search t)) >> >> Some abbrevs are case-sensitive. >> > > Okay... Will think about that too. Not sure if I added that because I > actually needed it to solve a problem, or if I was proactive and did not > need to be... > > >> > + (when (and (> word-count 0) >> > + (string-match (car expansion) words) >> >> (car expansion) is a string, not a regular expression, so you'd need to >> regexp-quote it before passing it to string-match. >> > > Okay, thanks! > > >> > + (abbrev--suggest-above-threshold expansion)) >> > + (setq abbrev-found (abbrev--suggest-shortest-abbrev >> > + expansion abbrev-found))))) >> > + (when abbrev-found >> > + (abbrev--suggest-inform-user abbrev-found)))) >> >> I suspect that abbrev--suggest-above-threshold will eliminate a fairly >> large number of abbrevs for some users (e.g. those using abbrevs to fix >> recurring typos) and it's a cheap test which doesn't need to allocate >> any memory, so I'd recommend performing it before the calls to >> abbrev--suggest-count-words and abbrev--suggest-get-previous-words. >> > > Will try that. > > > +(defun abbrev-suggest-try-expand-maybe-suggest () >> > + "Try to expand abbrev, look for suggestions of no abbrev found. >> > +This is the main entry to the abbrev suggest mechanism. This is >> > +set as the default value for `abbrev-expand-function'. If no >> > +abbrev expansion is found by `abbrev--default-expand', see if >> > +there is an abbrev defined for the word before point, and suggest >> > +it to the user." >> > + (or (abbrev--default-expand) >> > + (if abbrev-suggest >> > + (abbrev-suggest-maybe-suggest)))) >> >> Looks identical to the earlier abbrev--try-expand-maybe-suggest >> > > Yes, it was a leftover. > > I'll try to cook a new version now. Let's see if it takes another two > years :) > > Thanks for all the suggestions and time spent on this. > > /Mathias > > --0000000000007ee5e905a56704f4 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Tried out the "why focus on words?" approach and= it seems to work well. Here is the gist of that change:

(defun abbrev-suggest-previous-chars (n)
=C2=A0 (buffer-substring (- (p= oint) n) (point)))

(defun abbrev-suggest-expansion-exist-at-point (e= xpansion)
=C2=A0 (string=3D (car expansion)
=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0(abbrev-suggest-previous-chars (length (car expansion)))))=

(defun abbrev-suggest ()
=C2=A0 "Suggest an abbrev to the u= ser based on the text before point.
Uses `abbrev-suggest-hint-threshold&= #39; to find out if the user should be
informed about the existing abbre= v."
=C2=A0 (let (abbrev-found)
=C2=A0 =C2=A0 (dolist (expansion = (abbrev-get-active-expansions))
=C2=A0 =C2=A0 =C2=A0 (when (and (abbrev-= suggest-above-threshold expansion)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0(abbrev-suggest-expansion-exist-at-point expansi= on))
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (setq abbrev-found (abbre= v-suggest-best-abbrev
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 expansion ab= brev-found))))
=C2=A0 =C2=A0 (when abbrev-found
=C2=A0 =C2=A0 =C2=A0 = (abbrev-suggest-inform-user abbrev-found))))

T= he new abbrev-suggest function above also have that little optimization you= suggested, checking the threshold early on to avoid looking around in the = buffer.

I have yet to make some of the other chang= es.

/Mathias


On Mon, May 11, 2= 020 at 11:37 PM Mathias Dahl <= mathias.dahl@gmail.com> wrote:
Hi Stefan,

Thanks for yo= ur feedback. Now, one and a half year later I started working on this again= :) Still think it is a great idea to have this as part of the abbrev conce= pt.

On Tue, Sep 18, 2018= at 4:06 AM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
[ Note: your patch seems to have = its TABs converted into SPCs, messing
=C2=A0 up indentation.=C2=A0 I tried to fix those when quoting your code, b= ut
=C2=A0 I may have mishandled some parts.=C2=A0 ]

<= /div>
I'll have a look at that at my=C2=A0next attempt. Are you say= ing there should be TABs in Emacs' .el files?

> +(defcustom abbrev-suggest nil
> +=C2=A0 "Non-nil means we should suggest abbrevs to the user.
> +By enabling this option, if abbrev mode is enabled and if the
> +user has typed some text that exists as an abbrev, suggest to the
> +user to use the abbrev instead."
> +=C2=A0 :type 'boolean
> +=C2=A0 :group 'abbrev-mode
> +=C2=A0 :group 'convenience)

Does it really need to be in `convenience` as well?
Looking at the other defcustoms in the same file, probably not= . I guess I added that when I first wrote absug.el, thinking it was a conve= nience thing. To be honest I think abbrevs are a convenience thing, but I w= ill not fight for tagging it as such...
=C2=A0
What would be the disadvantages/risks of enabling it by default?

Of the top of my head, some people might be annoy= ed by the constant "nagging" about using abbrevs. They could of c= ourse turn it off, if we enable it by default. Since I personally like it I= would not bother keeping it on and perhaps new users would like it too. Th= e other disadvantage could be performance, but we have discussed that alrea= dy and perhaps it will never be a problem, not sure.

By the way, here are the two top defcustoms in abbrev.el:

=
(defcustom abbrev-file-name
=C2=A0 (locate-user-e= macs-file "abbrev_defs" ".abbrev_defs")
=C2= =A0 "Default name of file from which to read abbrevs."
= =C2=A0 :initialize 'custom-initialize-delay
=C2=A0 :type '= ;file)

(defcustom only-global-abbrevs nil
=C2=A0 "Non-nil means user plans to use global abbrevs only.
This makes the commands that normally define mode-specific abbrevs
=
define global abbrevs instead."
=C2=A0 :type 'boole= an
=C2=A0 :group 'abbrev-mode
=C2=A0 :group 'co= nvenience)

The first does not have a group s= et and the second has convenience as group as well. I have never thought ab= out if this is a normal or not. Is it?
=C2=A0
> -(defvar abbrev-expand-function #'abbrev--default-expand
> -=C2=A0 "Function that `expand-abbrev' uses to perform abbrev= expansion.
> +(defvar abbrev-expand-function #'abbrev--try-expand-maybe-suggest=
> +=C2=A0 =C2=A0 "Function that `expand-abbrev' uses to perform= abbrev expansion.
[...]
> +(defun abbrev--try-expand-maybe-suggest ()
> +=C2=A0 "Try to expand abbrev, look for suggestions if enabled. > +This is set as `abbrev-expand-function'.=C2=A0 If no abbrev expan= sion
> +is found by `abbrev--default-expand', see if there is an abbrev > +defined for the word before point, and suggest it to the user."<= br> > +=C2=A0 (or (abbrev--default-expand)
> +=C2=A0 =C2=A0 =C2=A0 (if abbrev-suggest
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (abbrev-suggest-maybe-suggest)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 nil)))

If you put the code directly into abbrev.el, then I think you may as
well modify expand-abbrev rather than hooking into abbrev-expand-function.<= br>

Yes, I was looking for other places to = put this, but it felt better to me to have the suggestions come at the end = of the existing code, when abbrevs was tried and not found. Also, I was wor= ried to mess with what is said here:

(defun a= bbrev--default-expand ()
=C2=A0 "Default function to use for= `abbrev-expand-function'.
This also respects the obsolete wr= apper hook `abbrev-expand-functions'.
\(See `with-wrapper-hoo= k' for details about wrapper hooks.)
Calls `abbrev-insert'= ; to insert any expansion, *and returns what it does.*" <=3D= =3D=3D=3D
=C2=A0 (subr--with-wrapper-hook-no-warnings abbrev-expa= nd-functions ()
=C2=A0 =C2=A0 (pcase-let ((`(,sym ,name ,wordstar= t ,wordend) (abbrev--before-point)))
=C2=A0 =C2=A0 ...

That is mentioned in other places too, that it should = return what was inserted, if any. It did not feel right to mess with that, = but perhaps it is okay... Here is an attempt:

(defun expand-abbrev ()
=C2=A0 "Expand the abbrev before po= int, if there is an abbrev there.
Effective when explicitly calle= d even when `abbrev-mode' is nil.
Before doing anything else,= runs `pre-abbrev-expand-hook'.
Calls the value of `abbrev-ex= pand-function' with no argument to do
the work, and returns w= hatever it does.=C2=A0 (That return value should
be the abbrev sy= mbol if expansion occurred, else nil.)"
=C2=A0 (interactive)=
=C2=A0 (run-hooks 'pre-abbrev-expand-hook)
=C2=A0 = (or (funcall abbrev-expand-function)
=C2=A0 =C2=A0 =C2=A0 (abbrev= -suggest-maybe-suggest)))

Is something like = that what you had in mind?
=C2=A0
The rest of the code defines abbrev--suggest-maybe-suggest rather than
abbrev-suggest-maybe-suggest.

Seems to = be a search and replace-mistake :)
=C2=A0
> +(defcustom abbrev-suggest-hint-threshold 3
> +=C2=A0 "Threshold for when to inform the user that there is an a= bbrev.
> +The threshold is the number of characters that differs between
> +the length of the abbrev and the length of the expansion.=C2=A0 The > +thinking is that if the expansion is only one or a few characters
> +longer than the abbrev, the benefit of informing the user is not
> +that big.=C2=A0 If you always want to be informed, set this value to<= br> > +`0' or less."
> +=C2=A0 :type 'number
> +=C2=A0 :group 'abbrev-mode
> +=C2=A0 :group 'convenience)

Do we really need to add it to `convenience`?
Just as above, I'd recommend you just remove both :group keywords and let the default behavior kick in (which will put it into `abbrev-mode`).

Sure, I'm fine with that.
= =C2=A0
> +(defun abbrev--suggest-get-active-abbrev-expansions ()
> +=C2=A0 "Return a list of all the active abbrev expansions.
> +Includes expansions from parent abbrev tables."
> +=C2=A0 (let (expansions)
> +=C2=A0 =C2=A0 (dolist (table (abbrev--suggest-get-active-tables-inclu= ding-parents))
> +=C2=A0 =C2=A0 =C2=A0 (mapatoms (lambda (e)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (let (= (value (symbol-value (abbrev--symbol e table))))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= (when value

I think you'd be better served defining a dolist-style macro or
a mapc-style function to loop over all abbrevs without creating an
intermediate list.

Sorry, I don't u= nderstand what you are suggesting, and why (performance, or saving memory?)= .
=C2=A0
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 (setq expansions
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 (cons (cons value (symbol-name e))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 expansions)))))

Aka=C2=A0 =C2=A0(push (cons value (symbol-name e)) expansions))))

You learn something new every day :) I'll tr= y that out.
=C2=A0
> +(defun abbrev--suggest-count-words (expansion)
> +=C2=A0 =C2=A0 "Return the number of words in EXPANSION.
> +Expansion is a string of one or more words."
> +=C2=A0 =C2=A0 (length (split-string expansion " " t)))

Why does your code pay attention to words?

<= div>I'll think about that... Are you thinking that only characters matt= er? I think my focus is primarily using abbrevs for sentences. I also have = some abbrevs that are only short forms for longer words, but I think I feel= that I get more benefit from sentence-like abbrevs... Again, I will think = about this and see if words are what I want to pay attention to.
=
> +(defun abbrev--suggest-inform-user (expansion)
> +=C2=A0 "Display a message to the user about the existing abbrev.=
> +EXPANSION is a cons cell where the `car' is the expansion and the=
> +`cdr' is the abbrev."
> +=C2=A0 (run-with-idle-timer
> +=C2=A0 =C2=A01 nil
> +=C2=A0 =C2=A0`(lambda ()
> +=C2=A0 =C2=A0 =C2=A0 (message "You can write `%s' using the = abbrev `%s'."
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0,(car expansio= n) ,(cdr expansion))))

Please don't quote your lambdas.=C2=A0 `abbrev.el` uses lexical-binding= , so
you can just write

=C2=A0 =C2=A0 =C2=A0 =C2=A0(run-with-idle-timer
=C2=A0 =C2=A0 =C2=A0 =C2=A0 1 nil
=C2=A0 =C2=A0 =C2=A0 =C2=A0 (lambda ()
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (message "You can write `%s' us= ing the abbrev `%s'."
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(car e= xpansion) (cdr expansion))))

Got it!
=C2=A0
> +=C2=A0 =C2=A0 (setq abbrev--suggest-saved-recommendations
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (cons expansion abbrev--suggest-sa= ved-recommendations)))

Aka=C2=A0 (push expansion abbrev--suggest-saved-recommendations))

Changed it.=C2=A0
=C2=A0
BTW, won't this list contain repetitions?

Yes it will, and it should. Or, at least I am doing that now to show= the user a count of how many times an expansion was typed manually. Have a= look at `abbrev--suggest-get-totals'.
=C2=A0
Maybe you should use `add-to-list` or `cl-pushnew` instead?

One more thing: I think it'd be even better to put abbrev objects (whic= h
are implemented as symbols) in there, so you have easy accesss to
a more info than just the expansion.

I = have access to the expansion as well as the abbrev, and this is now added t= o the optional report shown to the user. Not sure what I need more...
=

> +(defun abbrev--suggest-shortest-abbrev (new current)
> +=C2=A0 =C2=A0 "Return the shortest abbrev.
> +NEW and CURRENT are cons cells where the `car' is the expansion > +and the `cdr' is the abbrev."
> +=C2=A0 =C2=A0 (if (not current)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 new
> +=C2=A0 =C2=A0 =C2=A0 (if (< (length (cdr new))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(length (cdr current)= ))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 new
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 current)))

Maybe rather than the shortest abbrev, it would be better to choose the
abbrev with the largest difference between abbrev and expansion.

It sounds like an interesting idea. I will try it= out.
=C2=A0
> +(defun abbrev--suggest-maybe-suggest ()
> +=C2=A0 "Suggest an abbrev to the user based on the word(s) befor= e point.
> +Uses `abbrev-suggest-hint-threshold' to find out if the user shou= ld be
> +informed about the existing abbrev."
> +=C2=A0 (let (words abbrev-found word-count)
> +=C2=A0 =C2=A0 (dolist (expansion (abbrev--suggest-get-active-abbrev-e= xpansions))
> +=C2=A0 =C2=A0 =C2=A0 (setq word-count (abbrev--suggest-count-words (c= ar expansion))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 words (abbrev--suggest-get-= previous-words word-count))

Why not use something like

=C2=A0 =C2=A0 (buffer-substring (- (point) (length (car expansion))) (point= ))?

and when it's equal to (car expansion), then maybe check that there'= ;s
a word boundary?

I'll experiment wi= th not thinking about words. I think I cannot just compare things like that= because of newlines, but that can be handled since I did that in my "= word-based code".
=C2=A0
> +=C2=A0 =C2=A0 =C2=A0 (let ((case-fold-search t))

Some abbrevs are case-sensitive.

Okay..= . Will think about that too. Not sure if I added that because I actually ne= eded it to solve a problem, or if I was proactive and did not need to be...=
=C2=A0
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 (when (and (> word-count 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= (string-match (car expansion) words)

(car expansion) is a string, not a regular expression, so you'd need to=
regexp-quote it before passing it to string-match.
Okay, thanks!
=C2=A0
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= (abbrev--suggest-above-threshold expansion))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (setq abbrev-found (abbrev--sugges= t-shortest-abbrev
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 expansion abbrev-found)))))
> +=C2=A0 =C2=A0 (when abbrev-found
> +=C2=A0 =C2=A0 =C2=A0 (abbrev--suggest-inform-user abbrev-found))))
I suspect that abbrev--suggest-above-threshold will eliminate a fairly
large number of abbrevs for some users (e.g. those using abbrevs to fix
recurring typos) and it's a cheap test which doesn't need to alloca= te
any memory, so I'd recommend performing it before the calls to
abbrev--suggest-count-words and abbrev--suggest-get-previous-words.

Will try that.

> +(defun abbrev-suggest-try-expand-maybe-suggest ()
> +=C2=A0 "Try to expand abbrev, look for suggestions of no abbrev = found.
> +This is the main entry to the abbrev suggest mechanism.=C2=A0 This is=
> +set as the default value for `abbrev-expand-function'.=C2=A0 If n= o
> +abbrev expansion is found by `abbrev--default-expand', see if
> +there is an abbrev defined for the word before point, and suggest
> +it to the user."
> +=C2=A0 (or (abbrev--default-expand)
> +=C2=A0 =C2=A0 =C2=A0 (if abbrev-suggest
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (abbrev-suggest-maybe-suggest))))<= br>
Looks identical to the earlier abbrev--try-expand-maybe-suggest

Yes, it was a leftover.

I= 'll try to cook a new version now. Let's see if it takes another tw= o years :)

Thanks for all the suggestions and time= spent on this.

/Mathias

--0000000000007ee5e905a56704f4--