Re: Abbrev suggestions - feedback appreciated

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

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