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

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

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

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

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

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

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

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

Re: Performance degradation from long lines

[Phil Sainty]

The origin/scratch/so-long branch (adding so-long.el) is at a release
candidate stage, and I have rebased it over the current master branch.

Since the last discussions I've added a test suite, which is passing
on all the versions I've tested, being: 24.4 (the minimum supported
version), 24.5, 25.3, 26.1, 26.2, and current master, and I've dealt
to a variety of minor and backwards-compatibility issues in the process.

As previously discussed, the ELPA package (for Emacs 24,25,26) can be
built from this code.

https://git.savannah.nongnu.org/cgit/so-long.git/plain/so-long.el?h=wip
also has the same code, just with a different directory structure for
the test code).

There will be some continuing work to eliminate the use of advice in
Emacs 27 (e.g. via bug#35351), but I feel that the library is in a
good state, and could potentially be merged at this point.


-Phil

Re: Performance degradation from long lines

[Phil Sainty]

I'll merge scratch/so-long to master in another week or so if there's
no objections.

I'm keen to get additional input on some of the default config, but
it'll be that much easier to get people to test and provide additional
suggestions once it's been merged.


-Phil


On 28/06/19 1:46 AM, Phil Sainty wrote:
> The origin/scratch/so-long branch (adding so-long.el) is at a release
> candidate stage, and I have rebased it over the current master branch.
> 
> Since the last discussions I've added a test suite, which is passing
> on all the versions I've tested, being: 24.4 (the minimum supported
> version), 24.5, 25.3, 26.1, 26.2, and current master, and I've dealt

Now tested with 25.1 and 25.2 as well.

