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

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Stefan Monnier
> +FIXME: +++ once all documentation has been added.

No need for this FIXME: the whole point of this system is that you don't
need to do anything special to mark entries as "not yet handled" (any `**`
header without a preceding `---` or `+++` has not been handled yet), so
it's "fail safe".

> +;; Installation
> +;; ------------
> +;; And add the following to your init file to enable the library:
> +;;
> +;; (when (require 'so-long nil :noerror)
> +;;   (so-long-enable))

I think the above should just be

    (so-long-auto-mode 1)

and so-long-auto-mode (just my suggestion for a name, feel free to
chose something else) should be an autoloaded global minor mode which
activates so-long-mode.

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.

> +(defvar so-long-enabled t
> +  "Set to nil to prevent `so-long' from being triggered automatically.")

Loading a file should not change the behavior of Emacs, so the default
should be nil unless/until we decide to preload this package.

Rather than defadvice, please use advice-add.

Other than that, looks great, thanks.


        Stefan

Reply | Threaded
Open this post in threaded view
|

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

Phil Sainty
Hi Stefan,

Thanks for the feedback.  I didn't spot this until after I'd posted my
last message to the "Performance degradation from long lines" thread.


On 11/01/19 7:07 AM, Stefan Monnier wrote:
>> +FIXME: +++ once all documentation has been added.
>
> No need for this FIXME: the whole point of this system is that you don't
> need to do anything special to mark entries as "not yet handled" (any `**`
> header without a preceding `---` or `+++` has not been handled yet), so
> it's "fail safe".

That FIXME was just for me -- I knew that I wanted to add something to
the manual, but hadn't done so yet, and didn't want to mark the NEWS
item +++ until I'd added that, and I didn't want to forget to mark it
+++ once I'd done so :)  The current branch has this fixed.


>> +;; Installation
>> +;; ------------
>> +;; And add the following to your init file to enable the library:
>> +;;
>> +;; (when (require 'so-long nil :noerror)
>> +;;   (so-long-enable))
>
> I think the above should just be
>
>     (so-long-auto-mode 1)
>
> and so-long-auto-mode (just my suggestion for a name, feel free to
> chose something else) should be an autoloaded global minor mode which
> activates so-long-mode.

I actually spent some time experimenting with that idea last month,
as it seemed like a good idea to me as well, but it got sufficiently
awkward that I dropped the notion as being more hassle than it was
worth.

If I define a global mode, then there's another variable which is
ostensibly tracking the 'enabled' state of the functionality, which
seemed ugly.  I tried using :variable in the mode definition to make
that use `so-long-enabled' internally, but then in testing I found
that I could de-sync the mode and that variable (I forget how, but
possibly by calling `so-long-disable' which I need to keep for
backwards-compatibility), and then it just struck me that I was
making more work and a bit of a headache for myself in an effort to
allow people to add '(global-so-long-mode)' in their init files
instead of '(so-long-enable)' and I immediately lost all interest
in spending more time on achieving that.

`so-long-enable' is autoloaded, so I could drop the test for
(require 'so-long nil :noerror) in the Commentary.  That's a hold-
over from the earlier releases, and it seemed harmless to keep, but
I'm happy to remove it, as it's not needed for either 27 or ELPA.


> 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.

There's backwards-compatibility with the earlier releases to consider
(so-long-mode has always been a major mode); and I also see a benefit
to retaining a major mode option (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).  I did consider turning `so-long' into a
`so-long-minor-mode' at one point, but I think it either seemed like
a fairly needless change, or possibly the idea of implementing a
feature which was designed to mess with minor mode states *as* a minor
mode seemed wrong to me.  'M-x so-long' is also shorter :)


>> +(defvar so-long-enabled t
>> +  "Set to nil to prevent `so-long' from being triggered automatically.")
>
> Loading a file should not change the behavior of Emacs, so the default
> should be nil unless/until we decide to preload this package.

That var does nothing at all if (so-long-enable) hasn't been called;
but offhand I think it's fine to set it nil by default, so I'll do that.


> Rather than defadvice, please use advice-add.

I know defadvice and I don't know advice-add, so perhaps someone else
can write those changes.  Such changes will need to work in Emacs 25 at
least, and ideally back to 24.3 (but as I can no longer build 24.x on
my machine, I've not actually tested the current library on that version
myself, so I'm not certain that I've retained compatibility with 24.3 in
this release).

I have vague recollections that advice-add is very different when it
comes to ad-get-arg, so note in particular that the signature to
hack-local-variables changed in 26.1.  That's not an issue for the
advice I've written, but if it would be for nadvice, then that needs
to be accounted for in any rewrite.


> Other than that, looks great, thanks.

Cheers,

-Phil

Reply | Threaded
Open this post in threaded view
|

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

Stefan Monnier
> If I define a global mode, then there's another variable which is
> ostensibly tracking the 'enabled' state of the functionality, which
> seemed ugly.

I don't see why.  If you don't want to get rid of the so-long-enabled
variable, then just make it a defvaralias and you're done.

> that I could de-sync the mode and that variable (I forget how, but
> possibly by calling `so-long-disable' which I need to keep for
> backwards-compatibility),

Redefine so-long-enable as an obsolete function that just calls
(so-long-auto-mode 1).

Side note: I mistakenly thought so-long was pretty much a brand new
package.  How far back does it go?

> and then it just struck me that I was making more work and a bit of
> a headache for myself in an effort to allow people to add
> '(global-so-long-mode)' in their init files instead of
> '(so-long-enable)' and I immediately lost all interest in spending
> more time on achieving that.

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).

> `so-long-enable' is autoloaded, so I could drop the test for
> (require 'so-long nil :noerror) in the Commentary.  That's a hold-
> over from the earlier releases, and it seemed harmless to keep,

Users will copy it and wonder why Emacs has to be so complicated, and it
will then fail the day the file is renamed.

> but I'm happy to remove it, as it's not needed for either 27 or ELPA.

That would be good.

>> 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.

> 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.

> and I also see a benefit to retaining a major mode option (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.

> I did consider turning `so-long' into a
> `so-long-minor-mode' at one point, but I think it either seemed like
> a fairly needless change, or possibly the idea of implementing a
> feature which was designed to mess with minor mode states *as* a minor
> mode seemed wrong to me.  'M-x so-long' is also shorter :)

The point of a minor mode is to provide a standard UI to enable *and*
disable something.

>> Rather than defadvice, please use advice-add.
> I know defadvice and I don't know advice-add, so perhaps someone else
> can write those changes.

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))))

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.

