Abbrev suggestions - feedback appreciated

Abbrev suggestions - feedback appreciated

[Mathias Dahl]

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

Hi emacs-devel!

I want to have feedback on a small addition I have made to Emacs abbrev
functionality. First some background...

Some days back I was typing away in Emacs and after typing some hard to
spell word I realized that I probably had an abbrev defined for that
word. I checked by abbrev definitions and, yes, it was there. It turns
out I had many defined abbrevs that I had forgotten about, so I was
typing more than I needed. This got me thinking that it would be nice if
Emacs could tell me when I have typed a word that exists as an abbrev
expansion. After some research in abbrev.el I finally got something to
work, and you can find it below.

I would like to have people's comments on the idea, and know if this
might allredy exist (I tried to find something similar but couldn't), and
also if there are any performance problems or other in my code. The
internals of abbrevs are dark and mysterious to me, so I might miss some
crucial things. But, the code seems to do what I want it to.

Also, if people like this idea, perhaps it could be added to Emacs
proper. Surely it can live on its life as a separate library on Melpa,
Elpa or other place, but the code is quite short and this feels like
something that could be a simple option/toggle in Emacs standard abbrev
functionality. I have papers in place if this is something people would
like.

Without further ado, here is the current code I have:

;;; absug.el --- Suggest an abbrev based on the word before point

;; Copyright stuff to be added later...

;; Author: Mathias Dahl (mathias.dahl@gmail.com)

;;; Commentary:

;; This library helps the user remember defined abbrevs by suggesting
;; them after having typed an abbrev expansion manually.

;; For example, if the user has defined the abbrev `doc' with the
;; expansion `document', if the user manually types `document' with
;; `abbrev-mode' active, a message will be presented suggesting to use
;; the defined abbrev `doc' instead.

;; To install, load or require this file/library and also add the
;; following code to your .emacs or init.el file:

;; (absug-enable)

;; (It is also a command you can call interactively)

;; You can interactively turn off abbrev suggestion by calling the
;; command `absug-disable'.

;;; Code:

(defun absug-word-before-point ()
  "Return the word before point."
  (let ((lim (point))
        start end)
    (backward-word 1)
    (setq start (point))
    (forward-word 1)
    (setq end (min (point) lim))
    (buffer-substring-no-properties start end)))

(defun absug-get-active-tables-including-parents ()
  "Return a list of all active abbrev tables, including parent tables."
  (let* ((tables (abbrev--active-tables))
         (all tables))
    (dolist (table tables)
      (setq all (append (abbrev-table-get table :parents) all)))
    all))

(defun absug-get-active-abbrev-expansions ()
  "Return a list of all the active abbrev expansions.
Includes expansions from parent abbrev tables."
  (let (expansions)
    (dolist (table (absug-get-active-tables-including-parents))
      (mapatoms (lambda (e)
                  (let ((value (symbol-value (abbrev--symbol e table))))
                    (if value
                        (setq expansions
                              (cons (cons value (symbol-name e))
                                    expansions)))))
                table))
    expansions))

(defun absug-maybe-suggest ()
  "Suggest an abbrev to the user based on the word before point."
  (let* ((word (absug-word-before-point))
         (expansions (absug-get-active-abbrev-expansions))
         (abbrev (assoc word expansions)))
    (if abbrev
        (message "Abbrev suggestion: The word `%s' has the abbrev
`%s' defined" (car abbrev) (cdr abbrev)))))

(defun absug-default-expand ()
  "My version to use 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."
  (unless (abbrev--default-expand)
    (absug-maybe-suggest)))

(defun absug-disable ()
  "Disable abbrev suggestions"
  (interactive)
  (setq abbrev-expand-function #'abbrev--default-expand))

(defun absug-enable ()
  "Enable abbrev suggestions"
  (interactive)
  (setq abbrev-expand-function #'absug-default-expand))

(provide 'absug)

;;; absug.el ends here


Thanks!

/Mathias

[-- Attachment #2: Type: text/html, Size: 6082 bytes --]

Re: Abbrev suggestions - feedback appreciated

[Eli Zaretskii]

> From: Mathias Dahl <mathias.dahl@gmail.com>
> Date: Sat, 16 Sep 2017 09:51:53 +0200
> 
> Some days back I was typing away in Emacs and after typing some hard to
> spell word I realized that I probably had an abbrev defined for that
> word. I checked by abbrev definitions and, yes, it was there. It turns
> out I had many defined abbrevs that I had forgotten about, so I was
> typing more than I needed. This got me thinking that it would be nice if
> Emacs could tell me when I have typed a word that exists as an abbrev
> expansion. After some research in abbrev.el I finally got something to
> work, and you can find it below.
> 
> I would like to have people's comments on the idea, and know if this
> might allredy exist (I tried to find something similar but couldn't), and
> also if there are any performance problems or other in my code. The
> internals of abbrevs are dark and mysterious to me, so I might miss some
> crucial things. But, the code seems to do what I want it to.
> 
> Also, if people like this idea, perhaps it could be added to Emacs
> proper. Surely it can live on its life as a separate library on Melpa,
> Elpa or other place, but the code is quite short and this feels like
> something that could be a simple option/toggle in Emacs standard abbrev
> functionality. I have papers in place if this is something people would
> like.

Why not add this as an optional feature to abbrev.el, conditional on
some defcustom?

> (defun absug-maybe-suggest ()
>   "Suggest an abbrev to the user based on the word before point."
>   (let* ((word (absug-word-before-point))
>          (expansions (absug-get-active-abbrev-expansions))
>          (abbrev (assoc word expansions)))
>     (if abbrev
>         (message "Abbrev suggestion: The word `%s' has the abbrev
> `%s' defined" (car abbrev) (cdr abbrev)))))

I'd suggest to make the message text shorter, to make the probability
of its becoming longer than the echo-area width, which would resize
the mini-window and cause an annoying redisplay.  I'd just drop the
part before the colon, as the rest already says there is an abbrev.

Thanks.

Re: Abbrev suggestions - feedback appreciated

[Mathias Dahl]

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

Hi Eli, thanks for your feedback on this!

Why not add this as an optional feature to abbrev.el, conditional on
> some defcustom?
>

I should have mentioned that that is what I had in mind, actually,
if this would ever be included in Emacs.

>         (message "Abbrev suggestion: The word `%s' has the abbrev
> > `%s' defined" (car abbrev) (cdr abbrev)))))
>
> I'd suggest to make the message text shorter, to make the probability
> of its becoming longer than the echo-area width, which would resize
> the mini-window and cause an annoying redisplay.  I'd just drop the
> part before the colon, as the rest already says there is an abbrev.
>

I changed that to:

        (message "You can write `%s' using the abbrev `%s'."
                 (car abbrev) (cdr abbrev)))))

