Re: [Emacs-diffs] scratch/so-long 7273fb2: Add so-long library

[Phil Sainty]

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