GNU bug report logs - #63790
30.0.50; prog-fill-reindent-defun regression

Previous Next

To reply to this bug, email your comments to 63790 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; prog-fill-reindent-defun regression
Date: Mon, 29 May 2023 19:53:07 +0300

This regression is in master, not in emacs-29.

0. emacs-30 -Q
1. add to the beginning of the *scratch* buffer a list, so that
*scratch* looks like this:

(+
 1
    2
 3)

;; This buffer is for text that is not saved, and for Lisp evaluation.
;; To create a file, visit it with C-x C-f and enter text in its buffer.

2. Activate the region with the beginning at the start of the line with
the first comment, and the region end with point at the end of the buffer.

3. Type 'M-q' (prog-fill-reindent-defun)

It indents the list instead of the comment.

But when point is at the region beginning then 'M-q' correctly indents
the comments.

Message #8 received at 63790 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Juri Linkov <juri <at> linkov.net>, 63790 <at> debbugs.gnu.org
Subject: Re: bug#63790: 30.0.50; prog-fill-reindent-defun regression
Date: Sat, 3 Jun 2023 05:40:47 +0300

On 29/05/2023 19:53, Juri Linkov wrote:
> This regression is in master, not in emacs-29.

If it is a regression, then compared to what? emacs-29 doesn't have this 
function. Compared to some earlier revision?

> 0. emacs-30 -Q
> 1. add to the beginning of the *scratch* buffer a list, so that
> *scratch* looks like this:
> 
> (+
>   1
>      2
>   3)
> 
> ;; This buffer is for text that is not saved, and for Lisp evaluation.
> ;; To create a file, visit it with C-x C-f and enter text in its buffer.
> 
> 2. Activate the region with the beginning at the start of the line with
> the first comment, and the region end with point at the end of the buffer.
> 
> 3. Type 'M-q' (prog-fill-reindent-defun)
> 
> It indents the list instead of the comment.
> 
> But when point is at the region beginning then 'M-q' correctly indents
> the comments.

This happens because in this scenario point ends up outside of the 
comment (at eob). So when the function is called, in looks for a defun.

Did we at some point add (or decide to add) a condition when, if a 
region is active, it should only refill and not reindent?

Message #11 received at 63790 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: 63790 <at> debbugs.gnu.org
Subject: Re: bug#63790: 30.0.50; prog-fill-reindent-defun regression
Date: Sun, 04 Jun 2023 20:14:47 +0300

>> This regression is in master, not in emacs-29.
>
> If it is a regression, then compared to what? emacs-29 doesn't have this
> function. Compared to some earlier revision?

Sorry, the subject was not precise.  This is more elaborate:
the new function 'prog-fill-reindent-defun' caused a regression for
'M-q' compared to emacs-29 where 'M-q' was bound to 'fill-paragraph'.

>> 0. emacs-30 -Q
>> 1. add to the beginning of the *scratch* buffer a list, so that
>> *scratch* looks like this:
>> (+
>>   1
>>      2
>>   3)
>> ;; This buffer is for text that is not saved, and for Lisp evaluation.
>> ;; To create a file, visit it with C-x C-f and enter text in its buffer.
>> 2. Activate the region with the beginning at the start of the line with
>> the first comment, and the region end with point at the end of the buffer.
>> 3. Type 'M-q' (prog-fill-reindent-defun)
>> It indents the list instead of the comment.
>> But when point is at the region beginning then 'M-q' correctly indents
>> the comments.
>
> This happens because in this scenario point ends up outside of the comment
> (at eob). So when the function is called, in looks for a defun.
>
> Did we at some point add (or decide to add) a condition when, if a region
> is active, it should only refill and not reindent?

Maybe the logic of region detecting/handling could be copied from
'fill-paragraph' to 'prog-fill-reindent-defun'?

Message #14 received at 63790 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Juri Linkov <juri <at> linkov.net>
Cc: 63790 <at> debbugs.gnu.org
Subject: Re: bug#63790: 30.0.50; prog-fill-reindent-defun regression
Date: Tue, 6 Jun 2023 04:41:55 +0300

On 04/06/2023 20:14, Juri Linkov wrote:
>>> This regression is in master, not in emacs-29.
>> If it is a regression, then compared to what? emacs-29 doesn't have this
>> function. Compared to some earlier revision?
> Sorry, the subject was not precise.  This is more elaborate:
> the new function 'prog-fill-reindent-defun' caused a regression for
> 'M-q' compared to emacs-29 where 'M-q' was bound to 'fill-paragraph'.