I tried to make it similar to how we "hint" users about keybindings to
commands.

[-- Attachment #2: Type: text/html, Size: 1546 bytes --]

Re: Abbrev suggestions - feedback appreciated

[Stefan Monnier]

> (defun absug-maybe-suggest ()
>   "Suggest an abbrev to the user based on the word before point."
>   (let* ((word (absug-word-before-point))
>          (expansions (absug-get-active-abbrev-expansions))
>          (abbrev (assoc word expansions)))
>     (if abbrev
>         (message "Abbrev suggestion: The word `%s' has the abbrev
> `%s' defined" (car abbrev) (cdr abbrev)))))

I like the feature.  Here are some comments:

- The expansion is not necessarily a "word".
- The `expansions` list is built only to then pass it to `assoc`.
  You could avoid constructing the list by passing `word` to
  absug-get-active-abbrev-expansions and have it check equality as it goes.


        Stefan

Re: Abbrev suggestions - feedback appreciated

[Mathias Dahl]

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

Hi Stefan, thanks for your comments.

I like the feature.  Here are some comments:
>
> - The expansion is not necessarily a "word".
>

True. I thought about that and concluded that it would be a lot harder
to look for more than one words, matching a certain expansion. It will
be especially tricky, I think, if the words the user types and which
match an extension, spans more than one line. Of course not impossible but
I thought I would start with the easy and probably quite common use
case.

- The `expansions` list is built only to then pass it to `assoc`.
>   You could avoid constructing the list by passing `word` to
>   absug-get-active-abbrev-expansions and have it check equality as it
> goes.
>

That's a good optimization, thanks! At least as long as I am not trying
to tackle multi-word expansions. In that case, I don't know on
beforehand how many words to match. Again, surely possible to
solve... One way might be to save not one but, say, ten previous words,
and send that in to the function that will look for expansions matching
that. It would of course break on eleven words and above...

Any clever ideas? :) I might surely be able to do this, but I am also
concerned
about performance here.

[-- Attachment #2: Type: text/html, Size: 1854 bytes --]

Re: Abbrev suggestions - feedback appreciated

[Mathias Dahl]

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

>
> True. I thought about that and concluded that it would be a lot harder
> to look for more than one words, matching a certain expansion. It will
> be especially tricky, I think, if the words the user types and which
> match an extension, spans more than one line. Of course not impossible but
> I thought I would start with the easy and probably quite common use
> case.
>

Okay, so... Turns out it was not that hard... :)

With two helper functions I could rewrite the suggest function quite nicely:

(defun asug-count-words (expansion)
  (length (split-string expansion " " t)))

(defun asug-get-previous-words (n)
  (let ((end (point))
        words)
    (save-excursion
      (backward-word n)
      (replace-regexp-in-string
       "[\n\r]" " "
       (buffer-substring-no-properties (point) end)))))

(defun absug-maybe-suggest ()
  "Suggest an abbrev to the user based on the word(s) before point."
  (let ((expansions (absug-get-active-abbrev-expansions))
         words found expansion word-count)
    (while (and expansions
                (not found))
      (setq expansion (pop expansions))
      (when expansion
        (setq word-count (asug-count-words (car expansion))
              words (asug-get-previous-words word-count))
        (when (and (> word-count 0)
                   (string= words (car expansion)))
          (setq found t)
          (message "You can write `%s' using the abbrev `%s'."
                   (car expansion) (cdr expansion)))))))

It works for abbrevs that has an expansion with multiple words and
it also works across lines.

Now I will clean this up a bit and checkdoc it...

[-- Attachment #2: Type: text/html, Size: 2573 bytes --]

Re: Abbrev suggestions - feedback appreciated

[Stefan Monnier]

>> You could avoid constructing the list by passing `word` to
>> absug-get-active-abbrev-expansions and have it check equality as it
>> goes.
> That's a good optimization, thanks! At least as long as I am not trying
> to tackle multi-word expansions. In that case, I don't know on
> beforehand how many words to match. Again, surely possible to
> solve... One way might be to save not one but, say, ten previous words,
> and send that in to the function that will look for expansions matching
> that. It would of course break on eleven words and above...
> Any clever ideas?

Actually, my "clever" idea was that you don't read any word at all: just
go though the existing abbreviations, and check for each one of the if
the text before point matches the expansion.

Admittedly, I don't think we currently have a fast implementation of the
equivalent of (looking-back (regexp-quote expansion)), so I don't know
how well it will perform.


        Stefan

Re: Abbrev suggestions - feedback appreciated

[Mathias Dahl]

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

>
> Actually, my "clever" idea was that you don't read any word at all: just
> go though the existing abbreviations, and check for each one of the if
> the text before point matches the expansion.
>
> Admittedly, I don't think we currently have a fast implementation of the
> equivalent of (looking-back (regexp-quote expansion)), so I don't know
> how well it will perform.
>

My simple function `asug-get-previous-words' function performs quite well,
it seems. I could not get `looking-back' to work across lines. It was very
quick
though, when words was on the same line.

Also, collecting all active abbrev expansions was also very fast (1000 runs
takes 0.1 on my not very hot PC) so I did not bother with that
optimization. But
perhaps I will experiment a bit with that too.

All in all this seems to turn out quite nicely. I will try to integrate it
properly in
abbrev.el and add some option there to toggle this feature on.

Thanks for the pointers!

[-- Attachment #2: Type: text/html, Size: 1428 bytes --]

Re: Abbrev suggestions - feedback appreciated

[Kaushal Modi]

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

Hi Mathias,

This will be a pretty cool feature.

I see just one caveat. I don't use abbrev for "auto-correct", but I know
that many folks do[1].

So people could have abbrev entries like "eamcs" that "expand" to "emacs".
So with this feature, the user will be suggested to type "eamcs" each time
they correctly type "emacs"?

If so, can the suggestion be enabled only when the abbreviation is shorter
than the expansion by N number of characters, where that N can be set using
a defcustom? (I would default that to 2 or 3).

[1]:
http://endlessparentheses.com/ispell-and-abbrev-the-perfect-auto-correct.html
-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 1031 bytes --]

Re: Abbrev suggestions - feedback appreciated

[Mathias Dahl]

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

>
> Hi Mathias,


Hi, and thanks for your comments!

This will be a pretty cool feature.
>

I thought so too :)