> 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.


        Stefan

Reply | Threaded
Open this post in threaded view
|

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

Stefan Monnier
>>> 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.  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.


        Stefan

Reply | Threaded
Open this post in threaded view
|

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

Phil Sainty
In reply to this post by Stefan Monnier
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


Reply | Threaded
Open this post in threaded view
|

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

Stefan Monnier
>>     (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.

Oops, indeed, sorry.

> I've changed it to explicitly return retval.

Good, thanks.

> Can nadvice not do 'after' advice which knows the arguments?

The arguments, yes, but the just-computed return value, no.

> 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?)

I must admit that I do a lot of hacking on packages I never use, so
I like it when the code explains what it does without requiring the
coder to have read the doc ;-)

>>> 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.

IIUC that's a bug in Emacs-26.1 (and 26.2) because 26.[12] is not aware
that it comes with nadvice-1.0.

It really should have been fixed before Emacs-26.2, but I somehow forgot
to push that commit and it lingered on a machine I only used again
very recently :-(


        Stefan

Reply | Threaded
Open this post in threaded view
|

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

Phil Sainty
On 2019-04-15 03:14, Stefan Monnier wrote:

>> 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.
>
> IIUC that's a bug in Emacs-26.1 (and 26.2) because 26.[12] is not aware
> that it comes with nadvice-1.0.
>
> It really should have been fixed before Emacs-26.2, but I somehow
> forgot
> to push that commit and it lingered on a machine I only used again
> very recently :-(

Ah, ok.  I'll just stick with the new minimum version of 24.4, and side-
step the ELPA issue.  A single point-release increase in three years
isn't
so bad.


-Phil