bug#20419: 25.0.50; doc string of `recenter-last-op`

bug#20419: 25.0.50; doc string of `recenter-last-op`

[Drew Adams]

The doc string says that these are the possible values:

"`top', `middle', `bottom', integer or float numbers."

It should also mention `nil', say what `nil' means, and perhaps even say
that `nil' is the default value.

A user reading things like the binding of this variable to nil in
`perform-replace' is not helped by consulting the variable's doc.



In GNU Emacs 25.0.50.1 (i686-pc-mingw32)
 of 2014-10-20 on LEG570
Bzr revision: 118168 rgm@gnu.org-20141020195941-icp42t8ttcnud09g
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --enable-checking=yes,glyphs CPPFLAGS=-DGLYPH_DEBUG=1'

bug#20419: 25.0.50; doc string of `recenter-last-op`

[Eli Zaretskii]

> Date: Fri, 24 Apr 2015 10:16:11 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> 
> The doc string says that these are the possible values:
> 
> "`top', `middle', `bottom', integer or float numbers."
> 
> It should also mention `nil', say what `nil' means, and perhaps even say
> that `nil' is the default value.

How do you see that nil has any meaning whatsoever?

bug#20419: 25.0.50; doc string of `recenter-last-op`

[Drew Adams]

> > The doc string says that these are the possible values:
> > "`top', `middle', `bottom', integer or float numbers."
> > It should also mention `nil', say what `nil' means, and
> > perhaps even say that `nil' is the default value.
> 
> How do you see that nil has any meaning whatsoever?

`nil' means that the next `recenter-top-bottom' starts with
the first element of `recenter-positions'.

It has the effect (meaning) that, as the two comments in that
replace.el code take the trouble to say, it "resets recenter
cycling order to initial position".

With that info added to the doc string, those comments can be
removed, and Lisp users will understand immediately that they
can do the same thing: bind it to nil to get that effect.

bug#20419: 25.0.50; doc string of `recenter-last-op`

[Eli Zaretskii]

> Date: Sat, 25 Apr 2015 07:29:20 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 20419@debbugs.gnu.org
> 
> > > The doc string says that these are the possible values:
> > > "`top', `middle', `bottom', integer or float numbers."
> > > It should also mention `nil', say what `nil' means, and
> > > perhaps even say that `nil' is the default value.
> > 
> > How do you see that nil has any meaning whatsoever?
> 
> `nil' means that the next `recenter-top-bottom' starts with
> the first element of `recenter-positions'.

Yuck!  Why not set it to the first member of recenter-positions to
begin with?

FWIW, I consider this, including the exposure of this variable as some
kind of knob for other Lisp packages, bad style.  You should be
demanding cleaning this up instead of documenting it.

Fixed under protest.

bug#20419: 25.0.50; doc string of `recenter-last-op`

[Drew Adams]

> > > > The doc string says that these are the possible values:
> > > > "`top', `middle', `bottom', integer or float numbers."
> > > > It should also mention `nil', say what `nil' means, and
> > > > perhaps even say that `nil' is the default value.
> > >
> > > How do you see that nil has any meaning whatsoever?
> >
> > `nil' means that the next `recenter-top-bottom' starts with
> > the first element of `recenter-positions'.
> 
> Yuck!  Why not set it to the first member of recenter-positions to
> begin with?
> 
> FWIW, I consider this, including the exposure of this variable as
> some kind of knob for other Lisp packages, bad style.  You should be
> demanding cleaning this up instead of documenting it.
> 
> Fixed under protest.

Point taken.  A better design (for its use in `perform-replace')
might bind `recenter-last-op' to the car of `recenter-positions'.
(The code also resets it later in `perform-replace'.  That setq
would also need to be changed, the same way.)

Or get rid of `recenter-last-op' and just have code bind and
set option `recenter-positions'...  But Emacs dev generally
doesn't like to do that, even locally.

As long as we have something like `recenter-last-op', it is
like `kill-ring-yank-pointer': it will be used by other code
and needs to be documented.

Please consider filing that bug report/enhancement request (or
consider fixing it in the context of this one), if you like.

But the behavior of nil remains unchanged.  It still needs to
be mentioned in the doc string, as long as it has the effect
it has.  Thanks for having fixed that.

bug#20419: 25.0.50; doc string of `recenter-last-op`

[Eli Zaretskii]

> Date: Sat, 25 Apr 2015 09:12:07 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 20419-done@debbugs.gnu.org
> 
> > Yuck!  Why not set it to the first member of recenter-positions to
> > begin with?
> > 
> > FWIW, I consider this, including the exposure of this variable as
> > some kind of knob for other Lisp packages, bad style.  You should be
> > demanding cleaning this up instead of documenting it.
> > 
> > Fixed under protest.
> 
> Point taken.  A better design (for its use in `perform-replace')
> might bind `recenter-last-op' to the car of `recenter-positions'.

I'd rather suggest to have a simple function, recenter-reset-cycle,
say, and then hide its internals inside window.el.

bug#20419: 25.0.50; doc string of `recenter-last-op`

[Drew Adams]

> I'd rather suggest to have a simple function, recenter-reset-cycle,
> say, and then hide its internals inside window.el.

Do you really think that is better than the kind of thing we do
with/for `kill-ring-yank-pointer' and `search-ring-yank-pointer'?
That code intentionally exposes the pointer variable.

If you are sure, go for it.  You might want to change the existing 
code for that uses `*-ring-yank-pointer' similarly.

I assume you would introduce another (internal) variable, which
would get modified in place of the user option, or which would
be a pointer (like `recenter-last-op' is now) to its current
(last-used) index.

Anyway, whether you do something like you propose or not, as
long as we have the current design, the doc bug fix you made
is helpful.