[patch] priorities range reversed

[patch] priorities range reversed

[Joe Corneli via General discussions about Org-mode.]

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

In the case of numeric priorities [#1] [#9] and so on, there is a test
that is reversed in org.el.  This appears twice with a slight variation.

;; Are we are less than the highest or greater than the lowest?
(or (< (upcase new) org-priority-highest)
    (> (upcase new) org-priority-lowest))

The test is, in itself (and in principle) just fine.  The problem is
that it then triggers an error, which is exactly the opposite of what’s
wanted!

The attached patch reverses the test so that we enter the error case
when the priority is *outside* of the range of acceptable priority tags.

Here’s some test data for reproducing the problem and testing the
solution (with the patch applied).

Evaluate:

(setq org-default-priority 2)
(setq org-highest-priority 9)
(setq org-lowest-priority 1)

* This is a test
(Run M-x org-priority RET on the above line.)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: priorities_reversed.patch --]
[-- Type: text/x-diff, Size: 1001 bytes --]

diff --git a/lisp/org.el b/lisp/org.el
index ce68f4692..bd6fd3146 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -11337,7 +11337,7 @@ or a character."
 		     (= (upcase org-priority-lowest) org-priority-lowest))
 	    (setq new (upcase new)))
 	  (cond ((equal new ?\s) (setq remove t))
-		((or (< (upcase new) org-priority-highest) (> (upcase new) org-priority-lowest))
+		((or (> (upcase new) org-priority-highest) (< (upcase new) org-priority-lowest))
 		 (user-error
 		  (if nump
 		      "Priority must be between `%s' and `%s'"
@@ -11364,8 +11364,8 @@ or a character."
 			    org-priority-default
 			  (1+ org-priority-default))))))
 	 (t (user-error "Invalid action")))
-	(when (or (< (upcase new) org-priority-highest)
-		  (> (upcase new) org-priority-lowest))
+	(when (or (> (upcase new) org-priority-highest)
+		  (< (upcase new) org-priority-lowest))
 	  (if (and (memq action '(up down))
 		   (not have) (not (eq last-command this-command)))
 	      ;; `new' is from default priority

[-- Attachment #3: Type: text/plain, Size: 280 bytes --]



-- 
Dr Joseph A. Corneli (https://github.com/holtzermann17)

HYPERREAL ENTERPRISES LTD is a private company limited by shares, incorporated
25th, June 2019 as Company Number 634284 on the Register of Companies for
Scotland (https://beta.companieshouse.gov.uk/company/SC634284).

Re: [patch] priorities range reversed

[Joe Corneli via General discussions about Org-mode.]

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

Actually, there some bigger problems with the function as well...

— It didn’t update the priority cookie properly when one had been set before

* [#1] Exercise: update to [#2]

— After fixing that, I noticed that the included save-excursion doesn’t work
  (https://emacs.stackexchange.com/questions/7574/why-save-excursion-doesnt-save-point-position)

So, here’s a more comprehensive patch, including the previous changes
(in order to support numeric priorities) and then adjusting the
update/insert logic so that it makes more sense.

I did not adjust the indentation, so that you can more clearly see which
lines were changed.  Inline comments in the patch explain what’s going
on.


[-- Attachment #2: priorities_rearranged.patch --]
[-- Type: text/x-diff, Size: 2916 bytes --]

diff --git a/lisp/org.el b/lisp/org.el
index ce68f4692..69e333c84 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -11305,8 +11305,9 @@ or a character."
       (user-error "Priority commands are disabled"))
     (setq action (or action 'set))
     (let ((nump (< org-priority-lowest 65))
+          (saved-position (point))
 	  current new news have remove)
-      (save-excursion
+
 	(org-back-to-heading t)
 	(when (looking-at org-priority-regexp)
 	  (let ((ms (match-string 2)))
@@ -11337,7 +11338,7 @@ or a character."
 		     (= (upcase org-priority-lowest) org-priority-lowest))
 	    (setq new (upcase new)))
 	  (cond ((equal new ?\s) (setq remove t))
-		((or (< (upcase new) org-priority-highest) (> (upcase new) org-priority-lowest))
+		((or (> (upcase new) org-priority-highest) (< (upcase new) org-priority-lowest))
 		 (user-error
 		  (if nump
 		      "Priority must be between `%s' and `%s'"
@@ -11364,8 +11365,8 @@ or a character."
 			    org-priority-default
 			  (1+ org-priority-default))))))
 	 (t (user-error "Invalid action")))
