From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp1 ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms11 with LMTPS id 4L45Mf0Ktl5IPwAA0tVLHw (envelope-from ) for ; Sat, 09 May 2020 01:44:29 +0000 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp1 with LMTPS id WGw1KQoLtl6IAQAAbx9fmQ (envelope-from ) for ; Sat, 09 May 2020 01:44:42 +0000 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id D12D2940D34 for ; Sat, 9 May 2020 01:44:39 +0000 (UTC) Received: from localhost ([::1]:53804 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jXEXf-00071W-MJ for larch@yhetil.org; Fri, 08 May 2020 21:44:39 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:42420) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jXEXJ-00071P-Nu for emacs-orgmode@gnu.org; Fri, 08 May 2020 21:44:17 -0400 Received: from pb-smtp1.pobox.com ([64.147.108.70]:65520) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jXEXH-0001YQ-U0 for emacs-orgmode@gnu.org; Fri, 08 May 2020 21:44:17 -0400 Received: from pb-smtp1.pobox.com (unknown [127.0.0.1]) by pb-smtp1.pobox.com (Postfix) with ESMTP id 34FBA5F533; Fri, 8 May 2020 21:44:14 -0400 (EDT) (envelope-from kyle@kyleam.com) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=pobox.com; h=from:to:cc :subject:in-reply-to:references:date:message-id:mime-version :content-type; s=sasl; bh=ndFWc/BBnYqXYeol8wsNLXWmRMM=; b=fOAFdS 93yr/52VVHxFWb5quL63w8Yr0Vb21RMFqfcxiUOl9n0G+2ThhO0B7/6TfpL51L4r knwvOnNt+m9wO1U961wv6s/Y8zFLiDroN+qVmGWcpWt+xwh4OMVnbzhkubqHPH2Z jI4+wL3UHz0K+qF3Ko6Mh4nihSRqIk8rGsRQc= Received: from pb-smtp1.nyi.icgroup.com (unknown [127.0.0.1]) by pb-smtp1.pobox.com (Postfix) with ESMTP id 1B4A05F532; Fri, 8 May 2020 21:44:14 -0400 (EDT) (envelope-from kyle@kyleam.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed; d=kyleam.com; h=from:to:cc:subject:in-reply-to:references:date:message-id:mime-version:content-type; s=mesmtp; bh=OadgxKcug9fnCMbquxlSE+Rb+LkN2MeIRetQ/UMbtK8=; b=WqFR8yqVpgJ9OCSga3ddrvabvq/beTfPPcWRG0eWohZfsPdrMvcjG3om8yYofUVFoXQunRH84WBDWzJQSr0PGUDQVbPLhG4LoAaEY4Sa819TjEIqmcS+pgQUunJ3AgG1meDVaBsgz7BsRr0cB/8OZprq+tgr7fZbYipkvvjcdFc= Received: from localhost (unknown [45.33.91.115]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by pb-smtp1.pobox.com (Postfix) with ESMTPSA id 7C8315F530; Fri, 8 May 2020 21:44:13 -0400 (EDT) (envelope-from kyle@kyleam.com) From: Kyle Meyer To: Org Mode List Subject: Re: [PATCH] Fix moving cursor in org-set-tags-command In-Reply-To: <87ftca6ewc.fsf@nicolasgoaziou.fr> References: <871rnvi8g6.fsf@fastmail.fm> <87ftca6ewc.fsf@nicolasgoaziou.fr> Date: Sat, 09 May 2020 01:44:12 +0000 Message-ID: <87zhahdgr7.fsf@kyleam.com> MIME-Version: 1.0 Content-Type: text/plain X-Pobox-Relay-ID: 95F568CA-9196-11EA-BC27-C28CBED8090B-24757444!pb-smtp1.pobox.com Received-SPF: pass client-ip=64.147.108.70; envelope-from=kyle@kyleam.com; helo=pb-smtp1.pobox.com X-detected-operating-system: by eggs.gnu.org: First seen = 2020/05/08 21:44:14 X-ACL-Warn: Detected OS = Linux 2.2.x-3.x [generic] [fuzzy] X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, SPF_PASS=-0.001, URIBL_BLOCKED=0.001 autolearn=_AUTOLEARN X-Spam_action: no action X-BeenThere: emacs-orgmode@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Matt Lundin Errors-To: emacs-orgmode-bounces+larch=yhetil.org@gnu.org Sender: "Emacs-orgmode" X-Scanner: scn0 X-Spam-Score: -1.21 Authentication-Results: aspmx1.migadu.com; dkim=pass header.d=pobox.com header.s=sasl header.b=fOAFdS 9; dkim=pass header.d=kyleam.com header.s=mesmtp header.b=WqFR8yqV; dmarc=none; spf=pass (aspmx1.migadu.com: domain of emacs-orgmode-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=emacs-orgmode-bounces@gnu.org X-Scan-Result: default: False [-1.21 / 13.00]; RCVD_VIA_SMTP_AUTH(0.00)[]; GENERIC_REPUTATION(0.00)[-0.54022492270328]; DWL_DNSWL_FAIL(0.00)[kyleam.com:server fail,209.51.188.17:server fail]; R_SPF_ALLOW(-0.20)[+ip4:209.51.188.0/24:c]; IP_REPUTATION_HAM(0.00)[asn: 22989(0.09), country: US(-0.00), ip: 209.51.188.17(-0.54)]; TO_DN_ALL(0.00)[]; MX_GOOD(-0.50)[cached: eggs.gnu.org]; RCPT_COUNT_TWO(0.00)[2]; DKIM_TRACE(0.00)[pobox.com:+,kyleam.com:+]; MAILLIST(-0.20)[mailman]; FORGED_RECIPIENTS_MAILLIST(0.00)[]; RCVD_IN_DNSWL_FAIL(0.00)[209.51.188.17:server fail]; MIME_TRACE(0.00)[0:+]; RCVD_TLS_LAST(0.00)[]; ASN(0.00)[asn:22989, ipnet:209.51.188.0/24, country:US]; TAGGED_FROM(0.00)[larch=yhetil.org]; FROM_NEQ_ENVFROM(0.00)[kyle@kyleam.com,emacs-orgmode-bounces@gnu.org]; ARC_NA(0.00)[]; R_DKIM_ALLOW(-0.20)[pobox.com:s=sasl,kyleam.com:s=mesmtp]; MID_RHS_MATCH_FROM(0.00)[]; FROM_HAS_DN(0.00)[]; URIBL_BLOCKED(0.00)[imapmail.org:email,pobox.com:dkim,kyleam.com:dkim]; MIME_GOOD(-0.10)[text/plain]; DMARC_NA(0.00)[kyleam.com]; HAS_LIST_UNSUB(-0.01)[]; RWL_MAILSPIKE_POSSIBLE(0.00)[209.51.188.17:from]; FREEMAIL_CC(0.00)[imapmail.org]; RCVD_COUNT_SEVEN(0.00)[7]; FORGED_SENDER_MAILLIST(0.00)[] X-TUID: LnR0/BB4uwzF 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 (&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 "** 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 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."