[PATCH] Fix moving cursor in org-set-tags-command

[PATCH] Fix moving cursor in org-set-tags-command

[Matt Lundin]

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

Commit 44ec473c199262d89b372d8a6cd35bed7672164d from Feb. 23 causes
org-set-tags-command to move the cursor forward 1 char when situated on
headline asterisks.

So if I am on the following level 1 headline with the cursor on the
asterisk as below...

* Headline
^

...and I call org-set-tags command, it moves the cursor forward one space:

* Headline                                      :tag:
 ^

This is causes problems with org-speed-keys, which requires that the
cursor remain on the headline.

This commit modified a previous change on Feb. 21
(450452de4b790706d187291f9f71a286f8f62004). But that commit also had
problems, since it would move the cursor one asterisk forward on
headlines > 1, thus also interfering with org-speed-keys. In my view
org-set-tags-command should not move the cursor except to fix the very
specific thing that commit 450452de4b was meant to fix: namely the
cursor moving when on a blank headline: i.e., from here...

*** 
    ^        

...to here...

***                                             :tag:
   ^

I've attached a patch that corrects the problem, but it would be ideal
if we figured out why the cursor is moving in the first place.

Best,

Matt


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-bug-that-placed-cursor-incorrectly-when-setting-.patch --]
[-- Type: text/x-patch, Size: 1012 bytes --]

From ae5cf0e1110241426e49f573219e9740c25bf8ea Mon Sep 17 00:00:00 2001
From: Matt Lundin <mdl@imapmail.org>
Date: Thu, 7 May 2020 19:06:08 -0500
Subject: [PATCH 1/1] Fix bug that placed cursor incorrectly when setting tags

* lisp/org.el: (org-set-tags-command) Only fix cursor position in very
specific circumstances (i.e., when cursor is on an empty headline).
---
 lisp/org.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index dd017e662..0e4fd7be1 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -11846,8 +11846,9 @@ in Lisp code use `org-set-tags' instead."
 	  (org-set-tags tags)))))
     ;; `save-excursion' may not replace the point at the right
     ;; position.
-    (when (save-excursion (skip-chars-backward "*") (bolp))
-      (forward-char))))
+    (and (looking-at " ")
+	 (string-match "\\*+" (buffer-substring (point-at-bol) (point)))
+	 (forward-char))))
 
 (defun org-align-tags (&optional all)
   "Align tags in current entry.
-- 
2.26.2

Re: [PATCH] Fix moving cursor in org-set-tags-command

[Kyle Meyer]

Matt Lundin writes:

> Commit 44ec473c199262d89b372d8a6cd35bed7672164d from Feb. 23 causes
> org-set-tags-command to move the cursor forward 1 char when situated on
> headline asterisks.
[...]
> This commit modified a previous change on Feb. 21
> (450452de4b790706d187291f9f71a286f8f62004). But that commit also had
> problems, since it would move the cursor one asterisk forward on
> headlines > 1, thus also interfering with org-speed-keys. In my view
> org-set-tags-command should not move the cursor except to fix the very
> specific thing that commit 450452de4b was meant to fix: namely the
> cursor moving when on a blank headline: i.e., from here...

Thanks for the nice description of the problem.  I wouldn't mind if at
least a condensed version made its way into the commit message :)

> I've attached a patch that corrects the problem, but it would be ideal
> if we figured out why the cursor is moving in the first place.

I looked quickly at org-set-tags (and the functions it calls).  Based on
commenting bits out, I think there are at least a couple of parts that
modify the buffer in a way that prevent save-excursion from restoring
the intended location.  While not ideal, this after-the-fact adjustment
is probably the simplest way to deal with the issue.

> Subject: [PATCH 1/1] Fix bug that placed cursor incorrectly when setting tags
>
> * lisp/org.el: (org-set-tags-command) Only fix cursor position in very

nitpick: The colon should follow (org-set-tags-command).