Thanks for the clarification.

>>> 0. emacs-30 -Q
>>> 1. add to the beginning of the*scratch*  buffer a list, so that
>>> *scratch*  looks like this:
>>> (+
>>>    1
>>>       2
>>>    3)
>>> ;; This buffer is for text that is not saved, and for Lisp evaluation.
>>> ;; To create a file, visit it with C-x C-f and enter text in its buffer.
>>> 2. Activate the region with the beginning at the start of the line with
>>> the first comment, and the region end with point at the end of the buffer.
>>> 3. Type 'M-q' (prog-fill-reindent-defun)
>>> It indents the list instead of the comment.
>>> But when point is at the region beginning then 'M-q' correctly indents
>>> the comments.
>> This happens because in this scenario point ends up outside of the comment
>> (at eob). So when the function is called, in looks for a defun.
>>
>> Did we at some point add (or decide to add) a condition when, if a region
>> is active, it should only refill and not reindent?
> Maybe the logic of region detecting/handling could be copied from
> 'fill-paragraph' to 'prog-fill-reindent-defun'?

Makes sense. Do you want to suggest a patch?

Message #17 received at 63790 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: 63790 <at> debbugs.gnu.org
Subject: Re: bug#63790: 30.0.50; prog-fill-reindent-defun regression
Date: Tue, 06 Jun 2023 21:55:49 +0300

>>>> 3. Type 'M-q' (prog-fill-reindent-defun)
>>>> It indents the list instead of the comment.
>>>> But when point is at the region beginning then 'M-q' correctly indents
>>>> the comments.
>>> This happens because in this scenario point ends up outside of the comment
>>> (at eob). So when the function is called, in looks for a defun.
>>>
>>> Did we at some point add (or decide to add) a condition when, if a region
>>> is active, it should only refill and not reindent?
>> Maybe the logic of region detecting/handling could be copied from
>> 'fill-paragraph' to 'prog-fill-reindent-defun'?
>
> Makes sense. Do you want to suggest a patch?

Sorry, can't do, because I don't understand what this line is intended to do,
and there are no comments with explanations:

  (re-search-forward "\\s-*\\s<" (line-end-position) t)

It's nil in the reported case, so 'fill-paragraph' is not called.

Message #20 received at 63790 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Juri Linkov <juri <at> linkov.net>
Cc: 63790 <at> debbugs.gnu.org
Subject: Re: bug#63790: 30.0.50; prog-fill-reindent-defun regression
Date: Thu, 8 Jun 2023 03:35:41 +0300

On 06/06/2023 21:55, Juri Linkov wrote:
>>>>> 3. Type 'M-q' (prog-fill-reindent-defun)
>>>>> It indents the list instead of the comment.
>>>>> But when point is at the region beginning then 'M-q' correctly indents
>>>>> the comments.
>>>> This happens because in this scenario point ends up outside of the comment
>>>> (at eob). So when the function is called, in looks for a defun.
>>>>
>>>> Did we at some point add (or decide to add) a condition when, if a region
>>>> is active, it should only refill and not reindent?
>>> Maybe the logic of region detecting/handling could be copied from
>>> 'fill-paragraph' to 'prog-fill-reindent-defun'?
>> Makes sense. Do you want to suggest a patch?
> Sorry, can't do, because I don't understand what this line is intended to do,
> and there are no comments with explanations:
> 
>    (re-search-forward "\\s-*\\s<" (line-end-position) t)

It's looking for a comment that begins after point (possibly preceded by 
whitespace). There is no comment after point in the presented scenario.

> It's nil in the reported case, so 'fill-paragraph' is not called.

I guess when there is an active region, we would force the behavior to 
"refill" the region, no matter whether it is inside a comment, or 
contains a comment, or outside of any comments and simply contains code?

Message #23 received at 63790 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: 63790 <at> debbugs.gnu.org
Subject: Re: bug#63790: 30.0.50; prog-fill-reindent-defun regression
Date: Thu, 08 Jun 2023 19:59:53 +0300

>>    (re-search-forward "\\s-*\\s<" (line-end-position) t)
>
> It's looking for a comment that begins after point (possibly preceded by
> whitespace). There is no comment after point in the presented scenario.
>
>> It's nil in the reported case, so 'fill-paragraph' is not called.
>
> I guess when there is an active region, we would force the behavior to
> "refill" the region, no matter whether it is inside a comment, or contains
> a comment, or outside of any comments and simply contains code?

