Using `save-window-excursion' instead of `save-excursion' for `comment-region'?

Using `save-window-excursion' instead of `save-excursion' for `comment-region'?

[Bastien]

In Org buffer, you can comment code within source blocks.  This opens
a new buffer, insert the code there, comment it, and insert the buffer
contents back into Org's buffer.

With the current `comment-region' function, point is lost when Org
goes back to the org buffer.  Using `save-window-excursion' instead
of `save-excursion' fixes the problem.

Are you aware of problems that this change may trigger?

I cannot think of any, but I'm always a bit cautious when using
`save-window-excursion'.

Thanks for your feedback,

-- 
 Bastien

Re: Using `save-window-excursion' instead of `save-excursion' for `comment-region'?

[martin rudalics]

 > In Org buffer, you can comment code within source blocks.  This opens
 > a new buffer, insert the code there, comment it, and insert the buffer
 > contents back into Org's buffer.
 >
 > With the current `comment-region' function, point is lost when Org
 > goes back to the org buffer.  Using `save-window-excursion' instead
 > of `save-excursion' fixes the problem.
 >
 > Are you aware of problems that this change may trigger?
 >
 > I cannot think of any, but I'm always a bit cautious when using
 > `save-window-excursion'.

Please be.  `save-window-excursion' should be used iff you really change
the window configuration as, for example, by resizing, deleting or
creating windows, or showing a different buffer in some window.  WOW
everything covered by `window-configuration-change-hook'.  In the case
you describe above you do not show the buffer used for inserting the
code, commenting it, ... in a window IIUC so that requirement is not
satisfied.

Can you explain why `save-excursion' fails?

martin

Re: Using `save-window-excursion' instead of `save-excursion' for `comment-region'?

[Andreas Röhler]

Am 05.12.2013 11:18, schrieb Bastien:
> In Org buffer, you can comment code within source blocks.  This opens
> a new buffer, insert the code there, comment it, and insert the buffer
> contents back into Org's buffer.
>
> With the current `comment-region' function, point is lost when Org
> goes back to the org buffer.  Using `save-window-excursion' instead
> of `save-excursion' fixes the problem.
>
> Are you aware of problems that this change may trigger?
>
> I cannot think of any, but I'm always a bit cautious when using
> `save-window-excursion'.
>
> Thanks for your feedback,
>

Hi Bastien,

last time I've seen in, indirect buffers were used, which would be worth mentioning.

Andreas

Re: Using `save-window-excursion' instead of `save-excursion' for `comment-region'?

[Stefan Monnier]

> In Org buffer, you can comment code within source blocks.  This opens
> a new buffer, insert the code there, comment it, and insert the buffer
> contents back into Org's buffer.

> With the current `comment-region' function, point is lost when Org
> goes back to the org buffer.  Using `save-window-excursion' instead
> of `save-excursion' fixes the problem.

I'm not sure exactly what means "point is lost" in this case, but if
using save-window-excursion solves the problem, it's only by accident.


        Stefan

Re: Using `save-window-excursion' instead of `save-excursion' for `comment-region'?

[Bastien]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> In Org buffer, you can comment code within source blocks.  This opens
>> a new buffer, insert the code there, comment it, and insert the buffer
>> contents back into Org's buffer.
>
>> With the current `comment-region' function, point is lost when Org
>> goes back to the org buffer.  Using `save-window-excursion' instead
>> of `save-excursion' fixes the problem.
>
> I'm not sure exactly what means "point is lost" in this case, but if
> using save-window-excursion solves the problem, it's only by
> accident.

This is how it works:

comment-region calls comment-region-function within save-excursion
(assuming there is no window change.)

comment-region-function calls org-babel-do-in-edit-buffer which
inserts the source code in another buffer, then calls back again
comment-region with comment-region-function bound to the correct
mode-dependent function.

The "outward" comment-region does not restore the point position
correctly.

So I'm not sure why save-window-excursion would only works "by
accident" here.  I cannot think of a better fix right now, I'll
continue to travel through the Babel maze.

-- 
 Bastien

Re: Using `save-window-excursion' instead of `save-excursion' for `comment-region'?

[Aaron Ecay]

2013ko abenudak 5an, Bastien-ek idatzi zuen:
> 
> This is how it works:
> 
> comment-region calls comment-region-function within save-excursion
> (assuming there is no window change.)
> 
> comment-region-function calls org-babel-do-in-edit-buffer which
> inserts the source code in another buffer, then calls back again
> comment-region with comment-region-function bound to the correct
> mode-dependent function.
> 
> The "outward" comment-region does not restore the point position
> correctly.
> 
> So I'm not sure why save-window-excursion would only works "by
> accident" here.  I cannot think of a better fix right now, I'll
> continue to travel through the Babel maze.

I think the problem is that org-babel-do-in-edit-buffer deletes the
whole contents of the source code block, and re-inserts it.  This means
that the marker that save-excursion uses (pointing to somewhere in the
deleted and re-inserted span) no longer points to the desired position,
but rather to just before the span.

Org babel has its own point-restoration functionality (which works by
counting lines and columns), but this is executed inside of the scope of
comment-region’s save-excursion, so exiting the latter restores the
point to a bogus position (from the POV of the user).

-- 
Aaron Ecay

Re: Using `save-window-excursion' instead of `save-excursion' for `comment-region'?

[Stefan Monnier]

> I think the problem is that org-babel-do-in-edit-buffer deletes the
> whole contents of the source code block, and re-inserts it.  This means
> that the marker that save-excursion uses (pointing to somewhere in the
> deleted and re-inserted span) no longer points to the desired position,
> but rather to just before the span.

> Org babel has its own point-restoration functionality (which works by
> counting lines and columns), but this is executed inside of the scope of
> comment-region’s save-excursion, so exiting the latter restores the
> point to a bogus position (from the POV of the user).

I think your analysis is exactly right, and the reason why
save-window-excursion doesn't exhibit the problem is because
save-window-excursion does not save-excursion in the current buffer.
I.e. paradoxically it's because it "preserves less" rather than because
it "preserves more".  Replacing the save-excursion with
save-current-buffer would probably work as well.


        Stefan

Re: Using `save-window-excursion' instead of `save-excursion' for `comment-region'?

[Bastien]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I think your analysis is exactly right, and the reason why
> save-window-excursion doesn't exhibit the problem is because
> save-window-excursion does not save-excursion in the current buffer.
> I.e. paradoxically it's because it "preserves less" rather than because
> it "preserves more".  Replacing the save-excursion with
> save-current-buffer would probably work as well.

I see, thanks.

I've fixed the problem in Org by creating `org-comment-dwim',
which use comment-dwim when outside a source block, and
`org-babel-do-in-edit-buffer' (directly, not wrapped into
comment-region) when the point is within a buffer.

Aaron, thanks for your input and for testing this heavily.

-- 
 Bastien

Re: Using `save-window-excursion' instead of `save-excursion' for `comment-region'?

[Bastien]

Bastien <bzg@gnu.org> writes:

> I've fixed the problem in Org by creating `org-comment-dwim',
> which use comment-dwim when outside a source block, and
> `org-babel-do-in-edit-buffer' (directly, not wrapped into
> comment-region) when the point is within a buffer.
                                           ^^^^^^^^

"a source block"

of course,

-- 
 Bastien