GNU bug report logs - #4981
C-l during query-replace

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 4981 in the body.
You can then email your comments to 4981 AT debbugs.gnu.org in the normal way.

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

Message #3 received at submit <at> emacsbugs.donarmstrong.com (full text, mbox):

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

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

Message #8 received at 4981 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Dan Nicolaescu <dann <at> ics.uci.edu>
Cc: 4981 <at> debbugs.gnu.org
Subject: Re: bug#4981: C-l during query-replace
Date: Fri, 20 Nov 2009 11:29:20 +0200

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

Message #13 received at 4981 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Dan Nicolaescu <dann <at> ics.uci.edu>
Cc: 4981 <at> debbugs.gnu.org
Subject: Re: bug#4981: C-l during query-replace
Date: Mon, 30 Nov 2009 01:44:30 +0200

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

Message #18 received at 4981 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Juri Linkov <juri <at> jurta.org>
Cc: 4981 <at> debbugs.gnu.org, Dan Nicolaescu <dann <at> ics.uci.edu>
Subject: Re: bug#4981: C-l during query-replace
Date: Sun, 29 Nov 2009 20:28:07 -0500

> 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

Message #21 received at 4981 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Dan Nicolaescu <dann <at> ics.uci.edu>
To: Juri Linkov <juri <at> jurta.org>
Cc: 4981 <at> debbugs.gnu.org
Subject: Re: bug#4981: C-l during query-replace
Date: Sun, 29 Nov 2009 22:29:59 -0800 (PST)

Juri Linkov <juri <at> 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).

Message #26 received at 4981 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Juanma Barranquero <lekktu <at> gmail.com>
To: Dan Nicolaescu <dann <at> ics.uci.edu>
Cc: Juri Linkov <juri <at> jurta.org>, 4981 <at> debbugs.gnu.org
Subject: Re: bug#4981: C-l during query-replace
Date: Mon, 30 Nov 2009 12:12:29 +0100

On Mon, Nov 30, 2009 at 07:29, Dan Nicolaescu <dann <at> 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

Message #31 received at 4981 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Juanma Barranquero <lekktu <at> gmail.com>
Cc: Dan Nicolaescu <dann <at> ics.uci.edu>, 4981 <at> debbugs.gnu.org
Subject: Re: bug#4981: C-l during query-replace
Date: Mon, 30 Nov 2009 14:04:17 +0200

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

Message #36 received at 4981 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dan Nicolaescu <dann <at> ics.uci.edu>
Cc: 4981 <at> debbugs.gnu.org, Juri Linkov <juri <at> jurta.org>
Subject: Re: bug#4981: C-l during query-replace
Date: Mon, 30 Nov 2009 09:13:52 -0500

> 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

Message #41 received at 4981 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dan Nicolaescu <dann <at> ics.uci.edu>, 4981 <at> debbugs.gnu.org
Subject: Re: bug#4981: C-l during query-replace
Date: Mon, 30 Nov 2009 18:07:45 +0200

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

Message #46 received at 4981-done <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 4981-done <at> debbugs.gnu.org, Dan Nicolaescu <dann <at> ics.uci.edu>
Subject: Re: bug#4981: C-l during query-replace
Date: Mon, 30 Nov 2009 18:06:33 +0200

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

Message #51 received at 4981 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: "Drew Adams" <drew.adams <at> oracle.com>
To: "'Juri Linkov'" <juri <at> jurta.org>, <4981 <at> debbugs.gnu.org>,
        "'Stefan Monnier'" <monnier <at> iro.umontreal.ca>
Cc: "'Dan Nicolaescu'" <dann <at> ics.uci.edu>
Subject: RE: bug#4981: C-l during query-replace
Date: Mon, 30 Nov 2009 09:24:33 -0800

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

Message #56 received at 4981 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Juri Linkov <juri <at> jurta.org>
Cc: Dan Nicolaescu <dann <at> ics.uci.edu>, 4981 <at> debbugs.gnu.org
Subject: Re: bug#4981: C-l during query-replace
Date: Mon, 30 Nov 2009 15:55:10 -0500

> 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

Previous Next

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