> I see just one caveat. I don't use abbrev for "auto-correct", but I know
> that many folks do[1].
>

Neither do I, mostly (I have a few "corrections" as well, but mostly I used
it to type long or "hard" words using fewer characters.)

So people could have abbrev entries like "eamcs" that "expand" to "emacs".
> So with this feature, the user will be suggested to type "eamcs" each time
> they correctly type "emacs"?
>

Yes, or the last defined abbrev for "emacs" anyway (if there are more than
one).


> If so, can the suggestion be enabled only when the abbreviation is shorter
> than the expansion by N number of characters, where that N can be set using
> a defcustom? (I would default that to 2 or 3).
>

It would be possible to have an option like that, or similar. The option
could keep the diff in number of characters or the diff in percentage, or
whatever clever thing we want. I will try to implement it.

Also, someone mailed me about the scenario where there are more than one
abbrev for the same expansion. Right now, you will get a suggestion to use
the most recently defined abbrev. I wonder how useful it would be to see
the full list of abbrevs that might expand to a certain expansion... For
me, the primary idea is to remind me that I have at least one abbrev
defined for a certain expansion. If there are more than one, the reminder
is enough, at least for me. I would probably have a look (M-x edit-abbrevs
RET) to see what I have defined.

Does anyone else here think it would be useful to list all abbrevs that
expand to a certain expansion? Surely it would be doable, but it would also
be slower.

/Mathias

[-- Attachment #2: Type: text/html, Size: 2838 bytes --]

Re: Abbrev suggestions - feedback appreciated

[Stefan Monnier]

> Also, someone mailed me about the scenario where there are more than one
> abbrev for the same expansion. Right now, you will get a suggestion to use
> the most recently defined abbrev. I wonder how useful it would be to see
> the full list of abbrevs that might expand to a certain expansion...

I think it's enough to make sure we show the /shortest/ abbrev.


        Stefan

Re: Abbrev suggestions - feedback appreciated

[Mathias Dahl]

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

>
> > Also, someone mailed me about the scenario where there are more than one
> > abbrev for the same expansion. Right now, you will get a suggestion to
> use
> > the most recently defined abbrev. I wonder how useful it would be to see
> > the full list of abbrevs that might expand to a certain expansion...
>
> I think it's enough to make sure we show the /shortest/ abbrev.
>

I was able to implement that quite easily. The only worry I have is that
it might be slow, or I know it is slow-er at least since I need to go
through all active abbrev expansions to see if there is any shorter
one. I used benchmark-run 1000 times on one of the core functions though
and it takes less than 0.1 second on my not very new PC, so perhaps I
should not worry. I have very few abbrevs though... Should I be worried
about performance? We don't want the user's typing to be affected by this.

[-- Attachment #2: Type: text/html, Size: 1289 bytes --]

Re: Abbrev suggestions - feedback appreciated

[Mathias Dahl]

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

Hi again,

Please find below the latest version. It has the option requested
earlier and I also included the command `absug-report' that the user can
use to see what expansions that has been typed manually, and how many
times.

I'm starting to be quite happy with this now. This is still a separate
package, but if there is still interest I can try to merge the code into
abbrev.el (if so, I have some renaming to do...) and make it into an
option there, as suggested by Eli earlier. Then I can post a patch here.

Comments?

/Mathias

;;; absug.el --- Suggest an abbrev based on the word(s) before point

;; Copyright stuff to be added later...

;; Author: Mathias Dahl (mathias.dahl@gmail.com)

;;; Commentary:

;; This library helps the user remember defined abbrevs by suggesting
;; them after having typed an abbrev expansion manually.

;; For example, if the user has defined the abbrev `doc' with the
;; expansion `document', if the user manually types `document' with
;; `abbrev-mode' active, a message will be presented suggesting to use
;; the defined abbrev `doc' instead.

;; To install, load or require this file/library and also add the
;; following code to your .emacs or init.el file:

;; (absug-enable)

;; (It is also a command you can call interactively)

;; You can interactively turn off abbrev suggestions by calling the
;; command `absug-disable'.

;;; Ideas:

;; - Think about different ways to notify the user
;; - Display a report of statistics of abbrevs the user have forgotten

;;; Code:

(defcustom absug-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)

(defun absug-get-active-tables-including-parents ()
  "Return a list of all active abbrev tables, including parent tables."
  (let* ((tables (abbrev--active-tables))
         (all tables))
    (dolist (table tables)
      (setq all (append (abbrev-table-get table :parents) all)))
    all))

(defun absug-get-active-abbrev-expansions ()
  "Return a list of all the active abbrev expansions.
Includes expansions from parent abbrev tables."
  (let (expansions)
    (dolist (table (absug-get-active-tables-including-parents))
      (mapatoms (lambda (e)
                  (let ((value (symbol-value (abbrev--symbol e table))))
                    (when value
                      (setq expansions
                            (cons (cons value (symbol-name e))
                                  expansions)))))
                table))
    expansions))

(defun absug-count-words (expansion)
  "Return the number of words in EXPANSION.
Expansion is a string of one or more words."
  (length (split-string expansion " " t)))

(defun absug-get-previous-words (n)
  "Return the previous N words, spaces included.
Changes newlines into spaces."
  (let ((end (point))
        words)
    (save-excursion
      (backward-word n)
      (replace-regexp-in-string
       "\\s " " "
       (buffer-substring-no-properties (point) end)))))

(defun absug-above-threshold (expansion)
  "Return t if we are above the threshold.
EXPANSION is a cons cell where the car is the expansion and the
cdr is the abbrev."
  (>= (- (length (car expansion))
         (length (cdr expansion)))
      absug-hint-threshold))

(defvar absug-saved-recommendations nil
  "Keeps a list of expansions that have abbrevs defined.
The user can show this list by calling
`absug-show-recommendations'.")

(defun absug-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
   2 nil
   `(lambda ()
      (message "You can write `%s' using the abbrev `%s'."
               ,(car expansion) ,(cdr expansion))))
  (setq absug-saved-recommendations
        (cons expansion absug-saved-recommendations)))

