Re: Abbrev suggestions - feedback appreciated

[Mathias Dahl <mathias.dahl@gmail.com>]

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