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

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

[Drew Adams]

The code defining `rectangle--pos-cols' mistakenly moves point.  All of
the calls to `goto-char' in this function need to be wrapped in
`save-excursion'.  Below is a patch that does this.

(This bug causes a regression from the correct behavior in Emacs 24.5,
BTW.)

I think that most, if not all, of the other uses of `goto-char' in
rect.el also need to be wrapped with `save-excursion'.  But as I am not
sure of this, I leave it to someone else to take a close look and DTRT.

-------------------------

diff -u rect.el rect-2017-02-17-patched.el
--- rect.el	2017-02-17 09:37:56.938748300 -0800
+++ rect-2017-02-17-patched.el	2017-02-17 09:43:13.884439700 -0800
@@ -85,26 +85,26 @@
                     (cdr rectangle--mark-crutches)
                   (if rectangle--mark-crutches
                       (setq rectangle--mark-crutches nil))
-                  (goto-char end) (current-column))))
+                  (save-excursion (goto-char end) (current-column)))))
         (if (eq start end) (cons (min sc ec) (max sc ec)) (cons sc ec))))
      ((eq end (car cw))
       (if (eq start (car rectangle--mark-crutches))
           (cons (cdr rectangle--mark-crutches) (cdr cw))
         (if rectangle--mark-crutches (setq rectangle--mark-crutches nil))
-        (cons (progn (goto-char start) (current-column)) (cdr cw))))
+        (cons (save-excursion (goto-char start) (current-column)) (cdr cw))))
      ((progn
         (if cw (setf (window-parameter nil 'rectangle--point-crutches) nil))
         (eq start (car rectangle--mark-crutches)))
       (let ((sc (cdr rectangle--mark-crutches))
-            (ec (progn (goto-char end) (current-column))))
+            (ec (save-excursion (goto-char end) (current-column))))
         (if (eq start end) (cons (min sc ec) (max sc ec)) (cons sc ec))))
      ((eq end (car rectangle--mark-crutches))
-      (cons (progn (goto-char start) (current-column))
+      (cons (save-excursion (goto-char start) (current-column))
             (cdr rectangle--mark-crutches)))
      (t
       (if rectangle--mark-crutches (setq rectangle--mark-crutches nil))
-      (cons (progn (goto-char start) (current-column))
-            (progn (goto-char end) (current-column)))))))
+      (cons (save-excursion (goto-char start) (current-column))
+            (save-excursion (goto-char end) (current-column)))))))
 
 (defun rectangle--col-pos (col kind)
   (let ((c (move-to-column col)))

---------------------


In GNU Emacs 25.1.1 (x86_64-w64-mingw32)
 of 2016-11-15
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --without-dbus --without-compress-install 'CFLAGS=-O2
 -static -g3''

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

[Drew Adams]

As I mentioned, this is a regression.  I think the bug and the patch
are pretty self-explanatory, but if you feel you need a recipe to
show that this causes cursor movement then let me know.

I hope this patch or similar can be applied soon.  Thx.

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

[npostavs]

Drew Adams <drew.adams@oracle.com> writes:

> As I mentioned, this is a regression.  I think the bug and the patch
> are pretty self-explanatory, but if you feel you need a recipe to
> show that this causes cursor movement then let me know.

Yes, an example would be useful.  Perhaps it's the callers of
rectangle--pos-cols that should use save-excursion.

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

[Drew Adams]

> > As I mentioned, this is a regression.  I think the bug and the patch
> > are pretty self-explanatory, but if you feel you need a recipe to
> > show that this causes cursor movement then let me know.
> 
> Yes, an example would be useful.  Perhaps it's the callers of
> rectangle--pos-cols that should use save-excursion.

I can certainly wrap all calls of `rectangle--pos-cols' with
`save-excursion'.  But that should not be necessary.  There
is no reason for that function to leave point moved.  It's
only aim in moving point is to do so temporarily.

Point is moved in Emacs 25, and it was not moved before.
It's a gratuitous regression.  `rectangle--pos-cols' does
not benefit in any way by leaving point moved.

Anyway, thanks for looking at this.  Here is a recipe.  You
can try to make it more minimal if you like, but it is pretty
simple and it shows the problem easily enough.

Download library `modeline-posn.el', from here:
https://www.emacswiki.org/emacs/download/modeline-posn.el

Evaluate these 3 variables: option `modelinepos-style' and
defvars `modelinepos-region-acting-on' and `modelinepos-rect-p'.

Evaluate this:

