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