From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: [patch] make electric-pair-mode smarter/more useful Date: Thu, 12 Dec 2013 13:08:55 -0500 Message-ID: References: <87haalh806.fsf@gmail.com> <87ppp2ydqf.fsf@gmail.com> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1386871764 9250 80.91.229.3 (12 Dec 2013 18:09:24 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Thu, 12 Dec 2013 18:09:24 +0000 (UTC) Cc: emacs-devel@gnu.org To: joaotavora@gmail.com (=?windows-1252?B?Sm/jbyBU4XZvcmE=?=) Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Dec 12 19:09:29 2013 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1VrAhT-0006kH-F4 for ged-emacs-devel@m.gmane.org; Thu, 12 Dec 2013 19:09:27 +0100 Original-Received: from localhost ([::1]:38007 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VrAhT-0004Pf-03 for ged-emacs-devel@m.gmane.org; Thu, 12 Dec 2013 13:09:27 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:50279) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VrAhJ-0004NZ-J6 for emacs-devel@gnu.org; Thu, 12 Dec 2013 13:09:24 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VrAh7-00054S-Qn for emacs-devel@gnu.org; Thu, 12 Dec 2013 13:09:17 -0500 Original-Received: from chene.dit.umontreal.ca ([132.204.246.20]:54809) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VrAh7-00053e-M0 for emacs-devel@gnu.org; Thu, 12 Dec 2013 13:09:05 -0500 Original-Received: from fmsmemgm.homelinux.net (lechon.iro.umontreal.ca [132.204.27.242]) by chene.dit.umontreal.ca (8.14.1/8.14.1) with ESMTP id rBCI90pu007145; Thu, 12 Dec 2013 13:09:01 -0500 Original-Received: by fmsmemgm.homelinux.net (Postfix, from userid 20848) id 7436BAE350; Thu, 12 Dec 2013 13:08:55 -0500 (EST) In-Reply-To: <87ppp2ydqf.fsf@gmail.com> (=?windows-1252?Q?=22Jo=E3o_T=E1vo?= =?windows-1252?Q?ra=22's?= message of "Thu, 12 Dec 2013 03:01:12 +0000") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3.50 (gnu/linux) X-NAI-Spam-Flag: NO X-NAI-Spam-Threshold: 5 X-NAI-Spam-Score: 0 X-NAI-Spam-Rules: 1 Rules triggered RV4790=0 X-NAI-Spam-Version: 2.3.0.9362 : core <4790> : inlines <316> : streams <1089718> : uri <1622026> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 132.204.246.20 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 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-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:166343 Archived-At: > - `electric-pair-pairs` now defaults to nil. > See its updated docstring for the reasoning and the new (akwardly > named) variables `electric-pair-non-code-pairs' and > `electric-pair-non-code-syntax-table' for slightly better, more > flexible alternatives to it, in my opinion. I think this change is for the worst. I often rely on ".." pairing in things like text-mode where they don't have `string' syntax. The explanation for removing (?\" . ?\") doesn't make much sense: you seem to say that the list should be kept empty because balance can't be provided, but until now balance wasn't provided either. Of course, when the syntax-table gives string syntax to ", then this entry should be ignored since the syntax table gives more info, which allows balancing. I suggest you use "text" rather than "non-code" in the variable names (and indeed, the behavior in strings and comments should largely be the same as in text-mode). > get autopair.el's backtick-and-quote pairing for elisp comments and > strings... This is good, yes, thanks. > I read the other FIXME notes but didn't understand them so well. Can > you provide examples of the conflicts between `electric-indent-mode' > and other `electric-modes' that you mention there? I can't remember off-hand, sorry. > - I simplififed the previous `electric-pair--up-list' function and > renamed it `electric-pair--balance-info'. But I still couldn't make it > use `up-list' or get rid of some `forward-sexp'. `up-list' can > probably be done with enough care, but I think replacing > `forward-sexp' with `syntax-ppss' is only possible in the "pair", not > the "skip" case. We can use syntax-ppss to get the START of all the parens, but indeed, to find the overall balance, we need to look at least for the close-paren matching the outermost open paren, which syntax-ppss doesn't give us. > And only when outside comments or strings. The important case is the "pathological" case where we have to scan from "almost point-min" to "almost point-max", and that should hopefully never happen inside strings or comments. > In this implementation, I think the the number of `forward-sexp''s is > at most proportional to the nesting depth, which is less > dangerous. Why do you need more than a fixed number of calls? My naive understanding is that we only need to know if we have more openers than closers, or more closers than openers, or the same number of each; and for that we basically need to do: (goto-char (car (last (nth 9 (syntax-ppss))))) (forward-sexp 1) => failure means more openers than closers (up-list 1) => failure means the parens are balanced. Tho maybe even simpler is (save-excursion (car (syntax-ppss (point-max)))) this will also have the side effect of applying syntax-propertize over the whole buffer, which could lead to performance problems, but would also eliminate bugs when a sole-open paren is inside a special comment only recognized by syntax-propertize. This said, this dependence on correctly parsing the whole buffer makes this feature brittle. Both for the case where the major mode has a bug (so it fails to parse things properly), or when it knowingly doesn't handle all cases (same thing, tho it's not considered as a bug, e.g. "#ifdef FOO \n { \n #else \n {x \n #fi"), or when some (remote) part of the buffer is simply incorrect/incomplete. So I think we need a "electric-pair-preserve-balance" flag to control those features. > - some helper functions might be reinventing the wheel, such as > `electric-pair--looking-at-mismatched-string-p' and IIUC if the next string is "mismatched" that means it extends til EOB, so you can test it with (nth 3 (syntax-ppss (point-max))). > `electric-pair--inside-comment-string'. I think (nth 3 (parse-partial-sexp (nth 8 (syntax-ppss)) (point))) will do. But of course, this parses the content of the comment as if it were code, which is naive. You could use another syntax-table during the call to parse-partial-sexp (but not the call to syntax-ppss), but it'd still be naive. IOW the problem is not in the implementation but in the idea of detecting a string inside a comment. > - I'm also trying my luck and changed lisp-mode.el defaults to > accomodate some of my preferred defaults in variables > `electric-pair-skip-whitespace' and `electric-pair-non-code-pairs'). See comments below. > +(defun electric--sort-post-self-insertion-hook () > + "Ensure order of electric functions in `post-self-insertion-hook'. > + > +Hooks in this variable interact in non-trivial ways, so a > +relative order must be maintained within it." > + (let ((relative-order '(electric-pair-post-self-insert-function > + electric-layout-post-self-insert-function > + electric-indent-post-self-insert-function > + blink-paren-post-self-insert-function))) > + (setq post-self-insert-hook > + (sort post-self-insert-hook > + #'(lambda (fn1 fn2) > + (let ((fn1-tail (memq fn1 relative-order)) > + (fn2-tail (memq fn2 relative-order))) > + (cond ((and fn1-tail fn2-tail) > + (> (length fn1-tail) > + (length fn2-tail))) > + (fn1-tail t) > + (fn2-tail nil) > + (t nil)))))))) I think the idea is OK, but be careful: these functions are supposed to be placed on the global part of the hook, so you'll want to use `default-value' and `setq-default'. BTW: I resisted the temptation to do such a `sort', but I can't give a good reason why. I just wish we had a better way to handle such order-dependencies. The part I don't like is the lack of modularity. I generally tend to dislike "priorities", but maybe adding priorities would make sense here: blink-paren-post-self-insert-function would get a priority of 100 since it should "come last because it does a sit-for, electric-indent-post-self-insert-function would get a priority of 90 because "it just does some post-processing". The priorities can be added as symbol properties, so the "sort" function doesn't need to know about the existing hooks. > +(make-variable-buffer-local 'electric-pair-non-code-pairs) I think we don't want/need that. We can and do use setq-local when needed, which should be sufficient. > +(defcustom electric-pair-skip-whitespace t > + "If non-nil skip whitespace when skipping over closing parens. Unclear. IIUC from the code, what this doesn't just skip whitespace but deletes it. It should also say which whitespace is skipped/deleted (before/after the skipped paren). > +(defvar electric-pair-non-code-syntax-table prog-mode-syntax-table Why prog-mode-syntax-table, rather than (say) text-mode-syntax-table? BTW, syntax-ppss will get majorly confused if you call it while a different syntax-table is temporarily installed. > + :keymap (let ((map (make-sparse-keymap))) > + (define-key map [remap backward-delete-char-untabify] > + 'electric-pair-backward-delete-char-untabify) > + (define-key map [remap backward-delete-char] > + 'electric-pair-backward-delete-char) > + (define-key map [remap delete-backward-char] > + 'electric-pair-backward-delete-char) > + map) Don't use this :keymap arg; instead defvar electric-pair-mode-map. > - (setq-local prettify-symbols-alist lisp--prettify-symbols-alist)) > + (setq-local prettify-symbols-alist lisp--prettify-symbols-alist) > + ; electric > + (when elisp > + (setq-local electric-pair-non-code-pairs '((?\` . ?\')))) Good. > + (setq-local electric-pair-skip-whitespace 'chomp)) Hmm... lemme think... well... maybe we can't keep it tentatively. I suspect it will bite me sometimes, but let's give it a chance. > --- a/lisp/simple.el > +++ b/lisp/simple.el > @@ -607,7 +607,7 @@ In some text modes, where TAB inserts a tab, this command indents to the > column specified by the function `current-left-margin'." > (interactive "*") > (delete-horizontal-space t) > - (newline) > + (newline 1 (not (or executing-kbd-macro noninteractive))) > (indent-according-to-mode)) Could you explain this change? > +;;; electric-tests.el --- tests for electric.el -*- lexical-binding: t; -*- Great! Stefan