(defun absug-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)))

(defun absug-maybe-suggest ()
  "Suggest an abbrev to the user based on the word(s) before point.
Uses `absug-hint-threshold' to find out if the user should be
informed about the existing abbrev."
  (let (words abbrev-found expansion word-count)
    (dolist (expansion (absug-get-active-abbrev-expansions))
      (setq word-count (absug-count-words (car expansion))
            words (absug-get-previous-words word-count))
      (let ((case-fold t))
        (when (and (> word-count 0)
                   (string-match (car expansion) words)
                   (absug-above-threshold expansion))
          (setq abbrev-found (absug-shortest-abbrev
                              expansion abbrev-found)))))
    (when abbrev-found
      (absug-inform-user abbrev-found))))

(defun absug-default-expand ()
  "My version to use 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."
  (unless (abbrev--default-expand)
    (absug-maybe-suggest)))

(defun absug-get-totals ()
  "Return a list of all expansions and their usage.
Each expansion is a cons cell where the `car' is the expansion
and the `cdr' is the number of times the expansion has been
typed."
  (let (total cell)
    (dolist (expansion absug-saved-recommendations)
      (if (not (assoc (car expansion) total))
          (add-to-list 'total (cons (car expansion) 1))
        (setq cell (assoc (car expansion) total))
        (setcdr cell (1+ (cdr cell)))))
    total))

(defun absug-report ()
  "Show the user a report of abbrevs he could have used."
  (interactive)
  (let ((totals (absug-get-totals))
        (buf (get-buffer-create "*absug*")))
    (set-buffer buf)
    (erase-buffer)
    (insert "** Abbrev expansion usage **

Below is a list of expansions for which abbrevs are defined, and
the number of times the expansion was typed manually. To display
and edit all abbrevs, type `M-x edit-abbrevs RET'\n\n")
    (dolist (expansion totals)
      (insert (format " %s: %d\n" (car expansion) (cdr expansion))))
    (display-buffer buf)))

