orgalist-mode: wrong indentation in message mode after recent change in emacs

orgalist-mode: wrong indentation in message mode after recent change in emacs

[Gregor Zattler]

Dear org-mode developers, hello Nicolas,
     I use orgalist-mode while writing emails in
     notmuch-message-mode.  Today I updated emacs (from git), and
     since then, the second line of a paragraph get's indented
     (and succeeding lines too).

As demonstrated with this email.  While I write this email with
   my configurations, I checked with emacs -Q as follows: visited
   one org file (in order for org-mode to load), visited
   orgalist.el, eval'ed this buffer and started writing an email:
   bang! indented succeeding lines.



I bisected emacs like so:

make ; src/emacs -Q --eval '(org-mode)' -l ~/.emacs.d/elpa-27.0/orgalist-1.9/orgalist.el -f compose-mail --eval '(orgalist-mode)' -f end-of-buffer


and this is the relevant commit:

2e3deb09bd42d22a9b354937ce63b151fb493d8a is the first bad commit
commit 2e3deb09bd42d22a9b354937ce63b151fb493d8a
Author: Basil L. Contovounesios <contovob@tcd.ie>
Date:   Sun Mar 31 19:39:54 2019 +0100

    Do not set indent-line-function in text-mode

    For discussion, see thread starting at:
    https://lists.gnu.org/archive/html/emacs-devel/2019-03/msg01012.html
    * lisp/textmodes/text-mode.el (text-mode): Do not reset
    indent-line-function to its global default value of indent-relative.
    * doc/lispref/modes.texi (Example Major Modes):
    * etc/NEWS: Document change accordingly.

:040000 040000 88d3790ae5446f2ef84b66733c2b9e9bd27676a8 53a988cc46c8505b5751120b6f0813c5f252b171 M      doc
:040000 040000 0f12bd57efcc2a481b468ecefb0369ef3b3996c9 055f07d1469cee692acc35ba9ce5ec2611a6f7db M      etc
:040000 040000 0c34b9e6e31c833a7c3c4e464e0ccbf597156850 d330f001af22700ec84b0d1596571fa4548eafa8 M      lisp


I do not know what to do with this.


       Ciao; Gregor
--
 -... --- .-. . -.. ..--.. ...-.-

Re: orgalist-mode: wrong indentation in message mode after recent change in emacs

[Basil L. Contovounesios]

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

[CCing Stefan as the nadvice.el expert.]

Gregor Zattler <telegraph@gmx.net> writes:

> Dear org-mode developers, hello Nicolas,
>      I use orgalist-mode while writing emails in
>      notmuch-message-mode.  Today I updated emacs (from git), and
>      since then, the second line of a paragraph get's indented
>      (and succeeding lines too).
>
> As demonstrated with this email.  While I write this email with
>    my configurations, I checked with emacs -Q as follows: visited
>    one org file (in order for org-mode to load), visited
>    orgalist.el, eval'ed this buffer and started writing an email:
>    bang! indented succeeding lines.
>
>
>
> I bisected emacs like so:
>
> make ; src/emacs -Q --eval '(org-mode)' -l
> ~/.emacs.d/elpa-27.0/orgalist-1.9/orgalist.el -f compose-mail --eval
> '(orgalist-mode)' -f end-of-buffer
>
> and this is the relevant commit:
>
> 2e3deb09bd42d22a9b354937ce63b151fb493d8a is the first bad commit
> commit 2e3deb09bd42d22a9b354937ce63b151fb493d8a
> Author: Basil L. Contovounesios <contovob@tcd.ie>
> Date:   Sun Mar 31 19:39:54 2019 +0100
>
>     Do not set indent-line-function in text-mode
>
>     For discussion, see thread starting at:
>     https://lists.gnu.org/archive/html/emacs-devel/2019-03/msg01012.html
>     * lisp/textmodes/text-mode.el (text-mode): Do not reset
>     indent-line-function to its global default value of indent-relative.
>     * doc/lispref/modes.texi (Example Major Modes):
>     * etc/NEWS: Document change accordingly.

[...]

> I do not know what to do with this.

This seems to be a relapse(?) of bug#31361[1], which you have correctly
identified as being caused by my commit.  The following outlines what
happens now (you can safely skip the next four paragraphs if you don't
care about the technical details).