(defadvice rectangle-mark-mode
 (after bind-modelinepos-region-acting-on activate)
    (setq modelinepos-region-acting-on  rectangle-mark-mode
          modelinepos-rect-p            rectangle-mark-mode)  
    (redisplay 'force))

And evaluate the setq-default sexp for `mode-line-position'
(the one for Emacs > 22).

In buffer *scratch*:

M-x show-paren-mode RET
M-x set-variable RET show-paren-style RET mixed RET

Type this at the beginning of a line at the end of the buffer
(in *scratch*):

() RET () RET

So you see this, with the cursor (_) at eob, i.e., after the
final newline:

()
()
_

Then do `C-x SPC C-p'.  The cursor moves briefly up one line
and then immediately jumps back to where mark was, at the end
of the buffer.  It should stay where `C-p' put it, on the
second left paren.  The problem seems to come from an error
being raised in the context of the show-paren code.

An easier way to repro it is to just load file modeline-posn.el
and then do what I described starting with "In buffer *scratch:".
In that case you will see the intended effect of the library
when the region is active in `rectangle-mark-mode': The mode
line says how many rows & columns are in the rectangular region.
Of course, for the recipe the cursor will not move, but stays
stuck at eob, so the mode line just reads "0 rows, 0 cols".
Actually, you can see it briefly change to "1 rows, 0 cols"
and then change back to "0 rows, 0 cols".

To debug this, since I could not use the debugger (the code
is initiated while updating the mode-line during redisplay),
I had to just insert calls to `message' etc.  I commented
out parts of `modelinepos-style', to find the part that
matters.  Commenting out this, and replacing it by 999,
prevents the problem:

(let ((rpc (rectangle--pos-cols (region-beginning)
                                (region-end))))
  (abs (- (car rpc) (cdr rpc))))

I then replaced just the (abs...) with this:

(message "RPC: %S" rpc)
999

That printed (0 . 0) for RPC, but the problem was reproduced.
That told me that the problem came from just calling
`rectangle--pos-cols'.  Changing the above to just this
removed the problem:

(let ((rpc '(0 . 0))) (abs (- (car rpc) (cdr rpc))))

I checked the code of `rectangle--pos-cols' and saw that
it now does `goto-char' (and it did not do so before
Emacs 25.1).  Wrapping it in `save-excursion' fixed the
problem, without any negative effect on the function
(it does not need point to remain moved).

Such code should not gratuitously add cursor motion.
Other `goto-char' calls were also added, in other parts
of the same file, for Emacs 25.  Perhaps the person who
updated the code did not think about the consequence.

Point is not moved by the code in order to leave it in
a different place.  It is changed only for a temporary
effect.  That point-moving code should be wrapped in
`save-excursion'.  My advice is to check the other
such bare `goto-char' occurrences that were added in
Emacs 25.  I'm guessing that none of them need to leave
point moved.

Code should be able to call `rectangle--pos-cols' just
for its value, without getting side effects like cursor
movement.  If `rectangle--pos-cols' needed, or intended,
to leave point moved somewhere for some reason then yes,
of course, any calling code that doesn't want that should
be responsible for wrapping the calls with `save-excursion'.
But that's not the case: There is no reason for the function
to have the side effect of leaving point in a different position.

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

[npostavs]

Drew Adams <drew.adams@oracle.com> writes:

>
> I checked the code of `rectangle--pos-cols' and saw that
> it now does `goto-char' (and it did not do so before
> Emacs 25.1).

I'm confused as to what you mean by this, since `rectangle--pos-cols'
did not exist before 25.1.

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

[Drew Adams]

> > I checked the code of `rectangle--pos-cols' and saw that
> > it now does `goto-char' (and it did not do so before
> > Emacs 25.1).
> 
> I'm confused as to what you mean by this, since `rectangle--pos-cols'
> did not exist before 25.1.

Sorry, my bad about that expression.  I meant to say that
_there is no such problem in Emacs 24.5_.  In Emacs 24.5,
following the recipe DTRT: the C-p moves point up one line.
Emacs 24.5, like 25.1, has rectangle support.

Nothing changes in that regard for my code: I take advantage
of exactly the same functionality offered by rect.el as before.
What has changed is how rect.el obtains and returns the columns
delimiting the current region rectangle.  I changed my code
accordingly.

Perhaps I'm not being sufficiently clear about this regression.

I think it's great that a function such as `rectangle--pos-cols'
was added.  (But it has no business being considered "internal".)

