Accidental change of behaviour for electric-layout-mode?

Accidental change of behaviour for electric-layout-mode?

[Morgan Willcock]

It looks like Emacs 30 is now allowing electric-layout-mode to insert
newlines inside comments and strings, whereas previous versions seem to
explicitly stop this from happening.

The commit that does it is b1f8d98a119ab8845d25d80c480cde6e385d8749
(Eglot: rework eglot-imenu) which doesn't mention electric-layout-mode
in the commit message or the associated bug report (58431).

I think it may have been done accidentally and is likely not a backwards
compatible change.

(I actually wanted the new behaviour and only found this because my
tests for newline insertion failed on Emacs 27, 28, and 29.  Making the
new behaviour opt-in would be nice, so I could send a patch to do that
if the previous change does look to have been accidental.)

Morgan

-- 
Morgan Willcock

Re: Accidental change of behaviour for electric-layout-mode?

[Eli Zaretskii]

> From: Morgan Willcock <morgan@ice9.digital>
> Date: Mon, 23 Sep 2024 21:58:13 +0100
> 
> It looks like Emacs 30 is now allowing electric-layout-mode to insert
> newlines inside comments and strings, whereas previous versions seem to
> explicitly stop this from happening.
> 
> The commit that does it is b1f8d98a119ab8845d25d80c480cde6e385d8749
> (Eglot: rework eglot-imenu) which doesn't mention electric-layout-mode
> in the commit message or the associated bug report (58431).
> 
> I think it may have been done accidentally and is likely not a backwards
> compatible change.
> 
> (I actually wanted the new behaviour and only found this because my
> tests for newline insertion failed on Emacs 27, 28, and 29.  Making the
> new behaviour opt-in would be nice, so I could send a patch to do that
> if the previous change does look to have been accidental.)

João, was that change intentional, and if so, what was its reasons?

Thanks.

Re: Accidental change of behaviour for electric-layout-mode?

[João Távora]

No, it wasn't.  I was probably experimenting with something.

You can revert it (or make it optional), etc.

João

On Tue, Sep 24, 2024 at 12:24 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Morgan Willcock <morgan@ice9.digital>
> > Date: Mon, 23 Sep 2024 21:58:13 +0100
> >
> > It looks like Emacs 30 is now allowing electric-layout-mode to insert
> > newlines inside comments and strings, whereas previous versions seem to
> > explicitly stop this from happening.
> >
> > The commit that does it is b1f8d98a119ab8845d25d80c480cde6e385d8749
> > (Eglot: rework eglot-imenu) which doesn't mention electric-layout-mode
> > in the commit message or the associated bug report (58431).
> >
> > I think it may have been done accidentally and is likely not a backwards
> > compatible change.
> >
> > (I actually wanted the new behaviour and only found this because my
> > tests for newline insertion failed on Emacs 27, 28, and 29.  Making the
> > new behaviour opt-in would be nice, so I could send a patch to do that
> > if the previous change does look to have been accidental.)
>
> João, was that change intentional, and if so, what was its reasons?
>
> Thanks.



-- 
João Távora

Re: Accidental change of behaviour for electric-layout-mode?

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Tue, 24 Sep 2024 13:12:24 +0100
> Cc: Morgan Willcock <morgan@ice9.digital>, emacs-devel@gnu.org
> 
> No, it wasn't.  I was probably experimenting with something.
> 
> You can revert it (or make it optional), etc.

Thanks.  So Morgan, please post a patch that makes this feature
opt-in.

Re: Accidental change of behaviour for electric-layout-mode?

[Morgan Willcock]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Date: Tue, 24 Sep 2024 13:12:24 +0100
>> Cc: Morgan Willcock <morgan@ice9.digital>, emacs-devel@gnu.org
>>
>> No, it wasn't.  I was probably experimenting with something.
>>
>> You can revert it (or make it optional), etc.
>
> Thanks.  So Morgan, please post a patch that makes this feature
> opt-in.

Patch is attached.

-- 
Morgan Willcock

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Restore-comment-string-check-for-electric-layout-mod.patch --]
[-- Type: text/x-diff, Size: 2030 bytes --]

From 0fe0d66aff7d4969e99478469afadede7e40f4ac Mon Sep 17 00:00:00 2001
From: Morgan Willcock <morgan@ice9.digital>
Date: Tue, 24 Sep 2024 19:33:11 +0100
Subject: [PATCH] Restore comment/string check for electric-layout-mode

This reverts an accidental change which allowed
electric-layout-mode to insert newlines inside strings and
comments.  The recent behavior can be restored by setting the
new variable 'electric-layout-allow-in-comment-or-string' to a
non-nil value.

