Re: [PATCH] Fix moving cursor in org-set-tags-command
Nicolas Goaziou 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
Re: [PATCH] Fix moving cursor in org-set-tags-command
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
Hello, Kyle Meyer 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
Nicolas Goaziou writes: > Matt Lundin 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 "* " + (cl-letf (((symbol-function 'completing-read) + (lambda ( 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 ( 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 "** H1" + (cl-letf (((symbol-function 'completing-read) + (lambda ( 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 b" + (cl-letf (((symbol-function 'completing-read) + (lambda ( 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
Hello, Matt Lundin 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
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?
[PATCH] Fix moving cursor in org-set-tags-command
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 >From ae5cf0e1110241426e49f573219e9740c25bf8ea Mon Sep 17 00:00:00 2001 From: Matt Lundin 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 ( all) "Align tags in current entry. -- 2.26.2
