From: Stefan Monnier <monnier@iro.umontreal.ca>
To: emacs-devel@gnu.org
Subject: Re: Abbrev suggestions - feedback appreciated
Date: Mon, 17 Sep 2018 22:05:38 -0400 [thread overview]
Message-ID: <jwv5zz3dbny.fsf-monnier+gmane.emacs.devel@gnu.org> (raw)
In-Reply-To: CABrcCQ4goOhPUGCXwapSiy_8QyxzfuaELUpNO+NK7DR6iP9ffA@mail.gmail.com
[ 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
next prev parent reply other threads:[~2018-09-18 2:05 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-16 7:51 Abbrev suggestions - feedback appreciated Mathias Dahl
2017-09-16 8:46 ` Eli Zaretskii
2017-09-17 13:15 ` Mathias Dahl
2017-09-16 13:22 ` Stefan Monnier
2017-09-17 13:22 ` Mathias Dahl
2017-09-17 14:03 ` Mathias Dahl
2017-09-17 21:23 ` Stefan Monnier
2017-09-17 21:56 ` Mathias Dahl
2017-10-03 12:51 ` Kaushal Modi
2017-10-07 15:13 ` Mathias Dahl
2017-10-07 15:29 ` Stefan Monnier
2017-10-07 17:18 ` Mathias Dahl
2017-10-07 18:40 ` Mathias Dahl
2017-10-07 22:29 ` Ian Dunn
2017-10-07 22:44 ` Stefan Monnier
2017-10-08 16:38 ` Ian Dunn
2018-09-17 21:48 ` Mathias Dahl
2018-09-18 2:05 ` Stefan Monnier [this message]
2020-05-11 21:37 ` Mathias Dahl
2020-05-11 22:39 ` Mathias Dahl
2020-05-11 22:58 ` Stefan Monnier
2020-05-16 22:10 ` Mathias Dahl
2020-05-16 22:22 ` Mathias Dahl
2020-05-17 3:13 ` Stefan Monnier
2020-05-17 14:59 ` Mathias Dahl
2020-05-17 15:45 ` Eli Zaretskii
2020-05-17 18:43 ` Mathias Dahl
2020-05-17 21:20 ` Stefan Monnier
2020-05-18 22:00 ` Mathias Dahl
2020-06-04 20:14 ` Mathias Dahl
2017-10-07 22:40 ` Stefan Monnier
2017-10-08 15:28 ` Mathias Dahl
-- strict thread matches above, loose matches on Subject: below --
2017-10-08 8:15 Seweryn Kokot
2017-10-08 15:25 ` Mathias Dahl
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=jwv5zz3dbny.fsf-monnier+gmane.emacs.devel@gnu.org \
--to=monnier@iro.umontreal.ca \
--cc=emacs-devel@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.