* lisp/electric.el (electric-layout-allow-in-comment-or-string):
New variable to determine whether inserting newlines is
permitted within comments or strings.
(electric-layout-post-self-insert-function-1): Restore the
previous default behavior of not inserting newlines within
comments or strings.
---
 lisp/electric.el | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lisp/electric.el b/lisp/electric.el
index d02bcb4735b..d84faf5433f 100644
--- a/lisp/electric.el
+++ b/lisp/electric.el
@@ -385,6 +385,9 @@ electric-layout-rules
 (defvar electric-layout-allow-duplicate-newlines nil
   "If non-nil, allow duplication of `before' newlines.")
 
+(defvar electric-layout-allow-in-comment-or-string nil
+  "If non-nil, allow inserting newlines inside a comment or string.")
+
 (defun electric-layout-post-self-insert-function ()
   (when electric-layout-mode
     (electric-layout-post-self-insert-function-1)))
@@ -409,7 +412,10 @@ electric-layout-post-self-insert-function-1
                                 (goto-char pos)
                                 (funcall probe last-command-event))))
                          (when res (throw 'done res))))))))))
-    (when rule
+    (when (and rule
+               (or electric-layout-allow-in-comment-or-string
+                   ;; Not in a comment or string.
+                   (not (nth 8 (save-excursion (syntax-ppss pos))))))
       (goto-char pos)
       (when (functionp rule) (setq rule (funcall rule)))
       (dolist (sym (if (symbolp rule) (list rule) rule))
-- 
2.39.5

Re: Accidental change of behaviour for electric-layout-mode?

[Eli Zaretskii]

> From: Morgan Willcock <morgan@ice9.digital>
> Cc: emacs-devel@gnu.org
> Date: Tue, 24 Sep 2024 19:59:47 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> You can revert it (or make it optional), etc.
> >
> > Thanks.  So Morgan, please post a patch that makes this feature
> > opt-in.
> 
> Patch is attached.

Thanks, but shouldn't the new variable be a defcustom, i.e. a user
option?  AFAIU, the preference is on the user level.

Also, this needs a NEWS entry.

Re: Accidental change of behaviour for electric-layout-mode?

[Morgan Willcock]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Morgan Willcock <morgan@ice9.digital>
>> Cc: emacs-devel@gnu.org
>> Date: Tue, 24 Sep 2024 19:59:47 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> You can revert it (or make it optional), etc.
>> >
>> > Thanks.  So Morgan, please post a patch that makes this feature
>> > opt-in.
>> 
>> Patch is attached.
>
> Thanks, but shouldn't the new variable be a defcustom, i.e. a user
> option?  AFAIU, the preference is on the user level.

I am following the lead of electric-layout-allow-duplicate-newlines
which is not a user option.

For my use case, electric-layout-rules is being set by a major-mode and
the functions in those rules would need to be paired with the ability to
insert a newline within a comment.  If you needed another example of a
major-mode configuring it, python-mode is also setting rules at the mode
level:

https://git.savannah.gnu.org/cgit/emacs.git/commit/lisp/progmodes/python.el?id=dfecd6037d5ebe5778c40ff7b38bfcbaa3ef779e

If there is any doubt about the usage of the new variable I am happy to
just put the original behaviour back and omit the new option for the
moment.

> Also, this needs a NEWS entry.

Does it need a NEWS entry if it is not a user option and the default
behaviour is the same as Emacs 29?

-- 
Morgan Willcock

Re: Accidental change of behaviour for electric-layout-mode?

[Eli Zaretskii]

> From: Morgan Willcock <morgan@ice9.digital>
> Cc: emacs-devel@gnu.org
> Date: Tue, 24 Sep 2024 20:39:42 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Patch is attached.
> >
> > Thanks, but shouldn't the new variable be a defcustom, i.e. a user
> > option?  AFAIU, the preference is on the user level.
> 
> I am following the lead of electric-layout-allow-duplicate-newlines
> which is not a user option.

Which ironically has the following comment:

  ;; TODO: Make this a defcustom?
  (defvar electric-layout-allow-duplicate-newlines nil

So I think this new variable should be a defcustom, and maybe we
should also make electric-layout-allow-duplicate-newlines a defcustom.

> For my use case, electric-layout-rules is being set by a major-mode and
> the functions in those rules would need to be paired with the ability to
> insert a newline within a comment.

If you are saying that it never makes sense to modify the value of
this variable given specific rules, i.e. that any set of rules will
either always support embedded newlines or be unable to support them,
but will never be able to sensibly support both alternatives, then I
agree.  (But in that case, why do we need a variable at all? why not
let the rules specify this directly?)

> If you needed another example of a major-mode configuring it,
> python-mode is also setting rules at the mode level:
> 
> https://git.savannah.gnu.org/cgit/emacs.git/commit/lisp/progmodes/python.el?id=dfecd6037d5ebe5778c40ff7b38bfcbaa3ef779e

The way to let modes control this behavior to introduce an internal
variable which the mode could set and whose initial default value is
taken from the user option.

> 
> If there is any doubt about the usage of the new variable I am happy to
> just put the original behaviour back and omit the new option for the
> moment.

Do you really have such strong feelings about this not being a user
option that you'd rather forget the whole issue?  Why are you so
convinced that no user will ever want to customize this behavior, when
you are the first example of such users?

> > Also, this needs a NEWS entry.
> 
> Does it need a NEWS entry if it is not a user option and the default
> behaviour is the same as Emacs 29?

Let's first agree about the option issue, and then get back to NEWS.

Thanks.

Re: Accidental change of behaviour for electric-layout-mode?

[Morgan Willcock]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Morgan Willcock <morgan@ice9.digital>
>> Cc: emacs-devel@gnu.org
>> Date: Tue, 24 Sep 2024 20:39:42 +0100
>>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>> >> Patch is attached.
>> >
>> > Thanks, but shouldn't the new variable be a defcustom, i.e. a user
>> > option?  AFAIU, the preference is on the user level.
>>
>> I am following the lead of electric-layout-allow-duplicate-newlines
>> which is not a user option.
>
> Which ironically has the following comment:
>
>   ;; TODO: Make this a defcustom?
>   (defvar electric-layout-allow-duplicate-newlines nil
>
> So I think this new variable should be a defcustom, and maybe we
> should also make electric-layout-allow-duplicate-newlines a defcustom.
>
>> For my use case, electric-layout-rules is being set by a major-mode and
>> the functions in those rules would need to be paired with the ability to
>> insert a newline within a comment.
>
> If you are saying that it never makes sense to modify the value of
> this variable given specific rules, i.e. that any set of rules will
> either always support embedded newlines or be unable to support them,
> but will never be able to sensibly support both alternatives, then I
> agree.  (But in that case, why do we need a variable at all? why not
> let the rules specify this directly?)

With the current setup, functions which are used within rules cannot
decide for themselves on the context - the function call happens before
the guards that have traditionally been in place are checked.

>> If you needed another example of a major-mode configuring it,
>> python-mode is also setting rules at the mode level:
>>
>> https://git.savannah.gnu.org/cgit/emacs.git/commit/lisp/progmodes/python.el?id=dfecd6037d5ebe5778c40ff7b38bfcbaa3ef779e
>
> The way to let modes control this behavior to introduce an internal
> variable which the mode could set and whose initial default value is
> taken from the user option.
>
>>
>> If there is any doubt about the usage of the new variable I am happy to
>> just put the original behaviour back and omit the new option for the
>> moment.
>
> Do you really have such strong feelings about this not being a user
> option that you'd rather forget the whole issue?  Why are you so
> convinced that no user will ever want to customize this behavior, when
> you are the first example of such users?

It is more that I don't have strong enough feelings to be making
modifications which significantly change the user experience or design.

Just to make sure everything works in Emacs 30, I'd rather just revert
the accidental change than get further involved in the development of
this feature (at least in the short term).

I've attached a patch which just reverts the change that was
accidentally applied.  This should be all that is needed unless you
specifically want to accommodate features which accidentally worked in
an unreleased version of Emacs.

-- 
Morgan Willcock

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Restore-comment-string-check-for-electric-layout-mod.patch --]
[-- Type: text/x-diff, Size: 1262 bytes --]

From 89463c6c5aeeae14868a69922719b5c154fbf69d Mon Sep 17 00:00:00 2001
From: Morgan Willcock <morgan@ice9.digital>
Date: Wed, 25 Sep 2024 14:30:13 +0100
Subject: [PATCH] Restore comment/string check for electric-layout-mode

This reverts an accidental change which allowed
electric-layout-mode to insert newlines inside strings and
comments.

* lisp/electric.el
(electric-layout-post-self-insert-function-1): Restore the
previous default behavior of not inserting newlines within
comments or strings.
---
 lisp/electric.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lisp/electric.el b/lisp/electric.el
index d02bcb4735b..2fcb90efe7d 100644
--- a/lisp/electric.el
+++ b/lisp/electric.el
@@ -409,7 +409,9 @@ electric-layout-post-self-insert-function-1
                                 (goto-char pos)
                                 (funcall probe last-command-event))))
                          (when res (throw 'done res))))))))))
-    (when rule
+    (when (and rule
+               ;; Not in a string or comment.
+               (not (nth 8 (save-excursion (syntax-ppss pos)))))
       (goto-char pos)
       (when (functionp rule) (setq rule (funcall rule)))
       (dolist (sym (if (symbolp rule) (list rule) rule))
-- 
2.39.5

Re: Accidental change of behaviour for electric-layout-mode?

[Eli Zaretskii]

> From: Morgan Willcock <morgan@ice9.digital>
> Cc: emacs-devel@gnu.org
> Date: Wed, 25 Sep 2024 14:50:19 +0100
> 
> Just to make sure everything works in Emacs 30, I'd rather just revert
> the accidental change than get further involved in the development of
> this feature (at least in the short term).
> 
> I've attached a patch which just reverts the change that was
> accidentally applied.  This should be all that is needed unless you
> specifically want to accommodate features which accidentally worked in
> an unreleased version of Emacs.

Actually, having the variable is better, because we have run with this
code for quite some time, and someone might rely on the unintended
feature.  So I think your original patch is better.

Re: Accidental change of behaviour for electric-layout-mode?

[Morgan Willcock]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Morgan Willcock <morgan@ice9.digital>
>> Cc: emacs-devel@gnu.org
>> Date: Wed, 25 Sep 2024 14:50:19 +0100
>> 
>> Just to make sure everything works in Emacs 30, I'd rather just revert
>> the accidental change than get further involved in the development of
>> this feature (at least in the short term).
>> 
>> I've attached a patch which just reverts the change that was
>> accidentally applied.  This should be all that is needed unless you
>> specifically want to accommodate features which accidentally worked in
>> an unreleased version of Emacs.
>
> Actually, having the variable is better, because we have run with this
> code for quite some time, and someone might rely on the unintended
> feature.  So I think your original patch is better.

Possibly it depends on whether there will be an additional pretest
release or not?  If there was going to be one and it was accompanied by
a note to re-test any changes which were made to electric-layout-mode
rules, that might be enough.

-- 
Morgan Willcock

Re: Accidental change of behaviour for electric-layout-mode?

[Eli Zaretskii]

> From: Morgan Willcock <morgan@ice9.digital>
> Cc: emacs-devel@gnu.org
> Date: Thu, 03 Oct 2024 17:22:53 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Morgan Willcock <morgan@ice9.digital>
> >> Cc: emacs-devel@gnu.org
> >> Date: Wed, 25 Sep 2024 14:50:19 +0100
> >> 
> >> Just to make sure everything works in Emacs 30, I'd rather just revert
> >> the accidental change than get further involved in the development of
> >> this feature (at least in the short term).
> >> 
> >> I've attached a patch which just reverts the change that was
> >> accidentally applied.  This should be all that is needed unless you
> >> specifically want to accommodate features which accidentally worked in
> >> an unreleased version of Emacs.
> >
> > Actually, having the variable is better, because we have run with this
> > code for quite some time, and someone might rely on the unintended
> > feature.  So I think your original patch is better.
> 
> Possibly it depends on whether there will be an additional pretest
> release or not?  If there was going to be one and it was accompanied by
> a note to re-test any changes which were made to electric-layout-mode
> rules, that might be enough.

Unfortunately, doing this does not guarantee enough people will try
the changed behavior, and so the relatively short time we still have
until the release (even though there definitely will be one more
pretest) doesn't allow us to make sure we can safely change the
behavior at this late stage.

Re: Accidental change of behaviour for electric-layout-mode?

[Eli Zaretskii]

> Date: Thu, 03 Oct 2024 21:06:35 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> > From: Morgan Willcock <morgan@ice9.digital>
> > Cc: emacs-devel@gnu.org
> > Date: Thu, 03 Oct 2024 17:22:53 +0100
> > 
> > Possibly it depends on whether there will be an additional pretest
> > release or not?  If there was going to be one and it was accompanied by
> > a note to re-test any changes which were made to electric-layout-mode
> > rules, that might be enough.
> 
> Unfortunately, doing this does not guarantee enough people will try
> the changed behavior, and so the relatively short time we still have
> until the release (even though there definitely will be one more
> pretest) doesn't allow us to make sure we can safely change the
> behavior at this late stage.

I've now installed your original patch on the emacs-30 branch, thanks.