> to a variety of minor and backwards-compatibility issues in the process.
> 
> As previously discussed, the ELPA package (for Emacs 24,25,26) can be
> built from this code.
> 
> https://git.savannah.nongnu.org/cgit/so-long.git/plain/so-long.el?h=wip
> also has the same code, just with a different directory structure for
> the test code).
> 
> There will be some continuing work to eliminate the use of advice in
> Emacs 27 (e.g. via bug#35351), but I feel that the library is in a
> good state, and could potentially be merged at this point.
> 
> 
> -Phil
> 

Re: Performance degradation from long lines

[Eli Zaretskii]

> From: Phil Sainty <psainty@orcon.net.nz>
> Cc: Eli Zaretskii <eliz@gnu.org>, mithraeum <mithraeum@protonmail.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sun, 7 Jul 2019 02:18:17 +1200
> 
> I'll merge scratch/so-long to master in another week or so if there's
> no objections.
> 
> I'm keen to get additional input on some of the default config, but
> it'll be that much easier to get people to test and provide additional
> suggestions once it's been merged.

Thanks for merging this.

I made a few minor fixes to the docs.

One comment I have is that disabling bidi-display-reordering should
probably be removed from the defaults, because doing so puts the
display engine in a state that is not being tested, and can cause
inconsistencies and even bugs (because some portions of the code were
written under the assumption that this variable is never nil).

OTOH, I'd suggest setting bidi-paragraph-direction to 'left-to-right
by default when so-long-mode is turned on.

Also, I don't understand why the defaults disable
display-line-number-mode, it AFAIK does not slow down redisplay in any
significant ways.  Do you have any evidence it should be disabled in
buffers with long lines?

IME, truncate-lines sometimes makes display of long lines _faster_, so
I'm not sure we should disable that by default.

Re: Performance degradation from long lines

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> One comment I have is that disabling bidi-display-reordering should
> probably be removed from the defaults, because doing so puts the
> display engine in a state that is not being tested, and can cause
> inconsistencies and even bugs (because some portions of the code were
> written under the assumption that this variable is never nil).

I think some users are already setting bidi-display-reordering to nil.  I
stumbled into this the other day, for example:

    You can try and see if setting bidi-display-reordering to nil
improves the speed
    for you. This removes one significant contributor to line scans,
but sadly not
    the only one.

https://emacs.stackexchange.com/questions/598/how-do-i-prevent-extremely-long-lines-making-emacs-slow

Should Emacs support that?  Should it be added to its doc string that it could
be dangerous?

Thanks,
Stefan Kangas

Re: Performance degradation from long lines

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Sat, 13 Jul 2019 11:07:08 +0200
> Cc: Phil Sainty <psainty@orcon.net.nz>, mithraeum@protonmail.com, 
> 	Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > One comment I have is that disabling bidi-display-reordering should
> > probably be removed from the defaults, because doing so puts the
> > display engine in a state that is not being tested, and can cause
> > inconsistencies and even bugs (because some portions of the code were
> > written under the assumption that this variable is never nil).
> 
> I think some users are already setting bidi-display-reordering to nil.  I
> stumbled into this the other day, for example:

They do that at their own risk.

> Should Emacs support that?

If someone wants to work on that, they should feel free.  I won't.

> Should it be added to its doc string that it could be dangerous?

I'm okay with documenting that, thanks.  Although the variable was
supposed to be an internal on, but I guess that genie is out of the
bottle long ago.

Re: Performance degradation from long lines

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 374 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:
> > Should it be added to its doc string that it could be dangerous?>
> I'm okay with documenting that, thanks.  Although the variable was
> supposed to be an internal on, but I guess that genie is out of the
> bottle long ago.

How about the attached patch?  It's based on what you've said in this
thread so far.

Thanks,
Stefan Kangas

[-- Attachment #2: 0001-Add-warning-to-bidi-display-reordering-doc-string.patch --]
[-- Type: text/x-patch, Size: 1621 bytes --]

From cc3b8e1b0ecf7dca67246878f9374780685f0f5d Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sat, 13 Jul 2019 12:11:19 +0200
Subject: [PATCH] Add warning to bidi-display-reordering doc string

This explanation was given by Eli Zaretskii on emacs-devel.
For discussion, see:
https://lists.gnu.org/archive/html/emacs-devel/2019-07/msg00294.html

* src/buffer.c (syms_of_buffer): Add warning to doc string of
bidi-display-reordering to explain it could be dangerous in a
production session.
---
 src/buffer.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index 209e29f0f1..9b978c131e 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -5659,8 +5659,12 @@ syms_of_buffer (void)
 
   DEFVAR_PER_BUFFER ("bidi-display-reordering",
 		     &BVAR (current_buffer, bidi_display_reordering), Qnil,
-		     doc: /* Non-nil means reorder bidirectional text for display in the visual order.  */);
-
+		     doc: /* Non-nil means reorder bidirectional text for display in the visual order.
+WARNING: This variable is useful for debugging the display code, but
+using it in a production session is dangerous, because its puts the
+display engine in a state that is not being tested.  It can cause
+inconsistencies and even bugs (because some portions of the code were
+written under the assumption that this variable is never nil).  */);
   DEFVAR_PER_BUFFER ("bidi-paragraph-start-re",
 		     &BVAR (current_buffer, bidi_paragraph_start_re), Qnil,
 		     doc: /* If non-nil, a regexp matching a line that starts OR separates paragraphs.
-- 
2.11.0

Re: Performance degradation from long lines

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Sat, 13 Jul 2019 12:23:20 +0200
> Cc: Phil Sainty <psainty@orcon.net.nz>, mithraeum@protonmail.com, 
> 	Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
> 
> > I'm okay with documenting that, thanks.  Although the variable was
> > supposed to be an internal on, but I guess that genie is out of the
> > bottle long ago.
> 
> How about the attached patch?  It's based on what you've said in this
> thread so far.

Well, I guess I said too much ;-)

It should be enough to say something shorter like

  Setting this to nil is intended for use in debugging the display
  code.  Don't set to nil in normal sessions, as that is not supported.

Thanks.

Re: Performance degradation from long lines

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 348 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

> Well, I guess I said too much ;-)
>
> It should be enough to say something shorter like
>
>   Setting this to nil is intended for use in debugging the display
>   code.  Don't set to nil in normal sessions, as that is not supported.

OK, I've attached an updated patch with that text.

Thanks,
Stefan Kangas

[-- Attachment #2: 0001-Add-warning-to-bidi-display-reordering-doc-string.patch --]
[-- Type: text/x-patch, Size: 1408 bytes --]

From cbe0a0fa166ed1a0704a40c8566a72740c37de4c Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sat, 13 Jul 2019 12:11:19 +0200
Subject: [PATCH] Add warning to bidi-display-reordering doc string

This explanation was given by Eli Zaretskii on emacs-devel.
For discussion, see:
https://lists.gnu.org/archive/html/emacs-devel/2019-07/msg00294.html

* src/buffer.c (syms_of_buffer): Add warning to doc string of
bidi-display-reordering to explain that it should only be used for
debugging.
---
 src/buffer.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index 209e29f0f1..859c09f29f 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -5659,8 +5659,9 @@ syms_of_buffer (void)
 
   DEFVAR_PER_BUFFER ("bidi-display-reordering",
 		     &BVAR (current_buffer, bidi_display_reordering), Qnil,
-		     doc: /* Non-nil means reorder bidirectional text for display in the visual order.  */);
-
+		     doc: /* Non-nil means reorder bidirectional text for display in the visual order.
+Setting this to nil is intended for use in debugging the display code.
+Don't set to nil in normal sessions, as that is not supported.  */);
   DEFVAR_PER_BUFFER ("bidi-paragraph-start-re",
 		     &BVAR (current_buffer, bidi_paragraph_start_re), Qnil,
 		     doc: /* If non-nil, a regexp matching a line that starts OR separates paragraphs.
-- 
2.11.0

Re: Performance degradation from long lines

[Phil Sainty]

I think it would be good if that docstring also told users that
setting bidi-paragraph-direction to 'left-to-right is what they
should probably be doing instead?



On 13/07/19 10:38 PM, Stefan Kangas wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
>> Well, I guess I said too much ;-)
>>
>> It should be enough to say something shorter like
>>
>>   Setting this to nil is intended for use in debugging the display
>>   code.  Don't set to nil in normal sessions, as that is not supported.
> 
> OK, I've attached an updated patch with that text.
> 
> Thanks,
> Stefan Kangas
> 

Re: Performance degradation from long lines

[Eli Zaretskii]

> Cc: mithraeum@protonmail.com, Stefan Monnier <monnier@iro.umontreal.ca>,
>  emacs-devel@gnu.org
> From: Phil Sainty <psainty@orcon.net.nz>
> Date: Sat, 13 Jul 2019 22:58:51 +1200
> 
> I think it would be good if that docstring also told users that
> setting bidi-paragraph-direction to 'left-to-right is what they
> should probably be doing instead?

Done, thanks.

Re: Performance degradation from long lines

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Sat, 13 Jul 2019 12:38:44 +0200
> Cc: Phil Sainty <psainty@orcon.net.nz>, mithraeum@protonmail.com, 
> 	Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
> 
> >   Setting this to nil is intended for use in debugging the display
> >   code.  Don't set to nil in normal sessions, as that is not supported.
> 
> OK, I've attached an updated patch with that text.

Thanks, pushed to the emacs-26 branch.

Re: Performance degradation from long lines

[Phil Sainty]

On 13/07/19 8:07 PM, Eli Zaretskii wrote:
> One comment I have is that disabling bidi-display-reordering should
> probably be removed from the defaults, because doing so puts the
> display engine in a state that is not being tested, and can cause
> inconsistencies and even bugs (because some portions of the code
> were written under the assumption that this variable is never nil).
>
> OTOH, I'd suggest setting bidi-paragraph-direction to 'left-to-right
> by default when so-long-mode is turned on.

That sounds good to me.  I wasn't aware that nil was an invalid value
for bidi-display-reordering.  I would suggest that its docstring be
updated in this regard.  Currently the doc is just "Non-nil means
reorder bidirectional text for display in the visual order." which to
me implies that value of nil is ok.


> Also, I don't understand why the defaults disable
> display-line-number-mode, it AFAIK does not slow down redisplay in
> any significant ways.  Do you have any evidence it should be
> disabled in buffers with long lines?

No, I'd simply included it along with the older line-numbering minor
modes.  I believe I can see a *very* slight difference, depending on
the state of display-line-number-mode, when moving around the visual
lines in a ~1MB line; however it's not significant, so I don't object
to removing it from the list.

I'm not sure that there's a benefit to the end-user in having line-
numbering enabled in such a file, though (which was the other reason
I went ahead and added all of those modes).

Some of the default minor modes are in the list because I felt the
features they provided would be redundant in a long-lines situation
(or redundant with font-lock-mode disabled), rather than because I'd
done benchmarking and established that they were notable CPU hogs.

I'd concluded that any mode or feature which scans buffer contents in
order to highlight things would be reasonable to disable by default,
and my general approach has been to err on the side of being overly
conservative in order to provide maximum benefit in the really extreme
cases.

I'm certain that the default settings can be improved though, so I'm
very happy for them to be discussed, and I hope that people will do
some experimentation in their own configs and recommend changes and
additions to the defaults, so that 27.1 ships with better coverage in
general -- the current defaults really only reflect things which I've
used personally.


> IME, truncate-lines sometimes makes display of long lines _faster_,
> so I'm not sure we should disable that by default.

This should stay.  The combination of truncate-lines being disabled
and line-move-visual being enabled is a tremendous benefit when the
user tries to move vertically from an extremely long line to the next
line.  With truncated lines, Emacs will have to scan to the end of the
line (one of my test files is a 19M JSON file in a single line*),
whereas with the settings I've used the user can happily move up and
down in the early parts of the file with no problems at all.

(*) You can find that file in this Stack Overflow question:
https://emacs.stackexchange.com/q/598 (see the 'wget' command).


-Phil

Re: Performance degradation from long lines

[Eli Zaretskii]

> Cc: mithraeum@protonmail.com, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> From: Phil Sainty <psainty@orcon.net.nz>
> Date: Sat, 13 Jul 2019 21:33:19 +1200
> 
> On 13/07/19 8:07 PM, Eli Zaretskii wrote:
> > One comment I have is that disabling bidi-display-reordering should
> > probably be removed from the defaults, because doing so puts the
> > display engine in a state that is not being tested, and can cause
> > inconsistencies and even bugs (because some portions of the code
> > were written under the assumption that this variable is never nil).
> >
> > OTOH, I'd suggest setting bidi-paragraph-direction to 'left-to-right
> > by default when so-long-mode is turned on.
> 
> That sounds good to me.  I wasn't aware that nil was an invalid value
> for bidi-display-reordering.

It isn't invalid.  It is useful for debugging the display code, but
using it in production session is dangerous, because it causes the
code to execute control-flow paths that aren't supported in
general-purpose usage.

> > Also, I don't understand why the defaults disable
> > display-line-number-mode, it AFAIK does not slow down redisplay in
> > any significant ways.  Do you have any evidence it should be
> > disabled in buffers with long lines?
> 
> No, I'd simply included it along with the older line-numbering minor
> modes.  I believe I can see a *very* slight difference, depending on
> the state of display-line-number-mode, when moving around the visual
> lines in a ~1MB line; however it's not significant, so I don't object
> to removing it from the list.

In my measurements, the slow down is about 10% or less.

> I'm not sure that there's a benefit to the end-user in having line-
> numbering enabled in such a file, though (which was the other reason
> I went ahead and added all of those modes).

Log files is one important use case where line numbers might be of
benefit.

> > IME, truncate-lines sometimes makes display of long lines _faster_,
> > so I'm not sure we should disable that by default.
> 
> This should stay.  The combination of truncate-lines being disabled
> and line-move-visual being enabled is a tremendous benefit when the
> user tries to move vertically from an extremely long line to the next
> line.

If it's the combination that matters, I suggest to mention that in the
documentation (doc string, perhaps?), as I don't think this will be
otherwise evident.

Thanks.

Re: Performance degradation from long lines

[Stefan Monnier]

>> That sounds good to me.  I wasn't aware that nil was an invalid value
>> for bidi-display-reordering.
>
> It isn't invalid.  It is useful for debugging the display code, but
> using it in production session is dangerous, because it causes the
> code to execute control-flow paths that aren't supported in
> general-purpose usage.

How 'bout we rename it to clarify it. 
The new name could include "debug", "internal" and/or "--".


        Stefan

Re: Performance degradation from long lines

[Stefan Kangas]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> >> That sounds good to me.  I wasn't aware that nil was an invalid value
> >> for bidi-display-reordering.
> >
> > It isn't invalid.  It is useful for debugging the display code, but
> > using it in production session is dangerous, because it causes the
> > code to execute control-flow paths that aren't supported in
> > general-purpose usage.
>
> How 'bout we rename it to clarify it.
> The new name could include "debug", "internal" and/or "--".

It should probably be "-internal", according to
(info "(elisp)Tips for Defining")

‘...-internal’
     The variable is intended for internal use and is defined in C code.
     (Emacs code contributed before 2018 may follow other conventions,
     which are being phased out.)

Best regards,
Stefan Kangas

Re: Performance degradation from long lines

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sat, 13 Jul 2019 09:31:02 -0400
> Cc: Phil Sainty <psainty@orcon.net.nz>, mithraeum@protonmail.com,
>  emacs-devel@gnu.org
> 
> >> That sounds good to me.  I wasn't aware that nil was an invalid value
> >> for bidi-display-reordering.
> >
> > It isn't invalid.  It is useful for debugging the display code, but
> > using it in production session is dangerous, because it causes the
> > code to execute control-flow paths that aren't supported in
> > general-purpose usage.
> 
> How 'bout we rename it to clarify it. 
> The new name could include "debug", "internal" and/or "--".

And then introduce an alias or it, and maintain those for years?
Doesn't sound worth the hassle.

Re: Performance degradation from long lines

[Stefan Monnier]

> And then introduce an alias for it,

I was thinking of *not* having an alias for it.


        Stefan

Re: Performance degradation from long lines

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sat, 13 Jul 2019 10:17:18 -0400
> Cc: psainty@orcon.net.nz, mithraeum@protonmail.com, emacs-devel@gnu.org
> 
> > And then introduce an alias for it,
> 
> I was thinking of *not* having an alias for it.

I don't see how we could avoid that, this variable is quite old and is
known too widely to simply remove it.

Re: Performance degradation from long lines

[Stefan Monnier]

>> I was thinking of *not* having an alias for it.
> I don't see how we could avoid that,

Just like that: we rename it and announce the change in etc/NEWS.

> this variable is quite old and is known too widely to simply remove it.

I think the risk is very low.


        Stefan

Re: Performance degradation from long lines

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sat, 13 Jul 2019 18:22:43 -0400
> Cc: psainty@orcon.net.nz, mithraeum@protonmail.com, emacs-devel@gnu.org
> 
> >> I was thinking of *not* having an alias for it.
> > I don't see how we could avoid that,
> 
> Just like that: we rename it and announce the change in etc/NEWS.

Sorry, that would be burying our head in the sand.

Re: Performance degradation from long lines

[Dmitry Gutov]

On 14.07.2019 8:39, Eli Zaretskii wrote:

>> Just like that: we rename it and announce the change in etc/NEWS.
> 
> Sorry, that would be burying our head in the sand.

If the variable is only used out there for performance purposes, or 
temporarily for debugging, I don't think it would lead to any 
significant breakage, even if the former name suddenly stops working.

Re: Performance degradation from long lines

[Eli Zaretskii]

> Cc: psainty@orcon.net.nz, mithraeum@protonmail.com, emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 15 Jul 2019 16:12:26 +0300
> 
> On 14.07.2019 8:39, Eli Zaretskii wrote:
> 
> >> Just like that: we rename it and announce the change in etc/NEWS.
> > 
> > Sorry, that would be burying our head in the sand.
> 
> If the variable is only used out there for performance purposes, or 
> temporarily for debugging, I don't think it would lead to any 
> significant breakage, even if the former name suddenly stops working.

You are missing the point, I think: this variable's name is known
widely enough to make it a de-facto public interface.  Changing its
name without an alias will cause breakage to many people's
configurations.  However I dislike use of this variable in people's
configurations, I cannot break them all just because I dislike that.

IOW, it's a question of being fair to our users, even if they do
something we don't endorse.

Re: Performance degradation from long lines

[Stefan Monnier]

> You are missing the point, I think: this variable's name is known
> widely enough to make it a de-facto public interface.  Changing its
> name without an alias will cause breakage to many people's
> configurations.  However I dislike use of this variable in people's
> configurations, I cannot break them all just because I dislike that.

I think we all understand that.  But some of us think the breakage will
be minimal.

My cursory search through github seemed to indicate that it's very
rarely usedin Elisp packages and the few times it's used it comes with
a comment along the lines of "disable bidi for a marginal performance
gain".

As for such settings in people's .emacs, my educated guess is that they
date back to Emacs-23's introduction to work around problems in specific
modes that have since solved those problems (typically by setting
bidi-paragraph-direction).

IOW, any noticeable breakage that might result likely points to
a *real* bug.


        Stefan

Re: Performance degradation from long lines

[Clément Pit-Claudel]

On 2019-07-18 10:48, Stefan Monnier wrote:
> As for such settings in people's .emacs, my educated guess is that they
> date back to Emacs-23's introduction to work around problems in specific
> modes that have since solved those problems (typically by setting
> bidi-paragraph-direction).

It's also suggested in the top-voted answer on how to make Emacs faster with long lines on emacs.stackexchange (https://emacs.stackexchange.com/questions/598/how-do-i-prevent-extremely-long-lines-making-emacs-slow), so I'd expect many people to have it in their config for that reason.

Re: Performance degradation from long lines

[Stefan Monnier]

>> As for such settings in people's .emacs, my educated guess is that they
>> date back to Emacs-23's introduction to work around problems in specific
>> modes that have since solved those problems (typically by setting
>> bidi-paragraph-direction).
>
> It's also suggested in the top-voted answer on how to make Emacs faster with
> long lines on emacs.stackexchange
> (https://emacs.stackexchange.com/questions/598/how-do-i-prevent-extremely-long-lines-making-emacs-slow),
> so I'd expect many people to have it in their config for that reason.

Yes, but making it ineffective won't break their config.
It will merely make long-lines a bit slower in some cases.
And we can point to so-long for a better solution.
Nothing problematic, IMO.


        Stefan

Re: Performance degradation from long lines

[Andy Moreton]

On Thu 18 Jul 2019, Stefan Monnier wrote:

>> You are missing the point, I think: this variable's name is known
>> widely enough to make it a de-facto public interface.  Changing its
>> name without an alias will cause breakage to many people's
>> configurations.  However I dislike use of this variable in people's
>> configurations, I cannot break them all just because I dislike that.
>
> I think we all understand that.  But some of us think the breakage will
> be minimal.

It is perfectly reasonable to deprecate the variable. However it is
better to show the user how to fix the problem in their code than just
break previously working code (for some definition of working).

How about renaming all occurences of `bidi-display-reordering' to
`debug--bidi-display-reordering' or similar, and then add something like:

  (make-obsolete-variable 'bidi-display-reordering
   "not used. To disable reordering, use `(setq bidi-paragraph-direction
   'left-to-right)' instead." "27.1")

That would less hostile to users than immediate removal.

    AndyM