From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: Abbrev suggestions - feedback appreciated Date: Mon, 17 Sep 2018 22:05:38 -0400 Message-ID: References: <871smeoalc.fsf@gnu.org> <87fuatmw71.fsf@gnu.org> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: blaine.gmane.org 1537236276 22091 195.159.176.226 (18 Sep 2018 02:04:36 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Tue, 18 Sep 2018 02:04:36 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux) To: emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Tue Sep 18 04:04:32 2018 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1g25NQ-0005eE-1W for ged-emacs-devel@m.gmane.org; Tue, 18 Sep 2018 04:04:32 +0200 Original-Received: from localhost ([::1]:38094 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1g25PW-000145-JF for ged-emacs-devel@m.gmane.org; Mon, 17 Sep 2018 22:06:42 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:60962) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1g25Or-00012G-4X for emacs-devel@gnu.org; Mon, 17 Sep 2018 22:06:02 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1g25On-0008GP-Ng for emacs-devel@gnu.org; Mon, 17 Sep 2018 22:06:00 -0400 Original-Received: from [195.159.176.226] (port=34823 helo=blaine.gmane.org) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1g25Ok-0008CU-1Y for emacs-devel@gnu.org; Mon, 17 Sep 2018 22:05:55 -0400 Original-Received: from list by blaine.gmane.org with local (Exim 4.84_2) (envelope-from ) id 1g25MU-0004Wd-V2 for emacs-devel@gnu.org; Tue, 18 Sep 2018 04:03:34 +0200 X-Injected-Via-Gmane: http://gmane.org/ Original-Lines: 177 Original-X-Complaints-To: usenet@blaine.gmane.org Cancel-Lock: sha1:6AB64OI6AwY3vm9eRMByWC9gExI= X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 195.159.176.226 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:229929 Archived-At: [ 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