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

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

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

dima@secretsauce.net
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!



Reply | Threaded
Open this post in threaded view
|

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

Noam Postavsky
Dima Kogan <[hidden email]> 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 <[hidden email]>
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)
+
 
 ;; Support for C
 
--
2.11.0

Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> 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 <[hidden email]>
> 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)
> +
>  
>  ;; 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).



Reply | Threaded
Open this post in threaded view
|

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

Noam Postavsky
Alan Mackenzie <[hidden email]> 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).



Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> 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).



Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> writes:

> On Sat, May 11, 2019 at 10:06:42 -0400, Noam Postavsky wrote:
>> Alan Mackenzie <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Alan Mackenzie
In reply to this post by 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 <[hidden email]> 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).



Reply | Threaded
Open this post in threaded view
|

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

Alan Mackenzie
In reply to this post by Basil L. Contovounesios
Hello, Basil.

On Sat, May 11, 2019 at 20:34:51 +0100, Basil L. Contovounesios wrote:
> Alan Mackenzie <[hidden email]> writes:

> > On Sat, May 11, 2019 at 10:06:42 -0400, Noam Postavsky wrote:
> >> Alan Mackenzie <[hidden email]> 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).



Reply | Threaded
Open this post in threaded view
|

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

Noam Postavsky
In reply to this post by Alan Mackenzie
Alan Mackenzie <[hidden email]> 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)


From cbaf6ee73f9129fdb8f1e45ba680ca029981d290 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <[hidden email]>
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



> 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?
Reply | Threaded
Open this post in threaded view
|

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

Basil L. Contovounesios
In reply to this post by Alan Mackenzie
Alan Mackenzie <[hidden email]> writes:

> On Sat, May 11, 2019 at 20:34:51 +0100, Basil L. Contovounesios wrote:
>> Alan Mackenzie <[hidden email]> writes:
>
>> > On Sat, May 11, 2019 at 10:06:42 -0400, Noam Postavsky wrote:
>> >> Alan Mackenzie <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> 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).



Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> writes:

> On Sun, May 12, 2019 at 22:45:09 +0100, Basil L. Contovounesios wrote:
>> Alan Mackenzie <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Alan Mackenzie
In reply to this post by Noam Postavsky
Hello, João and Noam.

On Fri, May 10, 2019 at 23:12:06 -0400, Noam Postavsky wrote:
> Dima Kogan <[hidden email]> 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 <[hidden email]>
> 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).



Reply | Threaded
Open this post in threaded view
|

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

João Távora
On Mon, May 13, 2019 at 8:53 PM Alan Mackenzie <[hidden email]> wrote:
Hello, João and Noam.

On Fri, May 10, 2019 at 23:12:06 -0400, Noam Postavsky wrote:
> Dima Kogan <[hidden email]> 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 <[hidden email]>
> 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
Reply | Threaded
Open this post in threaded view
|

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

Stefan Monnier
In reply to this post by Alan Mackenzie
>> 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 ()



Reply | Threaded
Open this post in threaded view
|

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

Noam Postavsky
In reply to this post by João Távora
João Távora <[hidden email]> 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?



Reply | Threaded
Open this post in threaded view
|

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

Noam Postavsky
In reply to this post by Stefan Monnier
Stefan Monnier <[hidden email]> 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.



Reply | Threaded
Open this post in threaded view
|

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

João Távora
In reply to this post by Noam Postavsky
On Tue, May 14, 2019 at 12:38 AM Noam Postavsky <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

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



Reply | Threaded
Open this post in threaded view
|

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

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



12