bug#4981: C-l during query-replace

bug#4981: C-l during query-replace

[Dan Nicolaescu]

C-l during query-replace should run `recenter-top-bottom', not
`recenter' for consistency with what C-l normally does nowadays.

bug#4981: C-l during query-replace

[Juri Linkov]

> C-l during query-replace should run `recenter-top-bottom', not
> `recenter' for consistency with what C-l normally does nowadays.

I guess there are many other places that need replacing with the new
command (e.g. `gnus-recenter').

But with the patch I proposed in
http://thread.gmane.org/gmane.emacs.devel/110349/focus=115915
the name `recenter-top-bottom' makes no sense anymore.
Maybe we should rename it to something more suitable
before replacing `recenter' calls with the new name everywhere?

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#4981: C-l during query-replace

[Juri Linkov]

> I guess there are many other places that need replacing with the new
> command (e.g. `gnus-recenter').

I fixed `gnus-recenter' in gnus-sum.el.

> But with the patch I proposed in
> http://thread.gmane.org/gmane.emacs.devel/110349/focus=115915
> the name `recenter-top-bottom' makes no sense anymore.
> Maybe we should rename it to something more suitable
> before replacing `recenter' calls with the new name everywhere?

Installed.  Currently I have no opinion about renaming
`recenter-top-bottom' to something more reasonable.

>> C-l during query-replace should run `recenter-top-bottom', not
>> `recenter' for consistency with what C-l normally does nowadays.

I can't find a clean solution because in the case of query-replace,
`this-command' is always `query-replace'.

This patch kinda works (though it doesn't reset the cycling order),
but I don't like this.

Index: lisp/window.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/window.el,v
retrieving revision 1.190
diff -u -r1.190 window.el
--- lisp/window.el	29 Nov 2009 23:34:09 -0000	1.190
+++ lisp/window.el	29 Nov 2009 23:42:30 -0000
@@ -1654,7 +1654,8 @@
    (arg (recenter arg))			; Always respect ARG.
    (t
     (setq recenter-last-op
-	  (if (eq this-command last-command)
+	  (if (or (eq this-command last-command)
+		  (eq this-command 'query-replace))
 	      (car (or (cdr (member recenter-last-op recenter-positions))
 		       recenter-positions))
 	    (car recenter-positions)))

Index: lisp/replace.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/replace.el,v
retrieving revision 1.287
diff -u -r1.287 replace.el
--- lisp/replace.el	12 Nov 2009 06:55:43 -0000	1.287
+++ lisp/replace.el	29 Nov 2009 23:43:28 -0000
@@ -1785,7 +1788,9 @@
 			((eq def 'skip)
 			 (setq done t))
 			((eq def 'recenter)
-			 (recenter nil))
+			 (recenter-top-bottom))
 			((eq def 'edit)
 			 (let ((opos (point-marker)))
 			   (setq real-match-data (replace-match-data

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#4981: C-l during query-replace

[Stefan Monnier]

> I can't find a clean solution because in the case of query-replace,
> `this-command' is always `query-replace'.

Why not:

> Index: lisp/replace.el
> ===================================================================
> RCS file: /sources/emacs/emacs/lisp/replace.el,v
> retrieving revision 1.287
> diff -u -r1.287 replace.el
> --- lisp/replace.el	12 Nov 2009 06:55:43 -0000	1.287
> +++ lisp/replace.el	29 Nov 2009 23:43:28 -0000
> @@ -1785,7 +1788,9 @@
>  			((eq def 'skip)
>  			 (setq done t))
>  			((eq def 'recenter)
> -			 (recenter nil))
> +                      (let ((this-command 'recenter-top-bottom))
> +			 (recenter-top-bottom)))
>  			((eq def 'edit)
>  			 (let ((opos (point-marker)))
>  			   (setq real-match-data (replace-match-data


-- Stefan

bug#4981: C-l during query-replace

[Dan Nicolaescu]

Juri Linkov <juri@jurta.org> writes:

  > > I guess there are many other places that need replacing with the new
  > > command (e.g. `gnus-recenter').
  > 
  > I fixed `gnus-recenter' in gnus-sum.el.
  > 
  > > But with the patch I proposed in
  > > http://thread.gmane.org/gmane.emacs.devel/110349/focus=115915
  > > the name `recenter-top-bottom' makes no sense anymore.
  > > Maybe we should rename it to something more suitable
  > > before replacing `recenter' calls with the new name everywhere?
  > 
  > Installed.  Currently I have no opinion about renaming
  > `recenter-top-bottom' to something more reasonable.
  > 
  > >> C-l during query-replace should run `recenter-top-bottom', not
  > >> `recenter' for consistency with what C-l normally does nowadays.
  > 
  > I can't find a clean solution because in the case of query-replace,
  > `this-command' is always `query-replace'.
  > 
  > This patch kinda works (though it doesn't reset the cycling order),
  > but I don't like this.
  > 
  > Index: lisp/window.el
  > ===================================================================
  > RCS file: /sources/emacs/emacs/lisp/window.el,v
  > retrieving revision 1.190
  > diff -u -r1.190 window.el
  > --- lisp/window.el	29 Nov 2009 23:34:09 -0000	1.190
  > +++ lisp/window.el	29 Nov 2009 23:42:30 -0000
  > @@ -1654,7 +1654,8 @@
  >     (arg (recenter arg))			; Always respect ARG.
  >     (t
  >      (setq recenter-last-op
  > -	  (if (eq this-command last-command)
  > +	  (if (or (eq this-command last-command)
  > +		  (eq this-command 'query-replace))
  >  	      (car (or (cdr (member recenter-last-op recenter-positions))
  >  		       recenter-positions))
  >  	    (car recenter-positions)))
  > 
  > Index: lisp/replace.el
  > ===================================================================
  > RCS file: /sources/emacs/emacs/lisp/replace.el,v
  > retrieving revision 1.287
  > diff -u -r1.287 replace.el
  > --- lisp/replace.el	12 Nov 2009 06:55:43 -0000	1.287
  > +++ lisp/replace.el	29 Nov 2009 23:43:28 -0000
  > @@ -1785,7 +1788,9 @@
  >  			((eq def 'skip)
  >  			 (setq done t))
  >  			((eq def 'recenter)
  > -			 (recenter nil))
  > +			 (recenter-top-bottom))
  >  			((eq def 'edit)
  >  			 (let ((opos (point-marker)))
  >  			   (setq real-match-data (replace-match-data

Thanks for fixing this.  Are you sure that the new `recenter-positions'
is needed?  Given that there are 3 choices, it's easy to cycle through
them, so adding yet another defcustom that would be use by a very small
number of users does not seem justified (IMHO).

bug#4981: C-l during query-replace

[Juanma Barranquero]

On Mon, Nov 30, 2009 at 07:29, Dan Nicolaescu <dann@ics.uci.edu> wrote:

> Are you sure that the new `recenter-positions'
> is needed?  Given that there are 3 choices, it's easy to cycle through
> them

With Juri's `recenter-positions', there are more than 3 choices.

    Juanma

bug#4981: C-l during query-replace

[Juri Linkov]

>> Are you sure that the new `recenter-positions'
>> is needed?  Given that there are 3 choices, it's easy to cycle through
>> them
>
> With Juri's `recenter-positions', there are more than 3 choices.

Or what is more important, also *less* than 3 choices :-)

E.g. when two positions is enough, I'd like to use for
`recenter-positions' the value `(0.15 top)'.

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#4981: C-l during query-replace

[Stefan Monnier]

> Thanks for fixing this.  Are you sure that the new `recenter-positions'
> is needed?  Given that there are 3 choices, it's easy to cycle through
> them, so adding yet another defcustom that would be use by a very small
> number of users does not seem justified (IMHO).

I agree that it's overengineering.  This patch is only acceptable if
(to compensate) it unifies the two duplicate code paths of
move-to-window-line-top-bottom and recenter-top-bottom.


        Stefan

bug#4981: C-l during query-replace

[Juri Linkov]

>> Thanks for fixing this.  Are you sure that the new `recenter-positions'
>> is needed?  Given that there are 3 choices, it's easy to cycle through
>> them, so adding yet another defcustom that would be use by a very small
>> number of users does not seem justified (IMHO).
>
> I agree that it's overengineering.

I think what is overengineering is adding recenter-top-bottom
in the first place.  It imposes the arbitrary fixed cycling order
on users with no hope to customize such fundamental feature as
recentering.  `recenter-positions' mitigates this problem in the true
Emacs way as the *customizable* editor.

Please also note that even a minor feature `next-error' allows a similar
customization with `next-error-recenter'.  Perhaps we should try to merge
them, or at least provide a new option in `next-error-recenter' to use the
first value of `recenter-positions' as the primary position the user
prefers to put point after recentering.

> This patch is only acceptable if (to compensate) it unifies the two
> duplicate code paths of move-to-window-line-top-bottom and
> recenter-top-bottom.

Do you mean we should merge move-to-window-line-top-bottom and
recenter-top-bottom into one function?

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#4981: marked as done (C-l during query-replace)

[Emacs bug Tracking System]

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

Your message dated Mon, 30 Nov 2009 18:06:33 +0200
with message-id <87skbwt9xq.fsf@mail.jurta.org>
and subject line Re: bug#4981: C-l during query-replace
has caused the Emacs bug report #4981,
regarding C-l during query-replace
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact owner@emacsbugs.donarmstrong.com
immediately.)


-- 
4981: http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=4981
Emacs Bug Tracking System
Contact owner@emacsbugs.donarmstrong.com with problems

[-- Attachment #2: Type: message/rfc822, Size: 2707 bytes --]

From: Dan Nicolaescu <dann@ics.uci.edu>
To: bug-gnu-emacs <bug-gnu-emacs@gnu.org>
Subject: C-l during query-replace
Date: Thu, 19 Nov 2009 16:16:59 -0800 (PST)
Message-ID: <200911200016.nAK0Gxf5005670@godzilla.ics.uci.edu>


C-l during query-replace should run `recenter-top-bottom', not
`recenter' for consistency with what C-l normally does nowadays.



[-- Attachment #3: Type: message/rfc822, Size: 2400 bytes --]

From: Juri Linkov <juri@jurta.org>
To: Stefan Monnier <monnier@iro.umontreal.ca>
Cc: 4981-done@emacsbugs.donarmstrong.com, Dan Nicolaescu <dann@ics.uci.edu>
Subject: Re: bug#4981: C-l during query-replace
Date: Mon, 30 Nov 2009 18:06:33 +0200
Message-ID: <87skbwt9xq.fsf@mail.jurta.org>

> Why not:
>
>> Index: lisp/replace.el
>> ===================================================================
>> RCS file: /sources/emacs/emacs/lisp/replace.el,v
>> retrieving revision 1.287
>> diff -u -r1.287 replace.el
>> --- lisp/replace.el	12 Nov 2009 06:55:43 -0000	1.287
>> +++ lisp/replace.el	29 Nov 2009 23:43:28 -0000
>> @@ -1785,7 +1788,9 @@
>>  			((eq def 'skip)
>>  			 (setq done t))
>>  			((eq def 'recenter)
>> -			 (recenter nil))
>> +                      (let ((this-command 'recenter-top-bottom))
>> +			 (recenter-top-bottom)))
>>  			((eq def 'edit)
>>  			 (let ((opos (point-marker)))
>>  			   (setq real-match-data (replace-match-data

Thanks, this helped.  Fixed with more changes to keep the cycling order.

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#4981: C-l during query-replace

[Drew Adams]

> >> Thanks for fixing this.  Are you sure that the new 
> >> `recenter-positions' is needed?  Given that there
> >> are 3 choices, it's easy to cycle through
> >> them, so adding yet another defcustom that would be use by 
> >> a very small number of users does not seem justified (IMHO).
> >
> > I agree that it's overengineering.
> 
> I think what is overengineering is adding recenter-top-bottom
> in the first place.  It imposes the arbitrary fixed cycling order
> on users with no hope to customize such fundamental feature as
> recentering.  `recenter-positions' mitigates this problem in the true
> Emacs way as the *customizable* editor.

As the one originally responsible for `recenter-top-bottom', let me chime in.
;-)

1. Just as with `recenter', a `recenter-top-bottom' user can always provide a
prefix arg to get the exact behavior wanted. It imposes nothing more than
`recenter' imposed.

2. The fact that we seem to be extending the use of this to other areas
indicates that it has proved to be an improvement wrt `recenter'.

3. I have no objection to user's being able, via an option, to add more cycle
points and define their positions. That's not overengineering, IMO. I think the
default should be what `recenter-top-bottom' defined: 3 cycle points, top,
center, bottom.

 - Drew

bug#4981: C-l during query-replace

[Stefan Monnier]

> Do you mean we should merge move-to-window-line-top-bottom and
> recenter-top-bottom into one function?

No, but the core of both (which is identical) should be moved to a third
function, used by the two commands.  So as to remove the code duplication.


        Stefan