(defun absug-disable ()
  "Disable abbrev suggestions."
  (interactive)
  (setq abbrev-expand-function #'abbrev--default-expand))

(defun absug-enable ()
  "Enable abbrev suggestions."
  (interactive)
  (setq abbrev-expand-function #'absug-default-expand))

(provide 'absug)

;;; absug.el ends here

[-- Attachment #2: Type: text/html, Size: 13945 bytes --]

Re: Abbrev suggestions - feedback appreciated

[Ian Dunn]

>>>>> "Mathias" == Mathias Dahl <mathias.dahl@gmail.com> writes:

    Mathias> Hi again, Please find below the latest version. It has the
    Mathias> option requested earlier and I also included the command
    Mathias> `absug-report' that the user can use to see what expansions
    Mathias> that has been typed manually, and how many times.

    Mathias> I'm starting to be quite happy with this now. This is still
    Mathias> a separate package, but if there is still interest I can
    Mathias> try to merge the code into abbrev.el (if so, I have some
    Mathias> renaming to do...) and make it into an option there, as
    Mathias> suggested by Eli earlier. Then I can post a patch here.

    Mathias> Comments?

Two.

1. This looks amazing.  As an avid user of abbrevs, having something remind me when I've created a new abbrev and no longer need to type something out that was bugging me enough to make an abbrev will be great.

2. Having this be controlled by some property of abbrev tables and/or abbrevs themselves would be ideal, similar to the case-fixed property.  That way the expansions of auto-correct[1] and captain[2] abbrevs won't nag people all the time.

[1] https://elpa.gnu.org/packages/auto-correct.html
[2] https://elpa.gnu.org/packages/captain.html

-- 
Ian Dunn

Re: Abbrev suggestions - feedback appreciated

[Stefan Monnier]

> 2. Having this be controlled by some property of abbrev tables and/or
> abbrevs themselves would be ideal, similar to the case-fixed property.
> That way the expansions of auto-correct[1] and captain[2] abbrevs won't nag
> people all the time.
>
> [1] https://elpa.gnu.org/packages/auto-correct.html
> [2] https://elpa.gnu.org/packages/captain.html

For Captain, the abbrev and its "expansion" should have the same length,
so absug-hint-threshold should already skip them.

For auto-correct, it might be the case that the wrong spelling is
shorter by absug-hint-threshold, but then you could also argue that if
you often misspell a word and it gets auto-corrected and the wrong
spelling is shorter, you might take it as a feature and consciously use
the shorter/wrong spelling and rely on the abbrev to auto correct it.


        Stefan

Re: Abbrev suggestions - feedback appreciated

[Ian Dunn]

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

>>>>> "Stefan" == Stefan Monnier <monnier@iro.umontreal.ca> writes:

    >> 2. Having this be controlled by some property of abbrev tables
    >> and/or abbrevs themselves would be ideal, similar to the
    >> case-fixed property.  That way the expansions of auto-correct[1]
    >> and captain[2] abbrevs won't nag people all the time.
    >> 
    >> [1] https://elpa.gnu.org/packages/auto-correct.html [2]
    >> https://elpa.gnu.org/packages/captain.html

    Stefan> For Captain, the abbrev and its "expansion" should have the
    Stefan> same length, so absug-hint-threshold should already skip
    Stefan> them.

    Stefan> For auto-correct, it might be the case that the wrong
    Stefan> spelling is shorter by absug-hint-threshold, but then you
    Stefan> could also argue that if you often misspell a word and it
    Stefan> gets auto-corrected and the wrong spelling is shorter, you
    Stefan> might take it as a feature and consciously use the
    Stefan> shorter/wrong spelling and rely on the abbrev to auto
    Stefan> correct it.

I stand corrected.  My only other suggestion would be to use add-function on abbrev-expand-function instead of setting it directly to avoid overwriting other packages that may need it.  Something like the following:


[-- Attachment #2: Type: application/emacs-lisp, Size: 74 bytes --]

[-- Attachment #3: Type: text/plain, Size: 14 bytes --]


-- 
Ian Dunn

Re: Abbrev suggestions - feedback appreciated

[Mathias Dahl]

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

Hi all,

It's now more or less exactly a year ago since I last posted about this. I
finally now had a shot at integrating the abbrev suggest functionality into
abbrev.el itself. It worked out kind of okay, from what I can see and you
will find the patch below.

The code is more or less like it was a year ago, with the difference that I
tried to fit it into where I think it fits as naturally as possible in
abbrev.el, and I of course renamed things.

The highlights are:

- Add `abbrev-suggest' - A defcustom of type boolean that activates the
abbrev suggest functionality.
- Changing the defvar `abbrev-expand-function' to
#'abbrev--try-expand-maybe-suggest
- `abbrev--try-expand-maybe-suggest' first tries to expand the normal way.
If no expansion was done, and if abbrev suggest is enabled, try to suggest
an abbrev to the user.

Here's the complete diff:

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
diff --git a/lisp/abbrev.el b/lisp/abbrev.el
index cddce8f..bbae083 100644
--- a/lisp/abbrev.el
+++ b/lisp/abbrev.el
@@ -827,10 +827,171 @@ abbrev-expand-functions
   "Wrapper hook around `abbrev--default-expand'.")
 (make-obsolete-variable 'abbrev-expand-functions 'abbrev-expand-function
"24.4")

-(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.
 Takes no argument and should return the abbrev symbol if expansion took
place.")

+(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)
+
+(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)))
+
+(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)
+
+(defun abbrev--suggest-get-active-tables-including-parents ()
+  "Return a list of all active abbrev tables, including parent tables."
+  (let* ((tables (abbrev--active-tables))
+ (all tables))
+    (dolist (table tables)
+      (setq all (append (abbrev-table-get table :parents) all)))
+    all))
+
+(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
+ (setq expansions
+       (cons (cons value (symbol-name e))
+     expansions)))))
+   table))
+      expansions))
+
+(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)))
+
+(defun abbrev--suggest-get-previous-words (n)
+    "Return the previous N words, spaces included.
+Changes newlines into spaces."
+    (let ((end (point)))
+      (save-excursion
+ (backward-word n)
+ (replace-regexp-in-string
+ "\\s " " "
+ (buffer-substring-no-properties (point) end)))))
+
+(defun abbrev--suggest-above-threshold (expansion)
+    "Return t if we are above the threshold.
+EXPANSION is a cons cell where the car is the expansion and the
+cdr is the abbrev."
+    (>= (- (length (car expansion))
+    (length (cdr expansion)))
+ abbrev-suggest-hint-threshold))
+
+(defvar abbrev--suggest-saved-recommendations nil
+    "Keeps a list of expansions that have abbrevs defined.
+The user can show this list by calling
+`abbrev-suggest-show-report'.")
+
+(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))))
+    (setq abbrev--suggest-saved-recommendations
+   (cons expansion abbrev--suggest-saved-recommendations)))
+
+(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)))
+
+(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))
+ (let ((case-fold-search t))
+   (when (and (> word-count 0)
+      (string-match (car expansion) words)
+      (abbrev--suggest-above-threshold expansion))
+     (setq abbrev-found (abbrev--suggest-shortest-abbrev
+ expansion abbrev-found)))))
+      (when abbrev-found
+ (abbrev--suggest-inform-user abbrev-found))))
+
+(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))))
+
+(defun abbrev--suggest-get-totals ()
+    "Return a list of all expansions and their usage.
+Each expansion is a cons cell where the `car' is the expansion
+and the `cdr' is the number of times the expansion has been
+typed."
+    (let (total cell)
+      (dolist (expansion abbrev--suggest-saved-recommendations)
+ (if (not (assoc (car expansion) total))
+     (push (cons (car expansion) 1) 'total)
+   (setq cell (assoc (car expansion) total))
+   (setcdr cell (1+ (cdr cell)))))
+      total))
+
+(defun abbrev-suggest-show-report ()
+  "Show the user a report of abbrevs he could have used."
+  (interactive)
+  (let ((totals (abbrev--suggest-get-totals))
+ (buf (get-buffer-create "*abbrev-suggest*")))
+    (set-buffer buf)
+    (erase-buffer)
+        (insert "** Abbrev expansion usage **
+
+Below is a list of expansions for which abbrevs are defined, and
+the number of times the expansion was typed manually. To display
+and edit all abbrevs, type `M-x edit-abbrevs RET'\n\n")
+ (dolist (expansion totals)
+   (insert (format " %s: %d\n" (car expansion) (cdr expansion))))
+ (display-buffer buf)))
+
 (defun expand-abbrev ()
   "Expand the abbrev before point, if there is an abbrev there.
 Effective when explicitly called even when `abbrev-mode' is nil.
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

If you would like the patch/diff in another format, just let me know. I
checked out the latest code from git today, so that is what the diff is
made against.

If people think this is useful (if there does not turn out to be some bad
side effects from this, I think it is a really useful addition to the
abbrev functionality, that would make people more productive), perhaps
someone could commit this, as an experimental feature or other. I'd rather
not try that exercice myself, risking messing things up... Probably this
requires some small additions to the documentation as well, not sure. I
would volunteer in writing the actual text, if needed, but could use some
help getting it in the right place and format.

Ideas for improvements would be in the are of user interaction, how to best
"inform" the user. Also, that little report hack thing I cooked up probably
could be made better too.

Looking forward to your comments.

Thanks!

/Mathias


On Sun, Oct 8, 2017 at 6:39 PM Ian Dunn <dunni@gnu.org> wrote:

> >>>>> "Stefan" == Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>     >> 2. Having this be controlled by some property of abbrev tables
>     >> and/or abbrevs themselves would be ideal, similar to the
>     >> case-fixed property.  That way the expansions of auto-correct[1]
>     >> and captain[2] abbrevs won't nag people all the time.
>     >>
>     >> [1] https://elpa.gnu.org/packages/auto-correct.html [2]
>     >> https://elpa.gnu.org/packages/captain.html
>
>     Stefan> For Captain, the abbrev and its "expansion" should have the
>     Stefan> same length, so absug-hint-threshold should already skip
>     Stefan> them.
>
>     Stefan> For auto-correct, it might be the case that the wrong
>     Stefan> spelling is shorter by absug-hint-threshold, but then you
>     Stefan> could also argue that if you often misspell a word and it
>     Stefan> gets auto-corrected and the wrong spelling is shorter, you
>     Stefan> might take it as a feature and consciously use the
>     Stefan> shorter/wrong spelling and rely on the abbrev to auto
>     Stefan> correct it.
>
> I stand corrected.  My only other suggestion would be to use add-function
> on abbrev-expand-function instead of setting it directly to avoid
> overwriting other packages that may need it.  Something like the following:
>
>
> --
> Ian Dunn
>

[-- Attachment #2: Type: text/html, Size: 15380 bytes --]

Re: Abbrev suggestions - feedback appreciated

[Stefan Monnier]

[ 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.  ]

> +(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?
What would be the disadvantages/risks of enabling it by default?

> -(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.

The rest of the code defines abbrev--suggest-maybe-suggest rather than
abbrev-suggest-maybe-suggest.

> +(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`).

