bug#25777: 25.1; [PATCH] `rectangle--pos-cols' should not move point

[Drew Adams <drew.adams@oracle.com>]

> Ah, with this hint I figured out what the user-visible side-effect is,
> from 'emacs -Q' do:
> 
> C-x SPC
> C-3 C-p M-f ;; the bottom 2 lines of the rectangle now extend past eol
> C-x C-x ;; point is now past eol
> M-: (save-excursion (rectangle--pos-cols 1 3)) RET ;; => (0 . 2)
> C-f ;; Now point jumps to column 1 instead of 8.

Thanks.  Yes, I see that.

Except that even though point is in column 1,
M-: (current-column) says it is in column 0 (should it?).

> So calling rectangle--pos-cols with START and END not corresponding to
> the current rectangle can mess with the selection.

(I call it only with START and END for the selection,
i.e., the active region.)

> > The use case I have, in `modeline-posn.el', is about the
> > rectangular region.  It tries to report on displayed
> > columns, not buffer positions.  So I think that for my
> > use case, at least, I need the full-blown
> > `rectangle--pos-cols' code (under whatever name).
> 
> Perhaps you want this:
> 
> (defun rectangle-columns ()
>   "Return the current rectangle's columns.
> The return value is a cons of the form (START-COLUMN . END-COLUMN)."
>   (save-excursion
>    (rectangle--pos-cols (region-beginning) (region-end))))

(save-excursion
 (rectangle--pos-cols (region-beginning) (region-end)))

is indeed what I use.

> Which should call rectangle--pos-cols only with positions
> corresponding to the current rectangle, and so won't be able to affect
> the selection (I think?).

That makes sense.  But I don't think that is what a
function called `rectangle-columns' should do.  What
you show is a function that is better called something
like `rectangle-region-columns', and its doc string
would need to say that it returns the columns for the
region.  And I think it is probably not useful when
the region is active - in that case, what you showed
before, which just uses `current-column', is probably
more useful.

I'm not sure where we should go from here.

The fix to _this bug_ is to add `save-excursion',
however you want to do that.

Whether this function should be renamed or another
function should be created, and in the latter case,
whether it should be able to work correctly whether
or not the region is active, I'm not sure.

I propose that you (please) fix and close this bug
by adding `save-excursion', and you (please) submit a
question to emacs-devel about what should be done for
a (new) function that returns the column positions of
an arbitrary rectangle (defined by START and END
positions).  Is such a function useful, and if yes,
just what should it do?

One possibility might be to give such a function
conditional behavior:

. If region is active, use it (as you do above, and
  as I do in my code).

. If not, do the simpler calculation, not trying to
  take any special display matters into account,
  beyond what `current-column' handles.

Dunno.  I have the answer for my code (I wrap the
`rectangle--pos-cols' calls with `save-excursion').
And I think it would be good to fix `r--p-c' by
adding `save-excursion'.  (And I suspect that other
`goto-char' occurrences added for Emacs 25 to rect.el
should also be so wrapped.)

Beyond that, what might be a useful `rectangle-pos-cols'
function, I'm not sure.  We'd want a function that,
given `pos' (i.e., two positions START and END) returns
`cols' (i.e., two columns).  And I think we'd want it
to DTRT for both the active-region case and the case
where the region is not active (and so not taken into
account).

When the region is not active, I don't think (but
I'm not sure) that things need to be complicated,
since `current-column' should already DTRT wrt
wide chars etc.