all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#63790: 30.0.50; prog-fill-reindent-defun regression
@ 2023-05-29 16:53 Juri Linkov
  2023-06-03  2:40 ` Dmitry Gutov
  0 siblings, 1 reply; 9+ messages in thread
From: Juri Linkov @ 2023-05-29 16:53 UTC (permalink / raw)
  To: 63790

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.





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#63790: 30.0.50; prog-fill-reindent-defun regression
  2023-05-29 16:53 bug#63790: 30.0.50; prog-fill-reindent-defun regression Juri Linkov
@ 2023-06-03  2:40 ` Dmitry Gutov
  2023-06-04 17:14   ` Juri Linkov
  0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Gutov @ 2023-06-03  2:40 UTC (permalink / raw)
  To: Juri Linkov, 63790

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?





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#63790: 30.0.50; prog-fill-reindent-defun regression
  2023-06-03  2:40 ` Dmitry Gutov
@ 2023-06-04 17:14   ` Juri Linkov
  2023-06-06  1:41     ` Dmitry Gutov
  0 siblings, 1 reply; 9+ messages in thread
From: Juri Linkov @ 2023-06-04 17:14 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 63790

>> 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'?





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#63790: 30.0.50; prog-fill-reindent-defun regression
  2023-06-04 17:14   ` Juri Linkov
@ 2023-06-06  1:41     ` Dmitry Gutov
  2023-06-06 18:55       ` Juri Linkov
  0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Gutov @ 2023-06-06  1:41 UTC (permalink / raw)
  To: Juri Linkov; +Cc: 63790

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?





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#63790: 30.0.50; prog-fill-reindent-defun regression
  2023-06-06  1:41     ` Dmitry Gutov
@ 2023-06-06 18:55       ` Juri Linkov
  2023-06-08  0:35         ` Dmitry Gutov
  0 siblings, 1 reply; 9+ messages in thread
From: Juri Linkov @ 2023-06-06 18:55 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 63790

>>>> 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.





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#63790: 30.0.50; prog-fill-reindent-defun regression
  2023-06-06 18:55       ` Juri Linkov
@ 2023-06-08  0:35         ` Dmitry Gutov
  2023-06-08 16:59           ` Juri Linkov
  0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Gutov @ 2023-06-08  0:35 UTC (permalink / raw)
  To: Juri Linkov; +Cc: 63790

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?





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#63790: 30.0.50; prog-fill-reindent-defun regression
  2023-06-08  0:35         ` Dmitry Gutov
@ 2023-06-08 16:59           ` Juri Linkov
  2023-06-09  1:58             ` Dmitry Gutov
  0 siblings, 1 reply; 9+ messages in thread
From: Juri Linkov @ 2023-06-08 16:59 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 63790

>>    (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?





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#63790: 30.0.50; prog-fill-reindent-defun regression
  2023-06-08 16:59           ` Juri Linkov
@ 2023-06-09  1:58             ` Dmitry Gutov
  2023-06-09 17:37               ` Juri Linkov
  0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Gutov @ 2023-06-09  1:58 UTC (permalink / raw)
  To: Juri Linkov; +Cc: 63790

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.





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#63790: 30.0.50; prog-fill-reindent-defun regression
  2023-06-09  1:58             ` Dmitry Gutov
@ 2023-06-09 17:37               ` Juri Linkov
  0 siblings, 0 replies; 9+ messages in thread
From: Juri Linkov @ 2023-06-09 17:37 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 63790

>>>>     (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.





^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2023-06-09 17:37 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-29 16:53 bug#63790: 30.0.50; prog-fill-reindent-defun regression Juri Linkov
2023-06-03  2:40 ` Dmitry Gutov
2023-06-04 17:14   ` Juri Linkov
2023-06-06  1:41     ` Dmitry Gutov
2023-06-06 18:55       ` Juri Linkov
2023-06-08  0:35         ` Dmitry Gutov
2023-06-08 16:59           ` Juri Linkov
2023-06-09  1:58             ` Dmitry Gutov
2023-06-09 17:37               ` Juri Linkov

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.