unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Re: [Emacs-diffs] fix/undo-point-in-wrong-place 6b3cfe4 4/4: Prepare for record now separate function.
       [not found] ` <E1Zztvo-0005ey-0X@vcs.savannah.gnu.org>
@ 2015-11-22  5:21   ` Stefan Monnier
  2015-11-23 17:41     ` Phillip Lord
  0 siblings, 1 reply; 6+ messages in thread
From: Stefan Monnier @ 2015-11-22  5:21 UTC (permalink / raw)
  To: emacs-devel; +Cc: Phillip Lord

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



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Emacs-diffs] fix/undo-point-in-wrong-place 6b3cfe4 4/4: Prepare for record now separate function.
  2015-11-22  5:21   ` [Emacs-diffs] fix/undo-point-in-wrong-place 6b3cfe4 4/4: Prepare for record now separate function Stefan Monnier
@ 2015-11-23 17:41     ` Phillip Lord
  2015-11-25 22:46       ` Phillip Lord
  0 siblings, 1 reply; 6+ messages in thread
From: Phillip Lord @ 2015-11-23 17:41 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Emacs-diffs] fix/undo-point-in-wrong-place 6b3cfe4 4/4: Prepare for record now separate function.
  2015-11-23 17:41     ` Phillip Lord
@ 2015-11-25 22:46       ` Phillip Lord
  2015-11-26  4:00         ` Stefan Monnier
  0 siblings, 1 reply; 6+ messages in thread
From: Phillip Lord @ 2015-11-25 22:46 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Emacs-diffs] fix/undo-point-in-wrong-place 6b3cfe4 4/4: Prepare for record now separate function.
  2015-11-25 22:46       ` Phillip Lord
@ 2015-11-26  4:00         ` Stefan Monnier
  2015-11-26 10:32           ` Phillip Lord
  0 siblings, 1 reply; 6+ messages in thread
From: Stefan Monnier @ 2015-11-26  4:00 UTC (permalink / raw)
  To: Phillip Lord; +Cc: emacs-devel

> 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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Emacs-diffs] fix/undo-point-in-wrong-place 6b3cfe4 4/4: Prepare for record now separate function.
  2015-11-26  4:00         ` Stefan Monnier
@ 2015-11-26 10:32           ` Phillip Lord
  2015-11-26 15:56             ` Eli Zaretskii
  0 siblings, 1 reply; 6+ messages in thread
From: Phillip Lord @ 2015-11-26 10:32 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Emacs-diffs] fix/undo-point-in-wrong-place 6b3cfe4 4/4: Prepare for record now separate function.
  2015-11-26 10:32           ` Phillip Lord
@ 2015-11-26 15:56             ` Eli Zaretskii
  0 siblings, 0 replies; 6+ messages in thread
From: Eli Zaretskii @ 2015-11-26 15:56 UTC (permalink / raw)
  To: Phillip Lord; +Cc: monnier, emacs-devel

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2015-11-26 15:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20151120221320.21643.45342@vcs.savannah.gnu.org>
     [not found] ` <E1Zztvo-0005ey-0X@vcs.savannah.gnu.org>
2015-11-22  5:21   ` [Emacs-diffs] fix/undo-point-in-wrong-place 6b3cfe4 4/4: Prepare for record now separate function Stefan Monnier
2015-11-23 17:41     ` Phillip Lord
2015-11-25 22:46       ` Phillip Lord
2015-11-26  4:00         ` Stefan Monnier
2015-11-26 10:32           ` Phillip Lord
2015-11-26 15:56             ` Eli Zaretskii

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).