bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Dima Kogan]

Hi.

I'm seeing a regression introduced in 2019/01 by an update meant to make
electric-pair-mode and electric-layout-mode play nicely:

  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=fd943124439b7644392919bca8bc2a77e6316d92

Recipe:

1. 'emacs -Q' with any emacs more recent than that patch

2. Open up tst.c that contains this:

int main(int argc, char* argv[])
{
    return 0;
}


3. Move the point to the beginning of the line containing "return 0"

4. RET RET RET RET RET

Now there're a bunch of new lines between the { and the "return 0", as
expected. But these lines aren't empty: they contain the initial
indentation whitespace. This whitespace shouldn't be there; and it
wasn't there prior to this patch.

Thanks for working on emacs!

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Noam Postavsky]

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

Dima Kogan <dima@secretsauce.net> writes:

> Hi.
>
> I'm seeing a regression introduced in 2019/01 by an update meant to make
> electric-pair-mode and electric-layout-mode play nicely:
>
>   http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=fd943124439b7644392919bca8bc2a77e6316d92
>
> Recipe:
>
> 1. 'emacs -Q' with any emacs more recent than that patch
>
> 2. Open up tst.c that contains this:
>
> int main(int argc, char* argv[])
> {
>     return 0;
> }
>
>
> 3. Move the point to the beginning of the line containing "return 0"
>
> 4. RET RET RET RET RET
>
> Now there're a bunch of new lines between the { and the "return 0", as
> expected. But these lines aren't empty: they contain the initial
> indentation whitespace. This whitespace shouldn't be there; and it
> wasn't there prior to this patch.

Right, the problem is that electric-indent-inhibit only partially
disables electric indent, and the commit you reference changes which
parts.  The patch below disables it more completely.  Note however, that
this makes RET not do any electric indentation at all, just like in the
good old days of Emacs 24.3.  Possibly c-mode should rebind RET to a
c-electric-return command or something.


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