-	(when (or (< (upcase new) org-priority-highest)
-		  (> (upcase new) org-priority-lowest))
+	(when (or (> (upcase new) org-priority-highest)
+		  (< (upcase new) org-priority-lowest))
 	  (if (and (memq action '(up down))
 		   (not have) (not (eq last-command this-command)))
 	      ;; `new' is from default priority
@@ -11377,12 +11378,26 @@ or a character."
 	;; Numerical priorities are limited to 64, beyond that number,
 	;; assume the priority cookie is a character.
 	(setq news (if (> new 64) (format "%c" new) (format "%s" new)))
+        ;; Actually setting the priority isn’t so straightforward
+        ;; There are several cases
 	(if have
+            ;; ① have and remove
 	    (if remove
 		(replace-match "" t t nil 1)
-	      (replace-match news t t nil 2))
+              ;; ② have and not remove, i.e., update
+	      (let ((case-fold-search nil)) (looking-at org-priority-regexp))
+              (if (match-end 2)
+		  (progn
+		    (goto-char (match-end 2))
+                    (delete-region (match-beginning 2) (match-end 2))
+		    (insert news))
+	        (goto-char (match-end 1))
+	        (insert "[#" news "] "))
+              )
+          ;; ③ don’t have and remove — nonsense
 	  (if remove
 	      (user-error "No priority cookie found in line")
+            ;; ④ don’t have and not remove, i.e., insert
 	    (let ((case-fold-search nil)) (looking-at org-todo-line-regexp))
 	    (if (match-end 2)
 		(progn
@@ -11390,7 +11405,8 @@ or a character."
 		  (insert " [#" news "]"))
 	      (goto-char (match-beginning 3))
 	      (insert "[#" news "] "))))
-	(org-align-tags))
+	(org-align-tags)
+      (goto-char saved-position)
       (if remove
 	  (message "Priority removed")
 	(message "Priority of current item set to %s" news)))))

[-- Attachment #3: priorities_rearranged.patch --]
[-- Type: text/x-diff, Size: 2916 bytes --]

diff --git a/lisp/org.el b/lisp/org.el
index ce68f4692..69e333c84 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -11305,8 +11305,9 @@ or a character."
       (user-error "Priority commands are disabled"))
     (setq action (or action 'set))
     (let ((nump (< org-priority-lowest 65))
+          (saved-position (point))
 	  current new news have remove)
-      (save-excursion
+
 	(org-back-to-heading t)
 	(when (looking-at org-priority-regexp)
 	  (let ((ms (match-string 2)))
@@ -11337,7 +11338,7 @@ or a character."
 		     (= (upcase org-priority-lowest) org-priority-lowest))
 	    (setq new (upcase new)))
 	  (cond ((equal new ?\s) (setq remove t))
-		((or (< (upcase new) org-priority-highest) (> (upcase new) org-priority-lowest))
+		((or (> (upcase new) org-priority-highest) (< (upcase new) org-priority-lowest))
 		 (user-error
 		  (if nump
 		      "Priority must be between `%s' and `%s'"
@@ -11364,8 +11365,8 @@ or a character."
 			    org-priority-default
 			  (1+ org-priority-default))))))
 	 (t (user-error "Invalid action")))
