From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Phil Sainty Newsgroups: gmane.emacs.devel Subject: Re: [Emacs-diffs] scratch/so-long 7273fb2: Add so-long library Date: Mon, 15 Apr 2019 01:09:16 +1200 Message-ID: References: <20190107065207.21793.53271@vcs0.savannah.gnu.org> <20190107065208.BA36C21736@vcs0.savannah.gnu.org> <16bb3884-c2de-b178-afe8-0b13a8b116a8@orcon.net.nz> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="246673"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 Cc: emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sun Apr 14 15:10:22 2019 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([209.51.188.17]) by blaine.gmane.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hFetn-0011zg-CS for ged-emacs-devel@m.gmane.org; Sun, 14 Apr 2019 15:10:19 +0200 Original-Received: from localhost ([127.0.0.1]:35656 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hFetm-0002mc-CC for ged-emacs-devel@m.gmane.org; Sun, 14 Apr 2019 09:10:18 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:35509) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hFesy-0002mF-II for emacs-devel@gnu.org; Sun, 14 Apr 2019 09:09:30 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hFesw-0006KH-Ej for emacs-devel@gnu.org; Sun, 14 Apr 2019 09:09:28 -0400 Original-Received: from smtp-4.orcon.net.nz ([60.234.4.59]:34839) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1hFesv-0006Fw-Ol for emacs-devel@gnu.org; Sun, 14 Apr 2019 09:09:26 -0400 Original-Received: from [150.107.172.3] (port=57214 helo=[192.168.20.103]) by smtp-4.orcon.net.nz with esmtpa (Exim 4.86_2) (envelope-from ) id 1hFesn-0001Lp-EL; Mon, 15 Apr 2019 01:09:19 +1200 In-Reply-To: Content-Language: en-GB X-GeoIP: NZ X-Spam_score: -2.9 X-Spam_score_int: -28 X-Spam_bar: -- X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 60.234.4.59 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:235435 Archived-At: TL;DR: 1. Things I've changed: a) Per your suggestion, the automated behaviour is now enabled and disabled via a global minor mode, `global-so-long-mode'. b) The 'overrides-only action is now a buffer-local minor mode, which can be used/toggled by itself; so users now have a minor mode (`so-long-minor-mode') which will toggle the default performance mitigations. c) Switched from defadvice to nadvice. d) Improved / clarified documentation. 2. Things I'm not changing: a) `so-long-mode' remains a major mode. b) The `so-long' and `so-long-revert' commands remain as they were; dispatching to the action functions set in `so-long-action-alist'. As before, the latest code is here: https://git.savannah.nongnu.org/cgit/so-long.git/plain/so-long.el?h=wip Responses in depth: On 13/01/19 4:11 AM, Stefan Monnier wrote: > The issue is consistency (a minor mode is a standard interface and > UI to enable&disable&test a feature) and ease of use (a global minor > mode can be enabled via Customize). > > Redefine so-long-enable as an obsolete function that just calls > (so-long-auto-mode 1). Ok, using the Customize interface to enable the library seemed like a good reason to do this. I've added `global-so-long-mode' which runs the enable/disable behaviour appropriately, with `so-long-enable' and `so-long-disable' redefined as suggested, and the documentation changed to refer to the global mode. > Side note: I mistakenly thought so-long was pretty much a brand new > package. How far back does it go? The initial release was in January 2016, and version 0.7.6 (being the latest release until the recent enhancements) was in July 2016. Until the recent work, the `so-long-mode' major mode was the only mitigation the library provided, and so all of the behaviours were tied to that. >>> Rather than defadvice, please use advice-add. > > E.g. > > (defadvice hack-local-variables (after so-long--file-local-mode disable) > "..." > ;; The first arg to `hack-local-variables' is HANDLE-MODE since Emacs 26.1, > ;; and MODE-ONLY in earlier versions. In either case we are interested in > ;; whether it has the value `t'. > (and (eq (ad-get-arg 0) t) > ad-return-value ; A file-local mode was set. > (so-long-handle-file-local-mode ad-return-value))) > > can be turned into > > (advice-add 'hack-local-variables :around #'so-long--hlv) > (defun so-long--hlv (orig-fun &optional handle-mode &rest args) > ;; The first arg to `hack-local-variables' is HANDLE-MODE since Emacs 26.1, > ;; and MODE-ONLY in earlier versions. In either case we are interested in > ;; whether it has the value `t'. > (let ((retval (apply orig-fun handle-mode args))) > (and (eq handle-mode t) > retval ; A file-local mode was set. > (so-long-handle-file-local-mode retval)))) This is changing 'after' advice into 'around' advice, which means that your version changes the return value, so that's definitely not equivalent. I've changed it to explicitly return retval. Can nadvice not do 'after' advice which knows the arguments? > (defadvice hack-local-variables (after so-long--file-local-mode disable) > > BTW, while reading that code, I couldn't quite understand what this > advice is about. I think a comment describing a use-case would be > valuable. That advice and `so-long-check-header-modes' each handle one of the ways in which a file-local 'mode' could be set. If the file does indeed have a file-local mode, then `so-long-handle-file-local-mode' is called. This is on the basis that file-local modes present a special-case -- when a file contains long lines yet a file-local mode was also set, it is reasonable to suspect that, despite the long lines, the mode is actually safe to use in practice (as if the mode was prohibitively slow with that file, the author who added the file-local mode would probably not have done so). That's guess-work as far as so-long is concerned of course, so `so-long-file-local-mode-function' is a user option which lets the user decide how those situations should be handled (with the default behaviour being fairly conservative). The "Files with a file-local 'mode'" section of the Commentary says much the same, but I'd not yet added that at the time you queried this. I think that ought to provide the missing context for readers (as otherwise I think all the advice is documented pretty clearly?) >> Such changes will need to work in Emacs 25 at least, and ideally >> back to 24.3 > > advice-add was added to Emacs-24.4 and it is available in GNU ELPA > for earlier Emacsen, so it's OK. So: Package-Requires: ((emacs "24.3") nadvice), yes? No -- experimentally that causes Emacs 26.1 to install nadvice from ELPA instead of deferring to the library it already has. Can I tell package.el that it's an ELPA requirement only if nadvice.el can't already be located? (Is that not what Package-Requires should mean by default?) For now I've bumped the required Emacs version to 24.4 instead. On 13/01/19 10:01 AM, Stefan Monnier wrote: >>> On the design side, I think you should merge `so-long` and >>> `so-long-mode` into a single function and make that function a >>> (buffer-local) minor-mode (i.e. not have any major mode, just use >>> fundamental-mode instead). Making it a minor mode rather than a >>> major-mode will also make it easier to remember the previous >>> major-mode without any need for a change-major-mode-hook hack. >> >> These changes are too significant, so I don't wish to do any of that. > > I think they're just some minor shuffling of code. > > Also, I'm wondering which part you don't like. It's partly that I've put a lot of effort into the way it is now, and the idea of reworking things at this stage is honestly painful. I'd already tried a few different approaches along the way to see what seemed to work well in practice, and heavily reworked some aspects in order to make it more extensible and easier to customize for the user; and spent a lot of time figuring out edge cases and backwards-compatibility issues with earlier versions of Emacs, sorting them all out, and polishing it all to a release state I was happy with, including working pretty hard on documenting everything. I'll concur that it seems more complicated than it might be; but it's flexible, and it works well. My gut says that the changes you're suggesting will inevitably entail a lot more of those time-consuming activities aside from any "minor shuffling of code" (and I don't think it's going to be as minor as that); so assuming it would also be me doing the rework, the notion holds no appeal. Moreover, I just don't think merging `so-long' and `so-long-mode' is the right thing to do. I do appreciate that your suggestion *sounds* like it would be clean and simple (and consequently an appealing improvement), but I don't think it actually provides the same functionality (or doing so winds up adding other new complications). Having a major mode is important to the usability. Even though I've added the ability to choose different 'actions', I've kept `so-long-mode' as the default on purpose, because I think it's important that when the automated behaviour kicks in, the reason is very much "in your face". When the user visits a file and everything looks wrong ("What's happened? Where is all my syntax highlighting?"), a major mode makes it VERY apparent what the cause is, and the explanation is an easy 'C-h m' away, with the needed documentation right at the top of the *Help* buffer, rather than buried somewhere within a long list of minor modes (which the user wouldn't necessarily otherwise realise they were looking for). There are some other side-benefits of having a major mode option as well, but I believe this default makes the effects more obvious, and consequently the library more usable. The major mode also means that for this default case I can safely bind the familiar 'C-c C-c' sequence to the revert function, which then makes it super easy for users to revert in false-positive cases; so it's hardly even a bother if that happens. > There are fundamentally two aspects in my suggestion: > - merge the two > - use a minor mode > > I think the most important for the user is the first: the difference > between M-x so-long RET and M-x so-long-mode RET seems fairly subtle > to me and I expect users will have trouble remembering which is > which or even understand that they don't do the same. This is a documentation problem, so I've addressed it as such. At the time it hadn't been sufficiently obvious to me that the two would be confused, because I knew the differences so well myself. >> There's backwards-compatibility with the earlier releases to >> consider (so-long-mode has always been a major mode); > > Then use another name for the minor mode and make so-long-mode be the > combination of fundamental-mode + so-long-minor-mode. If there's no actual major mode, then we lose the benefits of having this functionality available in the form of a major mode. Also, `so-long-mode' does very specific things, while `so-long' does whatever `so-long-action' tells it to; so doing the equivalent of the latter in fundamental-mode is not necessarily similar to the current major mode. >> I have a use-case for using it as a file-local 'mode' on one >> project, and I've had cause to suggest the same thing to other >> users. > > Minor modes can also be used on the "mode:" thingy, tho I don't > think that should make a big difference. They can be, but, to the best of my understanding, they shouldn't be. Using a minor mode as a file-local 'mode' is explicitly deprecated -- it's specifically stated that only major modes should be used as a 'mode' value, and minor modes should be enabled via 'eval'. The comments in `set-auto-mode' (also in `so-long-check-header-modes', which needed to duplicate that code) read: ;; Once we drop the deprecated feature where mode: is also allowed to ;; specify minor-modes (ie, there can be more than one "mode:"), we can ;; remove this section and just let (hack-local-variables t) handle it. I also think the behaviour when there's a single 'mode' value but it isn't a major mode can easily be confusing. It seems like something which might be working only by accident and might be brittle. Or to put it another way, the notion of a non-major mode that gets used as if it were a major mode is, to me, a more confusing prospect than any confusion you're trying to avoid by proposing that approach. (Tangentially, I've often thought that modes should have an easy way of being inspected and identified as major or minor, buffer-local or global, or globalized (controller or control-ee); and if that happened then it could sensibly be used to enforce the restriction of 'mode' to major modes, which would then be at odds with any hack to treat a minor mode as a major mode.) Regarding using a minor mode instead of `so-long'/`so-long-revert'... > The point of a minor mode is to provide a standard UI to enable > *and* disable something. Having now established that I'm keeping the major mode, I think that a minor mode which controls a major mode would also be a confusing situation. i.e. At that point we would have: global-so-long-mode calling a buffer-local minor mode which is (potentially) calling our major mode, so all three could be listed when the user typed C-h m. The minor mode would then need to remain active after that particular major mode change (but also *not* remain active if some other major mode change occurred). I know that's not what you had in mind (on account of your wanting to eliminate the major mode); but as I feel strongly about retaining the major mode, I also feel strongly that we don't want to create the above situation -- especially not on the basis of an attempt to reduce the potential for confusion. The `so-long' command is really a dispatcher -- it invokes an action, and that action can be anything, and it's simpler to keep it that way. The part that conceptually seemed like more of a candidate for a minor mode was the `overrides-only' action, which was essentially the same as `so-long-mode' but without the major mode. Originally I had dismissed the notion of that being a minor mode (on the basis that it could add confusion for no real benefit); but I've changed my mind and implemented it: `so-long-minor-mode' now exists as the minor-mode equivalent to `so-long-mode'. This possibly achieves some middle ground with your suggestions, as the user can, if they wish, invoke this minor mode directly (to much the same effect as setting `so-long-action' to the minor mode value, and calling `so-long'). -Phil