From 1103fdfbf2be55d68d44151498c2598fd622ac08 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Fri, 10 May 2019 16:40:27 -0400
Subject: [PATCH] Inhibit electric indent completely in cc mode buffers
 (Bug#35254)

Electric indent mode's post-self-insert hook entry has 3 effects:

1. Indent the previous line.
2. Remove trailing whitespace from the previous line.
3. Indent the current line (when at beginning of line).

The change from 2019-01-22 "electric-layout-mode kicks in before
electric-pair-mode", makes 'electric-indent-inhibit' inhibit 1 and 2,
whereas before then it inhibited only 1.  While cc mode provides its
own electric commands and therefore sets 'electric-indent-inhibit', it
doesn't implement an electric newline command.  So if only one of
effects 2 and 3 from Electric indent mode occur, then hitting RET will
leave trailing whitespace.

* lisp/progmodes/cc-mode.el (c-electric-indent-mode-function): New
function.
(c-basic-common-init): Add it to electric-indent-functions to disable
electric indent completely (while still letting the
electric-indent-mode command/setting to control c-electric-flag as
before).
---
 lisp/progmodes/cc-mode.el | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lisp/progmodes/cc-mode.el b/lisp/progmodes/cc-mode.el
index bd62fc754a..e41f1101d0 100644
--- a/lisp/progmodes/cc-mode.el
+++ b/lisp/progmodes/cc-mode.el
@@ -634,6 +634,7 @@ (defun c-basic-common-init (mode default-style)
   ;; messing up CC Mode's, and set `c-electric-flag' if `electric-indent-mode'
   ;; has been called by the user.
   (when (boundp 'electric-indent-inhibit) (setq electric-indent-inhibit t))
+  (add-hook 'electric-indent-functions 'c-electric-indent-mode-function nil t)
   ;; CC-mode should obey Emacs's generic preferences, tho only do it if
   ;; Emacs's generic preferences can be set per-buffer (Emacs>=24.4).
   (when (fboundp 'electric-indent-local-mode)
@@ -2143,6 +2144,10 @@ (defun c-electric-indent-local-mode-hook ()
     (setq c-electric-flag electric-indent-mode)
     (c-update-modeline)))
 
+(defun c-electric-indent-mode-function (char)
+  ;; We never want `electric-indent-mode' to do anything.
+  'no-indent)
+
 \f
 ;; Support for C
 
-- 
2.11.0

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Alan Mackenzie]

Hello, Noam.

On Fri, May 10, 2019 at 23:12:06 -0400, Noam Postavsky wrote:
> Dima Kogan <dima@secretsauce.net> writes:

> > Hi.

> > I'm seeing a regression introduced in 2019/01 by an update meant to make
> > electric-pair-mode and electric-layout-mode play nicely:

> >   http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=fd943124439b7644392919bca8bc2a77e6316d92

> > Recipe:

> > 1. 'emacs -Q' with any emacs more recent than that patch

> > 2. Open up tst.c that contains this:

> > int main(int argc, char* argv[])
> > {
> >     return 0;
> > }


> > 3. Move the point to the beginning of the line containing "return 0"

> > 4. RET RET RET RET RET

> > Now there're a bunch of new lines between the { and the "return 0", as
> > expected. But these lines aren't empty: they contain the initial
> > indentation whitespace. This whitespace shouldn't be there; and it
> > wasn't there prior to this patch.

> Right, the problem is that electric-indent-inhibit only partially
> disables electric indent, and the commit you reference changes which
> parts.  The patch below disables it more completely.  Note however, that
> this makes RET not do any electric indentation at all, just like in the
> good old days of Emacs 24.3.

I don't quite agree with this.  The problem is confusion between effect
2 (below) and electric indentation.  This effect 2 is fundamental editor
functionality, and should be independent, MUST be independent of anything
called "electric indentation".  The two things are conceptually
unrelated.

> Possibly c-mode should rebind RET to a c-electric-return command or
> something.

CC Mode should be able to rely on the proper working of basic editor
functionality.  It shouldn't have to implement its own version of
`newline'.

> >>From 1103fdfbf2be55d68d44151498c2598fd622ac08 Mon Sep 17 00:00:00 2001
> From: Noam Postavsky <npostavs@gmail.com>
> Date: Fri, 10 May 2019 16:40:27 -0400
> Subject: [PATCH] Inhibit electric indent completely in cc mode buffers
>  (Bug#35254)

> Electric indent mode's post-self-insert hook entry has 3 effects:

> 1. Indent the previous line.
> 2. Remove trailing whitespace from the previous line.
> 3. Indent the current line (when at beginning of line).

> The change from 2019-01-22 "electric-layout-mode kicks in before
> electric-pair-mode", makes 'electric-indent-inhibit' inhibit 1 and 2,
> whereas before then it inhibited only 1.  While cc mode provides its
> own electric commands and therefore sets 'electric-indent-inhibit', it
> doesn't implement an electric newline command.  So if only one of
> effects 2 and 3 from Electric indent mode occur, then hitting RET will
> leave trailing whitespace.

> * lisp/progmodes/cc-mode.el (c-electric-indent-mode-function): New
> function.
> (c-basic-common-init): Add it to electric-indent-functions to disable
> electric indent completely (while still letting the
> electric-indent-mode command/setting to control c-electric-flag as
> before).
> ---
>  lisp/progmodes/cc-mode.el | 5 +++++
>  1 file changed, 5 insertions(+)

> diff --git a/lisp/progmodes/cc-mode.el b/lisp/progmodes/cc-mode.el
> index bd62fc754a..e41f1101d0 100644
> --- a/lisp/progmodes/cc-mode.el
> +++ b/lisp/progmodes/cc-mode.el
> @@ -634,6 +634,7 @@ (defun c-basic-common-init (mode default-style)
>    ;; messing up CC Mode's, and set `c-electric-flag' if `electric-indent-mode'
>    ;; has been called by the user.
>    (when (boundp 'electric-indent-inhibit) (setq electric-indent-inhibit t))
> +  (add-hook 'electric-indent-functions 'c-electric-indent-mode-function nil t)
>    ;; CC-mode should obey Emacs's generic preferences, tho only do it if
>    ;; Emacs's generic preferences can be set per-buffer (Emacs>=24.4).
>    (when (fboundp 'electric-indent-local-mode)
> @@ -2143,6 +2144,10 @@ (defun c-electric-indent-local-mode-hook ()
>      (setq c-electric-flag electric-indent-mode)
>      (c-update-modeline)))

> +(defun c-electric-indent-mode-function (char)
> +  ;; We never want `electric-indent-mode' to do anything.
> +  'no-indent)
> +
>  \f
>  ;; Support for C

I'm against this patch.  It is an unpleasant workaround in CC Mode for
problems in the Emacs core.  CC Mode has set electric-indent-inhibit to
t, and the Emacs core should respect that setting.  Using
electric-indent-functions in the way suggested couples CC Mode
undesirably with electric indentation, possibly leading to future
problems when electric indentation gets changed in the future.

In previous Emacs versions (?23.x, ?24.x) there were two simple commands
`newline' and `newline-and-indent'.  As far as I remember, they both
removed trailing WS from the "old" line, possibly as part of the filling
which was done.  They worked, and worked well.  Why can't we get this
functionality back again?

> -- 
> 2.11.0

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Noam Postavsky]

Alan Mackenzie <acm@muc.de> writes:

[rearranged a bit for better flow]

>> Right, the problem is that electric-indent-inhibit only partially
>> disables electric indent, and the commit you reference changes which
>> parts.  The patch below disables it more completely.  Note however, that
>> this makes RET not do any electric indentation at all, just like in the
>> good old days of Emacs 24.3.

>> Subject: [PATCH] Inhibit electric indent completely in cc mode buffers
>
>> Electric indent mode's post-self-insert hook entry has 3 effects:
>
>> 1. Indent the previous line.
>> 2. Remove trailing whitespace from the previous line.
>> 3. Indent the current line (when at beginning of line).

> I don't quite agree with this.  The problem is confusion between effect
> 2 [above] and electric indentation.  This effect 2 is fundamental editor
> functionality, and should be independent, MUST be independent of anything
> called "electric indentation".  The two things are conceptually
> unrelated.

So we should have an electric-delete-trailing-whitespace-mode?

>> The change from 2019-01-22 "electric-layout-mode kicks in before
>> electric-pair-mode", makes 'electric-indent-inhibit' inhibit 1 and 2,
>> whereas before then it inhibited only 1.  While cc mode provides its
>> own electric commands and therefore sets 'electric-indent-inhibit', it
>> doesn't implement an electric newline command.  So if only one of
>> effects 2 and 3 from Electric indent mode occur, then hitting RET will
>> leave trailing whitespace.
>
>> * lisp/progmodes/cc-mode.el (c-electric-indent-mode-function): New
>> function.
>> (c-basic-common-init): Add it to electric-indent-functions to disable
>> electric indent completely (while still letting the
>> electric-indent-mode command/setting to control c-electric-flag as
>> before).

>>    (when (boundp 'electric-indent-inhibit) (setq electric-indent-inhibit t))
>> +  (add-hook 'electric-indent-functions 'c-electric-indent-mode-function nil t)

>> +(defun c-electric-indent-mode-function (char)
>> +  ;; We never want `electric-indent-mode' to do anything.
>> +  'no-indent)

> I'm against this patch.  It is an unpleasant workaround in CC Mode for
> problems in the Emacs core.  CC Mode has set electric-indent-inhibit to
> t, and the Emacs core should respect that setting.  Using
> electric-indent-functions in the way suggested couples CC Mode
> undesirably with electric indentation, possibly leading to future
> problems when electric indentation gets changed in the future.

Hmm, from my point of view, the whole point of the patch is to
*UNcouple* CC mode from electric.el's electric indentation (since CC
mode has its own electric functionality), something that
electric-indent-inhibit does only partially (and based on your response
above I guess the reason for being partial is that there is some
disagreement over which parts exactly constitute "electric
indentation").

>> Possibly c-mode should rebind RET to a c-electric-return command or
>> something.
>
> CC Mode should be able to rely on the proper working of basic editor
> functionality.  It shouldn't have to implement its own version of
> `newline'.

> In previous Emacs versions (?23.x, ?24.x) there were two simple commands
> `newline' and `newline-and-indent'.  As far as I remember, they both
> removed trailing WS from the "old" line, possibly as part of the filling
> which was done.  They worked, and worked well.  Why can't we get this
> functionality back again?

Both these commands still exist.  As far as I can tell, `newline' never
removed trailing WS.  It does have some code to delete the "left
margin", but that doesn't seem to be intended for programming modes
where the margin is 0 (disabled).

`newline-and-indent' did, and still does, delete trailing whitespace.
In 24.4, C-j was rebound from `newline-and-indent' to
`electric-newline-and-maybe-indent' which only calls
`newline-and-indent' if `electric-indent-mode' is nil.  Of course c-mode
could rebind it in its mode map (I considered making
`electric-newline-and-maybe-indent' consult `electric-indent-functions'
as well but that won't work because that hook is supposed to run after
the character is inserted).

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Alan Mackenzie]

Hello, Noam.

On Sat, May 11, 2019 at 10:06:42 -0400, Noam Postavsky wrote:
> Alan Mackenzie <acm@muc.de> writes:

> [rearranged a bit for better flow]

:-)

> >> Right, the problem is that electric-indent-inhibit only partially
> >> disables electric indent, and the commit you reference changes which
> >> parts.  The patch below disables it more completely.  Note however, that
> >> this makes RET not do any electric indentation at all, just like in the
> >> good old days of Emacs 24.3.

> >> Subject: [PATCH] Inhibit electric indent completely in cc mode buffers

> >> Electric indent mode's post-self-insert hook entry has 3 effects:

> >> 1. Indent the previous line.
> >> 2. Remove trailing whitespace from the previous line.
> >> 3. Indent the current line (when at beginning of line).

> > I don't quite agree with this.  The problem is confusion between effect
> > 2 [above] and electric indentation.  This effect 2 is fundamental editor
> > functionality, and should be independent, MUST be independent of anything
> > called "electric indentation".  The two things are conceptually
> > unrelated.

> So we should have an electric-delete-trailing-whitespace-mode?

NO, NO, NO, NO, NO, NO, NO!!!!

Again, the cause of the current problem is that the deletion of the
trailing WS has got mixed up with electric stuff.  General confusion.

Trailing WS deletion has got NOTHING to do with electric indentation.  It
was part of `newline-and-indent' long before anybody started trying to
apply CC Mode's electric stuff to other modes.  It should be part of
`newline' now, not part of electric-indent-post-self-insert-function.

> >> The change from 2019-01-22 "electric-layout-mode kicks in before
> >> electric-pair-mode", makes 'electric-indent-inhibit' inhibit 1 and 2,
> >> whereas before then it inhibited only 1.  While cc mode provides its
> >> own electric commands and therefore sets 'electric-indent-inhibit', it
> >> doesn't implement an electric newline command.  So if only one of
> >> effects 2 and 3 from Electric indent mode occur, then hitting RET will
> >> leave trailing whitespace.

> >> * lisp/progmodes/cc-mode.el (c-electric-indent-mode-function): New
> >> function.
> >> (c-basic-common-init): Add it to electric-indent-functions to disable
> >> electric indent completely (while still letting the
> >> electric-indent-mode command/setting to control c-electric-flag as
> >> before).

> >>    (when (boundp 'electric-indent-inhibit) (setq electric-indent-inhibit t))
> >> +  (add-hook 'electric-indent-functions 'c-electric-indent-mode-function nil t)

> >> +(defun c-electric-indent-mode-function (char)
> >> +  ;; We never want `electric-indent-mode' to do anything.
> >> +  'no-indent)

> > I'm against this patch.  It is an unpleasant workaround in CC Mode for
> > problems in the Emacs core.  CC Mode has set electric-indent-inhibit to
> > t, and the Emacs core should respect that setting.  Using
> > electric-indent-functions in the way suggested couples CC Mode
> > undesirably with electric indentation, possibly leading to future
> > problems when electric indentation gets changed in the future.

> Hmm, from my point of view, the whole point of the patch is to
> *UNcouple* CC mode from electric.el's electric indentation (since CC
> mode has its own electric functionality), something that
> electric-indent-inhibit does only partially (and based on your response
> above I guess the reason for being partial is that there is some
> disagreement over which parts exactly constitute "electric
> indentation").

This further incursion of electric- stuff into CC Mode is an inevitable
consequence of the confusion described above.  Again, setting a single
flag saying NO! should be enough, without having to configure the
mechanisms which are supposedly disabled.

> >> Possibly c-mode should rebind RET to a c-electric-return command or
> >> something.

> > CC Mode should be able to rely on the proper working of basic editor
> > functionality.  It shouldn't have to implement its own version of
> > `newline'.

> > In previous Emacs versions (?23.x, ?24.x) there were two simple commands
> > `newline' and `newline-and-indent'.  As far as I remember, they both
> > removed trailing WS from the "old" line, possibly as part of the filling
> > which was done.  They worked, and worked well.  Why can't we get this
> > functionality back again?

> Both these commands still exist.  As far as I can tell, `newline' never
> removed trailing WS.

I think you're right.  It might even have been an annoyance at the time.
Personally I always used `newline-and-indent', bound to C-j.

> It does have some code to delete the "left margin", but that doesn't
> seem to be intended for programming modes where the margin is 0
> (disabled).

> `newline-and-indent' did, and still does, delete trailing whitespace.
> In 24.4, C-j was rebound from `newline-and-indent' to
> `electric-newline-and-maybe-indent' which only calls
> `newline-and-indent' if `electric-indent-mode' is nil.

Yes.  The ability simply to switch over the two keys between `newline'
and `newline-and-indent' by an option was confused into
electric-indent-mode, something I protested strongly against at the time.
My objections were not addressed, they were simply evaded and brushed
aside.

> Of course c-mode could rebind it in its mode map (I considered making
> `electric-newline-and-maybe-indent' consult `electric-indent-functions'
> as well but that won't work because that hook is supposed to run after
> the character is inserted).

I think we've got enough foggy complexity in the area as it is.  I
suppose CC Mode could bind <CR> to `newline-and-indent', but there's now
no longer a clean function which does what `newline' used to do, to bind
onto C-j.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Basil L. Contovounesios]

Alan Mackenzie <acm@muc.de> writes:

> On Sat, May 11, 2019 at 10:06:42 -0400, Noam Postavsky wrote:
>> Alan Mackenzie <acm@muc.de> writes:
>>
>> Of course c-mode could rebind it in its mode map (I considered making
>> `electric-newline-and-maybe-indent' consult `electric-indent-functions'
>> as well but that won't work because that hook is supposed to run after
>> the character is inserted).
>
> I think we've got enough foggy complexity in the area as it is.  I
> suppose CC Mode could bind <CR> to `newline-and-indent', but there's now
> no longer a clean function which does what `newline' used to do, to bind
> onto C-j.

Sorry if my question is completely naive or irrelevant (I haven't read
the discussion very carefully), but how does the command
c-context-line-break, which is described under "Making the <RET> key
indent the new line" in (info "(ccmode) Getting Started") relate to this
issue, if at all?

Thanks,

-- 
Basil

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Alan Mackenzie]

Hello again, Noam.

On Sat, May 11, 2019 at 16:19:03 +0000, Alan Mackenzie wrote:
> On Sat, May 11, 2019 at 10:06:42 -0400, Noam Postavsky wrote:
> > Alan Mackenzie <acm@muc.de> writes:

[ .... ]

> > So we should have an electric-delete-trailing-whitespace-mode?

> NO, NO, NO, NO, NO, NO, NO!!!!

First of all, apologies for being so unecessarily emphatic, yesterday.

> Again, the cause of the current problem is that the deletion of the
> trailing WS has got mixed up with electric stuff.  General confusion.

> Trailing WS deletion has got NOTHING to do with electric indentation.  It
> was part of `newline-and-indent' long before anybody started trying to
> apply CC Mode's electric stuff to other modes.  It should be part of
> `newline' now, not part of electric-indent-post-self-insert-function.

I think I now understand what's going on.  I was wrong in the previous
paragraph.  The connection between WS deletion and
newline-and-indent/electric indentation is that when n-a-i/e-i is in
play, Emacs assumes that the trailing WS on a line was put there by a
previous use of n-a-i/e-i, therefore it's the Right Thing to remove it.
Otherwise (newline, no electric indentation) the trailing WS stays.

The chaotic tangling of newline and electric indentation remains.  I
still think the best way to fix this bug (and the only way to fix its
cause) is to separate out the two functionalities.  If they hadn't been
tangled in the first place, the current bug couldn't have happened.

Back in Emacs 24.3 (or whenever it was), newline and newline-and-indent
were perfectly adequate for the job.  They still are, IMAO.

I propose that newline be restored to its former functionality (i.e.
with no indentation of the new line) and be bound to C-j; that
newline-and-indent become the standard binding for <CR>; that
indentation of a new line no longer be done by electric indentation,
since this is not needed; that the grossly misnamed, and now redundant
electric-indent-just-newline and the command of dubious utility
electric-newline-and-maybe-indent both be made obsolete.

The above amendments will leave the behaviour of Emacs unchanged for the
normal user.  They will cause mild incompatibilities, the inverse of
those which were caused in 24.4 (or whenever), when the current setup
was set up.

Then bugs like the current one will no longer happen in the future.

Clearly this would need to be discussed and settled in emacs-devel,
first.

What do you say?

[ .... ]

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Alan Mackenzie]

Hello, Basil.

On Sat, May 11, 2019 at 20:34:51 +0100, Basil L. Contovounesios wrote:
> Alan Mackenzie <acm@muc.de> writes:

> > On Sat, May 11, 2019 at 10:06:42 -0400, Noam Postavsky wrote:
> >> Alan Mackenzie <acm@muc.de> writes:

> >> Of course c-mode could rebind it in its mode map (I considered making
> >> `electric-newline-and-maybe-indent' consult `electric-indent-functions'
> >> as well but that won't work because that hook is supposed to run after
> >> the character is inserted).

> > I think we've got enough foggy complexity in the area as it is.  I
> > suppose CC Mode could bind <CR> to `newline-and-indent', but there's now
> > no longer a clean function which does what `newline' used to do, to bind
> > onto C-j.

> Sorry if my question is completely naive or irrelevant (I haven't read
> the discussion very carefully), but how does the command
> c-context-line-break, which is described under "Making the <RET> key
> indent the new line" in (info "(ccmode) Getting Started") relate to this
> issue, if at all?

c-context-line-break doesn't really have much to say in the matter.  The
function is mainly about how to indent the _new_ line, and inserting
various continuation markers.

This bug is about trailing space in the _old_ line not getting removed
on typing <CR>, about which c-context-line-break has nothing to say.

> Thanks,

No problem!

> -- 
> Basil

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Noam Postavsky]

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

Alan Mackenzie <acm@muc.de> writes:

>> > So we should have an electric-delete-trailing-whitespace-mode?
>
>> NO, NO, NO, NO, NO, NO, NO!!!!
>
> First of all, apologies for being so unecessarily emphatic, yesterday.

To be honest, I've gotten used to you being very emphatic in most of
your posts, so it didn't really phase me.

> The connection between WS deletion and newline-and-indent/electric
> indentation is that when n-a-i/e-i is in play, Emacs assumes that the
> trailing WS on a line was put there by a previous use of n-a-i/e-i,
> therefore it's the Right Thing to remove it.  Otherwise (newline, no
> electric indentation) the trailing WS stays.

Yes, this sounds right to me.

> I propose that newline be restored to its former functionality (i.e.
> with no indentation of the new line) and be bound to C-j; that
> newline-and-indent become the standard binding for <CR>; that
> indentation of a new line no longer be done by electric indentation,
> since this is not needed; that the grossly misnamed, and now redundant
> electric-indent-just-newline and the command of dubious utility
> electric-newline-and-maybe-indent both be made obsolete.

So like this: (I didn't do any obsoletion, since it doesn't affect
testing)


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

From cbaf6ee73f9129fdb8f1e45ba680ca029981d290 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 12 May 2019 13:53:31 -0400
Subject: [PATCH] Bind RET to newline-and-indent (Bug#35254)

* lisp/bindings.el: Bind RET to newline-and-indent, C-j to newline.
* lisp/electric.el: Don't bind electric-newline-and-maybe-indent to
C-j.
(electric-indent-chars): Remove newline from default value.
---
 lisp/bindings.el | 3 ++-
 lisp/electric.el | 4 +---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/lisp/bindings.el b/lisp/bindings.el
index 744bcc36a8..43b4f8b8cf 100644
--- a/lisp/bindings.el
+++ b/lisp/bindings.el
@@ -892,7 +892,8 @@ (define-key global-map "\C-g" 'keyboard-quit)
 ;; suspend only the relevant terminal.
 (substitute-key-definition 'suspend-emacs 'suspend-frame global-map)
 
-(define-key global-map "\C-m" 'newline)
+(define-key global-map "\C-m" 'newline-and-indent)
+(define-key global-map "\C-j" 'newline)
 (define-key global-map "\C-o" 'open-line)
 (define-key esc-map "\C-o" 'split-line)
 (define-key global-map "\C-q" 'quoted-insert)
diff --git a/lisp/electric.el b/lisp/electric.el
index 07da2f1d9e..e5c318e34f 100644
--- a/lisp/electric.el
+++ b/lisp/electric.el
@@ -207,7 +207,7 @@ (defun electric--sort-post-self-insertion-hook ()
 ;; should usually set this variable by adding elements to the default
 ;; value, which only works well if the variable is preloaded.
 ;;;###autoload
-(defvar electric-indent-chars '(?\n)
+(defvar electric-indent-chars '()
   "Characters that should cause automatic reindentation.")
 
 (defvar electric-indent-functions nil
@@ -306,8 +306,6 @@ (defun electric-indent-just-newline (arg)
     (newline arg 'interactive)))
 
 ;;;###autoload
-(define-key global-map "\C-j" 'electric-newline-and-maybe-indent)
-;;;###autoload
 (defun electric-newline-and-maybe-indent ()
   "Insert a newline.
 If `electric-indent-mode' is enabled, that's that, but if it
-- 
2.11.0


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


> The above amendments will leave the behaviour of Emacs unchanged for the
> normal user.  They will cause mild incompatibilities, the inverse of
> those which were caused in 24.4 (or whenever), when the current setup
> was set up.
>
> Then bugs like the current one will no longer happen in the future.
>
> Clearly this would need to be discussed and settled in emacs-devel,
> first.
>
> What do you say?

I'm one of the "normal" users who wouldn't be affected, so it's fine for
me.  And I kind of think binding C-j to newline makes more sense anyway
(?\C-j is literally the newline character, after all).  Pushing through
changes to default keybindings is always a tricky proposition though.  I
have the impression that part of the reason for implementing the newline
indentation via electric mode was because of this (i.e., add auto
indentation to RET, without changing its binding).

On the other hand, I don't think the idea of letting electric mode
handle the indent on newline is quite so illogical as you say.  The idea
of electric indent, as I understand it, is that when you insert some
character, Emacs will automatically perform indentation for you.
Newline is a character, so why not let electric indent perform
indentation after inserting it?

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Basil L. Contovounesios]

Alan Mackenzie <acm@muc.de> writes:

> On Sat, May 11, 2019 at 20:34:51 +0100, Basil L. Contovounesios wrote:
>> Alan Mackenzie <acm@muc.de> writes:
>
>> > On Sat, May 11, 2019 at 10:06:42 -0400, Noam Postavsky wrote:
>> >> Alan Mackenzie <acm@muc.de> writes:
>
>> >> Of course c-mode could rebind it in its mode map (I considered making
>> >> `electric-newline-and-maybe-indent' consult `electric-indent-functions'
>> >> as well but that won't work because that hook is supposed to run after
>> >> the character is inserted).
>
>> > I think we've got enough foggy complexity in the area as it is.  I
>> > suppose CC Mode could bind <CR> to `newline-and-indent', but there's now
>> > no longer a clean function which does what `newline' used to do, to bind
>> > onto C-j.
>
>> Sorry if my question is completely naive or irrelevant (I haven't read
>> the discussion very carefully), but how does the command
>> c-context-line-break, which is described under "Making the <RET> key
>> indent the new line" in (info "(ccmode) Getting Started") relate to this
>> issue, if at all?
>
> c-context-line-break doesn't really have much to say in the matter.  The
> function is mainly about how to indent the _new_ line, and inserting
> various continuation markers.
>
> This bug is about trailing space in the _old_ line not getting removed
> on typing <CR>, about which c-context-line-break has nothing to say.

AFAICS c-context-line-break removes trailing space on the old line:

0. emacs -Q
1. C-x h C-w
2. M-x c-mode RET
3. int main() {
4. RET RET

Line 2 now contains two trailing spaces.

5. M-x c-context-line-break RET

Line 3 is now empty (has no trailing space).

Have I misunderstood something?

Thanks,

-- 
Basil

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Alan Mackenzie]

Hello, Basil.

On Sun, May 12, 2019 at 22:45:09 +0100, Basil L. Contovounesios wrote:
> Alan Mackenzie <acm@muc.de> writes:

> > On Sat, May 11, 2019 at 20:34:51 +0100, Basil L. Contovounesios wrote:

> >> Sorry if my question is completely naive or irrelevant (I haven't read
> >> the discussion very carefully), but how does the command
> >> c-context-line-break, which is described under "Making the <RET> key
> >> indent the new line" in (info "(ccmode) Getting Started") relate to this
> >> issue, if at all?

> > c-context-line-break doesn't really have much to say in the matter.  The
> > function is mainly about how to indent the _new_ line, and inserting
> > various continuation markers.

> > This bug is about trailing space in the _old_ line not getting removed
> > on typing <CR>, about which c-context-line-break has nothing to say.

> AFAICS c-context-line-break removes trailing space on the old line:

> 0. emacs -Q
> 1. C-x h C-w
> 2. M-x c-mode RET
> 3. int main() {
> 4. RET RET

> Line 2 now contains two trailing spaces.

> 5. M-x c-context-line-break RET

> Line 3 is now empty (has no trailing space).

> Have I misunderstood something?

Er, no.  You're right, c-context-line-break does indeed remove the
trailing WS, at least on normal code lines.  Sorry about the mistake.

But I don't think I've really understood how this observation fits in
with the bug scenario.  The bug is about the current master's default
binding of <CR> (namely newline) not removing the trailing whitespace
from the line it's typed in.

I think you might be suggesting binding c-context-line-break to <CR> in
CC Mode as a workaround for the problem; or possibly using its ideas to
code up a CC Mode version of newline.

I still think the bug should be fixed in the Emacs core, so that other
modes which want the old line to have trailing spaces removed, yet don't
use electric-indent-mode, will just work.

> Thanks,

> -- 
> Basil

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Basil L. Contovounesios]

Alan Mackenzie <acm@muc.de> writes:

> On Sun, May 12, 2019 at 22:45:09 +0100, Basil L. Contovounesios wrote:
>> Alan Mackenzie <acm@muc.de> writes:
>
>> > On Sat, May 11, 2019 at 20:34:51 +0100, Basil L. Contovounesios wrote:
>
>> >> Sorry if my question is completely naive or irrelevant (I haven't read
>> >> the discussion very carefully), but how does the command
>> >> c-context-line-break, which is described under "Making the <RET> key
>> >> indent the new line" in (info "(ccmode) Getting Started") relate to this
>> >> issue, if at all?
>
>> > c-context-line-break doesn't really have much to say in the matter.  The
>> > function is mainly about how to indent the _new_ line, and inserting
>> > various continuation markers.
>
>> > This bug is about trailing space in the _old_ line not getting removed
>> > on typing <CR>, about which c-context-line-break has nothing to say.
>
>> AFAICS c-context-line-break removes trailing space on the old line:
>
>> 0. emacs -Q
>> 1. C-x h C-w
>> 2. M-x c-mode RET
>> 3. int main() {
>> 4. RET RET
>
>> Line 2 now contains two trailing spaces.
>
>> 5. M-x c-context-line-break RET
>
>> Line 3 is now empty (has no trailing space).
>
>> Have I misunderstood something?
>
> Er, no.  You're right, c-context-line-break does indeed remove the
> trailing WS, at least on normal code lines.  Sorry about the mistake.
>
> But I don't think I've really understood how this observation fits in
> with the bug scenario.  The bug is about the current master's default
> binding of <CR> (namely newline) not removing the trailing whitespace
> from the line it's typed in.
>
> I think you might be suggesting binding c-context-line-break to <CR> in
> CC Mode as a workaround for the problem; or possibly using its ideas to
> code up a CC Mode version of newline.

(Or pointing users in its direction should they wish to configure this
behaviour themselves.)

> I still think the bug should be fixed in the Emacs core, so that other
> modes which want the old line to have trailing spaces removed, yet don't
> use electric-indent-mode, will just work.

Indeed, that would be nice.

Thanks,

-- 
Basil

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Alan Mackenzie]

Hello, João and Noam.

On Fri, May 10, 2019 at 23:12:06 -0400, Noam Postavsky wrote:
> Dima Kogan <dima@secretsauce.net> writes:

> > Hi.

> > I'm seeing a regression introduced in 2019/01 by an update meant to make
> > electric-pair-mode and electric-layout-mode play nicely:

> >   http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=fd943124439b7644392919bca8bc2a77e6316d92

> > Recipe:

> > 1. 'emacs -Q' with any emacs more recent than that patch

> > 2. Open up tst.c that contains this:

> > int main(int argc, char* argv[])
> > {
> >     return 0;
> > }


> > 3. Move the point to the beginning of the line containing "return 0"

> > 4. RET RET RET RET RET

> > Now there're a bunch of new lines between the { and the "return 0", as
> > expected. But these lines aren't empty: they contain the initial
> > indentation whitespace. This whitespace shouldn't be there; and it
> > wasn't there prior to this patch.

> Right, the problem is that electric-indent-inhibit only partially
> disables electric indent, and the commit you reference changes which
> parts.  The patch below disables it more completely.  Note however, that
> this makes RET not do any electric indentation at all, just like in the
> good old days of Emacs 24.3.  Possibly c-mode should rebind RET to a
> c-electric-return command or something.

> >>From 1103fdfbf2be55d68d44151498c2598fd622ac08 Mon Sep 17 00:00:00 2001
> From: Noam Postavsky <npostavs@gmail.com>
> Date: Fri, 10 May 2019 16:40:27 -0400
> Subject: [PATCH] Inhibit electric indent completely in cc mode buffers
>  (Bug#35254)

> Electric indent mode's post-self-insert hook entry has 3 effects:

> 1. Indent the previous line.
> 2. Remove trailing whitespace from the previous line.
> 3. Indent the current line (when at beginning of line).

> The change from 2019-01-22 "electric-layout-mode kicks in before
> electric-pair-mode", makes 'electric-indent-inhibit' inhibit 1 and 2,
> whereas before then it inhibited only 1.  While cc mode provides its
> own electric commands and therefore sets 'electric-indent-inhibit', it
> doesn't implement an electric newline command.  So if only one of
> effects 2 and 3 from Electric indent mode occur, then hitting RET will
> leave trailing whitespace.

I interpret the problem a little differently.
electric-indent-post-self-insert-function, when electric-indent-inhibit
is set, is inhibiting actions which are not really part of electric
indentation, in particular action 2 (above).  This is the heart of the
bug.  The following patch fixes the bug.  It would need tidying up before
being committed:



diff --git a/lisp/electric.el b/lisp/electric.el
index 07da2f1d9e..15a42930c1 100644
--- a/lisp/electric.el
+++ b/lisp/electric.el
@@ -282,9 +282,15 @@ electric-indent-post-self-insert-function
                   (condition-case-unless-debug ()
                       (indent-according-to-mode)
                     (error (throw 'indent-error nil)))
-                  ;; The goal here will be to remove the trailing
-                  ;; whitespace after reindentation of the previous line
-                  ;; because that may have (re)introduced it.
+                  )
+                (unless (memq indent-line-function
+                              electric-indent-functions-without-reindent)
+                  ;; The goal here will be to remove the indentation
+                  ;; whitespace from an otherwise blank line after
+                  ;; typing <CR> twice in succession.  Also to remove
+                  ;; trailing whitespace after reindentation of the
+                  ;; previous line because that may have
+                  ;; (re)introduced it.
                   (goto-char before)
                   ;; We were at EOL in marker `before' before the call
                   ;; to `indent-according-to-mode' but after we may


João and Noam, what're your views on this proposed patch?

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[João Távora]

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

On Mon, May 13, 2019 at 8:53 PM Alan Mackenzie <acm@muc.de> wrote:

> Hello, João and Noam.
>
> On Fri, May 10, 2019 at 23:12:06 -0400, Noam Postavsky wrote:
> > Dima Kogan <dima@secretsauce.net> writes:
>
> > > Hi.
>
> > > I'm seeing a regression introduced in 2019/01 by an update meant to
> make
> > > electric-pair-mode and electric-layout-mode play nicely:
>
> > >
> http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=fd943124439b7644392919bca8bc2a77e6316d92
>
> > > Recipe:
>
> > > 1. 'emacs -Q' with any emacs more recent than that patch
>
> > > 2. Open up tst.c that contains this:
>
> > > int main(int argc, char* argv[])
> > > {
> > >     return 0;
> > > }
>
>
> > > 3. Move the point to the beginning of the line containing "return 0"
>
> > > 4. RET RET RET RET RET
>
> > > Now there're a bunch of new lines between the { and the "return 0", as
> > > expected. But these lines aren't empty: they contain the initial
> > > indentation whitespace. This whitespace shouldn't be there; and it
> > > wasn't there prior to this patch.
>
> > Right, the problem is that electric-indent-inhibit only partially
> > disables electric indent, and the commit you reference changes which
> > parts.  The patch below disables it more completely.  Note however, that
> > this makes RET not do any electric indentation at all, just like in the
> > good old days of Emacs 24.3.  Possibly c-mode should rebind RET to a
> > c-electric-return command or something.
>
> > >>From 1103fdfbf2be55d68d44151498c2598fd622ac08 Mon Sep 17 00:00:00 2001
> > From: Noam Postavsky <npostavs@gmail.com>
> > Date: Fri, 10 May 2019 16:40:27 -0400
> > Subject: [PATCH] Inhibit electric indent completely in cc mode buffers
> >  (Bug#35254)
>
> > Electric indent mode's post-self-insert hook entry has 3 effects:
>
> > 1. Indent the previous line.
> > 2. Remove trailing whitespace from the previous line.
> > 3. Indent the current line (when at beginning of line).
>
> > The change from 2019-01-22 "electric-layout-mode kicks in before
> > electric-pair-mode", makes 'electric-indent-inhibit' inhibit 1 and 2,
> > whereas before then it inhibited only 1.  While cc mode provides its
> > own electric commands and therefore sets 'electric-indent-inhibit', it
> > doesn't implement an electric newline command.  So if only one of
> > effects 2 and 3 from Electric indent mode occur, then hitting RET will
> > leave trailing whitespace.
>
> I interpret the problem a little differently.
> electric-indent-post-self-insert-function, when electric-indent-inhibit
> is set, is inhibiting actions which are not really part of electric
> indentation, in particular action 2 (above).  This is the heart of the
> bug.  The following patch fixes the bug.  It would need tidying up before
> being committed:
>
>
>
> diff --git a/lisp/electric.el b/lisp/electric.el
> index 07da2f1d9e..15a42930c1 100644
> --- a/lisp/electric.el
> +++ b/lisp/electric.el
> @@ -282,9 +282,15 @@ electric-indent-post-self-insert-function
>                    (condition-case-unless-debug ()
>                        (indent-according-to-mode)
>                      (error (throw 'indent-error nil)))
> -                  ;; The goal here will be to remove the trailing
> -                  ;; whitespace after reindentation of the previous line
> -                  ;; because that may have (re)introduced it.
> +                  )
> +                (unless (memq indent-line-function
> +                              electric-indent-functions-without-reindent)
> +                  ;; The goal here will be to remove the indentation
> +                  ;; whitespace from an otherwise blank line after
> +                  ;; typing <CR> twice in succession.  Also to remove
> +                  ;; trailing whitespace after reindentation of the
> +                  ;; previous line because that may have
> +                  ;; (re)introduced it.
>                    (goto-char before)
>                    ;; We were at EOL in marker `before' before the call
>                    ;; to `indent-according-to-mode' but after we may
>
>
> João and Noam, what're your views on this proposed patch?
>
> Good night Alan,

Unfortunately, I can't analyse this in depth right now or in the next few
days.
I can only ask succintly:

1. Does it fix the reported problem (assuming it is a problem, and not an
otherwise
   potentially desirable change in behaviour)?
2. Do any of you have suspicions that it might introduce problems
elsewhere?
3. Does it pass the automated test suite?

I am only a bit wary of it because, without having understood the problem
in depth,
it seems again caused by a c-electric quirk. My views on this are known: I
always prefer
that c-mode would, if slowly, move in the direction of accepting
electric.el abstractions as
closely as possible.

Than said, the patch looks very simple, which is always good, and has
comments explaining what's going on. So I'll let Noam (and Stefan?) decide.

João

[-- Attachment #2: Type: text/html, Size: 6541 bytes --]

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Stefan Monnier]

>> Electric indent mode's post-self-insert hook entry has 3 effects:
>
>> 1. Indent the previous line.
>> 2. Remove trailing whitespace from the previous line.
>> 3. Indent the current line (when at beginning of line).

Note that `newline` itself already does some subset of 2 (before running
electric-indent's post-self-insert hook).

> I interpret the problem a little differently.
> electric-indent-post-self-insert-function, when electric-indent-inhibit
> is set, is inhibiting actions which are not really part of electric
> indentation, in particular action 2 (above).  This is the heart of the
> bug.  The following patch fixes the bug.  It would need tidying up before
> being committed:

I haven't followed enough of the discussion to understand why that might
cause a bug.

> diff --git a/lisp/electric.el b/lisp/electric.el
> index 07da2f1d9e..15a42930c1 100644
> --- a/lisp/electric.el
> +++ b/lisp/electric.el
> @@ -282,9 +282,15 @@ electric-indent-post-self-insert-function
>                    (condition-case-unless-debug ()
>                        (indent-according-to-mode)
>                      (error (throw 'indent-error nil)))
> -                  ;; The goal here will be to remove the trailing
> -                  ;; whitespace after reindentation of the previous line
> -                  ;; because that may have (re)introduced it.
> +                  )
> +                (unless (memq indent-line-function
> +                              electric-indent-functions-without-reindent)
> +                  ;; The goal here will be to remove the indentation
> +                  ;; whitespace from an otherwise blank line after
> +                  ;; typing <CR> twice in succession.  Also to remove
> +                  ;; trailing whitespace after reindentation of the
> +                  ;; previous line because that may have
> +                  ;; (re)introduced it.
>                    (goto-char before)
>                    ;; We were at EOL in marker `before' before the call
>                    ;; to `indent-according-to-mode' but after we may

I don't understand why you distinguish

    electric-indent-inhibit

from

    (memq indent-line-function
          electric-indent-functions-without-reindent)

When I introduced these, electric-indent-functions-without-reindent was
only meant to paper over those pre-existing cases that don't set
electric-indent-inhibit.

So, I'd suggest an even simpler patch which just closes the `unless`
earlier.  Would that work?


        Stefan


diff --git a/lisp/electric.el b/lisp/electric.el
index 07da2f1d9e..beb3348755 100644
--- a/lisp/electric.el
+++ b/lisp/electric.el
@@ -281,7 +281,7 @@ electric-indent-post-self-insert-function
                   (goto-char before)
                   (condition-case-unless-debug ()
                       (indent-according-to-mode)
-                    (error (throw 'indent-error nil)))
+                    (error (throw 'indent-error nil))))
                   ;; The goal here will be to remove the trailing
                   ;; whitespace after reindentation of the previous line
                   ;; because that may have (re)introduced it.
@@ -290,7 +290,7 @@ electric-indent-post-self-insert-function
                   ;; to `indent-according-to-mode' but after we may
                   ;; not be (Bug#15767).
                   (when (and (eolp))
-                    (delete-horizontal-space t))))))
+                    (delete-horizontal-space t)))))
           (unless (and electric-indent-inhibit
                        (not at-newline))
             (condition-case-unless-debug ()

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Noam Postavsky]

João Távora <joaotavora@gmail.com> writes:

>> > Electric indent mode's post-self-insert hook entry has 3 effects:
>>
>> > 1. Indent the previous line.
>> > 2. Remove trailing whitespace from the previous line.
>> > 3. Indent the current line (when at beginning of line).
>>
>> > The change from 2019-01-22 "electric-layout-mode kicks in before
>> > electric-pair-mode", makes 'electric-indent-inhibit' inhibit 1 and 2,
>> > whereas before then it inhibited only 1.  While cc mode provides its
>> > own electric commands and therefore sets 'electric-indent-inhibit', it
>> > doesn't implement an electric newline command.  So if only one of
>> > effects 2 and 3 from Electric indent mode occur, then hitting RET will
>> > leave trailing whitespace.
>>
>> I interpret the problem a little differently.
>> electric-indent-post-self-insert-function, when electric-indent-inhibit
>> is set, is inhibiting actions which are not really part of electric
>> indentation, in particular action 2 (above).  This is the heart of the
>> bug.  The following patch fixes the bug.  It would need tidying up before
>> being committed:
>>
>>
>>
>> diff --git a/lisp/electric.el b/lisp/electric.el
>> index 07da2f1d9e..15a42930c1 100644
>> --- a/lisp/electric.el
>> +++ b/lisp/electric.el
>> @@ -282,9 +282,15 @@ electric-indent-post-self-insert-function
>>                    (condition-case-unless-debug ()
>>                        (indent-according-to-mode)
>>                      (error (throw 'indent-error nil)))
>> -                  ;; The goal here will be to remove the trailing
>> -                  ;; whitespace after reindentation of the previous line
>> -                  ;; because that may have (re)introduced it.
>> +                  )
>> +                (unless (memq indent-line-function
>> +                              electric-indent-functions-without-reindent)
>> +                  ;; The goal here will be to remove the indentation
>> +                  ;; whitespace from an otherwise blank line after
>> +                  ;; typing <CR> twice in succession.  Also to remove
>> +                  ;; trailing whitespace after reindentation of the
>> +                  ;; previous line because that may have
>> +                  ;; (re)introduced it.
>>                    (goto-char before)
>>                    ;; We were at EOL in marker `before' before the call
>>                    ;; to `indent-according-to-mode' but after we may
>>
>>
>> João and Noam, what're your views on this proposed patch?

> 1. Does it fix the reported problem (assuming it is a problem, and not
> an otherwise potentially desirable change in behaviour)?

It does fix the problem.

> 2. Do any of you have suspicions that it might introduce problems
> elsewhere?

I'm unsure.  It seems to be undoing a small part of [fd94312443]
2019-01-22 "electric-layout-mode kicks in before electric-pair-mode", so
I guess it might rebreak whatever that commit is fixing.  But I don't
quite understand what that commit is fixing (in particular, where the
commit message says "which can be a problem in some modes", which modes
are those?  What is "a problem"?).

> 3. Does it pass the automated test suite?

No, it breaks 3 tests in tests/lisp/electric.el:

3 unexpected results:
   FAILED  electric-layout-int-main-kernel-style
   FAILED  electric-layout-plainer-c-mode-use-c-style
   FAILED  electric-modes-int-main-allman-style

In each case, the reason for failure is that the expected result has
trailing whitespace that the actual result misses.  I guess
electric-layout does want to put trailing whitespace in certain cases?

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Noam Postavsky]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>>> Electric indent mode's post-self-insert hook entry has 3 effects:
>>
>>> 1. Indent the previous line.
>>> 2. Remove trailing whitespace from the previous line.
>>> 3. Indent the current line (when at beginning of line).
>
> Note that `newline` itself already does some subset of 2 (before running
> electric-indent's post-self-insert hook).

Do you mean `newline-and-indent`?   Or are you talking about the margin
stuff? (which doesn't apply to progmodes usually, as far as I can tell).

> I don't understand why you distinguish
>
>     electric-indent-inhibit
>
> from
>
>     (memq indent-line-function
>           electric-indent-functions-without-reindent)
>
> When I introduced these, electric-indent-functions-without-reindent was
> only meant to paper over those pre-existing cases that don't set
> electric-indent-inhibit.
>
> So, I'd suggest an even simpler patch which just closes the `unless`
> earlier.  Would that work?

It has the same effect as Alan's patch: effectively reverses a bit of
João's "electric-layout-mode kicks in before electric-pair-mode" change,
and breaks the same 3 tests I mentioned in https://debbugs.gnu.org/35254#50.

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[João Távora]

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

On Tue, May 14, 2019 at 12:38 AM Noam Postavsky <npostavs@gmail.com> wrote:

>
> > 1. Does it fix the reported problem (assuming it is a problem, and not
> > an otherwise potentially desirable change in behaviour)?
>
> It does fix the problem.
>

It reintroduces the previous behaviour, I gather. Can you explain
quickly why it was "a problem"?


> > 2. Do any of you have suspicions that it might introduce problems
> > elsewhere?
>
> I'm unsure.  It seems to be undoing a small part of [fd94312443]
> 2019-01-22 "electric-layout-mode kicks in before electric-pair-mode", so
> I guess it might rebreak whatever that commit is fixing.  But I don't
> quite understand what that commit is fixing (in particular, where the
> commit message says "which can be a problem in some modes", which modes
> are those?  What is "a problem"?).
>

Sorry, can't say without investigating much more than time allows. Can you
post the complete sentence?

I vaguely remember that if electric-pair-mode kicked in before
electric-layout-mode we would need more complex layout specs and more
painful indentation logic.  That's why I changed it.  There is a thread of
discussion with Stefan somewhere about this, not sure if public or off-list.


> > 3. Does it pass the automated test suite?
>
> No, it breaks 3 tests in tests/lisp/electric.el:
>
> 3 unexpected results:
>    FAILED  electric-layout-int-main-kernel-style
>    FAILED  electric-layout-plainer-c-mode-use-c-style
>    FAILED  electric-modes-int-main-allman-style
>
> In each case, the reason for failure is that the expected result has
> trailing whitespace that the actual result misses.  I guess
> electric-layout does want to put trailing whitespace in certain cases?
>

Yes, it certainly does.  That trailing whitespace is indentation, I
believe. And
the cursor should be left at that indentation.  Can you confirm? Anyway, if
it's
breaking tests it's almost certainly not what we want.  And if it breaks in
"plainer-c-mode" (a slightly better behaved c-mode), then its even more
certain that it's not what we want.

... unless the tests are demading something unreasonable from the electric
modes, of course.

-- 
João Távora

[-- Attachment #2: Type: text/html, Size: 3239 bytes --]

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Stefan Monnier]

> Do you mean `newline-and-indent`?   Or are you talking about the margin
> stuff? (which doesn't apply to progmodes usually, as far as I can tell).

I'm talking about "the margin stuff".

>> So, I'd suggest an even simpler patch which just closes the `unless`
>> earlier.  Would that work?
> It has the same effect as Alan's patch:

Indeed.  I think it just fixes an inconsistency in Alan's patch (I
assume Alan didn't know that (memq indent-line-function
electric-indent-functions-without-reindent) is a way to catch some
modes that failed to set electric-indent-inhibit).


        Stefan

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Stefan Monnier]

> 3 unexpected results:
>    FAILED  electric-layout-int-main-kernel-style
>    FAILED  electric-layout-plainer-c-mode-use-c-style
>    FAILED  electric-modes-int-main-allman-style
>
> In each case, the reason for failure is that the expected result has
> trailing whitespace that the actual result misses.  I guess
> electric-layout does want to put trailing whitespace in certain cases?

Indeed, it does because point ends up there (which is why it's not
considered as "trailing space" but just "proper indentation").


        Stefan

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Noam Postavsky]

João Távora <joaotavora@gmail.com> writes:

> Unfortunately, I can't analyse this in depth right now or in the next few
> days.

> Sorry, can't say without investigating much more than time allows.

I don't think there is really any rush here.  In fact, I think things
have a better chance of going smoothly if we all take our time composing
our messages.  So I'll give a fuller reply on the weekend.

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Alan Mackenzie]

Hello, João.

On Mon, May 13, 2019 at 23:39:33 +0100, João Távora wrote:
> On Mon, May 13, 2019 at 8:53 PM Alan Mackenzie <acm@muc.de> wrote:

[ .... ]

> > > Electric indent mode's post-self-insert hook entry has 3 effects:

> > > 1. Indent the previous line.
> > > 2. Remove trailing whitespace from the previous line.
> > > 3. Indent the current line (when at beginning of line).

> > > The change from 2019-01-22 "electric-layout-mode kicks in before
> > > electric-pair-mode", makes 'electric-indent-inhibit' inhibit 1 and 2,
> > > whereas before then it inhibited only 1.  While cc mode provides its
> > > own electric commands and therefore sets 'electric-indent-inhibit', it
> > > doesn't implement an electric newline command.  So if only one of
> > > effects 2 and 3 from Electric indent mode occur, then hitting RET will
> > > leave trailing whitespace.

> > I interpret the problem a little differently.
> > electric-indent-post-self-insert-function, when electric-indent-inhibit
> > is set, is inhibiting actions which are not really part of electric
> > indentation, in particular action 2 (above).  This is the heart of the
> > bug.  The following patch fixes the bug.  It would need tidying up before
> > being committed:



> > diff --git a/lisp/electric.el b/lisp/electric.el
> > index 07da2f1d9e..15a42930c1 100644
> > --- a/lisp/electric.el
> > +++ b/lisp/electric.el
> > @@ -282,9 +282,15 @@ electric-indent-post-self-insert-function
> >                    (condition-case-unless-debug ()
> >                        (indent-according-to-mode)
> >                      (error (throw 'indent-error nil)))
> > -                  ;; The goal here will be to remove the trailing
> > -                  ;; whitespace after reindentation of the previous line
> > -                  ;; because that may have (re)introduced it.
> > +                  )
> > +                (unless (memq indent-line-function
> > +                              electric-indent-functions-without-reindent)
> > +                  ;; The goal here will be to remove the indentation
> > +                  ;; whitespace from an otherwise blank line after
> > +                  ;; typing <CR> twice in succession.  Also to remove
> > +                  ;; trailing whitespace after reindentation of the
> > +                  ;; previous line because that may have
> > +                  ;; (re)introduced it.
> >                    (goto-char before)
> >                    ;; We were at EOL in marker `before' before the call
> >                    ;; to `indent-according-to-mode' but after we may


> > João and Noam, what're your views on this proposed patch?

> > Good night Alan,

> Unfortunately, I can't analyse this in depth right now or in the next
> few days.

As Stefan(?) said, there's no particular hurry (on a scale of days) to
fix this.

> I can only ask succintly:

> 1. Does it fix the reported problem (assuming it is a problem, and not an
> otherwise potentially desirable change in behaviour)?

It does fix it, yes.  It was a bug.

> 2. Do any of you have suspicions that it might introduce problems
> elsewhere?

I don't, as such, but I haven't dug around the code looking for potential
problems.  That was the main reason I was asking you, as maintainer of
the electric stuff.

> 3. Does it pass the automated test suite?

Noam has already tried this and said it fails on three tests.

> I am only a bit wary of it because, without having understood the
> problem in depth, it seems again caused by a c-electric quirk.

The same bug occurs in Python Mode.

Succinctly, the bug is that on pressing <CR> lots of times in a row, the
indentation WS is being left on the blank lines rather than being
removed.

> My views on this are known: I always prefer that c-mode would, if
> slowly, move in the direction of accepting electric.el abstractions as
> closely as possible.

I think we've already agreed that CC Mode should, in the long term,
change to using the electric-* facilities, assuming no loss in
functionality.  :-)

> Than said, the patch looks very simple, which is always good, and has
> comments explaining what's going on. So I'll let Noam (and Stefan?) decide.

> João

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Alan Mackenzie]

Hello, Stefan.

On Mon, May 13, 2019 at 19:32:49 -0400, Stefan Monnier wrote:
> >> Electric indent mode's post-self-insert hook entry has 3 effects:

> >> 1. Indent the previous line.
> >> 2. Remove trailing whitespace from the previous line.
> >> 3. Indent the current line (when at beginning of line).

> Note that `newline` itself already does some subset of 2 (before running
> electric-indent's post-self-insert hook).

> > I interpret the problem a little differently.
> > electric-indent-post-self-insert-function, when electric-indent-inhibit
> > is set, is inhibiting actions which are not really part of electric
> > indentation, in particular action 2 (above).  This is the heart of the
> > bug.  The following patch fixes the bug.  It would need tidying up before
> > being committed:

> I haven't followed enough of the discussion to understand why that might
> cause a bug.

The bug is, type lots of <CR>s in a row; the indentation WS isn't
getting removed from the blank lines.  Currently electric-indent-inhibit
is inhibiting this removal.

> > diff --git a/lisp/electric.el b/lisp/electric.el
> > index 07da2f1d9e..15a42930c1 100644
> > --- a/lisp/electric.el
> > +++ b/lisp/electric.el
> > @@ -282,9 +282,15 @@ electric-indent-post-self-insert-function
> >                    (condition-case-unless-debug ()
> >                        (indent-according-to-mode)
> >                      (error (throw 'indent-error nil)))
> > -                  ;; The goal here will be to remove the trailing
> > -                  ;; whitespace after reindentation of the previous line
> > -                  ;; because that may have (re)introduced it.
> > +                  )
> > +                (unless (memq indent-line-function
> > +                              electric-indent-functions-without-reindent)
> > +                  ;; The goal here will be to remove the indentation
> > +                  ;; whitespace from an otherwise blank line after
> > +                  ;; typing <CR> twice in succession.  Also to remove
> > +                  ;; trailing whitespace after reindentation of the
> > +                  ;; previous line because that may have
> > +                  ;; (re)introduced it.
> >                    (goto-char before)
> >                    ;; We were at EOL in marker `before' before the call
> >                    ;; to `indent-according-to-mode' but after we may

> I don't understand why you distinguish

>     electric-indent-inhibit

> from

>     (memq indent-line-function
>           electric-indent-functions-without-reindent)

It was quite late when I worked out the patch.  I just did it
mechanically, to get the discussion moving.

> When I introduced these, electric-indent-functions-without-reindent was
> only meant to paper over those pre-existing cases that don't set
> electric-indent-inhibit.

> So, I'd suggest an even simpler patch which just closes the `unless`
> earlier.  Would that work?

Probably.  Maybe João should check this, once he's fully back with us.

>         Stefan


> diff --git a/lisp/electric.el b/lisp/electric.el
> index 07da2f1d9e..beb3348755 100644
> --- a/lisp/electric.el
> +++ b/lisp/electric.el
> @@ -281,7 +281,7 @@ electric-indent-post-self-insert-function
>                    (goto-char before)
>                    (condition-case-unless-debug ()
>                        (indent-according-to-mode)
> -                    (error (throw 'indent-error nil)))
> +                    (error (throw 'indent-error nil))))
>                    ;; The goal here will be to remove the trailing
>                    ;; whitespace after reindentation of the previous line
>                    ;; because that may have (re)introduced it.
> @@ -290,7 +290,7 @@ electric-indent-post-self-insert-function
>                    ;; to `indent-according-to-mode' but after we may
>                    ;; not be (Bug#15767).
>                    (when (and (eolp))
> -                    (delete-horizontal-space t))))))
> +                    (delete-horizontal-space t)))))
>            (unless (and electric-indent-inhibit
>                         (not at-newline))
>              (condition-case-unless-debug ()

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Alan Mackenzie]

Hello, Stefan.

On Mon, May 13, 2019 at 19:32:49 -0400, Stefan Monnier wrote:
> >> Electric indent mode's post-self-insert hook entry has 3 effects:

> >> 1. Indent the previous line.
> >> 2. Remove trailing whitespace from the previous line.
> >> 3. Indent the current line (when at beginning of line).

> Note that `newline` itself already does some subset of 2 (before running
> electric-indent's post-self-insert hook).

Speaking of `newline', there's a strange regexp around this part:

            ;; If the newline leaves the previous line blank, and we
            ;; have a left margin, delete that from the blank line.
            (save-excursion
              (goto-char beforepos)
              (beginning-of-line)
              (and (looking-at "[ \t]$")      <=========================
                   (> (current-left-margin) 0)
                   (delete-region (point)
                                  (line-end-position))))


It's trying to match a single WS character.  Shouldn't this, perhaps, be
"[ \t]+$"?

[ .... ]

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[João Távora]

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

On Tue, May 14, 2019 at 10:27 AM Alan Mackenzie <acm@muc.de> wrote:

> The bug is, type lots of <CR>s in a row; the indentation WS isn't
> getting removed from the blank lines.  Currently electric-indent-inhibit
> is inhibiting this removal.
>

Do you mean the "removal of the WS in the lines preceding the current".
In other words, do you mean "removal of the trailing WS that was once
proper indentation"?

Or do you think that the current line, the one where point stands, should
not be indented at all in certain electric-* variable combinations and or
c-electric-* variable?  Which of those combinations?


> Probably.  Maybe João should check this, once he's fully back with us.
>

I'm afraid I can't put a date on that. There's a bun in the oven...

An important development towards figuring out this issue is that a
significant fraction of us agrees on what the behavior should be
in what cases.  Then we should code tests that assert that behavior
possibly reusing the fixtures in electric-tests.el.

> The same bug occurs in Python Mode.
> Succinctly, the bug is that on pressing <CR> lots of times in a row, the
> indentation WS is being left on the blank lines rather than being
> removed.

I see.  That does make sense. But, to be sure, we _dont_ what to
remove the indentation WS on the "current" line, right?

João

[-- Attachment #2: Type: text/html, Size: 2290 bytes --]

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Stefan Monnier]

> It's trying to match a single WS character.  Shouldn't this, perhaps, be
> "[ \t]+$"?

I think so, yes.
[ But FWIW, I've never used the "left-margin" support, nor can I remember
  anyone using it.  ]


        Stefan

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Alan Mackenzie]

Hello, João.

On Tue, May 14, 2019 at 11:34:24 +0100, João Távora wrote:
> On Tue, May 14, 2019 at 10:27 AM Alan Mackenzie <acm@muc.de> wrote:

> > The bug is, type lots of <CR>s in a row; the indentation WS isn't
> > getting removed from the blank lines.  Currently electric-indent-inhibit
> > is inhibiting this removal.


> Do you mean the "removal of the WS in the lines preceding the current".
> In other words, do you mean "removal of the trailing WS that was once
> proper indentation"?

Yes.  To be absolutely clear, supposing we have point at the end of a
line containing nothing but indentation space (e.g., we've just typed
<CR>):

<spaces>!
        ^
      point

Type <CR> again.  What we are currently seeing is:

<spaces>
<spaces>!

.  What we want to see is

<nothing>
<spaces>!

.

> Or do you think that the current line, the one where point stands, should
> not be indented at all in certain electric-* variable combinations and or
> c-electric-* variable?  Which of those combinations?

When electric-indent-inhibit is set, the (electric) indentation of the
current line should not be done by electric-indent-mode.  For the
moment, in CC Mode it should be done by c-electric-brace, and friends,
if so configured in CC Mode (the default being enabled).

> > Probably.  Maybe João should check this, once he's fully back with us.


> I'm afraid I can't put a date on that. There's a bun in the oven...

Well, congratulations!  I hope everything goes well.

> An important development towards figuring out this issue is that a
> significant fraction of us agrees on what the behavior should be
> in what cases.  Then we should code tests that assert that behavior
> possibly reusing the fixtures in electric-tests.el.

Yes.

> > The same bug occurs in Python Mode.
> > Succinctly, the bug is that on pressing <CR> lots of times in a row, the
> > indentation WS is being left on the blank lines rather than being
> > removed.

> I see.  That does make sense. But, to be sure, we _dont_ what to
> remove the indentation WS on the "current" line, right?

Right.  Unless, and until, the current line becomes the "previous" line,
still otherwise being blank.

I think we're agreed on everything.  :-)

> João

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[João Távora]

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

Well,

I've just come across the bug myself, and it is indeed annoying.
Can you check if this patch, which seems the simplest, serves
all purposes? It also adds a test to prevent future regressions

Thanks,
João

diff --git a/lisp/electric.el b/lisp/electric.el
index 657913a396..f15a95bf91 100644
--- a/lisp/electric.el
+++ b/lisp/electric.el
@@ -281,10 +281,13 @@ electric-indent-post-self-insert-function
                   (goto-char before)
                   (condition-case-unless-debug ()
                       (indent-according-to-mode)
-                    (error (throw 'indent-error nil)))
-                  ;; The goal here will be to remove the trailing
-                  ;; whitespace after reindentation of the previous line
-                  ;; because that may have (re)introduced it.
+                    (error (throw 'indent-error nil))))
+                (unless (eq electric-indent-inhibit 'electric-layout-mode)
+                  ;; Unless we're operating under
+                  ;; `electric-layout-mode' (Bug#35254), the goal here
+                  ;; will be to remove the trailing whitespace after
+                  ;; reindentation of the previous line because that
+                  ;; may have (re)introduced it.
                   (goto-char before)
                   ;; We were at EOL in marker `before' before the call
                   ;; to `indent-according-to-mode' but after we may
@@ -464,7 +467,7 @@ electric-layout-post-self-insert-function-1
                       ;; really wants to reindent, then
                       ;; `last-command-event' should be in
                       ;; `electric-indent-chars'.
-                      (let ((electric-indent-inhibit t))
+                      (let ((electric-indent-inhibit
'electric-layout-mode))
                         (funcall nl-after)))))))
             (pcase sym
               ('before (funcall nl-before))
diff --git a/test/lisp/electric-tests.el b/test/lisp/electric-tests.el
index 4f1e5729be..22b7a18ba5 100644
--- a/test/lisp/electric-tests.el
+++ b/test/lisp/electric-tests.el
@@ -876,6 +876,24 @@ electric-layout-for-c-style-du-jour
       (call-interactively (key-binding `[,last-command-event])))
     (should (equal (buffer-string) "int main () {\n  \n}"))))

+(ert-deftest electric-layout-control-reindentation ()
+  "Same as `e-l-int-main-kernel-style', but checking Bug#35254."
+  (ert-with-test-buffer ()
+    (plainer-c-mode)
+    (electric-layout-local-mode 1)
+    (electric-pair-local-mode 1)
+    (electric-indent-local-mode 1)
+    (setq-local electric-layout-rules
+                '((?\{ . (after))
+                  (?\} . (before))))
+    (insert "int main () ")
+    (let ((last-command-event ?\{))
+      (call-interactively (key-binding `[,last-command-event])))
+    (should (equal (buffer-string) "int main () {\n  \n}"))
+    ;; insert an additional newline and check indentation
+    (call-interactively 'newline)
+    (should (equal (buffer-string) "int main () {\n\n  \n}"))))
+
 (define-derived-mode plainer-c-mode c-mode "pC"
   "A plainer/saner C-mode with no internal electric machinery."
   (c-toggle-electric-state -1)




On Wed, May 15, 2019 at 11:03 AM Alan Mackenzie <acm@muc.de> wrote:

> Hello, João.
>
> On Tue, May 14, 2019 at 11:34:24 +0100, João Távora wrote:
> > On Tue, May 14, 2019 at 10:27 AM Alan Mackenzie <acm@muc.de> wrote:
>
> > > The bug is, type lots of <CR>s in a row; the indentation WS isn't
> > > getting removed from the blank lines.  Currently
> electric-indent-inhibit
> > > is inhibiting this removal.
>
>
> > Do you mean the "removal of the WS in the lines preceding the current".
> > In other words, do you mean "removal of the trailing WS that was once
> > proper indentation"?
>
> Yes.  To be absolutely clear, supposing we have point at the end of a
> line containing nothing but indentation space (e.g., we've just typed
> <CR>):
>
> <spaces>!
>         ^
>       point
>
> Type <CR> again.  What we are currently seeing is:
>
> <spaces>
> <spaces>!
>
> .  What we want to see is
>
> <nothing>
> <spaces>!
>
> .
>
> > Or do you think that the current line, the one where point stands, should
> > not be indented at all in certain electric-* variable combinations and or
> > c-electric-* variable?  Which of those combinations?
>
> When electric-indent-inhibit is set, the (electric) indentation of the
> current line should not be done by electric-indent-mode.  For the
> moment, in CC Mode it should be done by c-electric-brace, and friends,
> if so configured in CC Mode (the default being enabled).
>
> > > Probably.  Maybe João should check this, once he's fully back with us.
>
>
> > I'm afraid I can't put a date on that. There's a bun in the oven...
>
> Well, congratulations!  I hope everything goes well.
>
> > An important development towards figuring out this issue is that a
> > significant fraction of us agrees on what the behavior should be
> > in what cases.  Then we should code tests that assert that behavior
> > possibly reusing the fixtures in electric-tests.el.
>
> Yes.
>
> > > The same bug occurs in Python Mode.
> > > Succinctly, the bug is that on pressing <CR> lots of times in a row,
> the
> > > indentation WS is being left on the blank lines rather than being
> > > removed.
>
> > I see.  That does make sense. But, to be sure, we _dont_ what to
> > remove the indentation WS on the "current" line, right?
>
> Right.  Unless, and until, the current line becomes the "previous" line,
> still otherwise being blank.
>
> I think we're agreed on everything.  :-)
>
> > João
>
> --
> Alan Mackenzie (Nuremberg, Germany).
>


-- 
João Távora

[-- Attachment #2: Type: text/html, Size: 7565 bytes --]

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Stefan Monnier]

> I've just come across the bug myself, and it is indeed annoying.
> Can you check if this patch, which seems the simplest, serves
> all purposes?

FWIW, I find it rather ugly (makes the two minor modes too tightly
intertwined).

> It also adds a test to prevent future regressions

Another approach might be to do the whitespace erasure in
electric-indent without paying any attention to electric-layout/pair,
and then in electric-pair to explicitly cause (re)indentation
after moving point to between the opened pair?

Yet another option is to tell electric-indent about the final position
of point and have it refrain from deleting whitespace before that final
position, as in the patch below.  WDYT?


        Stefan


diff --git a/lisp/elec-pair.el b/lisp/elec-pair.el
index 3be09d87b4..a14efff241 100644
--- a/lisp/elec-pair.el
+++ b/lisp/elec-pair.el
@@ -551,7 +551,8 @@ electric-pair-post-self-insert-function
                          (goto-char pos)
                          (funcall electric-pair-inhibit-predicate
                                   last-command-event)))))
-         (save-excursion (electric-pair--insert pair)))))
+         (let ((electric-indent--destination (point-marker)))
+           (save-excursion (electric-pair--insert pair))))))
       (_
        (when (and (if (functionp electric-pair-open-newline-between-pairs)
                       (funcall electric-pair-open-newline-between-pairs)
diff --git a/lisp/electric.el b/lisp/electric.el
index 07da2f1d9e..71ebb9cf45 100644
--- a/lisp/electric.el
+++ b/lisp/electric.el
@@ -231,6 +231,14 @@ electric-indent-functions-without-reindent
 not try to reindent lines.  It is normally better to make the major
 mode set `electric-indent-inhibit', but this can be used as a workaround.")
 
+(defun electric-indent--inhibited-p ()
+  (or electric-indent-inhibit
+      (memq indent-line-function
+            electric-indent-functions-without-reindent)))
+
+(defvar electric-indent--destination nil
+  "If non-nil, position to which point will be later restored.")
+
 (defun electric-indent-post-self-insert-function ()
   "Function that `electric-indent-mode' adds to `post-self-insert-hook'.
 This indents if the hook `electric-indent-functions' returns non-nil,
@@ -272,26 +280,26 @@ electric-indent-post-self-insert-function
           (when at-newline
             (let ((before (copy-marker (1- pos) t)))
               (save-excursion
-                (unless
-                    (or (memq indent-line-function
-                              electric-indent-functions-without-reindent)
-                        electric-indent-inhibit)
+                (unless (electric-indent--inhibited-p)
                   ;; Don't reindent the previous line if the
                   ;; indentation function is not a real one.
                   (goto-char before)
                   (condition-case-unless-debug ()
                       (indent-according-to-mode)
-                    (error (throw 'indent-error nil)))
-                  ;; The goal here will be to remove the trailing
-                  ;; whitespace after reindentation of the previous line
-                  ;; because that may have (re)introduced it.
-                  (goto-char before)
-                  ;; We were at EOL in marker `before' before the call
-                  ;; to `indent-according-to-mode' but after we may
-                  ;; not be (Bug#15767).
-                  (when (and (eolp))
-                    (delete-horizontal-space t))))))
-          (unless (and electric-indent-inhibit
+                    (error (throw 'indent-error nil))))
+                ;; The goal here will be to remove the trailing
+                ;; whitespace after reindentation of the previous line
+                ;; because that may have (re)introduced it.
+                (goto-char before)
+                ;; We were at EOL in marker `before' before the call
+                ;; to `indent-according-to-mode' but after we may
+                ;; not be (Bug#15767).
+                (when (and (eolp)
+                           ;; Don't delete "trailing space" before point!
+                           (not (and electric-indent--destination
+                                     (= (point) electric-indent--destination))))
+                  (delete-horizontal-space t)))))
+          (unless (and (electric-indent--inhibited-p)
                        (not at-newline))
             (condition-case-unless-debug ()
                 (indent-according-to-mode)
diff --git a/test/lisp/electric-tests.el b/test/lisp/electric-tests.el
index 4f1e5729be..0b67fb3f1f 100644
--- a/test/lisp/electric-tests.el
+++ b/test/lisp/electric-tests.el
@@ -876,15 +876,6 @@ electric-layout-for-c-style-du-jour
       (call-interactively (key-binding `[,last-command-event])))
     (should (equal (buffer-string) "int main () {\n  \n}"))))
 
-(define-derived-mode plainer-c-mode c-mode "pC"
-  "A plainer/saner C-mode with no internal electric machinery."
-  (c-toggle-electric-state -1)
-  (setq-local electric-indent-local-mode-hook nil)
-  (setq-local electric-indent-mode-hook nil)
-  (electric-indent-local-mode 1)
-  (dolist (key '(?\" ?\' ?\{ ?\} ?\( ?\) ?\[ ?\]))
-    (local-set-key (vector key) 'self-insert-command)))
-
 (ert-deftest electric-modes-int-main-allman-style ()
   (ert-with-test-buffer ()
     (plainer-c-mode)

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[João Távora]

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

On Wed, May 15, 2019 at 2:19 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> > I've just come across the bug myself, and it is indeed annoying.
> > Can you check if this patch, which seems the simplest, serves
> > all purposes?
>
> FWIW, I find it rather ugly (makes the two minor modes too tightly
> intertwined).
>

It's no work of art. But the modes are already intertwined.
(And the code within a mode ain't no work of art, too :-))

> It also adds a test to prevent future regressions
>
> Another approach might be to do the whitespace erasure in
> electric-indent without paying any attention to electric-layout/pair,
> and then in electric-pair to explicitly cause (re)indentation
> after moving point to between the opened pair?
>

Maybe, I don't have time to implement that, and it looks like it
could hold surprises.  But if you do it and it passes the existing
tests and the new one, nothing against


> Yet another option is to tell electric-indent about the final position
> of point and have it refrain from deleting whitespace before that final
> position, as in the patch below.  WDYT?
>

It looks a little like an elaboration of a third abstraction to basically
create another intertwining between e-p-m and e-i-m. Sure it's the
best? Anyway, if it passes all  tests, new and old, great, push it!

(but why are you deleting plainer-c-mode in test/lisp/electric-tests.el?)

João




>
>
>         Stefan
>
>
> diff --git a/lisp/elec-pair.el b/lisp/elec-pair.el
> index 3be09d87b4..a14efff241 100644
> --- a/lisp/elec-pair.el
> +++ b/lisp/elec-pair.el
> @@ -551,7 +551,8 @@ electric-pair-post-self-insert-function
>                           (goto-char pos)
>                           (funcall electric-pair-inhibit-predicate
>                                    last-command-event)))))
> -         (save-excursion (electric-pair--insert pair)))))
> +         (let ((electric-indent--destination (point-marker)))
> +           (save-excursion (electric-pair--insert pair))))))
>        (_
>         (when (and (if (functionp electric-pair-open-newline-between-pairs)
>                        (funcall electric-pair-open-newline-between-pairs)
> diff --git a/lisp/electric.el b/lisp/electric.el
> index 07da2f1d9e..71ebb9cf45 100644
> --- a/lisp/electric.el
> +++ b/lisp/electric.el
> @@ -231,6 +231,14 @@ electric-indent-functions-without-reindent
>  not try to reindent lines.  It is normally better to make the major
>  mode set `electric-indent-inhibit', but this can be used as a
> workaround.")
>
> +(defun electric-indent--inhibited-p ()
> +  (or electric-indent-inhibit
> +      (memq indent-line-function
> +            electric-indent-functions-without-reindent)))
> +
> +(defvar electric-indent--destination nil
> +  "If non-nil, position to which point will be later restored.")
> +
>  (defun electric-indent-post-self-insert-function ()
>    "Function that `electric-indent-mode' adds to `post-self-insert-hook'.
>  This indents if the hook `electric-indent-functions' returns non-nil,
> @@ -272,26 +280,26 @@ electric-indent-post-self-insert-function
>            (when at-newline
>              (let ((before (copy-marker (1- pos) t)))
>                (save-excursion
> -                (unless
> -                    (or (memq indent-line-function
> -                              electric-indent-functions-without-reindent)
> -                        electric-indent-inhibit)
> +                (unless (electric-indent--inhibited-p)
>                    ;; Don't reindent the previous line if the
>                    ;; indentation function is not a real one.
>                    (goto-char before)
>                    (condition-case-unless-debug ()
>                        (indent-according-to-mode)
> -                    (error (throw 'indent-error nil)))
> -                  ;; The goal here will be to remove the trailing
> -                  ;; whitespace after reindentation of the previous line
> -                  ;; because that may have (re)introduced it.
> -                  (goto-char before)
> -                  ;; We were at EOL in marker `before' before the call
> -                  ;; to `indent-according-to-mode' but after we may
> -                  ;; not be (Bug#15767).
> -                  (when (and (eolp))
> -                    (delete-horizontal-space t))))))
> -          (unless (and electric-indent-inhibit
> +                    (error (throw 'indent-error nil))))
> +                ;; The goal here will be to remove the trailing
> +                ;; whitespace after reindentation of the previous line
> +                ;; because that may have (re)introduced it.
> +                (goto-char before)
> +                ;; We were at EOL in marker `before' before the call
> +                ;; to `indent-according-to-mode' but after we may
> +                ;; not be (Bug#15767).
> +                (when (and (eolp)
> +                           ;; Don't delete "trailing space" before point!
> +                           (not (and electric-indent--destination
> +                                     (= (point)
> electric-indent--destination))))
> +                  (delete-horizontal-space t)))))
> +          (unless (and (electric-indent--inhibited-p)
>                         (not at-newline))
>              (condition-case-unless-debug ()
>                  (indent-according-to-mode)
> diff --git a/test/lisp/electric-tests.el b/test/lisp/electric-tests.el
> index 4f1e5729be..0b67fb3f1f 100644
> --- a/test/lisp/electric-tests.el
> +++ b/test/lisp/electric-tests.el
> @@ -876,15 +876,6 @@ electric-layout-for-c-style-du-jour
>        (call-interactively (key-binding `[,last-command-event])))
>      (should (equal (buffer-string) "int main () {\n  \n}"))))
>
> -(define-derived-mode plainer-c-mode c-mode "pC"
> -  "A plainer/saner C-mode with no internal electric machinery."
> -  (c-toggle-electric-state -1)
> -  (setq-local electric-indent-local-mode-hook nil)
> -  (setq-local electric-indent-mode-hook nil)
> -  (electric-indent-local-mode 1)
> -  (dolist (key '(?\" ?\' ?\{ ?\} ?\( ?\) ?\[ ?\]))
> -    (local-set-key (vector key) 'self-insert-command)))
> -
>  (ert-deftest electric-modes-int-main-allman-style ()
>    (ert-with-test-buffer ()
>      (plainer-c-mode)
>
>

-- 
João Távora

[-- Attachment #2: Type: text/html, Size: 8338 bytes --]

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[João Távora]

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

On Wed, May 15, 2019 at 2:55 PM João Távora <joaotavora@gmail.com> wrote:

> (but why are you deleting plainer-c-mode in test/lisp/electric-tests.el?)
>

Never mind, I see there was a duplicate definition.

Thanks,
João

[-- Attachment #2: Type: text/html, Size: 613 bytes --]

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[João Távora]

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

On Wed, May 15, 2019 at 2:55 PM João Távora <joaotavora@gmail.com> wrote:

> On Wed, May 15, 2019 at 2:19 PM Stefan Monnier <monnier@iro.umontreal.ca>
> wrote:
>
>> > I've just come across the bug myself, and it is indeed annoying.
>> > Can you check if this patch, which seems the simplest, serves
>> > all purposes?
>>
>> FWIW, I find it rather ugly (makes the two minor modes too tightly
>> intertwined).
>>
>
> It's no work of art. But the modes are already intertwined.
>

Hi Stefan,

I still got the patch outstanding that would fix this bug, perhaps we
can install it until someone comes up with a less ugly solution (tho
as I stated I don't think its thaat bad).

João

[-- Attachment #2: Type: text/html, Size: 1399 bytes --]

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Alan Mackenzie]

Hello, João.

On Mon, Jul 01, 2019 at 13:24:28 +0100, João Távora wrote:
> On Wed, May 15, 2019 at 2:55 PM João Távora <joaotavora@gmail.com> wrote:

> > On Wed, May 15, 2019 at 2:19 PM Stefan Monnier <monnier@iro.umontreal.ca>
> > wrote:

> >> > I've just come across the bug myself, and it is indeed annoying.
> >> > Can you check if this patch, which seems the simplest, serves
> >> > all purposes?

> >> FWIW, I find it rather ugly (makes the two minor modes too tightly
> >> intertwined).


> > It's no work of art. But the modes are already intertwined.


> Hi Stefan,

> I still got the patch outstanding that would fix this bug, perhaps we
> can install it until someone comes up with a less ugly solution (tho
> as I stated I don't think its thaat bad).

Could you possibly give us a reference (like the date and time you
posted it), so that we can be sure which patch we're talking about?  It
was a month and a half ago since we were talking about this bug.  I also
posted a patch, a relatively simple one (From 2019-05-13T19:53, modified
by Stefan at 2019-05-13T19:32), which fixes the bug.

> João

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Noam Postavsky]

tags 35254 fixed
close 35254 
quit

Alan Mackenzie <acm@muc.de> writes:

>> I still got the patch outstanding that would fix this bug, perhaps we
>> can install it until someone comes up with a less ugly solution (tho
>> as I stated I don't think its thaat bad).
>
> Could you possibly give us a reference (like the date and time you
> posted it), so that we can be sure which patch we're talking about?  It
> was a month and a half ago since we were talking about this bug.  I also
> posted a patch, a relatively simple one (From 2019-05-13T19:53, modified
> by Stefan at 2019-05-13T19:32), which fixes the bug.

I guess it would be this one which was installed to master.

[1: 5e88b50d54]: 2019-07-02 16:10:45 +0100
  Correctly reindent previous line in electric-indent-mode
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=5e88b50d542b6d1c4ff43f8ae0fabe8a647d842e

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[João Távora]

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

On Sat, Jul 6, 2019, 17:24 Noam Postavsky <npostavs@gmail.com> wrote:

> tags 35254 fixed
> close 35254
> quit
>
> Alan Mackenzie <acm@muc.de> writes:
>
> >> I still got the patch outstanding that would fix this bug, perhaps we
> >> can install it until someone comes up with a less ugly solution (tho
> >> as I stated I don't think its thaat bad).
> >
> > Could you possibly give us a reference (like the date and time you
> > posted it), so that we can be sure which patch we're talking about?  It
> > was a month and a half ago since we were talking about this bug.  I also
> > posted a patch, a relatively simple one (From 2019-05-13T19:53, modified
> > by Stefan at 2019-05-13T19:32), which fixes the bug.
>
> I guess it would be this one which was installed to master.
>
> [1: 5e88b50d54]: 2019-07-02 16:10:45 +0100
>   Correctly reindent previous line in electric-indent-mode
>
> https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=5e88b50d542b6d1c4ff43f8ae0fabe8a647d842e


Yes, that was the one. I pushed to master but forgot  to close the bug
here. Sorry about that and thanks Noam for the housekeeping. It'd be nice
if someone could confirm the original bug is fixed.

João

João

[-- Attachment #2: Type: text/html, Size: 1942 bytes --]

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[João Távora]

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

On Mon, Jul 1, 2019, 14:34 Alan Mackenzie <acm@muc.de> wrote:

> Hello, João.
>
> On Mon, Jul 01, 2019 at 13:24:28 +0100, João Távora wrote:
> > On Wed, May 15, 2019 at 2:55 PM João Távora <joaotavora@gmail.com>
> wrote:
>
> > > On Wed, May 15, 2019 at 2:19 PM Stefan Monnier <
> monnier@iro.umontreal.ca>
> > > wrote:
>
> > >> > I've just come across the bug myself, and it is indeed annoying.
> > >> > Can you check if this patch, which seems the simplest, serves
> > >> > all purposes?
>
> > >> FWIW, I find it rather ugly (makes the two minor modes too tightly
> > >> intertwined).
>
>
> > > It's no work of art. But the modes are already intertwined.
>
>
> > Hi Stefan,
>
> > I still got the patch outstanding that would fix this bug, perhaps we
> > can install it until someone comes up with a less ugly solution (tho
> > as I stated I don't think its thaat bad).
>
> Could you possibly give us a reference (like the date and time you
> posted it), so that we can be sure which patch we're talking about?  It
> was a month and a half ago since we were talking about this bug.  I also
> posted a patch, a relatively simple one (From 2019-05-13T19:53, modified
> by Stefan at 2019-05-13T19:32), which fixes the bug.
>

Sorry, this message flew under the radar.

If I recall correctly and from reading my replies to that patch, both your
version and Stefan's modification have unwanted consequences, as evidenced
by the associated test breakage. Have you tried the latest master?

João

>

[-- Attachment #2: Type: text/html, Size: 2430 bytes --]

bug#35254: 27.0.50; cc-mode/electric-pair-mode/electric-layout-mode: bad trailing whitespace behavior in cc-mode

[Noam Postavsky]

João Távora <joaotavora@gmail.com> writes:
>
>
> Yes, that was the one. I pushed to master but forgot  to close the bug
> here. Sorry about that and thanks Noam for the housekeeping. It'd be nice
> if someone could confirm the original bug is fixed.

Yes, the original scenario is fixed, that's why I closed the bug.