emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
* [PATCH] Fix moving cursor in org-set-tags-command
@ 2020-05-08  0:20 Matt Lundin
  2020-05-08  2:42 ` Kyle Meyer
  2020-05-08  7:53 ` Nicolas Goaziou
  0 siblings, 2 replies; 7+ messages in thread
From: Matt Lundin @ 2020-05-08  0:20 UTC (permalink / raw)
  To: Org Mode List

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


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

* Re: [PATCH] Fix moving cursor in org-set-tags-command
  2020-05-08  0:20 [PATCH] Fix moving cursor in org-set-tags-command Matt Lundin
@ 2020-05-08  2:42 ` Kyle Meyer
  2020-05-08  7:53 ` Nicolas Goaziou
  1 sibling, 0 replies; 7+ messages in thread
From: Kyle Meyer @ 2020-05-08  2:42 UTC (permalink / raw)
  To: Matt Lundin, Org Mode List

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?


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

* Re: [PATCH] Fix moving cursor in org-set-tags-command
  2020-05-08  0:20 [PATCH] Fix moving cursor in org-set-tags-command Matt Lundin
  2020-05-08  2:42 ` Kyle Meyer
@ 2020-05-08  7:53 ` Nicolas Goaziou
  2020-05-09  1:44   ` Kyle Meyer
  2020-05-10 17:39   ` Matthew Lundin
  1 sibling, 2 replies; 7+ messages in thread
From: Nicolas Goaziou @ 2020-05-08  7:53 UTC (permalink / raw)
  To: Matt Lundin; +Cc: Org Mode List

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


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

* Re: [PATCH] Fix moving cursor in org-set-tags-command
  2020-05-08  7:53 ` Nicolas Goaziou
@ 2020-05-09  1:44   ` Kyle Meyer
  2020-05-09  8:12     ` Nicolas Goaziou
  2020-05-10 17:39   ` Matthew Lundin
  1 sibling, 1 reply; 7+ messages in thread
From: Kyle Meyer @ 2020-05-09  1:44 UTC (permalink / raw)
  To: Org Mode List; +Cc: Matt Lundin

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


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

* Re: [PATCH] Fix moving cursor in org-set-tags-command
  2020-05-09  1:44   ` Kyle Meyer
@ 2020-05-09  8:12     ` Nicolas Goaziou
  2020-05-09 20:01       ` Kyle Meyer
  0 siblings, 1 reply; 7+ messages in thread
From: Nicolas Goaziou @ 2020-05-09  8:12 UTC (permalink / raw)
  To: Kyle Meyer; +Cc: Matt Lundin, Org Mode List

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


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

* Re: [PATCH] Fix moving cursor in org-set-tags-command
  2020-05-09  8:12     ` Nicolas Goaziou
@ 2020-05-09 20:01       ` Kyle Meyer
  0 siblings, 0 replies; 7+ messages in thread
From: Kyle Meyer @ 2020-05-09 20:01 UTC (permalink / raw)
  To: Org Mode List; +Cc: Matt Lundin

Nicolas Goaziou writes:

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

Pushed (6e50b22ff).  Thanks.


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

* Re: [PATCH] Fix moving cursor in org-set-tags-command
  2020-05-08  7:53 ` Nicolas Goaziou
  2020-05-09  1:44   ` Kyle Meyer
@ 2020-05-10 17:39   ` Matthew Lundin
  1 sibling, 0 replies; 7+ messages in thread
From: Matthew Lundin @ 2020-05-10 17:39 UTC (permalink / raw)
  To: Nicolas Goaziou; +Cc: Org Mode List

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


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

end of thread, other threads:[~2020-05-10 17:40 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-08  0:20 [PATCH] Fix moving cursor in org-set-tags-command Matt Lundin
2020-05-08  2:42 ` Kyle Meyer
2020-05-08  7:53 ` Nicolas Goaziou
2020-05-09  1:44   ` Kyle Meyer
2020-05-09  8:12     ` Nicolas Goaziou
2020-05-09 20:01       ` Kyle Meyer
2020-05-10 17:39   ` Matthew Lundin

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

	https://git.savannah.gnu.org/cgit/emacs/org-mode.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).