> +(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.

> +                      (setq expansions
> +                            (cons (cons value (symbol-name e))
> +                                  expansions)))))

Aka   (push (cons value (symbol-name e)) expansions))))

> +(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?

> +(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))))

> +    (setq abbrev--suggest-saved-recommendations
> +          (cons expansion abbrev--suggest-saved-recommendations)))

Aka  (push expansion abbrev--suggest-saved-recommendations))

BTW, won't this list contain repetitions?
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.

> +(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.

> +(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?

> +      (let ((case-fold-search t))

Some abbrevs are case-sensitive.

> +        (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.

> +                   (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.

> +(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.


        Stefan

Re: Abbrev suggestions - feedback appreciated

[Mathias Dahl]

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

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 <monnier@iro.umontreal.ca>
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

[-- Attachment #2: Type: text/html, Size: 17626 bytes --]

Re: Abbrev suggestions - feedback appreciated

[Mathias Dahl]

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

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 <mathias.dahl@gmail.com>
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 <monnier@iro.umontreal.ca>
> 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
>
>

[-- Attachment #2: Type: text/html, Size: 19395 bytes --]

Re: Abbrev suggestions - feedback appreciated

[Stefan Monnier]

>> [ 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?

No: neither that it should nor that it shouldn't.  Just that indentation
was weird and the reason was probably that some TABs were mishandled by
the mail transport.

> 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,

I think we all agree: the `abbrev-mode` group is inside the `abbrev`
group which is inside the `convenience` group ;-)

Maybe this could be restructured to make it less deep, but I'm not the
right person to talk to for that.

>> 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.

Could be, but I suspect those people who'd be annoyed don't have any
abbrevs to start with, so abbrev suggestions won't annoy them ;-)

> They could of course turn it off, if we enable it by default.

Yes, I'm tempted to enable it by default.  But indeed it's important
that it can be disabled easily (and maybe we'll end up disabling by
default).

> 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?

I think the `:group` args there should be removed, indeed.

> 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?

That seems good, yes.  Tho please make sure
`abbrev-suggest-maybe-suggest` returns nil, or use

   (or (funcall abbrev-expand-function)
       (progn
         (abbrev-suggest-maybe-suggest)
         nil))

>> 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?).

The idea is to avoid the memory allocation, yes.  It should improve
performance and reduce the frequency of GC pauses.

>> > +(defun abbrev--suggest-count-words (expansion)
>> Why does your code pay attention to words?
> I'll think about that... Are you thinking that only characters matter?

No, it's just that currently Abbrevs don't treat words specially, and
some uses of abbrevs can expand to things that aren't just made of words.

> I think my focus is primarily using abbrevs for sentences.

Right, but abbrevs are also used in programming languages and other
"non-prose" contexts.  So, it's better if we can make it work without
assuming "words".

> I'll try to cook a new version now.

Looking forward to it,


        Stefan

Re: Abbrev suggestions - feedback appreciated

[Mathias Dahl]

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

>
> >> 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?).
>
> The idea is to avoid the memory allocation, yes.  It should improve
> performance and reduce the frequency of GC pauses.
>

I think I don't understand the alternatives here. Firstly, I think I need
to use
mapatoms to look for the abbrev names and values. That then, needs to run
in some kind of loop, for all the active abbrev tables. As I find non-nil
entries
in the table, I need to collect those in some "list" of some kind, which I
return in
the end. This intermediate list you mention, is that hidden in the dolist
macro,
or what is your thinking? And, if so, is the idea to make something similar
to dolist
that does "the mapatoms thing" as well?

Here is the function again, for convenience:

 (defun abbrev-get-active-expansions ()
  "Return a list of all the active abbrev expansions.
Includes expansions from parent abbrev tables."
  (let (expansions)
    (dolist (table (abbrev--active-tables-including-parents))
      (mapatoms (lambda (e)
                  (let ((value (symbol-value (abbrev--symbol e table))))
                    (when value
                      (push (cons value (symbol-name e)) expansions))))
                table))
    expansions))

Thanks!

/Mathias

[-- Attachment #2: Type: text/html, Size: 2079 bytes --]

Re: Abbrev suggestions - feedback appreciated

[Mathias Dahl]

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

On Sun, May 17, 2020 at 12:10 AM Mathias Dahl <mathias.dahl@gmail.com>
wrote:

> >> 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?).
>>
>> The idea is to avoid the memory allocation, yes.  It should improve
>> performance and reduce the frequency of GC pauses.
>>
>
> I think I don't understand the alternatives here. Firstly, I think I need
> to use
> mapatoms to look for the abbrev names and values. That then, needs to run
> in some kind of loop, for all the active abbrev tables. As I find non-nil
> entries
> in the table, I need to collect those in some "list" of some kind, which I
> return in
> the end. This intermediate list you mention, is that hidden in the dolist
> macro,
> or what is your thinking? And, if so, is the idea to make something
> similar to dolist
> that does "the mapatoms thing" as well?
>
> Here is the function again, for convenience:
>
>  (defun abbrev-get-active-expansions ()
>   "Return a list of all the active abbrev expansions.
> Includes expansions from parent abbrev tables."
>   (let (expansions)
>     (dolist (table (abbrev--active-tables-including-parents))
>       (mapatoms (lambda (e)
>                   (let ((value (symbol-value (abbrev--symbol e table))))
>                     (when value
>                       (push (cons value (symbol-name e)) expansions))))
>                 table))
>     expansions))
>

I gave the mapc approach a naïve try, but I am not sure if it is better:

(defun abbrev-get-active-expansions ()
  "Return a list of all the active abbrev expansions.
Includes expansions from parent abbrev tables."
  (let (expansions)
    (mapc (lambda (table)
            (mapatoms (lambda (e)
                        (let ((value (symbol-value (abbrev--symbol e
table))))
                          (when value
                            (push (cons value (symbol-name e))
expansions))))
                      table))
          (abbrev--active-tables-including-parents))
    expansions))

[-- Attachment #2: Type: text/html, Size: 3089 bytes --]

Re: Abbrev suggestions - feedback appreciated

[Stefan Monnier]

> I gave the mapc approach a naïve try, but I am not sure if it is better:
>
> (defun abbrev-get-active-expansions ()
>   "Return a list of all the active abbrev expansions.
> Includes expansions from parent abbrev tables."

I'm not suggesting to implement
`abbrev-get-active-expansions` differently.
I'm suggesting to implement something else.  I.e.

    (defmacro abbrev-do-active-expansions (VAR &rest BODY)
      "Bind VAR to an active expansion and run BODY with it.
    Repeat it for all active expansions."
      ...)

or

    (defun abbrev-mapc-active-expansions (FUNC)
      "Call FUNC on each active expansion."

This way you don't need to build a list of active expansions: you run
FUNC or BODY on each expansion when you encounter it.


        Stefan

Re: Abbrev suggestions - feedback appreciated

[Mathias Dahl]

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

>
> I'm not suggesting to implement
> `abbrev-get-active-expansions` differently.
> I'm suggesting to implement something else.  I.e.
>
>     (defmacro abbrev-do-active-expansions (VAR &rest BODY)
>       "Bind VAR to an active expansion and run BODY with it.
>     Repeat it for all active expansions."
>       ...)
>
> or
>
>     (defun abbrev-mapc-active-expansions (FUNC)
>       "Call FUNC on each active expansion."
>
> This way you don't need to build a list of active expansions: you run
> FUNC or BODY on each expansion when you encounter it.
>

Ah, of course, got it now! :) I tried a few versions of this now. I need to
go through
the complete list of active abbrevs/expansions since I want to find one
that is valid
and I also want to find the "best" one (according to what we discussed
before).

I defined 10,000 dummy abbrevs (a1 .. a10000) and used the profiler to
compare
memory usage and... I did not get any conclusive results. And between runs
I get different
memory values reported.

If I can draw any conclusions at all it would be that the old version (that
used another
function to get the active abbrevs) and the new one (that does what needs
to be done
while collecting the active abbrevs) are more or less the same, from the
perspective
on memory consumption.

Here is the original version, slightly modified. I added a new cache to
make the
 abbrev-suggest-expansion-exist-at-point function not have to call
buffer-substring
for a certain length many times.

(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."
  (interactive)
  (abbrev-suggest-previous-chars-clear)
  (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))))

One of the profiler runs:

 Function                                                        Bytes    %
- command-execute                                           6,191,706  99%
 - call-interactively                                       6,191,706  99%
  - funcall-interactively                                   6,191,706  99%
   + profiler-stop                                          3,178,331  51%
   - abbrev-suggest                                         3,012,912  48%
    - let                                                   3,012,912  48%
     - let                                                  3,012,760  48%
      - while                                               2,046,520  33%
       - let                                                1,704,376  27%
        - if                                                1,704,376  27%
         - and                                              1,704,376  27%
          - abbrev-suggest-expansion-exist-at-point         1,023,256  16%
           - string=                                        1,023,256  16%
            - abbrev-suggest-previous-chars                   682,168  11%
             + let                                              1,048   0%
      - abbrev-get-active-expansions                          966,240  15%
       - let                                                  966,240  15%
        - let                                                 966,240  15%
         - while                                              966,240  15%
          - let                                               966,240  15%
           - mapatoms                                         966,240  15%
            - #<lambda 0x19941c7d83254423>                    870,144  14%
             - let                                            771,936  12%
              - symbol-value                                  675,840  10%
               - abbrev--symbol                               483,648   7%
                + let*                                        289,344   4%
              + if                                             96,096   1%
     + if                                                         152   0%

Here is the new version:

(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."
  (interactive)
  (abbrev-suggest-previous-chars-clear)
  (let (best-abbrev)
    (dolist (table (abbrev--active-tables-including-parents))
      (mapatoms (lambda (e)
                  (let ((value (symbol-value (abbrev--symbol e table)))
                        abbrev)
                    (when value
                      (setq abbrev (cons value (symbol-name e)))
                      (when (and (abbrev-suggest-above-threshold abbrev)
                                 (abbrev-suggest-expansion-exist-at-point
abbrev))
                        (setq best-abbrev (abbrev-suggest-best-abbrev
abbrev best-abbrev))))))
                table))
    (when best-abbrev
      (abbrev-suggest-inform-user best-abbrev))))

Here is a run with the profiler of the above:

 Function                                                        Bytes    %
- command-execute                                           4,996,751  99%
 - call-interactively                                       4,996,751  99%
  - funcall-interactively                                   4,996,751  99%
   + profiler-stop                                          3,179,816  63%
   - abbrev-suggest                                         1,816,472  36%
    - let                                                   1,816,472  36%
     - let                                                  1,815,264  36%
      - while                                               1,815,264  36%
       - let                                                1,815,264  36%
        - mapatoms                                          1,815,264  36%
         - #<lambda -0xf0a185e354afe4a>                     1,815,264  36%
          - let                                               284,064   5%
           - if                                               284,064   5%
            - progn                                           284,064   5%
               setq                                           284,064   5%
     + if                                                       1,208   0%
     profiler-start                                               463   0%

Are these numbers as expected?

Sometimes, all of a sudden, regardless of what version of the function I
use, the memory
consumed by abbrev-suggest can be as low as 80,000 bytes. What have
happened then?
Something the GC does?

Thanks!

/Mathias

[-- Attachment #2: Type: text/html, Size: 9458 bytes --]

Re: Abbrev suggestions - feedback appreciated

[Eli Zaretskii]

> From: Mathias Dahl <mathias.dahl@gmail.com>
> Date: Sun, 17 May 2020 16:59:11 +0200
> Cc: emacs-devel@gnu.org
> 
> I defined 10,000 dummy abbrevs (a1 .. a10000) and used the profiler to compare
> memory usage and... I did not get any conclusive results. And between runs I get different
> memory values reported.

The "memory" profiler doesn't really measure memory consumption, it
just uses memory-allocation calls as a poor-man's timer that ticks
frequently enough.

IOW, don't use the "memory" profiler if you need to profile memory
consumption.  We don't (yet) have such a beast in Emacs.

Re: Abbrev suggestions - feedback appreciated

[Mathias Dahl]

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

>
>
> > I defined 10,000 dummy abbrevs (a1 .. a10000) and used the profiler to
> compare
> > memory usage and... I did not get any conclusive results. And between
> runs I get different
> > memory values reported.
>
> The "memory" profiler doesn't really measure memory consumption, it
> just uses memory-allocation calls as a poor-man's timer that ticks
> frequently enough.
>
> IOW, don't use the "memory" profiler if you need to profile memory
> consumption.  We don't (yet) have such a beast in Emacs.
>

Hi Eli,

Thanks for the input.

I also did a benchmark-run (100 laps) on the two versions as well. Here are
the results:

Old version

(20.113209378 361 11.676905440000002)

New version

(19.808761477999997 356 11.392537358999999)

I'll leave it up to Stefan and others to decide which of the versions of
abbrev-suggest
we should keep, if we want to include this in Emacs.

I'll send a new patch here soon.

Thanks!

/Mathias

[-- Attachment #2: Type: text/html, Size: 1485 bytes --]

Re: Abbrev suggestions - feedback appreciated

[Stefan Monnier]

> Old version
>
> (20.113209378 361 11.676905440000002)
>
> New version
>
> (19.808761477999997 356 11.392537358999999)

BTW, your profiles seemed to indicate you were profiling the
interpreted code.
Is the above benchmark applied to code that was byte-compiled or not?


        Stefan

Re: Abbrev suggestions - feedback appreciated

[Mathias Dahl]

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

>
> BTW, your profiles seemed to indicate you were profiling the
> interpreted code.
> Is the above benchmark applied to code that was byte-compiled or not?
>

It was interpreted code :) I byte compiled the file, loaded it and made new
benchmarks (100 runs):

Old function, interpreted:

(8.512507332 115 3.1926403050000003)

New function, interpreted:

(10.542568204 163 4.423713608)

Old function, byte-compiled:

(7.071314908000001 88 2.3837053029999993)

New function, byte-compiled :

(6.988860584 74 2.041772453)

I also tested with the whole abbrev.el interpreted (I did M-x eval-buffer
on it, which I hope will "undo" most byte-compiled code) and then both
versions took around 20 seconds instead of the above.

My take on this now is that, at least from a "speed" perspective (Eli says
we cannot measure memory consumption in a good way), both functions are
equivalent and I would rather keep the old version that uses the
intermediate list of expansions, for readability. Also don't forget that
we're talking about insane amounts of abbrevs here still (10,000).

Would you agree with my conclusion or do you still want to see the version
that should be better in theory?

/Mathias

[-- Attachment #2: Type: text/html, Size: 1787 bytes --]

Re: Abbrev suggestions - feedback appreciated

[Mathias Dahl]

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

Since there were no more comments on my previous reply I will pick the
option I like best :) The performance is anyway very good regardless
which option I take.

Now for another matter: documentation. I've been thinking about where to
best describe this new feature. It does not need a very length
description I think, but we should mention it in the manual. Here is the
current "menu" for Abbrevs:

* Abbrev Concepts       Fundamentals of defined abbrevs.
* Defining Abbrevs      Defining an abbrev, so it will expand when typed.
* Expanding Abbrevs     Controlling expansion: prefixes, canceling
expansion.
* Editing Abbrevs       Viewing or editing the entire list of defined
abbrevs.
* Saving Abbrevs        Saving the entire list of abbrevs for another
session.
* Dynamic Abbrevs       Abbreviations for words already in the buffer.
* Dabbrev Customization  What is a word, for dynamic abbrevs.  Case
handling.

Any thoughts on this?

Also, once the documentation is done, what would be the steps, in git,
to get this into Emacs? Are we using feature branches? Do people commit
to master? Or can I send a patch by e-mail? (less risk of me messing
up...)

Thanks!

[-- Attachment #2: Type: text/html, Size: 1408 bytes --]

Re: Abbrev suggestions - feedback appreciated

[Stefan Monnier]

> I was able to implement that quite easily. The only worry I have is that
> it might be slow, or I know it is slow-er at least since I need to go
> through all active abbrev expansions to see if there is any shorter
> one.

I don't think that should be slower than when you go through all abbrevs
and conclude that there aren't any that apply, which AFAICT is the most
common case.


        Stefan

Re: Abbrev suggestions - feedback appreciated

[Mathias Dahl]

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

>
> > I was able to implement that quite easily. The only worry I have is that
> > it might be slow, or I know it is slow-er at least since I need to go
> > through all active abbrev expansions to see if there is any shorter
> > one.
>
> I don't think that should be slower than when you go through all abbrevs
> and conclude that there aren't any that apply, which AFAICT is the most
> common case.
>

You're very right, of course :) Sometimes you don't see the forest for the
trees...

[-- Attachment #2: Type: text/html, Size: 795 bytes --]

Re: Abbrev suggestions - feedback appreciated

[Seweryn Kokot]

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

Hi Mathias and others,
Regarding the performance, in fact in my case with more than 2600 abbrevs,
the message is displayed in more than 1 second, which is quite slow. My
processor is core i5.
Regards,
Seweryn Kokot

[-- Attachment #2: Type: text/html, Size: 283 bytes --]

Re: Abbrev suggestions - feedback appreciated

[Mathias Dahl]

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

>
> Regarding the performance, in fact in my case with more than 2600 abbrevs,
> the message is displayed in more than 1 second, which is quite slow. My
> processor is core i5.
>

If you mean that it takes a second before the message appears, that is on
purpose. The message will be displayed when Emacs has been idle for 2
seconds. Probably I should make that 1 second. I could inform the user
straight away but I was thinking that the user would miss the message then.
I'm also thinking if I should "queue up" all the messages, if there are
more than one, and show all when the user is idle. Perhaps, if more than
one message has been queued up, I should recommend the user to run M-x
absug-report to show them all?

[-- Attachment #2: Type: text/html, Size: 1007 bytes --]