-	(when (or (< (upcase new) org-priority-highest)
-		  (> (upcase new) org-priority-lowest))
+	(when (or (> (upcase new) org-priority-highest)
+		  (< (upcase new) org-priority-lowest))
 	  (if (and (memq action '(up down))
 		   (not have) (not (eq last-command this-command)))
 	      ;; `new' is from default priority
@@ -11377,12 +11378,26 @@ or a character."
 	;; Numerical priorities are limited to 64, beyond that number,
 	;; assume the priority cookie is a character.
 	(setq news (if (> new 64) (format "%c" new) (format "%s" new)))
+        ;; Actually setting the priority isn’t so straightforward
+        ;; There are several cases
 	(if have
+            ;; ① have and remove
 	    (if remove
 		(replace-match "" t t nil 1)
-	      (replace-match news t t nil 2))
+              ;; ② have and not remove, i.e., update
+	      (let ((case-fold-search nil)) (looking-at org-priority-regexp))
+              (if (match-end 2)
+		  (progn
+		    (goto-char (match-end 2))
+                    (delete-region (match-beginning 2) (match-end 2))
+		    (insert news))
+	        (goto-char (match-end 1))
+	        (insert "[#" news "] "))
+              )
+          ;; ③ don’t have and remove — nonsense
 	  (if remove
 	      (user-error "No priority cookie found in line")
+            ;; ④ don’t have and not remove, i.e., insert
 	    (let ((case-fold-search nil)) (looking-at org-todo-line-regexp))
 	    (if (match-end 2)
 		(progn
@@ -11390,7 +11405,8 @@ or a character."
 		  (insert " [#" news "]"))
 	      (goto-char (match-beginning 3))
 	      (insert "[#" news "] "))))
-	(org-align-tags))
+	(org-align-tags)
+      (goto-char saved-position)
       (if remove
 	  (message "Priority removed")
 	(message "Priority of current item set to %s" news)))))

[-- Attachment #4: Type: text/plain, Size: 279 bytes --]


-- 
Dr Joseph A. Corneli (https://github.com/holtzermann17)

HYPERREAL ENTERPRISES LTD is a private company limited by shares, incorporated
25th, June 2019 as Company Number 634284 on the Register of Companies for
Scotland (https://beta.companieshouse.gov.uk/company/SC634284).

Re: [patch] priorities range reversed

[Timothy]

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

Hi Joe,

Thanks for looking into this and figuring out a patch. I’ve just taken a peek at
your patches, and I have two minor comments:
⁃ Why use saved-position instead of save-excursion
⁃ I’m think ascii versions of ② et. al would be preferable

Also, I notice that your patches are just diffs and don’t have a commit message.
Are you planning on writing them later?

Joe Corneli via “General discussions about Org-mode.” <emacs-orgmode@gnu.org> writes:

> Actually, there some bigger problems with the function as well…
>
> — It didn’t update the priority cookie properly when one had been set before
>
> * [#1] Exercise: update to [#2]
>
> — After fixing that, I noticed that the included save-excursion doesn’t work
>   (<https://emacs.stackexchange.com/questions/7574/why-save-excursion-doesnt-save-point-position>)
>
> So, here’s a more comprehensive patch, including the previous changes
> (in order to support numeric priorities) and then adjusting the
> update/insert logic so that it makes more sense.

All the best,
Timothy

Re: [patch] priorities range reversed

[Bastien]

Hi Joe,

Joe Corneli via "General discussions about Org-mode."
<emacs-orgmode@gnu.org> writes:

> In the case of numeric priorities [#1] [#9] and so on, there is a test
> that is reversed in org.el.  

See the docstring of `org-priority-highest':

  The highest priority of TODO items.
  
  A character like ?A, ?B, etc., or a numeric value like 1, 2, etc.
  
  The default is the character ?A, which is 65 as a numeric value.
  
  If you set ‘org-priority-highest’ to a numeric value inferior to
  65, Org assumes you want to use digits for the priority cookie.
  If you set it to >=65, Org assumes you want to use alphabetical
  characters.
  
  In both cases, the value of ‘org-priority-highest’ must be
  smaller than ‘org-priority-lowest’: for example, if "A" is the
  highest priority, it is smaller than the lowest "C" priority:
  65 < 67.

Are you sure the numeric value for `org-priority-highest' is smaller
than that of `org-priority-lowest'?

For numeric priorities, [#1] is always "higher" than [#2].

If I'm not wrong and if you think we can throw a more useful error
earlier to the useful, could you provide a patch for this?

Let me know if I'm missing something, thanks,

-- 
 Bastien

Re: [patch] priorities range reversed

[Bastien Guerry]

Hi Joe,

Bastien <bzg@gnu.org> writes:

> See the docstring of `org-priority-highest':

I'm discarding this patch right now - feel free to submit a bug report
if there is something I missed.

Thanks,

-- 
 Bastien