Re: Abbrev suggestions - feedback appreciated

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

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