text-mode no longer makes indent-line-function buffer-local.
orgalist-mode advises indent-line-function buffer-locally using
add-function.  add-function uses advice--buffer-local to find which
place form to set.  advice--buffer-local sees that indent-line-function
is not a local variable, so it wraps it in a custom closure before
making it buffer-local.

orgalist-mode also advises indent-according-to-mode in order to work
around bug#31361 by temporarily unwrapping the advice added to
indent-line-function.  This is done because indent-according-to-mode
behaves differently if indent-line-function is set to indent-relative or
indent-relative-first-indent-point (née indent-relative-maybe).

Back when text-mode made indent-line-function buffer-local before
orgalist-mode advised it, (advice--cd*r indent-line-function) would
simply return indent-relative, and the advice on
indent-according-to-mode worked as intended.

Now that add-function sets indent-line-function to a custom closure,
however, (advice--cd*r indent-line-function) no longer returns
indent-relative and the workaround breaks.

The simplest fix would be to make indent-line-function buffer-local in
orgalist-mode before advising it with add-function, as the code already
expects and relies on this:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: orgalist.diff --]
[-- Type: text/x-diff, Size: 725 bytes --]

diff --git a/orgalist.el b/orgalist.el
index 3a3cfaa..cc14430 100644
--- a/orgalist.el
+++ b/orgalist.el
@@ -818,6 +818,11 @@ orgalist-mode
     (add-function :before-until
                   (local 'fill-paragraph-function)
                   #'orgalist--fill-item)
+    ;; Unless `indent-line-function' is buffer-local before it is
+    ;; advised with `add-function', the workaround for bug#31361 below
+    ;; will not work, as (advice--cd*r indent-line-function) will not
+    ;; compare `eq' to `indent-relative' in `indent-according-to-mode'.
+    (make-local-variable 'indent-line-function)
     (add-function :before-until
                   (local 'indent-line-function)
                   #'orgalist--indent-line)

[-- Attachment #3: Type: text/plain, Size: 1029 bytes --]


Another option, of course, would be to revert my commit.  I'm not in the
slightest against reverting, but I'm not yet convinced it's necessary,
as orgalist-mode currently seems to rely on the alignment of several
implementation details.

I do wonder about three things, though.

The first is whether orgalist-mode couldn't use a custom
indent-line-function instead of advising what may or may not be set to
indent-relative by the user.

The second is why advice--buffer-local does what it does.  Stefan, why
does it behave differently depending on local-variable-p?  Why can't it
simply call make-local-variable before returning the symbol-value?

The third is why indent-according-to-mode hard-codes the check for
indent-relative and indent-relative-first-indent-point.  Wouldn't it be
nice if this check instead looked up some variable akin to
electric-indent-functions-without-reindent, that can be more easily
customised?

[1]: https://debbugs.gnu.org/31361

Thanks for reporting this, and sorry for the fallout.

-- 
Basil

Re: orgalist-mode: wrong indentation in message mode after recent change in emacs

[Stefan Monnier]

> Now that add-function sets indent-line-function to a custom closure,
> however, (advice--cd*r indent-line-function) no longer returns
> indent-relative and the workaround breaks.

I think this qualifies as a problem in nadvice: the ad-hoc closure
introduced to "redirect to the default-value" should be handled by
advice--cd*r (or rather by some new function which can then be used
instead of advice--cd*r).


        Stefan

Re: orgalist-mode: wrong indentation in message mode after recent change in emacs

[Stefan Monnier]

> The third is why indent-according-to-mode hard-codes the check for
> indent-relative and indent-relative-first-indent-point.  Wouldn't it be
> nice if this check instead looked up some variable akin to
> electric-indent-functions-without-reindent, that can be more easily
> customised?

Agreed.  Comparing functions is always fraught with dangers, as we are
witnessing here (this very fundamental problem was arguably the original
motivation for inventing type-classes in Haskell to avoid the ugly
ad-hoc "eqtypes" of SML).

We need the comparison here for backward compatibility, but we should
supplement it with a variable, like we did with
`electric-indent-inhibit` (and not with
electric-indent-functions-without-reindent which just suffers from the
same fundamental problem).


        Stefan

Re: orgalist-mode: wrong indentation in message mode after recent change in emacs

[Nicolas Goaziou]

Hello,

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> The first is whether orgalist-mode couldn't use a custom
> indent-line-function instead of advising what may or may not be set to
> indent-relative by the user.

I don't know how that would work in practice. But a minor mode taking
control over `indent-line-function' sounds wrong.

> The second is why advice--buffer-local does what it does.  Stefan, why
> does it behave differently depending on local-variable-p?  Why can't it
> simply call make-local-variable before returning the symbol-value?
>
> The third is why indent-according-to-mode hard-codes the check for
> indent-relative and indent-relative-first-indent-point.  Wouldn't it be
> nice if this check instead looked up some variable akin to
> electric-indent-functions-without-reindent, that can be more easily
> customised?

So what is the current status of this? Do I still need to add
a workaround around a workaround around a genuine Emacs bug? :)

Regards,

-- 
Nicolas Goaziou

Re: orgalist-mode: wrong indentation in message mode after recent change in emacs

[Basil L. Contovounesios]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> The first is whether orgalist-mode couldn't use a custom
>> indent-line-function instead of advising what may or may not be set to
>> indent-relative by the user.
>
> I don't know how that would work in practice.

Me neither.

> But a minor mode taking control over `indent-line-function' sounds
> wrong.

Well, orgalist already "takes control" over indent-line-function and
indent-according-to-mode via advice, and the latter advice seems to
assume that indent-line-function is set to the default indent-relative
or indent-relative-first-indent-point.

>> The second is why advice--buffer-local does what it does.  Stefan, why
>> does it behave differently depending on local-variable-p?  Why can't it
>> simply call make-local-variable before returning the symbol-value?
>>
>> The third is why indent-according-to-mode hard-codes the check for
>> indent-relative and indent-relative-first-indent-point.  Wouldn't it be
>> nice if this check instead looked up some variable akin to
>> electric-indent-functions-without-reindent, that can be more easily
>> customised?
>
> So what is the current status of this? Do I still need to add
> a workaround around a workaround around a genuine Emacs bug? :)

Yes. :) I think the patch I proposed in my previous email should be
applied to orgalist, as a first step at the very least.

Thanks,

-- 
Basil

Re: orgalist-mode: wrong indentation in message mode after recent change in emacs

[Stefan Monnier]

>>> The first is whether orgalist-mode couldn't use a custom
>>> indent-line-function instead of advising what may or may not be set to
>>> indent-relative by the user.
>> I don't know how that would work in practice.
> Me neither.
>> But a minor mode taking control over `indent-line-function' sounds
>> wrong.
> Well, orgalist already "takes control" over indent-line-function and
> indent-according-to-mode via advice, and the latter advice seems to
> assume that indent-line-function is set to the default indent-relative
> or indent-relative-first-indent-point.

I haven't bothered to look at the advice to have an opinion here, so
I'll let you guys figure out this part.

>>> The second is why advice--buffer-local does what it does.  Stefan, why
>>> does it behave differently depending on local-variable-p?  Why can't it
>>> simply call make-local-variable before returning the symbol-value?

Normally a hook runs both its local part and its global part, where the
global part is represented in the local list by the special element `t`.

When you do `make-local-variable` you're basically *copying* the global
part to the local part, with the usual implications: when the global
part is later modified those modifications won't be properly reflected
in the local copy.  That's why we had `make-local-hook` which is now
automatically performed by `add-hook` depending on the `local` arg.

The exact same thing goes for `add-function` when applied to a variable.

In the current case, I think calling `make-local-variable` is likely
harmless because *we* know the global value should likely never change,
but that's not something that `add-function` knows to be true in
general.  So instead, `add-function` sets the local value to a function
that looks up the global value and runs it, which is the moral
equivalent of the `t` element on normal hooks.

>>> The third is why indent-according-to-mode hard-codes the check for
>>> indent-relative and indent-relative-first-indent-point.

History.  Comparing functions is always a bad idea.
But I couldn't and still can't see how to avoid it here without
introducing worse problems.

>>> Wouldn't it be nice if this check instead looked up some variable
>>> akin to electric-indent-functions-without-reindent, that can be more
>>> easily customised?

Yes, tho it probably wouldn't help much here: not only we'd still be
comparing functions, but we'd also need for orgalist to go through the
trouble of manually adding the closure dynamically created by
`add-function` to this list.


        Stefan

Re: orgalist-mode: wrong indentation in message mode after recent change in emacs

[Nicolas Goaziou]

Hello,

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Yes. :) I think the patch I proposed in my previous email should be
> applied to orgalist, as a first step at the very least.

Yay! 35 weeks later, but still before the coming year, I applied your
patch to Orgalist code base. Thank you.

Happy New Year,

-- 
Nicolas Goaziou