bug#10899: 24.0.93; c-forward-conditional should not move the mark

bug#10899: 24.0.93; c-forward-conditional should not move the mark

[Dani Moncayo]

[-- Attachment #1: Type: text/plain, Size: 487 bytes --]

Recipe from "emacs -Q":
1. Visit the attached file.
2. Type: C-SPC C-c C-n C-c C-n

--> Expected result: the region covers the two "#ifdef" preprocessor
conditionals.
--> Observed result: it covers only the second one, because the mark
has moved when I typed the second "C-c C-n".


In GNU Emacs 24.0.93.1 (i386-mingw-nt6.1.7601)
 of 2012-02-14 on DANI-PC
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --with-gcc (4.6)'

-- 
Dani Moncayo

[-- Attachment #2: test.c --]
[-- Type: text/x-csrc, Size: 62 bytes --]


#ifdef AA
AA_CODE;
#endif

#ifdef BB
BB_CODE;
#endif

bug#10899: 24.0.93; c-forward-conditional should not move the mark

[Dani Moncayo]

> Recipe from "emacs -Q":
> 1. Visit the attached file.
> 2. Type: C-SPC C-c C-n C-c C-n
>
> --> Expected result: the region covers the two "#ifdef" preprocessor
> conditionals.
> --> Observed result: it covers only the second one, because the mark
> has moved when I typed the second "C-c C-n".

I didn't read the docstring of the function, which says:

  Move forward across a preprocessor conditional, leaving mark behind.
  A prefix argument acts as a repeat count.  With a negative argument,
  move backward across a preprocessor conditional.

What does "leaving mark behind" mean exactly here?  It seem to mean
"setting the mark at point, and then moving the point".  At least it
is the behavior I observe.

But this behavior is undesirable (IMO - this is a movement command.
whats the point of setting the mark here?), and inconsistent with
analogous movement commands such as `forward-list'.


-- 
Dani Moncayo

bug#10899: 24.0.93; c-forward-conditional should not move the mark

[Eli Zaretskii]

> Date: Mon, 27 Feb 2012 20:29:55 +0100
> From: Dani Moncayo <dmoncayo@gmail.com>
> 
> What does "leaving mark behind" mean exactly here?  It seem to mean
> "setting the mark at point, and then moving the point".  At least it
> is the behavior I observe.

What else could it possibly mean?

> But this behavior is undesirable (IMO - this is a movement command.
> whats the point of setting the mark here?)

A conditional could, and frequently does, enclose a large portion of
the code.  Leaving the mark where you were lets you get back there
with "C-x C-x", obviously, which is a convenience.  In general, Emacs
commands that could potentially move a long way leave the mark behind.

> and inconsistent with analogous movement commands such as
> `forward-list'.

There's no need for consistency in all movement commands.  In
particular, those that generally move short distances need not behave
the same as the other kind.

And you have "C-u C-c C-n" to do what you want.

bug#10899: 24.0.93; c-forward-conditional should not move the mark

[Dani Moncayo]

>> What does "leaving mark behind" mean exactly here?  It seem to mean
>> "setting the mark at point, and then moving the point".  At least it
>> is the behavior I observe.
>
> What else could it possibly mean?

I don't know, but "to leave X behind" does not imply to move "X" from
its original position.  So this wording seems confusing to me.

>> But this behavior is undesirable (IMO - this is a movement command.
>> whats the point of setting the mark here?)
>
> A conditional could, and frequently does, enclose a large portion of
> the code.  Leaving the mark where you were lets you get back there
> with "C-x C-x", obviously, which is a convenience.  In general, Emacs
> commands that could potentially move a long way leave the mark behind.
>
>> and inconsistent with analogous movement commands such as
>> `forward-list'.
>
> There's no need for consistency in all movement commands.  In
> particular, those that generally move short distances need not behave
> the same as the other kind.
>

Consistency is a good design principle to follow, unless there is good
reason not to.  And frankly, I don't see a good reason here.

> And you have "C-u C-c C-n" to do what you want.

And if "C-c C-n" didn't set the mark, you would have "C-SPC C-c C-n"
to do what you want.

-- 
Dani Moncayo

bug#10899: 24.0.93; c-forward-conditional should not move the mark

[Dani Moncayo]

> Consistency is a good design principle to follow, unless there is good
> reason not to.  And frankly, I don't see a good reason here.

...and I'll explain why:

Well-known movement commands like `M-<', `M->' or the search commands
set the mark (IMO) because they meet two criteria:
1. They may move point to a distant location.
2. There is not an "inverse" command to move point back to the
original position (or near it):  commands like `forward-list' or
`forward-page' meet criterion #1, but they don't set the mark because
you have `backward-list' and `backward-page' to move to the original
position.

And when command meet these two criteria, it should set the mark, but
only if it is inactive, because when the mark is active, the user
surely wants to extend the _current_ region, i.e., "leave the mark
alone where I've set it, and just move the point".

IMO, this is the behavior users expect, and are used to.

Summarizing:
a. `c-forward-conditional' and `c-backward-conditional' should not set
the mark, because each one has an inverse movement command.
b. Even if you disagree, those commands should not set the mark when
it is active.

-- 
Dani Moncayo

bug#10899: 24.0.93; c-forward-conditional should not move the mark

[Juri Linkov]

> Summarizing:
> a. `c-forward-conditional' and `c-backward-conditional' should not set
> the mark, because each one has an inverse movement command.
> b. Even if you disagree, those commands should not set the mark when
> it is active.

All similar movement commands like `c-beginning-of-defun' and `c-end-of-defun'
take precautions against the behavior you found.  They do this by using
the following condition before leaving mark behind:

  (and transient-mark-mode mark-active)

The patch below fixes the remaining movement commands to do the same,
except `c-mark-function' that needs to be rewritten to follow the logic
of `mark-defun' for setting the mark.

=== modified file 'lisp/progmodes/cc-cmds.el'
--- lisp/progmodes/cc-cmds.el	2012-01-25 17:54:01 +0000
+++ lisp/progmodes/cc-cmds.el	2012-02-28 10:27:49 +0000
@@ -2910,7 +2910,8 @@ (defun c-up-conditional (count)
 forward."
   (interactive "p")
   (let ((new-point (c-scan-conditionals (- count) -1)))
-    (push-mark)
+    (or (and transient-mark-mode mark-active)
+	(push-mark))
     (goto-char new-point))
   (c-keep-region-active))
 
@@ -2920,7 +2921,8 @@ (defun c-up-conditional-with-else (count
 directives."
   (interactive "p")
   (let ((new-point (c-scan-conditionals (- count) -1 t)))
-    (push-mark)
+    (or (and transient-mark-mode mark-active)
+	(push-mark))
     (goto-char new-point))
   (c-keep-region-active))
 
@@ -2934,7 +2936,8 @@ (defun c-down-conditional (count)
 backward."
   (interactive "p")
   (let ((new-point (c-scan-conditionals count 1)))
-    (push-mark)
+    (or (and transient-mark-mode mark-active)
+	(push-mark))
     (goto-char new-point))
   (c-keep-region-active))
 
@@ -2944,7 +2947,8 @@ (defun c-down-conditional-with-else (cou
 directives."
   (interactive "p")
   (let ((new-point (c-scan-conditionals count 1 t)))
-    (push-mark)
+    (or (and transient-mark-mode mark-active)
+	(push-mark))
     (goto-char new-point))
   (c-keep-region-active))
 
@@ -2959,7 +2963,8 @@ (defun c-backward-conditional (count &op
 to call `c-scan-conditionals' directly instead."
   (interactive "p")
   (let ((new-point (c-scan-conditionals (- count) target-depth with-else)))
-    (push-mark)
+    (or (and transient-mark-mode mark-active)
+	(push-mark))
     (goto-char new-point))
   (c-keep-region-active))
 
@@ -2980,7 +2985,8 @@ (defun c-forward-conditional (count &opt
 to call `c-scan-conditionals' directly instead."
   (interactive "p")
   (let ((new-point (c-scan-conditionals count target-depth with-else)))
-    (push-mark)
+    (or (and transient-mark-mode mark-active)
+	(push-mark))
     (goto-char new-point)))
 
 (defun c-scan-conditionals (count &optional target-depth with-else)

bug#10899: 24.0.93; c-forward-conditional should not move the mark

[Dani Moncayo]

>> Summarizing:
>> a. `c-forward-conditional' and `c-backward-conditional' should not set
>> the mark, because each one has an inverse movement command.
>> b. Even if you disagree, those commands should not set the mark when
>> it is active.
>
> All similar movement commands like `c-beginning-of-defun' and `c-end-of-defun'
> take precautions against the behavior you found.  They do this by using
> the following condition before leaving mark behind:
>
>  (and transient-mark-mode mark-active)
>
> The patch below fixes the remaining movement commands to do the same,
> except `c-mark-function' that needs to be rewritten to follow the logic
> of `mark-defun' for setting the mark.

Thanks Juri, for working on this and many other fixes/improvements to Emacs.

Regarding your patch, I think it clearly improve the current behavior,
but I want to emphasize again that, IMO, setting the mark in too many
commands is bad, because it overfills the mark ring, thus making
harder to return to earlier positions.


-- 
Dani Moncayo

bug#10899: 24.0.93; c-forward-conditional should not move the mark

[Juri Linkov]

> Regarding your patch, I think it clearly improve the current behavior,
> but I want to emphasize again that, IMO, setting the mark in too many
> commands is bad, because it overfills the mark ring, thus making
> harder to return to earlier positions.

What I wanted to achieve is to make C movement commands to behave exactly
as their Lisp counterparts.  I see the following correspondence:

                                              leaves mark behind?
c-beginning-of-defun     beginning-of-defun   yes
c-end-of-defun           end-of-defun         yes
c-mark-function          mark-defun           yes
c-up-conditional         backward-up-list     no
c-down-conditional       down-list            no
c-backward-conditional   backward-list        no
c-forward-conditional    forward-list         no

So I agree that `c-forward-conditional' and `c-backward-conditional'
should not push the mark.

Regarding bug#10906, I think `c-mark-function' should be rewritten
to follow the logic of `mark-defun'.

bug#10899: 24.0.93; c-forward-conditional should not move the mark

[Stefan Monnier]

> Summarizing:
> a. `c-forward-conditional' and `c-backward-conditional' should not set
> the mark, because each one has an inverse movement command.
> b. Even if you disagree, those commands should not set the mark when
> it is active.

FWIW I completely agree.


        Stefan

bug#10899: 24.0.93; c-forward-conditional should not move the mark

[Lars Ingebrigtsen]

Alan, I don't know if you saw this bug report?  The gist of it is that
is seems inconsistent for `c-forward-conditional' and
`c-backward-conditional' to do a `push-mark'.

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

>> a. `c-forward-conditional' and `c-backward-conditional' should not set
>> the mark, because each one has an inverse movement command.
>> b. Even if you disagree, those commands should not set the mark when
>> it is active.
>
> FWIW I completely agree.

Juri Linkov <juri@jurta.org> writes:

>> Regarding your patch, I think it clearly improve the current behavior,
>> but I want to emphasize again that, IMO, setting the mark in too many
>> commands is bad, because it overfills the mark ring, thus making
>> harder to return to earlier positions.
>
> What I wanted to achieve is to make C movement commands to behave exactly
> as their Lisp counterparts.  I see the following correspondence:
>
>                                               leaves mark behind?
> c-beginning-of-defun     beginning-of-defun   yes
> c-end-of-defun           end-of-defun         yes
> c-mark-function          mark-defun           yes
> c-up-conditional         backward-up-list     no
> c-down-conditional       down-list            no
> c-backward-conditional   backward-list        no
> c-forward-conditional    forward-list         no
>
> So I agree that `c-forward-conditional' and `c-backward-conditional'
> should not push the mark.
>
> Regarding bug#10906, I think `c-mark-function' should be rewritten
> to follow the logic of `mark-defun'.

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