> diff --git a/lisp/org.el b/lisp/org.el
> index dd017e662..0e4fd7be1 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -11846,8 +11846,9 @@ in Lisp code use `org-set-tags' instead."
>  	  (org-set-tags tags)))))
>      ;; `save-excursion' may not replace the point at the right
>      ;; position.
> -    (when (save-excursion (skip-chars-backward "*") (bolp))
> -      (forward-char))))
> +    (and (looking-at " ")
> +	 (string-match "\\*+" (buffer-substring (point-at-bol) (point)))
> +	 (forward-char))))

Looks fine to me, with the minor nit that I think looking-at-p and
string-match-p would be preferable here.

Would you mind adding a regression test?

Re: [PATCH] Fix moving cursor in org-set-tags-command

[Nicolas Goaziou]

Hello,

Matt Lundin <mdl@imapmail.org> writes:

> -    (when (save-excursion (skip-chars-backward "*") (bolp))
> -      (forward-char))))
> +    (and (looking-at " ")
> +	 (string-match "\\*+" (buffer-substring (point-at-bol) (point)))
> +	 (forward-char))))

Please replace `and' with `when' if side-effects are involved.

Also, note that

  (save-excursion (skip-chars-backward "*") (bolp))

is faster and more accurate than

  (string-match "\\*+" (buffer-substring (point-at-bol) (point)))

because the latter matches, e.g.,

  ab*|c

where "|" is point.

Besides, I don't understand how this is related to empty headlines
since, AFAICT, this part of code is supposed to fix the issue on empty
headlines.

Regards,

-- 
Nicolas Goaziou

Re: [PATCH] Fix moving cursor in org-set-tags-command

[Kyle Meyer]

Nicolas Goaziou writes:

> Matt Lundin <mdl@imapmail.org> writes:
>
>> -    (when (save-excursion (skip-chars-backward "*") (bolp))
>> -      (forward-char))))
>> +    (and (looking-at " ")
>> +	 (string-match "\\*+" (buffer-substring (point-at-bol) (point)))
>> +	 (forward-char))))
>
> Please replace `and' with `when' if side-effects are involved.
>
> Also, note that
>
>   (save-excursion (skip-chars-backward "*") (bolp))
>
> is faster and more accurate than
>
>   (string-match "\\*+" (buffer-substring (point-at-bol) (point)))
>
> because the latter matches, e.g.,
>
>   ab*|c
>
> where "|" is point.

Ah, I didn't spot the inaccuracy that you point out, thinking about it
as though the regexp was anchored.  I think anchoring on both ends would
resolve the inaccuracy, though I agree with your preference for the
skip-chars-backward variant.

> Besides, I don't understand how this is related to empty headlines
> since, AFAICT, this part of code is supposed to fix the issue on empty
> headlines.

I've tried to capture the issues in the tests below.  The first added
check would fail before 450452de4 (and its replacement, 44ec473c1).  The
second check would fail with 44ec473c1, the third with both 450452de4
and 44ec473c1.  Matt's patch would get past the first three checks, but
fail with the last one, due to the issue you note.  All these checks
pass if the string-match call is anchored or replaced by the
skip-chars-backward form you suggest.

diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 7d420b599..29ac0a8f9 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -6184,7 +6184,47 @@ (ert-deftest test-org/set-tags-command ()
    (equal "* H1 :foo:\n* H2 :bar:"
 	  (org-test-with-temp-text "* H1    :foo:\n* H2    :bar:"
 	    (let ((org-tags-column 1)) (org-set-tags-command '(4)))
-	    (buffer-string)))))
+	    (buffer-string))))
+  ;; Point does not move with empty headline.
+  (should
+   (equal ":foo:"
+	  (org-test-with-temp-text "* <point>"
+	    (cl-letf (((symbol-function 'completing-read)
+		       (lambda (&rest args) ":foo:")))
+	      (let ((org-use-fast-tag-selection nil)
+		    (org-tags-column 1))
+		(org-set-tags-command)))
+	    (buffer-substring (point) (line-end-position)))))
+  ;; Point does not move at start of line.
+  (should
+   (equal "* H1 :foo:"
+	  (org-test-with-temp-text "* H1"
+	    (cl-letf (((symbol-function 'completing-read)
+		       (lambda (&rest args) ":foo:")))
+	      (let ((org-use-fast-tag-selection nil)
+		    (org-tags-column 1))
+		(org-set-tags-command)))
+	    (buffer-substring (point) (line-end-position)))))
+  ;; Point does not move when within *'s.
+  (should
+   (equal "* H1 :foo:"
+	  (org-test-with-temp-text "*<point>* H1"
+	    (cl-letf (((symbol-function 'completing-read)
+		       (lambda (&rest args) ":foo:")))
+	      (let ((org-use-fast-tag-selection nil)
+		    (org-tags-column 1))
+		(org-set-tags-command)))
+	    (buffer-substring (point) (line-end-position)))))
+  ;; Point workaround does not get fooled when looking at a space.
+  (should
+   (equal " b :foo:"
+	  (org-test-with-temp-text "* a<point> b"
+	    (cl-letf (((symbol-function 'completing-read)
+		       (lambda (&rest args) ":foo:")))
+	      (let ((org-use-fast-tag-selection nil)
+		    (org-tags-column 1))
+		(org-set-tags-command)))
+	    (buffer-substring (point) (line-end-position))))))
 
 (ert-deftest test-org/toggle-tag ()
   "Test `org-toggle-tag' specifications."

Re: [PATCH] Fix moving cursor in org-set-tags-command

[Nicolas Goaziou]

Hello,

Kyle Meyer <kyle@kyleam.com> writes:

> I've tried to capture the issues in the tests below.  The first added
> check would fail before 450452de4 (and its replacement, 44ec473c1).  The
> second check would fail with 44ec473c1, the third with both 450452de4
> and 44ec473c1.  Matt's patch would get past the first three checks, but
> fail with the last one, due to the issue you note.  All these checks
> pass if the string-match call is anchored or replaced by the
> skip-chars-backward form you suggest.

OK. Thank you for the clarification.

Then let's push Matt Lundin's solution (with skip-chars-backward), along
with your tests!

Regards,

-- 
Nicolas Goaziou

Re: [PATCH] Fix moving cursor in org-set-tags-command

[Kyle Meyer]

Nicolas Goaziou writes:

> Then let's push Matt Lundin's solution (with skip-chars-backward), along
> with your tests!

Pushed (6e50b22ff).  Thanks.

Re: [PATCH] Fix moving cursor in org-set-tags-command

[Matthew Lundin]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

>
>> -    (when (save-excursion (skip-chars-backward "*") (bolp))
>> -      (forward-char))))
>> +    (and (looking-at " ")
>> +	 (string-match "\\*+" (buffer-substring (point-at-bol) (point)))
>> +	 (forward-char))))
>
> Please replace `and' with `when' if side-effects are involved.
>

Thanks, this is really helpful to know for future patches. And thanks so
much for the fixes you and Kyle made.

> Also, note that
>
>   (save-excursion (skip-chars-backward "*") (bolp))
>
> is faster and more accurate than
>
>   (string-match "\\*+" (buffer-substring (point-at-bol) (point)))
>
> because the latter matches, e.g.,
>
>   ab*|c
>
> where "|" is point.

Oops, yes I see the problem in the string-match there!

> Besides, I don't understand how this is related to empty headlines
> since, AFAICT, this part of code is supposed to fix the issue on empty
> headlines.

Thanks. What I meant was fixing a very specific circumstance "when the
cursor is at the beginning of an empty headline." This is the scenario
affected by the original bug. The earlier patches (450452de4b and
44ec473c1) introduced a more general logic of moving the cursor forward
at any point before the beginning of an empty headline (including when
positioned on an asterisk). I see the commit has already been made, so
hopefully this email will serve as clarification.

Best,

Matt