Re: [Emacs-diffs] fix/undo-point-in-wrong-place 6b3cfe4 4/4: Prepare for record now separate function.

Re: [Emacs-diffs] fix/undo-point-in-wrong-place 6b3cfe4 4/4: Prepare for record now separate function.

[Stefan Monnier]

>     PT is the position of point that will naturally occur as a result of the
>     undo record that will be added just after this command terminates.  */

This comment is invalidated by your change.

> +      record_point (beg + SCHARS (string));

Hmm I thought the sign on sbeg took care of this case already (i.e. the
record_point should only record something when point was neither at the
beginning nor at the end of the deleted string).

As for the source of the bug (i.e. what change caused the new behavior):
in the old code, undo-boundary was called right before every command
(whether there was a need to push a boundary or not), so contrary to the
comment in the code, last_boundary_position was actually recording
"position of point at beginning of the command" rather than "position of
point last time we inserted a boundary".

So the hunk below should recover the old behavior (well, more or less:
it wouldn't compile as is, but I hope you get the idea).  But to fix it
right, we should rename these vars and adjust their comment to better
reflect the way they're really used.


        Stefan


diff --git a/src/keyboard.c b/src/keyboard.c
index 849066c..125091e 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -1448,6 +1448,8 @@ command_loop_1 (void)
             /* Ensure that we have added appropriate undo-boundaries as a
                result of changes from the last command. */
             call0 (Qundo_auto__add_boundary);
+            last_boundary_position = PT;
+            last_boundary_buffer = current_buffer;
 
             call1 (Qcommand_execute, Vthis_command);
 

Re: [Emacs-diffs] fix/undo-point-in-wrong-place 6b3cfe4 4/4: Prepare for record now separate function.

[Phillip Lord]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>>     PT is the position of point that will naturally occur as a result of the
>>     undo record that will be added just after this command terminates.  */
>
> This comment is invalidated by your change.
>
>> +      record_point (beg + SCHARS (string));
>
> Hmm I thought the sign on sbeg took care of this case already (i.e. the
> record_point should only record something when point was neither at the
> beginning nor at the end of the deleted string).


Yep, you are right.

Although, the strange thing is, this should be enough to solve the
problem with a "kill-region" command (which needs record_point to work).
Except that this doesn't work because, AFAICT, the value of point gets
changed *before* we get to the record_delete. delete-backward-char works
without using record_point, as does kill-word.

I think it is buffer-substring--filter that is to blame which resets
point to the start of the region immediately before deleting it.



> As for the source of the bug (i.e. what change caused the new
> behavior): in the old code, undo-boundary was called right before
> every command (whether there was a need to push a boundary or not), so
> contrary to the comment in the code, last_boundary_position was
> actually recording "position of point at beginning of the command"
> rather than "position of point last time we inserted a boundary".

Ah, yes.


>
> So the hunk below should recover the old behavior (well, more or less:
> it wouldn't compile as is, but I hope you get the idea).  But to fix it
> right, we should rename these vars and adjust their comment to better
> reflect the way they're really used.
>

I've pushed an alternative solution (yes, I know I said I would just do
what you told me, but I could not resist). There is already
last_point_position and prev_buffer variables which do this, as far as I
can tell.

Phil

Re: [Emacs-diffs] fix/undo-point-in-wrong-place 6b3cfe4 4/4: Prepare for record now separate function.

[Phillip Lord]

Phillip Lord <phillip.lord@russet.org.uk> writes:

> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
>
>> So the hunk below should recover the old behavior (well, more or less:
>> it wouldn't compile as is, but I hope you get the idea).  But to fix it
>> right, we should rename these vars and adjust their comment to better
>> reflect the way they're really used.
>>
>
> I've pushed an alternative solution (yes, I know I said I would just do
> what you told me, but I could not resist). There is already
> last_point_position and prev_buffer variables which do this, as far as I
> can tell.

Does this seem a reasonable solution (I've used the idea behind your
patch, but with variables that already exist in keyboard.c)?

Phil

Re: [Emacs-diffs] fix/undo-point-in-wrong-place 6b3cfe4 4/4: Prepare for record now separate function.

[Stefan Monnier]

> Does this seem a reasonable solution (I've used the idea behind your
> patch, but with variables that already exist in keyboard.c)?

As pointed out in the reply to the actual commit-diff, the new code has
another problem: it always uses the "point at beginning of command",
which is usually right but not when an undo-boundary was pushed between
"beginning of command" and the moment when we try to record point.


        Stefan

Re: [Emacs-diffs] fix/undo-point-in-wrong-place 6b3cfe4 4/4: Prepare for record now separate function.

[Phillip Lord]

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

>> Does this seem a reasonable solution (I've used the idea behind your
>> patch, but with variables that already exist in keyboard.c)?
>
> As pointed out in the reply to the actual commit-diff,

Sorry, I think I missed that one.

> the new code has another problem: it always uses the "point at
> beginning of command", which is usually right but not when an
> undo-boundary was pushed between "beginning of command" and the moment
> when we try to record point.

I really should stop thinking for myself and just do what I am told.

I've pushed a new version.

Phil

Re: [Emacs-diffs] fix/undo-point-in-wrong-place 6b3cfe4 4/4: Prepare for record now separate function.

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Date: Thu, 26 Nov 2015 10:32:53 +0000
> Cc: emacs-devel@gnu.org
> 
> I really should stop thinking for myself

No, please don't.