Just as, for Emacs 24.5, I reused some code from rect.el to
do what I need to get the rectangle columns, so I reused
the corresponding code from Emacs 25.

Look at function `apply-on-rectangle'.  In Emacs 25 it calls
`rectangle--pos-cols' to get the rectangle columns - and so
do I.  In Emacs 24.5 it also does just what I do (I stole
the code) to obtain those columns.

Do you really think that a function whose only purpose is
to let you know what the rectangle columns are should move
point and leave it in a position that is not one of the
rectangle corners?

If so, then you will say that all callers of this function
should wrap with `save-excursion' if they don't want point
moved.  I think that's the job of a function whose only
purpose is to return those columns.

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

[Noam Postavsky]

On Mon, Feb 27, 2017 at 12:51 PM, Drew Adams <drew.adams@oracle.com> wrote:
> Just as, for Emacs 24.5, I reused some code from rect.el to
> do what I need to get the rectangle columns, so I reused
> the corresponding code from Emacs 25.
>
> Look at function `apply-on-rectangle'.  In Emacs 25 it calls
> `rectangle--pos-cols' to get the rectangle columns - and so
> do I.  In Emacs 24.5 it also does just what I do (I stole
> the code) to obtain those columns.

In both 24.5 and 25.1, apply-on-rectangle wraps its body in
save-excursion, and in both versions the body contains no nested
save-excursions. So it looks to me like the regression is in your code
which decides not to call save-excursion when running on Emacs 25.

   (if modelinepos-rect-p
        (if (fboundp 'rectangle--pos-cols) ; Emacs 25+
            (let ((rpc  (rectangle--pos-cols (region-beginning) (region-end))))
              (abs (- (car rpc) (cdr rpc))))
          (let ((start  (region-beginning))
                (end    (region-end))
                startcol endcol)
            (save-excursion ...

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

[Drew Adams]

> > Just as, for Emacs 24.5, I reused some code from rect.el to
> > do what I need to get the rectangle columns, so I reused
> > the corresponding code from Emacs 25.
> >
> > Look at function `apply-on-rectangle'.  In Emacs 25 it calls
> > `rectangle--pos-cols' to get the rectangle columns - and so
> > do I.  In Emacs 24.5 it also does just what I do (I stole
> > the code) to obtain those columns.
> 
> In both 24.5 and 25.1, apply-on-rectangle wraps its body in
> save-excursion, and in both versions the body contains no nested
> save-excursions. So it looks to me like the regression is in your code
> which decides not to call save-excursion when running on Emacs 25.

OK, clearly I'm not getting through to you.  The bug will remain, and
I'll wrap my calls to `rectangle--pos-cols' in `save-excursion'.

For the record:

1. I do not call `apply-on-rectangle'.

2. The purpose of `rectangle--pos-cols', just as was the purpose
of the previous (Emacs 24.5) code, is to return the rectangle
columns.  Nothing more.  Its purpose is not to move point and
leave it in some other place that is not a rectangle corner.

3. `rectangle--pos-cols' is a general function.  It should not
be considered internal.  It is useful generally - I have
reused it, as one example.  (I have not used `apply-on-rectangle'.)

Is there a reason to have `rectangle--pos-cols' move point,
instead of use `save-excursion'?  You've given none.  Do you
argue that performance matters here?  What's the argument in
favor of not having this code be as clean as it was in Emacs
24.5?  Why favor this regression?

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

[Noam Postavsky]