While 'prog-fill-reindent-defun' doesn't support indentation of an
arbitrary region of code and indents only the top-level list (defun),
it looks like the right thing is to fill the region.

I still don't understand why 'M-q' now does the same what 'C-M-q' was
doing all the time with code indentation?  Also why 'prog-fill-reindent-defun'
can't indent the region of code, but only the region of comments?

Shouldn't 'M-q' only refill comments, and 'C-M-q' only indent code, as before?

Message #26 received at 63790 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Juri Linkov <juri <at> linkov.net>
Cc: 63790 <at> debbugs.gnu.org
Subject: Re: bug#63790: 30.0.50; prog-fill-reindent-defun regression
Date: Fri, 9 Jun 2023 04:58:35 +0300

On 08/06/2023 19:59, Juri Linkov wrote:
>>>     (re-search-forward "\\s-*\\s<" (line-end-position) t)
>>
>> It's looking for a comment that begins after point (possibly preceded by
>> whitespace). There is no comment after point in the presented scenario.
>>
>>> It's nil in the reported case, so 'fill-paragraph' is not called.
>>
>> I guess when there is an active region, we would force the behavior to
>> "refill" the region, no matter whether it is inside a comment, or contains
>> a comment, or outside of any comments and simply contains code?
> 
> While 'prog-fill-reindent-defun' doesn't support indentation of an
> arbitrary region of code and indents only the top-level list (defun),
> it looks like the right thing is to fill the region.

Okay?

> I still don't understand why 'M-q' now does the same what 'C-M-q' was
> doing all the time with code indentation?

C-M-q (indent-pp-sexp) reindents the list that follows point. Not the 
same thing. And it's only available in Lisp.

> Also why 'prog-fill-reindent-defun'
> can't indent the region of code, but only the region of comments?

Do you want it to?

> Shouldn't 'M-q' only refill comments, and 'C-M-q' only indent code, as before?

Up until now, we thought that making two actions on one key binding 
available is a good thing, given that the context usually helps to 
disambiguate. This one seems like an exception, but IMHO not a strong 
enough one to roll back the change.

Message #29 received at 63790 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: 63790 <at> debbugs.gnu.org
Subject: Re: bug#63790: 30.0.50; prog-fill-reindent-defun regression
Date: Fri, 09 Jun 2023 20:37:04 +0300

>>>>     (re-search-forward "\\s-*\\s<" (line-end-position) t)
>>>
>>> It's looking for a comment that begins after point (possibly preceded by
>>> whitespace). There is no comment after point in the presented scenario.
>>>
>>>> It's nil in the reported case, so 'fill-paragraph' is not called.
>>>
>>> I guess when there is an active region, we would force the behavior to
>>> "refill" the region, no matter whether it is inside a comment, or contains
>>> a comment, or outside of any comments and simply contains code?
>> While 'prog-fill-reindent-defun' doesn't support indentation of an
>> arbitrary region of code and indents only the top-level list (defun),
>> it looks like the right thing is to fill the region.
>
> Okay?

Unless it's possible to make 'M-q' more predictable.

>> I still don't understand why 'M-q' now does the same what 'C-M-q' was
>> doing all the time with code indentation?
>
> C-M-q (indent-pp-sexp) reindents the list that follows point. Not the same
> thing. And it's only available in Lisp.

I tried in emacs-28 and in ruby-mode 'C-M-q' reindents the code.
This is from the Help window:

  C-M-q runs the command prog-indent-sexp (found in ruby-mode-map),
  which is an interactive byte-compiled Lisp function in ‘prog-mode.el’.

  It is bound to C-M-q, <menu-bar> <ruby> <Indent Sexp-9>.

  (prog-indent-sexp &optional DEFUN)

  Indent the expression after point.
  When interactively called with prefix, indent the enclosing defun
  instead.

>> Also why 'prog-fill-reindent-defun'
>> can't indent the region of code, but only the region of comments?
>
> Do you want it to?

It would be nice, and it's easy to implement just by calling 'indent-region'.

>> Shouldn't 'M-q' only refill comments, and 'C-M-q' only indent code, as before?
>
> Up until now, we thought that making two actions on one key binding
> available is a good thing, given that the context usually helps to
> disambiguate. This one seems like an exception, but IMHO not a strong
> enough one to roll back the change.

Before the change, the distinction was clear: 'C-M-q' reindents code, 'M-q'
refills text in comments.  Whereas I admit that 'M-q' is useless on code,
now the distinction is blurred, and DWIM is not reliable.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.