all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* [PATCH] org: add property names from #+PROPERTY keywords to completion list
@ 2020-07-07  2:01 Nick Dokos
  2020-07-07  2:40 ` Nick Dokos
  2020-07-07  4:23 ` Kyle Meyer
  0 siblings, 2 replies; 11+ messages in thread
From: Nick Dokos @ 2020-07-07  2:01 UTC (permalink / raw)
  To: emacs-orgmode

Here's a patch to enhance the property name completion list with names from
#+PROPERTY keyword lines: at the moment, only property names found in property
drawers are used to populate the completion list.

Keith Pinson posted the question on Emacs SE:

    https://emacs.stackexchange.com/questions/59448/

Please let me know if the patch looks reasonable. If it does, I should
probably add a test.

Thanks!

--8<---------------cut here---------------start------------->8---
org: add property names from #+PROPERTY keywords to completion list

* lisp/org.el (org-buffer-property-keys): ehhance the completion list
with property names from #+PROPERTY keywords, not just property
drawers.

See https://emacs.stackexchange.com/questions/59448/ for details.
---
 lisp/org.el | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lisp/org.el b/lisp/org.el
index 748c058ca..0e83162e8 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -13084,6 +13084,11 @@ COLUMN formats in the current buffer."
 	(props (append
 		(and specials org-special-properties)
 		(and defaults (cons org-effort-property org-default-properties))
+		;; Get property names from #+PROPERTY keywords as well
+		(mapcar (lambda (s)
+			  (let ((split (split-string s)))
+			    (nth 0 split)))
+			(cdar (org-collect-keywords '("PROPERTY"))))
 		nil)))
     (org-with-wide-buffer
      (goto-char (point-min))
-- 
2.25.1
--8<---------------cut here---------------end--------------->8---

-- 
Nick

"There are only two hard problems in computer science: cache
invalidation, naming things, and off-by-one errors." -Martin Fowler



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

* Re: [PATCH] org: add property names from #+PROPERTY keywords to completion list
  2020-07-07  2:01 [PATCH] org: add property names from #+PROPERTY keywords to completion list Nick Dokos
@ 2020-07-07  2:40 ` Nick Dokos
  2020-07-07  4:23 ` Kyle Meyer
  1 sibling, 0 replies; 11+ messages in thread
From: Nick Dokos @ 2020-07-07  2:40 UTC (permalink / raw)
  To: emacs-orgmode

Adding a simple test to the previous patch:

--8<---------------cut here---------------start------------->8---
From cae6b5596f69968003c053f53cb45ffb4139a5ad Mon Sep 17 00:00:00 2001
From: Nick Dokos <ndokos@gmail.com>
Date: Mon, 6 Jul 2020 21:07:01 -0400
Subject: [PATCH] org: add property names from #+PROPERTY keywords to
 completion list

* lisp/org.el (org-buffer-property-keys): ehhance the completion list
with property names from #+PROPERTY keywords, not just property
drawers.

See https://emacs.stackexchange.com/questions/59448/ for details.
---
 lisp/org.el              | 5 +++++
 testing/lisp/test-org.el | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/lisp/org.el b/lisp/org.el
index 748c058ca..0e83162e8 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -13084,6 +13084,11 @@ COLUMN formats in the current buffer."
 	(props (append
 		(and specials org-special-properties)
 		(and defaults (cons org-effort-property org-default-properties))
+		;; Get property names from #+PROPERTY keywords as well
+		(mapcar (lambda (s)
+			  (let ((split (split-string s)))
+			    (nth 0 split)))
+			(cdar (org-collect-keywords '("PROPERTY"))))
 		nil)))
     (org-with-wide-buffer
      (goto-char (point-min))
diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 3c563f344..ddda96105 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -5352,6 +5352,11 @@ Paragraph<point>"
    (equal '("A")
 	  (org-test-with-temp-text "* H\n:PROPERTIES:\n:A: 1\n:A+: 2\n:END:"
 	    (org-buffer-property-keys))))
+  ;; Retrieve properties from #+PROPERTY keyword lines
+  (should
+   (equal '("A" "C")
+	  (org-test-with-temp-text "#+PROPERTY: C foo\n* H\n:PROPERTIES:\n:A: 1\n:A+: 2\n:END:"
+            (org-buffer-property-keys))))
   ;; With non-nil COLUMNS, extract property names from columns.
   (should
    (equal '("A" "B")
-- 
2.25.4
--8<---------------cut here---------------end--------------->8---


-- 
Nick

"There are only two hard problems in computer science: cache
invalidation, naming things, and off-by-one errors." -Martin Fowler



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

* Re: [PATCH] org: add property names from #+PROPERTY keywords to completion list
  2020-07-07  2:01 [PATCH] org: add property names from #+PROPERTY keywords to completion list Nick Dokos
  2020-07-07  2:40 ` Nick Dokos
@ 2020-07-07  4:23 ` Kyle Meyer
  2020-07-07 12:44   ` Nick Dokos
                     ` (2 more replies)
  1 sibling, 3 replies; 11+ messages in thread
From: Kyle Meyer @ 2020-07-07  4:23 UTC (permalink / raw)
  To: Nick Dokos; +Cc: emacs-orgmode

Nick Dokos writes:

> Here's a patch to enhance the property name completion list with names from
> #+PROPERTY keyword lines: at the moment, only property names found in property
> drawers are used to populate the completion list.

Thanks for the patch.

> org: add property names from #+PROPERTY keywords to completion list
>
> * lisp/org.el (org-buffer-property-keys): ehhance the completion list

Typo: enhance.  And as a convention nit, it should be capitalized.

> with property names from #+PROPERTY keywords, not just property
> drawers.
>
> See https://emacs.stackexchange.com/questions/59448/ for details.
> ---
>  lisp/org.el | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/lisp/org.el b/lisp/org.el
> index 748c058ca..0e83162e8 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -13084,6 +13084,11 @@ COLUMN formats in the current buffer."
>  	(props (append
>  		(and specials org-special-properties)
>  		(and defaults (cons org-effort-property org-default-properties))
> +		;; Get property names from #+PROPERTY keywords as well
> +		(mapcar (lambda (s)
> +			  (let ((split (split-string s)))
> +			    (nth 0 split)))
> +			(cdar (org-collect-keywords '("PROPERTY"))))
>  		nil)))

IMO the let-binding doesn't add any clarity over

  (nth 0 (split-string s))


I wondered about possible duplicates, but it looks like
org-buffer-property-keys already takes care of that at the end.

I think this patch is a clear improvement as is, but in the context of
completion (and the stack exchange post you link to), isn't the handling
around *_ALL keywords still a bit off?  It seems a caller would want to
complete without the _ALL; to use the example from that post, with
"#+PROPERTY: GENRE_ALL ...", the caller would want to complete "GENRE".
Is it worth providing special handling here?


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

* Re: [PATCH] org: add property names from #+PROPERTY keywords to completion list
  2020-07-07  4:23 ` Kyle Meyer
@ 2020-07-07 12:44   ` Nick Dokos
  2020-07-07 13:38   ` Nick Dokos
  2020-07-07 16:48   ` Nick Dokos
  2 siblings, 0 replies; 11+ messages in thread
From: Nick Dokos @ 2020-07-07 12:44 UTC (permalink / raw)
  To: emacs-orgmode

Hi Kyle,

thanks for the review.

Kyle Meyer <kyle@kyleam.com> writes:

> Nick Dokos writes:
>
>> Here's a patch to enhance the property name completion list with names from
>> #+PROPERTY keyword lines: at the moment, only property names found in property
>> drawers are used to populate the completion list.
>
> Thanks for the patch.
>
>> org: add property names from #+PROPERTY keywords to completion list
>>
>> * lisp/org.el (org-buffer-property-keys): ehhance the completion list
>
> Typo: enhance.  And as a convention nit, it should be capitalized.
>
Fixed.

>> with property names from #+PROPERTY keywords, not just property
>> drawers.
>>
>> ... 
>> +		(mapcar (lambda (s)
>> +			  (let ((split (split-string s)))
>> +			    (nth 0 split)))
>> +			(cdar (org-collect-keywords '("PROPERTY"))))
>>  		nil)))
>
> IMO the let-binding doesn't add any clarity over
>
>   (nth 0 (split-string s))
>
Fixed.
>
> I wondered about possible duplicates, but it looks like
> org-buffer-property-keys already takes care of that at the end.
>
> I think this patch is a clear improvement as is, but in the context of
> completion (and the stack exchange post you link to), isn't the handling
> around *_ALL keywords still a bit off?  It seems a caller would want to
> complete without the _ALL; to use the example from that post, with
> "#+PROPERTY: GENRE_ALL ...", the caller would want to complete "GENRE".
> Is it worth providing special handling here?
>
>
My assumption was that one could put in two properties (that's what
the OP was doing in his setup file): a GENRE_ALL one already properly
set-up and the corresponding bare GENRE property as a placeholder with
an empty value, just to get the completion. It seems to me like a
reasonable way to do it, without making org-buffer-property-keys too
opinionated.

I could see stripping all the _ALL suffixes from the property names
at the end, but personally it just makes me a bit queasy :) But if
there is consensus one way or the other, I'd be happy to implement it.

Here's the updated patch:

--8<---------------cut here---------------start------------->8---
From 50c625f935d5581952d37801943550ff44c473ff Mon Sep 17 00:00:00 2001
From: Nick Dokos <ndokos@redhat.com>
Date: Mon, 6 Jul 2020 21:49:41 -0400
Subject: [PATCH] org: add property names from #+PROPERTY keywords to
 completion list

* lisp/org.el (org-buffer-property-keys): Enhance the completion list
with property names from #+PROPERTY keywords, not just property
drawers.

See https://emacs.stackexchange.com/questions/59448/ for details.
---
 lisp/org.el | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lisp/org.el b/lisp/org.el
index 4a1a83d0f..8deaa1ed9 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -13084,6 +13084,10 @@ COLUMN formats in the current buffer."
 	(props (append
 		(and specials org-special-properties)
 		(and defaults (cons org-effort-property org-default-properties))
+		;; Get property names from #+PROPERTY keywords as well
+		(mapcar (lambda (s)
+			  (nth 0 (split-string s)))
+			(cdar (org-collect-keywords '("PROPERTY"))))
 		nil)))
     (org-with-wide-buffer
      (goto-char (point-min))
-- 
2.25.1

--8<---------------cut here---------------end--------------->8---

-- 
Nick

"There are only two hard problems in computer science: cache
invalidation, naming things, and off-by-one errors." -Martin Fowler



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

* Re: [PATCH] org: add property names from #+PROPERTY keywords to completion list
  2020-07-07  4:23 ` Kyle Meyer
  2020-07-07 12:44   ` Nick Dokos
@ 2020-07-07 13:38   ` Nick Dokos
  2020-07-07 16:48   ` Nick Dokos
  2 siblings, 0 replies; 11+ messages in thread
From: Nick Dokos @ 2020-07-07 13:38 UTC (permalink / raw)
  To: emacs-orgmode

Kyle Meyer <kyle@kyleam.com> writes:

> ,,,
> I think this patch is a clear improvement as is, but in the context of
> completion (and the stack exchange post you link to), isn't the handling
> around *_ALL keywords still a bit off?  It seems a caller would want to
> complete without the _ALL; to use the example from that post, with
> "#+PROPERTY: GENRE_ALL ...", the caller would want to complete "GENRE".
> Is it worth providing special handling here?
>

With a bit of caffeine in my system, I now see what you wrote and I
think you are right: if the _ALL property is present, then the bare
property should be added to the completion list if not already there.

-- 
Nick

"There are only two hard problems in computer science: cache
invalidation, naming things, and off-by-one errors." -Martin Fowler



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

* Re: [PATCH] org: add property names from #+PROPERTY keywords to completion list
  2020-07-07  4:23 ` Kyle Meyer
  2020-07-07 12:44   ` Nick Dokos
  2020-07-07 13:38   ` Nick Dokos
@ 2020-07-07 16:48   ` Nick Dokos
  2020-07-08  3:52     ` Kyle Meyer
  2 siblings, 1 reply; 11+ messages in thread
From: Nick Dokos @ 2020-07-07 16:48 UTC (permalink / raw)
  To: emacs-orgmode

Here's the amended patch: it includes the fixes from Kyle's review, the modification
he suggested that adds the plain property for each _ALL property to the list
and a few test cases to the test/org-buffer-property-keys test.

--8<---------------cut here---------------start------------->8---
From 60b9ababe42c91ec6fcd2c53f6923d75daa12454 Mon Sep 17 00:00:00 2001
From: Nick Dokos <ndokos@redhat.com>
Date: Mon, 6 Jul 2020 21:49:41 -0400
Subject: [PATCH] org: add property names from #+PROPERTY keywords to
 completion list

* lisp/org.el (org-buffer-property-keys): Enhance the completion list
with property names from #+PROPERTY keywords, not just property
drawers. Also, for each xxx_ALL property, make sure that the bare xxx
property is added too.

* testing/lisp/test-org.el (test-org/buffer-property-keys): Add test
cases for #+PROPERTY keywords and also for xxx_ALL --> xxx properties.

See https://emacs.stackexchange.com/questions/59448/ for details.
---
 lisp/org.el              | 17 +++++++++++++++--
 testing/lisp/test-org.el | 21 +++++++++++++++++++++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index 4a1a83d0f..ccdb0dd5c 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -13084,7 +13084,12 @@ COLUMN formats in the current buffer."
 	(props (append
 		(and specials org-special-properties)
 		(and defaults (cons org-effort-property org-default-properties))
-		nil)))
+		;; Get property names from #+PROPERTY keywords as well
+		(mapcar (lambda (s)
+			  (nth 0 (split-string s)))
+			(cdar (org-collect-keywords '("PROPERTY"))))
+		nil))
+	bare-props)
     (org-with-wide-buffer
      (goto-char (point-min))
      (while (re-search-forward org-property-start-re nil t)
@@ -13132,7 +13137,15 @@ COLUMN formats in the current buffer."
 		 (let ((p (match-string-no-properties 1 value)))
 		   (unless (member-ignore-case p org-special-properties)
 		     (push p props))))))))))
-    (sort (delete-dups props) (lambda (a b) (string< (upcase a) (upcase b))))))
+    (sort (delete-dups (append props
+			       ;; for each xxx_ALL property, make sure
+			       ;; the bare xxx property is also
+			       ;; included
+			       (dolist (x props bare-props)
+				 (if (string-match "_ALL\\b" x)
+				     (setq bare-props (cons (substring x 0 -4)
+							    bare-props))))))
+	  (lambda (a b) (string< (upcase a) (upcase b))))))
 
 (defun org-property-values (key)
   "List all non-nil values of property KEY in current buffer."
diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 30a8067db..61e642d4f 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -5409,6 +5409,27 @@ Paragraph<point>"
    (equal '("A")
 	  (org-test-with-temp-text "* H\n:PROPERTIES:\n:A: 1\n:A+: 2\n:END:"
 	    (org-buffer-property-keys))))
+  ;; Add bare property if xxx_ALL property is there
+  (should
+   (equal '("A" "B" "B_ALL")
+	  (org-test-with-temp-text "* H\n:PROPERTIES:\n:A: 1\n:A+: 2\n:B_ALL: foo bar\n:END:"
+	    (org-buffer-property-keys))))
+  ;; Add bare property if xxx_ALL property is there - check dupes
+  (should
+   (equal '("A" "B" "B_ALL")
+	  (org-test-with-temp-text "* H\n:PROPERTIES:\n:A: 1\n:B: 2\n:B_ALL: foo bar\n:END:"
+	    (org-buffer-property-keys))))
+  ;; Retrieve properties from #+PROPERTY keyword lines
+  (should
+   (equal '("A" "C")
+	  (org-test-with-temp-text "#+PROPERTY: C foo\n* H\n:PROPERTIES:\n:A: 1\n:A+: 2\n:END:"
+	    (org-buffer-property-keys))))
+  ;; Retrieve properties from #+PROPERTY keyword lines - make sure an _ALL property also
+  ;; adds the bare property
+  (should
+   (equal '("A" "C" "C_ALL")
+	  (org-test-with-temp-text "#+PROPERTY: C_ALL foo bar\n* H\n:PROPERTIES:\n:A: 1\n:A+: 2\n:END:"
+	    (org-buffer-property-keys))))
   ;; With non-nil COLUMNS, extract property names from columns.
   (should
    (equal '("A" "B")
-- 
2.25.1

--8<---------------cut here---------------end--------------->8---

-- 
Nick

"There are only two hard problems in computer science: cache
invalidation, naming things, and off-by-one errors." -Martin Fowler



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

* Re: [PATCH] org: add property names from #+PROPERTY keywords to completion list
  2020-07-07 16:48   ` Nick Dokos
@ 2020-07-08  3:52     ` Kyle Meyer
  2020-07-08 15:47       ` Nick Dokos
  2020-07-08 16:07       ` Nick Dokos
  0 siblings, 2 replies; 11+ messages in thread
From: Kyle Meyer @ 2020-07-08  3:52 UTC (permalink / raw)
  To: Nick Dokos; +Cc: emacs-orgmode

Nick Dokos writes:

> Here's the amended patch: it includes the fixes from Kyle's review, the modification
> he suggested that adds the plain property for each _ALL property to the list
> and a few test cases to the test/org-buffer-property-keys test.

Thank you for the updates.  Applied (bc4fa8a00).

> -		nil)))
> +		;; Get property names from #+PROPERTY keywords as well
> +		(mapcar (lambda (s)
> +			  (nth 0 (split-string s)))
> +			(cdar (org-collect-keywords '("PROPERTY"))))
> +		nil))

I didn't spot it earlier, but this nil (not added by your patch) is
unnecessary.  Since the patch is touching the line anyway, I've dropped
it on apply.

> +	bare-props)
>      (org-with-wide-buffer
>       (goto-char (point-min))
>       (while (re-search-forward org-property-start-re nil t)
> @@ -13132,7 +13137,15 @@ COLUMN formats in the current buffer."
>  		 (let ((p (match-string-no-properties 1 value)))
>  		   (unless (member-ignore-case p org-special-properties)
>  		     (push p props))))))))))
> -    (sort (delete-dups props) (lambda (a b) (string< (upcase a) (upcase b))))))
> +    (sort (delete-dups (append props
> +			       ;; for each xxx_ALL property, make sure
> +			       ;; the bare xxx property is also
> +			       ;; included
> +			       (dolist (x props bare-props)
> +				 (if (string-match "_ALL\\b" x)
> +				     (setq bare-props (cons (substring x 0 -4)
> +							    bare-props))))))

I did a cosmetic rewrite here to use mapcar, which I hope you won't
mind.

Thanks again.


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

* Re: [PATCH] org: add property names from #+PROPERTY keywords to completion list
  2020-07-08  3:52     ` Kyle Meyer
@ 2020-07-08 15:47       ` Nick Dokos
  2020-07-08 16:07       ` Nick Dokos
  1 sibling, 0 replies; 11+ messages in thread
From: Nick Dokos @ 2020-07-08 15:47 UTC (permalink / raw)
  To: emacs-orgmode

Kyle Meyer <kyle@kyleam.com> writes:

> Nick Dokos writes:
>
>> Here's the amended patch: it includes the fixes from Kyle's review, the modification
>> he suggested that adds the plain property for each _ALL property to the list
>> and a few test cases to the test/org-buffer-property-keys test.
>
> Thank you for the updates.  Applied (bc4fa8a00).
>
>> -		nil)))
>> +		;; Get property names from #+PROPERTY keywords as well
>> +		(mapcar (lambda (s)
>> +			  (nth 0 (split-string s)))
>> +			(cdar (org-collect-keywords '("PROPERTY"))))
>> +		nil))
>
> I didn't spot it earlier, but this nil (not added by your patch) is
> unnecessary.  Since the patch is touching the line anyway, I've dropped
> it on apply.
>
>> +	bare-props)
>>      (org-with-wide-buffer
>>       (goto-char (point-min))
>>       (while (re-search-forward org-property-start-re nil t)
>> @@ -13132,7 +13137,15 @@ COLUMN formats in the current buffer."
>>  		 (let ((p (match-string-no-properties 1 value)))
>>  		   (unless (member-ignore-case p org-special-properties)
>>  		     (push p props))))))))))
>> -    (sort (delete-dups props) (lambda (a b) (string< (upcase a) (upcase b))))))
>> +    (sort (delete-dups (append props
>> +			       ;; for each xxx_ALL property, make sure
>> +			       ;; the bare xxx property is also
>> +			       ;; included
>> +			       (dolist (x props bare-props)
>> +				 (if (string-match "_ALL\\b" x)
>> +				     (setq bare-props (cons (substring x 0 -4)
>> +							    bare-props))))))
>
> I did a cosmetic rewrite here to use mapcar, which I hope you won't
> mind.

Not at all - thanks!

>
> Thanks again.
>
>

-- 
Nick



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

* Re: [PATCH] org: add property names from #+PROPERTY keywords to completion list
  2020-07-08  3:52     ` Kyle Meyer
  2020-07-08 15:47       ` Nick Dokos
@ 2020-07-08 16:07       ` Nick Dokos
  2020-07-09  4:36         ` [PATCH] agenda: Fold case when retrieving user-configured effort values Kyle Meyer
  1 sibling, 1 reply; 11+ messages in thread
From: Nick Dokos @ 2020-07-08 16:07 UTC (permalink / raw)
  To: emacs-orgmode

Kyle Meyer <kyle@kyleam.com> writes:

> Nick Dokos writes:
>
>> Here's the amended patch: it includes the fixes from Kyle's review, the modification
>> he suggested that adds the plain property for each _ALL property to the list
>> and a few test cases to the test/org-buffer-property-keys test.
>
> Thank you for the updates.  Applied (bc4fa8a00).
>
>> -		nil)))
>> +		;; Get property names from #+PROPERTY keywords as well
>> +		(mapcar (lambda (s)
>> +			  (nth 0 (split-string s)))
>> +			(cdar (org-collect-keywords '("PROPERTY"))))
>> +		nil))
>
> I didn't spot it earlier, but this nil (not added by your patch) is
> unnecessary.  Since the patch is touching the line anyway, I've dropped
> it on apply.
>
>> +	bare-props)
>>      (org-with-wide-buffer
>>       (goto-char (point-min))
>>       (while (re-search-forward org-property-start-re nil t)
>> @@ -13132,7 +13137,15 @@ COLUMN formats in the current buffer."
>>  		 (let ((p (match-string-no-properties 1 value)))
>>  		   (unless (member-ignore-case p org-special-properties)
>>  		     (push p props))))))))))
>> -    (sort (delete-dups props) (lambda (a b) (string< (upcase a) (upcase b))))))
>> +    (sort (delete-dups (append props
>> +			       ;; for each xxx_ALL property, make sure
>> +			       ;; the bare xxx property is also
>> +			       ;; included
>> +			       (dolist (x props bare-props)
>> +				 (if (string-match "_ALL\\b" x)
>> +				     (setq bare-props (cons (substring x 0 -4)
>> +							    bare-props))))))
>
> I did a cosmetic rewrite here to use mapcar, which I hope you won't
> mind.
>

BTW, I just thought of a possible problem: the manual says that property
keys are case-insensitive (although all the examples I can find spell
"_ALL" in upper case, but if I write

  :PROPERTIES:
  :foo_all: bar baz
  :END:

I don't think that the code is going to handle it correctly. There
are other places that also use "_ALL" without a let of case-fold-search
(at least AFAICT).

Am I paranoid or is this a problem?

-- 
Nick



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

* [PATCH] agenda: Fold case when retrieving user-configured effort values
  2020-07-08 16:07       ` Nick Dokos
@ 2020-07-09  4:36         ` Kyle Meyer
  2020-07-09  7:50           ` Nick Dokos
  0 siblings, 1 reply; 11+ messages in thread
From: Kyle Meyer @ 2020-07-09  4:36 UTC (permalink / raw)
  To: Nick Dokos; +Cc: emacs-orgmode

Nick Dokos writes:

> BTW, I just thought of a possible problem: the manual says that property
> keys are case-insensitive (although all the examples I can find spell
> "_ALL" in upper case, but if I write
>
>   :PROPERTIES:
>   :foo_all: bar baz
>   :END:
>
> I don't think that the code is going to handle it correctly. There
> are other places that also use "_ALL" without a let of case-fold-search
> (at least AFAICT).
>
> Am I paranoid or is this a problem?

case-fold-search is let-bound to t at the beginning of
org-buffer-property-keys, so the added string-match is covered.

When collecting keys, there's no attempt to normalize to upper or lower,
so the delete-dups call won't take care of any keys that are present in
various case styles.  I don't think that's something worth worrying
about (and perhaps it's even preferable, since we can't know which
variant the caller would want to complete).

As for other spots in the code base: looking through grep hits for
"_ALL", it seems like most should be okay because they go through
org-entry-get.  Two places in org-agenda are case-sensitive, though.

-- >8 --
Subject: [PATCH] agenda: Fold case when retrieving user-configured effort
 values

* lisp/org-agenda.el (org-agenda-filter-by-effort):
(org-agenda-filter-completion-function): Ignore case when querying
effort property key in org-global-properties since property keys are
documented as case-insensitive.
---
 lisp/org-agenda.el | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/lisp/org-agenda.el b/lisp/org-agenda.el
index 90129b23e..5c2933b0a 100644
--- a/lisp/org-agenda.el
+++ b/lisp/org-agenda.el
@@ -7611,8 +7611,9 @@ (defun org-agenda-filter-by-effort (strip-or-accumulate)
 consistency with the other filter commands."
   (interactive "P")
   (let* ((efforts (split-string
-		   (or (cdr (assoc (concat org-effort-property "_ALL")
-				   org-global-properties))
+		   (or (cdr (assoc-string (concat org-effort-property "_ALL")
+					  org-global-properties
+					  t))
 		       "0 0:10 0:30 1:00 2:00 3:00 4:00 5:00 6:00 7:00")))
 	 ;; XXX: the following handles only up to 10 different
 	 ;; effort values.
@@ -7777,8 +7778,9 @@ (defun org-agenda-filter-completion-function (string _predicate &optional flag)
 			  (org-agenda-get-represented-tags))))
      ((member operator '("<" ">" "="))
       (setq table (split-string
-		   (or (cdr (assoc (concat org-effort-property "_ALL")
-				   org-global-properties))
+		   (or (cdr (assoc-string (concat org-effort-property "_ALL")
+					  org-global-properties
+					  t))
 		       "0 0:10 0:30 1:00 2:00 3:00 4:00 5:00 6:00 7:00")
 		   " +")))
      (t (setq table nil)))

base-commit: eac255d911e0793513b2e2f14b06b94194a04daf
-- 
2.26.2



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

* Re: [PATCH] agenda: Fold case when retrieving user-configured effort values
  2020-07-09  4:36         ` [PATCH] agenda: Fold case when retrieving user-configured effort values Kyle Meyer
@ 2020-07-09  7:50           ` Nick Dokos
  0 siblings, 0 replies; 11+ messages in thread
From: Nick Dokos @ 2020-07-09  7:50 UTC (permalink / raw)
  To: emacs-orgmode

Kyle Meyer <kyle@kyleam.com> writes:

> Nick Dokos writes:
>
>> BTW, I just thought of a possible problem: the manual says that property
>> keys are case-insensitive (although all the examples I can find spell
>> "_ALL" in upper case, but if I write
>>
>>   :PROPERTIES:
>>   :foo_all: bar baz
>>   :END:
>>
>> I don't think that the code is going to handle it correctly. There
>> are other places that also use "_ALL" without a let of case-fold-search
>> (at least AFAICT).
>>
>> Am I paranoid or is this a problem?
>
> case-fold-search is let-bound to t at the beginning of
> org-buffer-property-keys, so the added string-match is covered.
>

Wow - not only paranoid but blind as well.

> When collecting keys, there's no attempt to normalize to upper or lower,
> so the delete-dups call won't take care of any keys that are present in
> various case styles.  I don't think that's something worth worrying
> about (and perhaps it's even preferable, since we can't know which
> variant the caller would want to complete).
>
> As for other spots in the code base: looking through grep hits for
> "_ALL", it seems like most should be okay because they go through
> org-entry-get.  Two places in org-agenda are case-sensitive, though.
>
> -- >8 --
> Subject: [PATCH] agenda: Fold case when retrieving user-configured effort
>  values
>
> * lisp/org-agenda.el (org-agenda-filter-by-effort):
> (org-agenda-filter-completion-function): Ignore case when querying
> effort property key in org-global-properties since property keys are
> documented as case-insensitive.
> ---
>  lisp/org-agenda.el | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/lisp/org-agenda.el b/lisp/org-agenda.el
> index 90129b23e..5c2933b0a 100644
> --- a/lisp/org-agenda.el
> +++ b/lisp/org-agenda.el
> @@ -7611,8 +7611,9 @@ (defun org-agenda-filter-by-effort (strip-or-accumulate)
>  consistency with the other filter commands."
>    (interactive "P")
>    (let* ((efforts (split-string
> -		   (or (cdr (assoc (concat org-effort-property "_ALL")
> -				   org-global-properties))
> +		   (or (cdr (assoc-string (concat org-effort-property "_ALL")
> +					  org-global-properties
> +					  t))
>  		       "0 0:10 0:30 1:00 2:00 3:00 4:00 5:00 6:00 7:00")))
>  	 ;; XXX: the following handles only up to 10 different
>  	 ;; effort values.
> @@ -7777,8 +7778,9 @@ (defun org-agenda-filter-completion-function (string _predicate &optional flag)
>  			  (org-agenda-get-represented-tags))))
>       ((member operator '("<" ">" "="))
>        (setq table (split-string
> -		   (or (cdr (assoc (concat org-effort-property "_ALL")
> -				   org-global-properties))
> +		   (or (cdr (assoc-string (concat org-effort-property "_ALL")
> +					  org-global-properties
> +					  t))
>  		       "0 0:10 0:30 1:00 2:00 3:00 4:00 5:00 6:00 7:00")
>  		   " +")))
>       (t (setq table nil)))
>
> base-commit: eac255d911e0793513b2e2f14b06b94194a04daf

Thanks! LGTM.

-- 
Nick



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

end of thread, other threads:[~2020-07-09  7:51 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-07-07  2:01 [PATCH] org: add property names from #+PROPERTY keywords to completion list Nick Dokos
2020-07-07  2:40 ` Nick Dokos
2020-07-07  4:23 ` Kyle Meyer
2020-07-07 12:44   ` Nick Dokos
2020-07-07 13:38   ` Nick Dokos
2020-07-07 16:48   ` Nick Dokos
2020-07-08  3:52     ` Kyle Meyer
2020-07-08 15:47       ` Nick Dokos
2020-07-08 16:07       ` Nick Dokos
2020-07-09  4:36         ` [PATCH] agenda: Fold case when retrieving user-configured effort values Kyle Meyer
2020-07-09  7:50           ` Nick Dokos

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.