On Mon, Feb 27, 2017 at 2:21 PM, Drew Adams <drew.adams@oracle.com> wrote:
> 2. The purpose of `rectangle--pos-cols', just as was the purpose
> of the previous (Emacs 24.5) code, is to return the rectangle
> columns.  Nothing more.  Its purpose is not to move point and
> leave it in some other place that is not a rectangle corner.

The code you extracted in 24.5 is (modulo a few dropped lines)

    (save-excursion
      (goto-char start)
      (setq startcol (current-column))
      (beginning-of-line)
      (setq startpt (point)) ;; [dropped in modeline-posn]
      (goto-char end)
      (setq endcol (current-column))
      (forward-line 1) ;; [dropped in modeline-posn]
      (setq endpt (point-marker)) ;; [dropped in modeline-posn]
      ;; ensure the start column is the left one.
      (if (< endcol startcol)
      (let ((col startcol))
        (setq startcol endcol endcol col)))


The equivalent in 25.1 is

  (save-excursion
    (let* ((cols (rectangle--pos-cols start end))
           (startcol (car cols))
           (endcol (cdr cols))

But for some reason you don't want to extract the save-excursion from 25.1.

>
> 3. `rectangle--pos-cols' is a general function.  It should not
> be considered internal.  It is useful generally - I have
> reused it, as one example.  (I have not used `apply-on-rectangle'.)
>
> Is there a reason to have `rectangle--pos-cols' move point,
> instead of use `save-excursion'?

I don't want to add unneeded clutter.

> What's the argument in
> favor of not having this code be as clean as it was in Emacs
> 24.5?  Why favor this regression?

It's clearly not a regression.

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

[Drew Adams]

> > 2. The purpose of `rectangle--pos-cols', just as was the purpose
> > of the previous (Emacs 24.5) code, is to return the rectangle
> > columns.  Nothing more.  Its purpose is not to move point and
> > leave it in some other place that is not a rectangle corner.
> 
> The code you extracted in 24.5 is (modulo a few dropped lines)
> 
>     (save-excursion
>       (goto-char start)
>       (setq startcol (current-column))
>       (beginning-of-line)
>       (setq startpt (point)) ;; [dropped in modeline-posn]
>       (goto-char end)
>       (setq endcol (current-column))
>       (forward-line 1) ;; [dropped in modeline-posn]
>       (setq endpt (point-marker)) ;; [dropped in modeline-posn]
>       ;; ensure the start column is the left one.
>       (if (< endcol startcol)
>       (let ((col startcol))
>         (setq startcol endcol endcol col)))
> 
> The equivalent in 25.1 is
> 
>   (save-excursion
>     (let* ((cols (rectangle--pos-cols start end))
>            (startcol (car cols))
>            (endcol (cdr cols))
> 
> But for some reason you don't want to extract the
> save-excursion from 25.1.

You still don't understand, it seems.  The workaround for my
code is obvious, and I have made it: wrapped each call to
`rectangle--pos-cols' in `save-excursion'.

The point of filing the bug is to help Emacs, not my code.
I wanted you to be able to test to repro the bug using my
code before adding a `save-excursion' wrapper.

Clearly I have to add such a wrapper anyway, since my library
needs to support Emacs 25.1 (in addition to 22-24).  A fix of
`rectangle--pos-cols' for Emacs 25.2 or later would not let me
avoid wrapping with `save-excursion'.  It cannot possibly be
for the benefit of my code that this bug gets fixed or that I
filed the bug.

Clearly I think that this function should clean up after
itself, using `save-excursion'.  Its _only_ purpose, AFAICT,
is its return value, not (also) the side effect of leaving
point somewhere other than at the rectangle corner.

Clearly you disagree about that - that it should clean up
after itself.  Do you also think that it is important that
it leave point at a new location?  Is there a reason that
it _should_ have that side effect, and not simply return
the proper value?

> > 3. `rectangle--pos-cols' is a general function.  It should not
> > be considered internal.  It is useful generally - I have
> > reused it, as one example.  (I have not used `apply-on-rectangle'.)
> >
> > Is there a reason to have `rectangle--pos-cols' move point,
> > instead of use `save-excursion'?
> 
> I don't want to add unneeded clutter.

What you call "unneeded" and "clutter" I call normal and clean.
What you call uncluttered code I call irresponsible code.  We
will, I guess, just have to agree to disagree about this.

> > What's the argument in
> > favor of not having this code be as clean as it was in Emacs
> > 24.5?  Why favor this regression?
> 
> It's clearly not a regression.

Not if code calls only `apply-on-rectangle', no.

Apparently you don't think that `rectangle--pos-cols' is (or
could be, if the bug were fixed) a useful general function.
That's OK.  Users will just roll their own.  I think we can
end this discussion now - feel free to close it as wont-fix.
Thanks for spending time looking into it, anyway (sincerely).

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

[npostavs]

Drew Adams <drew.adams@oracle.com> writes:

> I think it's great that a function such as `rectangle--pos-cols'
> was added.  (But it has no business being considered "internal".)
>
> Do you really think that a function whose only purpose is
> to let you know what the rectangle columns are should move
> point and leave it in a position that is not one of the
> rectangle corners?

After looking over this thread again, I conclude we got totally
side-tracked on the question of whether this is a regression or not.
That's just a distraction.

I think it's reasonable to make rectangle--pos-cols preserve point.
Could you update your patch to

1. Use a single save-excursion around the whole body, instead of adding
several.

2. Rename rectangle--pos-cols to rectangle-pos-cols.

3. Add a docstring to rectangle-pos-cols.

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

[Drew Adams]

> > I think it's great that a function such as `rectangle--pos-cols'
> > was added.  (But it has no business being considered "internal".)
> >
> > Do you really think that a function whose only purpose is
> > to let you know what the rectangle columns are should move
> > point and leave it in a position that is not one of the
> > rectangle corners?
> 
> After looking over this thread again, I conclude we got totally
> side-tracked on the question of whether this is a regression or not.
> That's just a distraction.

I agree, and that was my fault.  In my mind it caused my code
to regress.  My bad.

> I think it's reasonable to make rectangle--pos-cols preserve point.
> Could you update your patch to
> 
> 1. Use a single save-excursion around the whole body, instead of adding
> several.
> 2. Rename rectangle--pos-cols to rectangle-pos-cols.
> 3. Add a docstring to rectangle-pos-cols.

I could do that, yes.

But IMHO, it is generally better to scope a `save-excursion'
as tightly as possible around the movement that you want to
control (hide, erase, undo).

Unless there is something critical for performance (and I
don't think `save-excursion' is costly), that's better.

Why?  Because it makes the code much clearer.  It tells you
that outside the `save-excursion' zones point is unlikely
to be moved.  And that makes maintenance easier and less
error-prone.

Putting a `s-e' at a wider location is analogous to putting
a mutex block at an unnecessarily wide location.  (Yes,
there is no real connection between those two, but it comes
down to doing something only where/when it's needed.)

If you confirm that you really want that wider scope here
for `s-e' then I'll do that.  Otherwise, I'll keep the
`s-e' occurrences where they are but do the renaming and
add a doc string.  Let me know.  Thx.

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

[npostavs]

On Tue, Feb 28, 2017 at 10:11 AM, Drew Adams <drew.adams@oracle.com> wrote:
>
> But IMHO, it is generally better to scope a `save-excursion'
> as tightly as possible around the movement that you want to
> control (hide, erase, undo).

Well, my opinion is just about the opposite, but I won't block the patch
for that.

> Why?  Because it makes the code much clearer.  It tells you
> that outside the `save-excursion' zones point is unlikely
> to be moved.  And that makes maintenance easier and less
> error-prone.

I find wider scoping of `save-excursion' to be clearer.  It tells you
that the code outside the save-excursion zone cares about the original
value of point.  If there are multiple `save-excursion' zones, I have
to look at what comes after each of them, to see what they're using
point for.  Therefore maintenance becomes harder and more error-prone
with many smaller save-excursion calls.

With a single save-excursion, it's immediately clear that the value of
point is unchanged by the function.  And it's clear that adding more
code to the function will preserve that property, so maintenance is
easier.

> Putting a `s-e' at a wider location is analogous to putting
> a mutex block at an unnecessarily wide location.  (Yes,
> there is no real connection between those two, but it comes
> down to doing something only where/when it's needed.)

I agree they are roughly analogous, but again come to the opposite
conclusion.  It would be simpler and clearer to make the mutex as wide
as possible without causing deadlock, but for performance reasons, we
choose as narrow a scope as possible.  The performance reasons don't
apply to save-excursion though.  It comes down to doing something
(grabbing/releasing a mutex or saving/restoring point) only where/when
it's needed (which would be once at the beginning and end of the
function in this case) rather than many redundant times (which would
be each time goto-char is called).

> If you confirm that you really want that wider scope here
> for `s-e' then I'll do that.  Otherwise, I'll keep the
> `s-e' occurrences where they are but do the renaming and
> add a doc string.  Let me know.  Thx.

As I've explained, I prefer wider scope.  However, the Emacs project
doesn't have a policy on this as far as I know, so we're in a similar
situation with pcase vs cond thing: the person making the patch makes
the final decision.  If you found my arguments for it unconvincing,
and still feel strongly that narrower scope is better, I'll accept a
patch with the small scopes too.

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

[Drew Adams]

> > But IMHO, it is generally better to scope a `save-excursion'
> > as tightly as possible around the movement that you want to
> > control (hide, erase, undo).
> 
> Well, my opinion is just about the opposite, but I won't block
> the patch for that.
> 
> > Why?  Because it makes the code much clearer.  It tells you
> > that outside the `save-excursion' zones point is unlikely
> > to be moved.  And that makes maintenance easier and less
> > error-prone.
> 
> I find wider scoping of `save-excursion' to be clearer.  It tells you
> that the code outside the save-excursion zone cares about the original
> value of point.

It cannot really tell you that code outside the zone cares about
point.  That's not what `s-e' does.  `s-e' only acts on code
inside it.  It can only tell you that code inside it will not
move point (because of the `s-e').  Which means that any code
outside it CAN care about (rely on) point.

And that applies to the code outside the more narrowly defined
zones too: they CAN care about point.  A maintainer can use the
value of point anywhere outside those narrowly defined zones.

Whether the current code actually cares about point outside
a given `s-e' is less important for maintenence than knowing
where code in general _could_ care about point.

And if do want `s-e' placement to suggest something about
some code outside it actually using point then `s-e', i.e.,
if you want to adopt that as a coding convention (since it
is not something that `s-e' tells you, itself), then such
a convention does a poor job of telling you which outside
code does the using.

The `s-e' would have to be placed so that the _use_ of point
were immediately next to the `s-e' occurrence (_just_ outside
it).  Otherwise, your principle would not be followed - you
would not be masking as much point movement as possible, just
up to where point is actually used.

Such placement is too coarse - far coarser than placement
with the intention (and the actual effect, because it is what
`s-e' does) of indicating where `s-e' NEEDS to protect from
point movement.

But I accord that people can disagree about whether `s-e'
placement should be done only where it is _needed_ or as
widely as possible, just barely beyond where point is used.

> If there are multiple `save-excursion' zones, I have
> to look at what comes after each of them, to see what
> they're using point for.

No, you don't.  What you can be pretty sure of is that
_any_ code there _could_ use point.  IOW, the normal state
of affairs pertains outside the `s-e': you can use point.

The default in Emacs is to not use `save-excursion'
gratuitously.  Code uses it only when/where it matters -
where it's needed.  Limiting its scope to where it really
matters makes it clearer just where/when that is.  There
is no need to wrap wider, so why do it?

> Therefore maintenance becomes harder and more error-prone
> with many smaller save-excursion calls.

That doesn't follow.  Outside those zones, including
anywhere else within the function body, maintenance is free
to use or change point anyway that is called for.

And on the contrary: If you put lots of stuff inside `s-e',
stuff that need not use point, then when you go to make a
change to something there you have to figure out whether -
at that position inside - you could use point if you moved
the `s-e' a bit etc.  Where `s-e' is should tell you, "Here
we're moving point only temporarily.  We undo any such
movement, so that beyond this zone you can continue to use
it normally."

> With a single save-excursion, it's immediately clear that
> the value of point is unchanged by the function.

By the function!  Sure.  But it tells you nothing about
the code within the function.  You seem to be assuming
that all that matters is whether the function moves point.
And that abstracts totally from the possibility of making
future changes to the function code.

What's more: that argument you just made is the same one
I'm making, but for the code where the movement can be
undone.  IOW, you are making the same argument as I, but
you are applying it only to the whole function.  The whole
function _need not_ have point-movement-must-be-undone
imposed on it.  That stops you from making changes to the
code of the function that might need, here or there, to
make use of point.

> And it's clear that adding more code to the function will
> preserve that property, so maintenance is easier.

If you wrap the entire world with a `s-e' then clearly
you can add code to the world and it will still be true
that the world will not move point.

However, you will be unable to treat any of the code in the
function itself as places where you can make _use_ of point.
You will not be able to use point anywhere in the world, if
you've wrapped it all in `s-e'.

> It comes down to doing something
> (grabbing/releasing a mutex or saving/restoring point)
> only where/when it's needed

That's _my_ argument.  I put it only where it is _needed_,
which is where point movement _needs_ to be undone.  You
gratuitously want to envelope also other code, where point
movement does _not_ need to be undone.  Doing that, you tie
your hands for future changes to other parts of the same
function where you might want to use point.

> (which would be once at the beginning and end of the
> function in this case) rather than many redundant times
> (which would be each time goto-char is called).

We seem to have different meanings of "needing" to cancel
(undo) point movement.  It seems clear to me that if only
the parts that actually move point have that movement
canceled the result is correct behavior.  Which means that
there is no need to wrap more code.  Wrapping more code is
gratuitous, not necessary.

And those places where point is moved can be independent.
There is nothing inherently "redundant" about them.

> > If you confirm that you really want that wider scope here
> > for `s-e' then I'll do that.  Otherwise, I'll keep the
> > `s-e' occurrences where they are but do the renaming and
> > add a doc string.  Let me know.  Thx.
> 
> As I've explained, I prefer wider scope.  However, the Emacs project
> doesn't have a policy on this as far as I know, so we're in a similar
> situation with pcase vs cond thing: the person making the patch makes
> the final decision.  If you found my arguments for it unconvincing,
> and still feel strongly that narrower scope is better, I'll accept a
> patch with the small scopes too.

And I feel just as accommodating. ;-)  I wanted to make my
argument, and have done so, but I really don't care what is
done here to fix the bug.

My suggestion, since I think we've both had our say and are
unlikely to convince one another on the level of principle
or guideline, is for you to please make the fix.

Since it is you who will kindly make the actual code change
and commit, you should/can do it however you like.  I have
no problem with that.

But do you really need another patch for that, especially
since wrapping widely, a single time, is so much simpler? ;-)
Could you please make the change?  Your way is fine by me,
for this.  Thx.

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

[Drew Adams]

> My suggestion, since I think we've both had our say and are
> unlikely to convince one another on the level of principle
> or guideline, is for you to please make the fix.
> 
> Since it is you who will kindly make the actual code change
> and commit, you should/can do it however you like.  I have
> no problem with that.
> 
> But do you really need another patch for that, especially
> since wrapping widely, a single time, is so much simpler? ;-)
> Could you please make the change?  Your way is fine by me,
> for this.  Thx.

Here is a possible doc string for the renamed `rectangle-pos-cols':

"Return cons (START-COLUMN . END-COLUMN) of columns delimiting a rectangle.
START and END are buffer positions of the rectangle corners.

Optional argument WINDOW is the window displaying the rectangle.
It defaults to the selected window."

I'm not sure it is accurate, but I think so.  (I'm no expert on this.)

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

[npostavs]

Drew Adams <drew.adams@oracle.com> writes:

>> My suggestion, since I think we've both had our say and are
>> unlikely to convince one another on the level of principle
>> or guideline, is for you to please make the fix.
>> 
>> Since it is you who will kindly make the actual code change
>> and commit, you should/can do it however you like.  I have
>> no problem with that.
>> 
>> But do you really need another patch for that, especially
>> since wrapping widely, a single time, is so much simpler? ;-)
>> Could you please make the change?  Your way is fine by me,
>> for this.  Thx.
>
> Here is a possible doc string for the renamed `rectangle-pos-cols':

Yeah, the doc string is the tricky part.

>
> "Return cons (START-COLUMN . END-COLUMN) of columns delimiting a rectangle. 

I think the first line is too long.

> START and END are buffer positions of the rectangle corners.
>
> Optional argument WINDOW is the window displaying the rectangle.
> It defaults to the selected window."
>
> I'm not sure it is accurate, but I think so.  (I'm no expert on this.)

And looking at the function body again, I think it's checking some other
things, and seems to have some side effects with respect to the current
rectangle.

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

[Drew Adams]

> > Here is a possible doc string for the renamed `rectangle-pos-cols':
> 
> Yeah, the doc string is the tricky part.
> 
> >
> > "Return cons (START-COLUMN . END-COLUMN) of columns delimiting a
> rectangle.
> 
> I think the first line is too long.

"Return cons (START-COLUMN . END-COLUMN) of rectangle columns."

> > START and END are buffer positions of the rectangle corners.
> >
> > Optional argument WINDOW is the window displaying the rectangle.
> > It defaults to the selected window."
> >
> > I'm not sure it is accurate, but I think so.  (I'm no expert on this.)
> 
> And looking at the function body again, I think it's checking some other
> things, and seems to have some side effects with respect to the current
> rectangle.

No, I don't think so.  What did you have in mind?  It can
reset window parameter `rectangle--point-crutches' or variable
`rectangle--mark-crutches', but I don't think those actions are
worth mentioning.  Do you?  Or did you have something else in
mind?  What side effects do you think it has on the current
rectangle?

Feel free to propose something different or let me know what
you think needs to be said.  I'll try to help with the wording,
if needed.

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

[npostavs]

Drew Adams <drew.adams@oracle.com> writes:

>> And looking at the function body again, I think it's checking some other
>> things, and seems to have some side effects with respect to the current
>> rectangle.
>
> No, I don't think so.  What did you have in mind?  It can
> reset window parameter `rectangle--point-crutches' or variable
> `rectangle--mark-crutches', but I don't think those actions are
> worth mentioning.  Do you?

I thought they might be important.  I'm not really sure what the
user-visible effect of those are though.  But perhaps if you're not
interested in them, we should just add a function that does only what
you want?

(defun rectangle-columns (start end)
  "Return cons (START-COLUMN . END-COLUMN) of rectangle columns.
START and END are buffer positions of the rectangle corners."
  (save-excursion
    (let ((startcol (progn (goto-char start) (current-column)))
          (endcol (progn (goto-char end) (current-column))))
      ;; ensure the start column is the left one.
      (when (< endcol startcol)
        (let ((col startcol))
          (setq startcol endcol endcol col)))
      (cons starcol endcol))))

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

[Drew Adams]

> >> And looking at the function body again, I think it's checking some other
> >> things, and seems to have some side effects with respect to the current
> >> rectangle.
> >
> > No, I don't think so.  What did you have in mind?  It can
> > reset window parameter `rectangle--point-crutches' or variable
> > `rectangle--mark-crutches', but I don't think those actions are
> > worth mentioning.  Do you?
> 
> I thought they might be important.  I'm not really sure what the
> user-visible effect of those are though.

AFAICT, those make use of recorded "crutches" (which are
nowhere described explicitly, but which apparently associate
a buffer position with a display column).  And that, for a
given window.

They seem important for proper handling of a rectangle
regarded with respect to display (columns are a display
thing in this context).

Put differently, and comparing the code and doc for Emacs 25
with previous releases, it seems that Emacs 25 started
treating rectangles, in at least some cases, wrt _display_
and not just buffer position: respect of wide chars, window,
places past eol, overlay, redisplay highlighting, even bidi
to some extent, etc.  The Emacs 25 NEWS says:

 Rectangle Mark mode can have corners past EOL or in the
 middle of a TAB.  'C-x C-x' in 'rectangle-mark-mode' now
 cycles through the four corners.  'string-rectangle'
 provides on-the-fly preview of the result.

This seems to be an ongoing initiative (see the code comments,
including for `rectangle--*-char').  We can perhaps expect
even more treatment of a "rectangle" wrt its display.

> But perhaps if you're not interested in them, we should just
> add a function that does only what you want?
> 
> (defun rectangle-columns (start end)...

That handles a rectangle in the older sense, but not, I
guess, wrt the new concerns that Emacs 25 starts to address
(display characteristics and needs - see above).

I am really no expert on this.  Perhaps the author of the
Emacs 25 rectangle code could weigh in on it.  My suggestion
would be to stick to the code as is, since it is the way it
is in order to handle rectangle display better.

Sure, the housekeeping code it includes that deals with
the "crutches" might not be needed to return columns in
most cases.  But it is apparently needed in some cases.
Columns do need to take wide chars etc. into account.

In any case, this code is used in the context of other
rect.el code, where that housekeeping seems to play a role.

I'd opt for keeping the display/housekeeping code, since
the more complex code and the use cases that it handles
subsumes the simpler use case that you propose.  You can
use it wrt rectangle appearance generally, including the
rectangular region in a particular window.

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

Now, you will say that `current-column' also claims to
DTRT wrt display, taking into account "widths of all the
displayed representations of the character...".  I don't
understand this stuff well enough to say why so much more
code apparatus (crutches, a window parameter) is needed
for the Emacs 25 support of rectangles.  Partly out of
concern for my ignorance I'd opt to not try to simplify
this.

Just one opinion, and quite uninformed in this case.
Enlightenment welcome.

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

[Noam Postavsky]

On Fri, Mar 3, 2017 at 11:44 AM, Drew Adams <drew.adams@oracle.com> wrote:
>> >
>> > No, I don't think so.  What did you have in mind?  It can
>> > reset window parameter `rectangle--point-crutches' or variable
>> > `rectangle--mark-crutches', but I don't think those actions are
>> > worth mentioning.  Do you?
>>
>> I thought they might be important.  I'm not really sure what the
>> user-visible effect of those are though.
>
> AFAICT, those make use of recorded "crutches" (which are
> nowhere described explicitly, but which apparently associate
> a buffer position with a display column).  And that, for a
> given window.
>
> They seem important for proper handling of a rectangle
> regarded with respect to display (columns are a display
> thing in this context).
>
> Put differently, and comparing the code and doc for Emacs 25
> with previous releases, it seems that Emacs 25 started
> treating rectangles, in at least some cases, wrt _display_
> and not just buffer position: respect of wide chars, window,
> places past eol,

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.

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

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

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

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

[Drew Adams]

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

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

[Lars Ingebrigtsen]

npostavs@users.sourceforge.net writes:

> I think it's reasonable to make rectangle--pos-cols preserve point.
> Could you update your patch to
>
> 1. Use a single save-excursion around the whole body, instead of adding
> several.

I've now done this change, which seem obviously correct.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no