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

[Phil Sainty <psainty@orcon.net.nz>]

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