About org-sort -> org-sort-list with custom sort function

About org-sort -> org-sort-list with custom sort function

[Zhitao Gong]

Hi All,

I think there is a bug in org-sort or org-sort-list function.

If you call org-sort (C-c ^) on list items, this function will call
org-sort-list.  However, org-sort calls org-sort-list with only one
argument, i.e., the with-case (see the code below)

#+BEGIN_SRC emacs-lisp
((org-at-item-p) (org-call-with-arg 'org-sort-list with-case))
#+END_SRC emacs-lisp

The problem is that if you choose ?f (sorting with custom key function),
then org-sort-list expects another argument, the compare-func, which is
not passed to it.

IMHO, there are two ways to solve this

1. Ask for the compare-func in org-sort-list, as it does for the
   getkey-func.  A default value could be provided for compare-func,
   e.g., string<, <, etc.  Or
2. Restrict the return type to a string (or integer) so that we could
   fix the compare-func

-- 
gongzhitaao / 半緣脩道半緣君

Re: About org-sort -> org-sort-list with custom sort function

[Kyle Meyer]

Zhitao Gong <zhitaao.gong@gmail.com> writes:

> I think there is a bug in org-sort or org-sort-list function.
>
> If you call org-sort (C-c ^) on list items, this function will call
> org-sort-list.  However, org-sort calls org-sort-list with only one
> argument, i.e., the with-case (see the code below)
>
> #+BEGIN_SRC emacs-lisp
> ((org-at-item-p) (org-call-with-arg 'org-sort-list with-case))
> #+END_SRC emacs-lisp

org-sort actually isn't calling org-sort-list with one argument; it's
calling it interactively, while let-binding current-prefix-arg:

    (defsubst org-call-with-arg (command arg)
      "Call COMMAND interactively, but pretend prefix arg was ARG."
      (let ((current-prefix-arg arg)) (call-interactively command)))

I'm a bit confused about why org-call-with-arg is necessary because I
think call-interactively already propagates the current prefix argument,
but perhaps I'm missing some subtlety here.  Either way ...

> The problem is that if you choose ?f (sorting with custom key function),
> then org-sort-list expects another argument, the compare-func, which is
> not passed to it.
>
> IMHO, there are two ways to solve this
>
> 1. Ask for the compare-func in org-sort-list, as it does for the
>    getkey-func.  A default value could be provided for compare-func,
>    e.g., string<, <, etc.  Or
> 2. Restrict the return type to a string (or integer) so that we could
>    fix the compare-func

I see it as a documentation issue.  org-sort-list's docstring doesn't
make it clear which part of the description applies to an interactive
caller versus a Lisp caller.  An interactive caller can choose the ?f
sorting type, but they can't specify the compare-func.  Entries are
compared using sort-subr's default comparison behavior (see its
docstring), and getkey-func has to return a value that's compatible with
this behavior.

And I think it's OK to not expose compare-func to the interactive
caller.  In cases where sort-subr's default behavior won't do and a user
wants to supply a value for compare-func, they can create their own
command that wraps a non-interactive org-sort-list call.

What do you think?

-- 
Kyle

Re: About org-sort -> org-sort-list with custom sort function

[Nicolas Goaziou]

Hello,

Kyle Meyer <kyle@kyleam.com> writes:

> I'm a bit confused about why org-call-with-arg is necessary because I
> think call-interactively already propagates the current prefix argument,
> but perhaps I'm missing some subtlety here.  Either way ...

I thought the same. I think we can replace `org-call-with-arg' with
`call-interactively' in master.

>> The problem is that if you choose ?f (sorting with custom key function),
>> then org-sort-list expects another argument, the compare-func, which is
>> not passed to it.
>>
>> IMHO, there are two ways to solve this
>>
>> 1. Ask for the compare-func in org-sort-list, as it does for the
>>    getkey-func.  A default value could be provided for compare-func,
>>    e.g., string<, <, etc.  Or
>> 2. Restrict the return type to a string (or integer) so that we could
>>    fix the compare-func

I'd rather have 1. 

Actually, there's some confusion in `org-sort-list' about how
getkey-func and compare-func are handled. For example, the question
asked to bind GETKEY-FUNC is "Sort using function: ", which should
really be asked for COMPARE-FUNC.

IOW, we need to move this question to COMPARE-FUNC and ask a new one for
GETKEY-FUNC.


> And I think it's OK to not expose compare-func to the interactive
> caller.  In cases where sort-subr's default behavior won't do and a user
> wants to supply a value for compare-func, they can create their own
> command that wraps a non-interactive org-sort-list call.

I disagree. getkey-func and compare-func work hand in hand. You cannot
let users provide one but not the other without crippling functionality.

Regards,

-- 
Nicolas Goaziou

Re: About org-sort -> org-sort-list with custom sort function

[Kyle Meyer]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Kyle Meyer <kyle@kyleam.com> writes:
>
>> I'm a bit confused about why org-call-with-arg is necessary because I
>> think call-interactively already propagates the current prefix argument,
>> but perhaps I'm missing some subtlety here.  Either way ...
>
> I thought the same. I think we can replace `org-call-with-arg' with
> `call-interactively' in master.

OK, I'll have a closer look at which org-call-with-arg calls are
unnecessary.

> Actually, there's some confusion in `org-sort-list' about how
> getkey-func and compare-func are handled. For example, the question
> asked to bind GETKEY-FUNC is "Sort using function: ", which should
> really be asked for COMPARE-FUNC.
>
> IOW, we need to move this question to COMPARE-FUNC and ask a new one for
> GETKEY-FUNC.

True, at the very least, the prompt is confusingly phrased, but it's
probably an indication that the intended interface wasn't fully
implemented.

>> And I think it's OK to not expose compare-func to the interactive
>> caller.  In cases where sort-subr's default behavior won't do and a user
>> wants to supply a value for compare-func, they can create their own
>> command that wraps a non-interactive org-sort-list call.
>
> I disagree. getkey-func and compare-func work hand in hand. You cannot
> let users provide one but not the other without crippling functionality.

The interactive caller is certainly losing functionality, but my guess
was that sort-subr's default behavior is sufficient for handling the
keys returned by most custom key functions.  Probably a bad guess.

-- 
Kyle

Re: About org-sort -> org-sort-list with custom sort function

[Kyle Meyer]

Kyle Meyer <kyle@kyleam.com> writes:

> Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:
>
>> Kyle Meyer <kyle@kyleam.com> writes:
>>
>>> I'm a bit confused about why org-call-with-arg is necessary because I
>>> think call-interactively already propagates the current prefix argument,
>>> but perhaps I'm missing some subtlety here.  Either way ...
>>
>> I thought the same. I think we can replace `org-call-with-arg' with
>> `call-interactively' in master.
>
> OK, I'll have a closer look at which org-call-with-arg calls are
> unnecessary.

Grepping around, it looks like org-sort is the only place that uses
org-call-with-arg to send the argument through untouched.

Though, on second thought, using org-call-with-arg here rather than
call-interactively means that

  * the byte-compiler doesn't warn about WITH-CASE being unused. (I
    think marking it with an underscore would be confusing because it is
    "used".)

  * WITH-CASE isn't ignored when passed by a Lisp caller.

-- 
Kyle

Re: About org-sort -> org-sort-list with custom sort function

[Nicolas Goaziou]

Hello,

Kyle Meyer <kyle@kyleam.com> writes:

>   * the byte-compiler doesn't warn about WITH-CASE being unused. (I
>     think marking it with an underscore would be confusing because it is
>     "used".)

WITH-CASE should be optional. It conforms to (interactive "P")
expectations.

>   * WITH-CASE isn't ignored when passed by a Lisp caller.

The we may not need `call-interactively' at all. 

WDYT?

Regards,

-- 
Nicolas Goaziou

Re: About org-sort -> org-sort-list with custom sort function

[Kyle Meyer]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> The we may not need `call-interactively' at all. 
>
> WDYT?

Yeah, I agree that there's no need for call-interactively here because
the interactive forms of org-table-sort-lines, org-sort-list,
org-sort-entries are covered by org-sort's.

Switched call-interactively to funcall in c1addc825.

-- 
Kyle

Re: About org-sort -> org-sort-list with custom sort function

[Nicolas Goaziou]

Hello,

Kyle Meyer <kyle@kyleam.com> writes:

> Yeah, I agree that there's no need for call-interactively here because
> the interactive forms of org-table-sort-lines, org-sort-list,
> org-sort-entries are covered by org-sort's.
>
> Switched call-interactively to funcall in c1addc825.

Thank you. 

I think the patch can also go into maint branch, since this is a fix.

Now, we still need to find interactive use of `org-sort-list', which
should ask appropriate questions for getkey-func and compare-func.

Regards,

-- 
Nicolas Goaziou                                                0x80A93738

Re: About org-sort -> org-sort-list with custom sort function

[Kyle Meyer]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> I think the patch can also go into maint branch, since this is a fix.

It's more of a cosmetic change, no?  (The one "fix" would be that
org-sort documents WITH-CASE as optional even though it was actually a
required argument.)

Anyway, I'll cherry pick it over the next time I touch maint.

> Now, we still need to find interactive use of `org-sort-list', which
> should ask appropriate questions for getkey-func and compare-func.

Sure, I'll send a patch proposing changes (though it may be a day or two
before I get around to it).

-- 
Kyle

Re: About org-sort -> org-sort-list with custom sort function

[Nicolas Goaziou]

Kyle Meyer <kyle@kyleam.com> writes:

> Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:
>
>> I think the patch can also go into maint branch, since this is a fix.
>
> It's more of a cosmetic change, no?  (The one "fix" would be that
> org-sort documents WITH-CASE as optional even though it was actually a
> required argument.)

You're right. It doesn't really matter then.

Regards,

Re: About org-sort -> org-sort-list with custom sort function

[Kyle Meyer]

Kyle Meyer <kyle@kyleam.com> writes:

> Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:
>
>> The we may not need `call-interactively' at all. 
>>
>> WDYT?
>
> Yeah, I agree that there's no need for call-interactively here because
> the interactive forms of org-table-sort-lines, org-sort-list,
> org-sort-entries are covered by org-sort's.
>
> Switched call-interactively to funcall in c1addc825.

Ehh, I should have looked more closely at org-table-sort-lines.  Unlike
org-sort-entries and org-sort-list, it uses called-interactively-p to
determine whether it should prompt the user.  I've put the
org-call-with-arg back, at least for now.

-- 
Kyle

[PATCH] org-sort: Read compare-func in interactive calls

[Kyle Meyer]

* lisp/org-macs.el (org-read-function): New function.
* lisp/org.el (org-sort-entries):
* lisp/org-table.el (org-table-sort-lines):
* lisp/org-list.el (org-sort-list): Read COMPARE-FUNC when called
interactively rather than being restricted to the default behavior of
sort-subr's PREDICATE parameter.  Guard prompts for GETKEY-FUNC and
COMPARE-FUNCTION with called-interactively-p, like
org-table-sort-lines already did for GETKEY-FUNC.

Suggested-by: Zhitao Gong <zhitaao.gong@gmail.com>
<https://lists.gnu.org/archive/html/emacs-orgmode/2017-05/msg00040.html>
---
 lisp/org-list.el  | 35 +++++++++++++++++++++--------------
 lisp/org-macs.el  | 10 ++++++++++
 lisp/org-table.el | 20 +++++++++++---------
 lisp/org.el       | 44 ++++++++++++++++++++++++++------------------
 4 files changed, 68 insertions(+), 41 deletions(-)

diff --git a/lisp/org-list.el b/lisp/org-list.el
index b49bff8b9..17ff5d160 100644
--- a/lisp/org-list.el
+++ b/lisp/org-list.el
@@ -2863,9 +2863,8 @@ (defun org-sort-list (&optional with-case sorting-type getkey-func compare-func)
 
 If the SORTING-TYPE is ?f or ?F, then GETKEY-FUNC specifies
 a function to be called with point at the beginning of the
-record.  It must return either a string or a number that should
-serve as the sorting key for that record.  It will then use
-COMPARE-FUNC to compare entries.
+record.  It must return a value that is compatible with COMPARE-FUNC,
+the function used to compare entries.
 
 Sorting is done against the visible part of the headlines, it
 ignores hidden links."
@@ -2881,23 +2880,31 @@ (defun org-sort-list (&optional with-case sorting-type getkey-func compare-func)
 		(message
 		 "Sort plain list: [a]lpha  [n]umeric  [t]ime  [f]unc  [x]checked  A/N/T/F/X means reversed:")
 		(read-char-exclusive))))
+	 (dcst (downcase sorting-type))
 	 (getkey-func
-	  (or getkey-func
-	      (and (= (downcase sorting-type) ?f)
-		   (intern (completing-read "Sort using function: "
-					    obarray 'fboundp t nil nil))))))
+	  (and (= dcst ?f)
+	       (or getkey-func
+		   (and (called-interactively-p 'any)
+			(org-read-function "Function for extracting keys: "))
+		   (error "Missing key extractor"))))
+	 (sort-func
+	  (cond
+	   ((= dcst ?a) #'string<)
+	   ((= dcst ?f)
+	    (or compare-func
+		(and (called-interactively-p 'any)
+		     (org-read-function
+		      (concat "Function for comparing keys"
+			      "(empty for default `sort-subr' predicate): ")
+		      'allow-empty))))
+	   ((= dcst ?t) #'<)
+	   ((= dcst ?x) #'string<))))
     (message "Sorting items...")
     (save-restriction
       (narrow-to-region start end)
       (goto-char (point-min))
-      (let* ((dcst (downcase sorting-type))
-	     (case-fold-search nil)
+      (let* ((case-fold-search nil)
 	     (now (current-time))
-	     (sort-func (cond
-			 ((= dcst ?a) 'string<)
-			 ((= dcst ?f) compare-func)
-			 ((= dcst ?t) '<)
-			 ((= dcst ?x) 'string<)))
 	     (next-record (lambda ()
 			    (skip-chars-forward " \r\t\n")
 			    (or (eobp) (beginning-of-line))))
diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index e4b39a2c2..ca47e5a5a 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -294,6 +294,16 @@ (defun org-unbracket-string (pre post string)
       (substring string (length pre) (- (length post)))
     string))
 
+(defun org-read-function (prompt &optional allow-empty?)
+  "Prompt for a function.
+If ALLOW-EMPTY? is non-nil, return nil rather than raising an
+error when the user input is empty."
+  (let ((func (completing-read prompt obarray #'fboundp t)))
+    (cond ((not (string= func ""))
+	   (intern func))
+	  (allow-empty? nil)
+	  (t (user-error "Empty input is not valid")))))
+
 (provide 'org-macs)
 
 ;;; org-macs.el ends here
diff --git a/lisp/org-table.el b/lisp/org-table.el
index 84e2b4d4e..d37edbe83 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -1671,11 +1671,9 @@ (defun org-table-sort-lines (with-case &optional sorting-type getkey-func compar
 sorting should be done in reverse order.
 
 If the SORTING-TYPE is ?f or ?F, then GETKEY-FUNC specifies
-a function to be called to extract the key.  It must return either
-a string or a number that should serve as the sorting key for that
-row.  It will then use COMPARE-FUNC to compare entries.  If GETKEY-FUNC
-is specified interactively, the comparison will be either a string or
-numeric compare based on the type of the first key in the table."
+a function to be called to extract the key.  It must return a value
+that is compatible with COMPARE-FUNC, the function used to compare
+entries."
   (interactive "P")
   (when (org-region-active-p) (goto-char (region-beginning)))
   ;; Point must be either within a field or before a data line.
@@ -1735,16 +1733,20 @@ (defun org-table-sort-lines (with-case &optional sorting-type getkey-func compar
 		((?f ?F)
 		 (or getkey-func
 		     (and (called-interactively-p 'any)
-			  (intern
-			   (completing-read "Sort using function: "
-					    obarray #'fboundp t)))
+			  (org-read-function "Function for extracting keys: "))
 		     (error "Missing key extractor to sort rows")))
 		(t (user-error "Invalid sorting type `%c'" sorting-type))))
 	     (predicate
 	      (cl-case sorting-type
 		((?n ?N ?t ?T) #'<)
 		((?a ?A) #'string<)
-		((?f ?F) compare-func))))
+		((?f ?F)
+		 (or compare-func
+		     (and (called-interactively-p 'any)
+			  (org-read-function
+			   (concat "Fuction for comparing keys "
+				   "(empty for default `sort-subr' predicate): ")
+			   'allow-empty)))))))
 	(goto-char (point-min))
 	(sort-subr (memq sorting-type '(?A ?N ?T ?F))
 		   (lambda ()
diff --git a/lisp/org.el b/lisp/org.el
index 20f130478..251b19cb7 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -9120,8 +9120,9 @@ (defun org-sort-entries
 Capital letters will reverse the sort order.
 
 If the SORTING-TYPE is ?f or ?F, then GETKEY-FUNC specifies a function to be
-called with point at the beginning of the record.  It must return either
-a string or a number that should serve as the sorting key for that record.
+called with point at the beginning of the record.  It must return a
+value that is compatible with COMPARE-FUNC, the function used to
+compare entries.
 
 Comparing entries ignores case by default.  However, with an optional argument
 WITH-CASE, the sorting considers case as well.
@@ -9199,21 +9200,22 @@ (defun org-sort-entries
                [t]ime [s]cheduled  [d]eadline  [c]reated  cloc[k]ing
                A/N/P/R/O/F/T/S/D/C/K means reversed:"
        what)
-      (setq sorting-type (read-char-exclusive))
-
-      (unless getkey-func
-	(and (= (downcase sorting-type) ?f)
-	     (setq getkey-func
-		   (completing-read "Sort using function: "
-				    obarray 'fboundp t nil nil))
-	     (setq getkey-func (intern getkey-func))))
-
-      (and (= (downcase sorting-type) ?r)
-	   (not property)
-           (setq property
-                 (completing-read "Property: "
-				  (mapcar #'list (org-buffer-property-keys t))
-				  nil t))))
+      (setq sorting-type (read-char-exclusive)))
+
+    (unless getkey-func
+      (and (= (downcase sorting-type) ?f)
+	   (setq getkey-func
+		 (or (and (called-interactively-p 'any)
+			  (org-read-function
+			   "Function for extracting keys: "))
+		     (error "Missing key extractor")))))
+
+    (and (= (downcase sorting-type) ?r)
+	 (not property)
+	 (setq property
+	       (completing-read "Property: "
+				(mapcar #'list (org-buffer-property-keys t))
+				nil t)))
 
     (when (member sorting-type '(?k ?K)) (org-clock-sum))
     (message "Sorting entries...")
@@ -9297,7 +9299,13 @@ (defun org-sort-entries
          nil
          (cond
           ((= dcst ?a) 'string<)
-          ((= dcst ?f) compare-func)
+          ((= dcst ?f)
+	   (or compare-func
+	       (and (called-interactively-p 'any)
+		    (org-read-function
+		     (concat "Function for comparing keys "
+			     "(empty for default `sort-subr' predicate): ")
+		     'allow-empty))))
           ((member dcst '(?p ?t ?s ?d ?c ?k)) '<)))))
     (run-hooks 'org-after-sorting-entries-or-items-hook)
     ;; Reset the clock marker if needed
-- 
2.12.2

Re: [PATCH] org-sort: Read compare-func in interactive calls

[Nicolas Goaziou]

Hello,

Kyle Meyer <kyle@kyleam.com> writes:

> * lisp/org-macs.el (org-read-function): New function.
> * lisp/org.el (org-sort-entries):
> * lisp/org-table.el (org-table-sort-lines):
> * lisp/org-list.el (org-sort-list): Read COMPARE-FUNC when called
> interactively rather than being restricted to the default behavior of
> sort-subr's PREDICATE parameter.  Guard prompts for GETKEY-FUNC and
> COMPARE-FUNCTION with called-interactively-p, like
> org-table-sort-lines already did for GETKEY-FUNC.

Thank you. I have but one comment.
> +	 (sort-func
> +	  (cond
> +	   ((= dcst ?a) #'string<)
> +	   ((= dcst ?f)
> +	    (or compare-func
> +		(and (called-interactively-p 'any)

The above should be avoided. See `called-interactively-p' docstring. The
same applies in other places.

Regards,

-- 
Nicolas Goaziou

Re: [PATCH] org-sort: Read compare-func in interactive calls

[Kyle Meyer]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

>> * lisp/org-list.el (org-sort-list): Read COMPARE-FUNC when called
>> interactively rather than being restricted to the default behavior of
>> sort-subr's PREDICATE parameter.  Guard prompts for GETKEY-FUNC and
>> COMPARE-FUNCTION with called-interactively-p, like
>> org-table-sort-lines already did for GETKEY-FUNC.
>
> Thank you. I have but one comment.

Thanks for taking a look.

>> +	 (sort-func
>> +	  (cond
>> +	   ((= dcst ?a) #'string<)
>> +	   ((= dcst ?f)
>> +	    (or compare-func
>> +		(and (called-interactively-p 'any)
>
> The above should be avoided. See `called-interactively-p' docstring. The
> same applies in other places.

Yeah, in order to make org-sort-entries, org-sort-list, and
org-table-sort-lines behave the same way with respect to how they read
getkey-func and compare-func, I ignored the warning in
called-interactively-p's docstring that says it can be "brittle" if a
function is advised or being debugged.

Instead of adding called-interactively-p to org-sort-entries and
org-sort-list, I initially looked at removing called-interactively-p
from org-table-sort-lines, but I think that would 1) require changing
its behavior for determining the sorting column, and 2) going against
org-table-sort-lines's docstring that says no prompting will take place
if called from Lisp.

So I'm fine removing called-interactively-p from org-table-sort-lines,
but I'm not sure how it should behave, particularly with respect to the
column prompt.

Thoughts?

-- 
Kyle

Re: [PATCH] org-sort: Read compare-func in interactive calls

[Nicolas Goaziou]

Hello,

Kyle Meyer <kyle@kyleam.com> writes:

> Yeah, in order to make org-sort-entries, org-sort-list, and
> org-table-sort-lines behave the same way with respect to how they read
> getkey-func and compare-func, I ignored the warning in
> called-interactively-p's docstring that says it can be "brittle" if a
> function is advised or being debugged.
>
> Instead of adding called-interactively-p to org-sort-entries and
> org-sort-list, I initially looked at removing called-interactively-p
> from org-table-sort-lines, but I think that would 1) require changing
> its behavior for determining the sorting column, and 2) going against
> org-table-sort-lines's docstring that says no prompting will take place
> if called from Lisp.
>
> So I'm fine removing called-interactively-p from org-table-sort-lines,
> but I'm not sure how it should behave, particularly with respect to the
> column prompt.
>
> Thoughts?

Couldn't we use (interactive "p") instead, as suggested in
`called-interactively-p' docstring, in order to tell if we need to ask
for a function or not?

Regards,

-- 
Nicolas Goaziou

Re: [PATCH v2] org-sort: Read compare-func in interactive calls

[Kyle Meyer]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Kyle Meyer <kyle@kyleam.com> writes:

[...]

>> So I'm fine removing called-interactively-p from org-table-sort-lines,
>> but I'm not sure how it should behave, particularly with respect to the
>> column prompt.
>>
>> Thoughts?
>
> Couldn't we use (interactive "p") instead, as suggested in
> `called-interactively-p' docstring, in order to tell if we need to ask
> for a function or not?

I think (interactive "p"), or (interactive "P\np"), would be undesirable
because we'd be 1) changing the call signatures in a way that's not
backward compatible and 2) positioning an argument that shouldn't
concern most users toward the front of the argument list.

But the below patch is similar in spirit.

-- >8 --
Subject: [PATCH v2] org-sort: Read compare-func in interactive calls

* lisp/org-macs.el (org-read-function): New function.
* lisp/org-table.el (org-table-sort-lines): Make WITH-CASE an optional
argument to match org-sort-entries and org-sort-list.
* lisp/org.el (org-sort-entries):
* lisp/org-table.el (org-table-sort-lines):
* lisp/org-list.el (org-sort-list): Read COMPARE-FUNC when called
interactively rather than being restricted to the default behavior of
sort-subr's PREDICATE parameter.  Only prompt for for GETKEY-FUNC and
COMPARE-FUNC during an interactive call, like org-table-sort-lines
already did for GETKEY-FUNC, but use an argument rather than relying
on the brittle called-interactively-p.

Suggested-by: Zhitao Gong <zhitaao.gong@gmail.com>
<https://lists.gnu.org/archive/html/emacs-orgmode/2017-05/msg00040.html>
---
 lisp/org-list.el  | 45 ++++++++++++++++++++++++++++-----------------
 lisp/org-macs.el  | 10 ++++++++++
 lisp/org-table.el | 32 +++++++++++++++++++-------------
 lisp/org.el       | 54 +++++++++++++++++++++++++++++++++---------------------
 4 files changed, 90 insertions(+), 51 deletions(-)

diff --git a/lisp/org-list.el b/lisp/org-list.el
index b49bff8b9..92c57ae5e 100644
--- a/lisp/org-list.el
+++ b/lisp/org-list.el
@@ -2837,7 +2837,8 @@ (defun org-cycle-item-indentation ()
 	   (t (user-error "Cannot move item"))))
 	t))))
 
-(defun org-sort-list (&optional with-case sorting-type getkey-func compare-func)
+(defun org-sort-list
+    (&optional with-case sorting-type getkey-func compare-func interactive?)
   "Sort list items.
 The cursor may be at any item of the list that should be sorted.
 Sublists are not sorted.  Checkboxes, if any, are ignored.
@@ -2863,13 +2864,15 @@ (defun org-sort-list (&optional with-case sorting-type getkey-func compare-func)
 
 If the SORTING-TYPE is ?f or ?F, then GETKEY-FUNC specifies
 a function to be called with point at the beginning of the
-record.  It must return either a string or a number that should
-serve as the sorting key for that record.  It will then use
-COMPARE-FUNC to compare entries.
+record.  It must return a value that is compatible with COMPARE-FUNC,
+the function used to compare entries.
 
 Sorting is done against the visible part of the headlines, it
-ignores hidden links."
-  (interactive "P")
+ignores hidden links.
+
+A non-nil value for INTERACTIVE? is used to signal that this
+function is being called interactively."
+  (interactive (list current-prefix-arg nil nil nil t))
   (let* ((case-func (if with-case 'identity 'downcase))
          (struct (org-list-struct))
          (prevs (org-list-prevs-alist struct))
@@ -2881,23 +2884,31 @@ (defun org-sort-list (&optional with-case sorting-type getkey-func compare-func)
 		(message
 		 "Sort plain list: [a]lpha  [n]umeric  [t]ime  [f]unc  [x]checked  A/N/T/F/X means reversed:")
 		(read-char-exclusive))))
+	 (dcst (downcase sorting-type))
 	 (getkey-func
-	  (or getkey-func
-	      (and (= (downcase sorting-type) ?f)
-		   (intern (completing-read "Sort using function: "
-					    obarray 'fboundp t nil nil))))))
+	  (and (= dcst ?f)
+	       (or getkey-func
+		   (and interactive?
+			(org-read-function "Function for extracting keys: "))
+		   (error "Missing key extractor"))))
+	 (sort-func
+	  (cond
+	   ((= dcst ?a) #'string<)
+	   ((= dcst ?f)
+	    (or compare-func
+		(and interactive?
+		     (org-read-function
+		      (concat "Function for comparing keys"
+			      "(empty for default `sort-subr' predicate): ")
+		      'allow-empty))))
+	   ((= dcst ?t) #'<)
+	   ((= dcst ?x) #'string<))))
     (message "Sorting items...")
     (save-restriction
       (narrow-to-region start end)
       (goto-char (point-min))
-      (let* ((dcst (downcase sorting-type))
-	     (case-fold-search nil)
+      (let* ((case-fold-search nil)
 	     (now (current-time))
-	     (sort-func (cond
-			 ((= dcst ?a) 'string<)
-			 ((= dcst ?f) compare-func)
-			 ((= dcst ?t) '<)
-			 ((= dcst ?x) 'string<)))
 	     (next-record (lambda ()
 			    (skip-chars-forward " \r\t\n")
 			    (or (eobp) (beginning-of-line))))
diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index e4b39a2c2..ca47e5a5a 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -294,6 +294,16 @@ (defun org-unbracket-string (pre post string)
       (substring string (length pre) (- (length post)))
     string))
 
+(defun org-read-function (prompt &optional allow-empty?)
+  "Prompt for a function.
+If ALLOW-EMPTY? is non-nil, return nil rather than raising an
+error when the user input is empty."
+  (let ((func (completing-read prompt obarray #'fboundp t)))
+    (cond ((not (string= func ""))
+	   (intern func))
+	  (allow-empty? nil)
+	  (t (user-error "Empty input is not valid")))))
+
 (provide 'org-macs)
 
 ;;; org-macs.el ends here
diff --git a/lisp/org-table.el b/lisp/org-table.el
index 84e2b4d4e..40a715aeb 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -1647,7 +1647,8 @@ (defun org-table-kill-row ()
 			      dline -1 dline))))
 
 ;;;###autoload
-(defun org-table-sort-lines (with-case &optional sorting-type getkey-func compare-func)
+(defun org-table-sort-lines
+    (&optional with-case sorting-type getkey-func compare-func interactive?)
   "Sort table lines according to the column at point.
 
 The position of point indicates the column to be used for
@@ -1671,12 +1672,13 @@ (defun org-table-sort-lines (with-case &optional sorting-type getkey-func compar
 sorting should be done in reverse order.
 
 If the SORTING-TYPE is ?f or ?F, then GETKEY-FUNC specifies
-a function to be called to extract the key.  It must return either
-a string or a number that should serve as the sorting key for that
-row.  It will then use COMPARE-FUNC to compare entries.  If GETKEY-FUNC
-is specified interactively, the comparison will be either a string or
-numeric compare based on the type of the first key in the table."
-  (interactive "P")
+a function to be called to extract the key.  It must return a value
+that is compatible with COMPARE-FUNC, the function used to compare
+entries.
+
+A non-nil value for INTERACTIVE? is used to signal that this
+function is being called interactively."
+  (interactive (list current-prefix-arg nil nil nil t))
   (when (org-region-active-p) (goto-char (region-beginning)))
   ;; Point must be either within a field or before a data line.
   (save-excursion
@@ -1686,7 +1688,7 @@ (defun org-table-sort-lines (with-case &optional sorting-type getkey-func compar
   ;; Set appropriate case sensitivity and column used for sorting.
   (let ((column (let ((c (org-table-current-column)))
 		  (cond ((> c 0) c)
-			((called-interactively-p 'any)
+			(interactive?
 			 (read-number "Use column N for sorting: "))
 			(t 1))))
 	(sorting-type
@@ -1734,17 +1736,21 @@ (defun org-table-sort-lines (with-case &optional sorting-type getkey-func compar
 			 (t 0))))
 		((?f ?F)
 		 (or getkey-func
-		     (and (called-interactively-p 'any)
-			  (intern
-			   (completing-read "Sort using function: "
-					    obarray #'fboundp t)))
+		     (and interactive?
+			  (org-read-function "Function for extracting keys: "))
 		     (error "Missing key extractor to sort rows")))
 		(t (user-error "Invalid sorting type `%c'" sorting-type))))
 	     (predicate
 	      (cl-case sorting-type
 		((?n ?N ?t ?T) #'<)
 		((?a ?A) #'string<)
-		((?f ?F) compare-func))))
+		((?f ?F)
+		 (or compare-func
+		     (and interactive?
+			  (org-read-function
+			   (concat "Fuction for comparing keys "
+				   "(empty for default `sort-subr' predicate): ")
+			   'allow-empty)))))))
 	(goto-char (point-min))
 	(sort-subr (memq sorting-type '(?A ?N ?T ?F))
 		   (lambda ()
diff --git a/lisp/org.el b/lisp/org.el
index 20f130478..a400ba278 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -9090,7 +9090,8 @@ (defvar org-after-sorting-entries-or-items-hook nil
 will be in the first entry of the sorted region/list.")
 
 (defun org-sort-entries
-    (&optional with-case sorting-type getkey-func compare-func property)
+    (&optional with-case sorting-type getkey-func compare-func property
+	       interactive?)
   "Sort entries on a certain level of an outline tree.
 If there is an active region, the entries in the region are sorted.
 Else, if the cursor is before the first entry, sort the top-level items.
@@ -9120,8 +9121,9 @@ (defun org-sort-entries
 Capital letters will reverse the sort order.
 
 If the SORTING-TYPE is ?f or ?F, then GETKEY-FUNC specifies a function to be
-called with point at the beginning of the record.  It must return either
-a string or a number that should serve as the sorting key for that record.
+called with point at the beginning of the record.  It must return a
+value that is compatible with COMPARE-FUNC, the function used to
+compare entries.
 
 Comparing entries ignores case by default.  However, with an optional argument
 WITH-CASE, the sorting considers case as well.
@@ -9129,8 +9131,11 @@ (defun org-sort-entries
 Sorting is done against the visible part of the headlines, it ignores hidden
 links.
 
-When sorting is done, call `org-after-sorting-entries-or-items-hook'."
-  (interactive "P")
+When sorting is done, call `org-after-sorting-entries-or-items-hook'.
+
+A non-nil value for INTERACTIVE? is used to signal that this
+function is being called interactively."
+  (interactive (list current-prefix-arg nil nil nil nil t))
   (let ((case-func (if with-case 'identity 'downcase))
 	(cmstr
 	 ;; The clock marker is lost when using `sort-subr', let's
@@ -9199,21 +9204,22 @@ (defun org-sort-entries
                [t]ime [s]cheduled  [d]eadline  [c]reated  cloc[k]ing
                A/N/P/R/O/F/T/S/D/C/K means reversed:"
        what)
-      (setq sorting-type (read-char-exclusive))
-
-      (unless getkey-func
-	(and (= (downcase sorting-type) ?f)
-	     (setq getkey-func
-		   (completing-read "Sort using function: "
-				    obarray 'fboundp t nil nil))
-	     (setq getkey-func (intern getkey-func))))
-
-      (and (= (downcase sorting-type) ?r)
-	   (not property)
-           (setq property
-                 (completing-read "Property: "
-				  (mapcar #'list (org-buffer-property-keys t))
-				  nil t))))
+      (setq sorting-type (read-char-exclusive)))
+
+    (unless getkey-func
+      (and (= (downcase sorting-type) ?f)
+	   (setq getkey-func
+		 (or (and interactive?
+			  (org-read-function
+			   "Function for extracting keys: "))
+		     (error "Missing key extractor")))))
+
+    (and (= (downcase sorting-type) ?r)
+	 (not property)
+	 (setq property
+	       (completing-read "Property: "
+				(mapcar #'list (org-buffer-property-keys t))
+				nil t)))
 
     (when (member sorting-type '(?k ?K)) (org-clock-sum))
     (message "Sorting entries...")
@@ -9297,7 +9303,13 @@ (defun org-sort-entries
          nil
          (cond
           ((= dcst ?a) 'string<)
-          ((= dcst ?f) compare-func)
+          ((= dcst ?f)
+	   (or compare-func
+	       (and interactive?
+		    (org-read-function
+		     (concat "Function for comparing keys "
+			     "(empty for default `sort-subr' predicate): ")
+		     'allow-empty))))
           ((member dcst '(?p ?t ?s ?d ?c ?k)) '<)))))
     (run-hooks 'org-after-sorting-entries-or-items-hook)
     ;; Reset the clock marker if needed
-- 
2.12.2

Re: [PATCH v2] org-sort: Read compare-func in interactive calls

[Nicolas Goaziou]

Hello,

Kyle Meyer <kyle@kyleam.com> writes:

> I think (interactive "p"), or (interactive "P\np"), would be undesirable
> because we'd be 1) changing the call signatures in a way that's not
> backward compatible and 2) positioning an argument that shouldn't
> concern most users toward the front of the argument list.

I don't understand this. Why would using (interactive "p") instead of
(interactive "P") would be incompatible? AFAIU, the only difference is
how the argument value is treated within the callee.

Regards,

-- 
Nicolas Goaziou

Re: [PATCH v2] org-sort: Read compare-func in interactive calls

[Kyle Meyer]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Kyle Meyer <kyle@kyleam.com> writes:
>
>> I think (interactive "p"), or (interactive "P\np"), would be undesirable
>> because we'd be 1) changing the call signatures in a way that's not
>> backward compatible and 2) positioning an argument that shouldn't
>> concern most users toward the front of the argument list.
>
> I don't understand this. Why would using (interactive "p") instead of
> (interactive "P") would be incompatible?

I misunderstood, thinking you wanted to add an additional argument
rather than using (interactive "p") for WITH-CASE.

> AFAIU, the only difference is how the argument value is treated within
> the callee.

Won't using a numeric prefix argument change the behavior for both
interactive and Lisp calls?

As examples,

  * M-1 M-x org-sort-list is currently interpreted as a non-nil value
    for WITH-CASE.  Instead, it would be indistinguishable from M-x
    org-sort-list.

  * A Lisp caller can currently set WITH-CASE to any non-nil value.
    Using (interactive "p") for WITH-CASE, how do we distinguish a
    numeric argument passed for WITH-CASE from an interactive call?

Using an additional argument whose only purpose is to serve as a
interactive flag, which is what called-interactively-p's docstring
suggests, avoids these issues.

-- 
Kyle

Re: [PATCH v2] org-sort: Read compare-func in interactive calls

[Nicolas Goaziou]

Kyle Meyer <kyle@kyleam.com> writes:

> Won't using a numeric prefix argument change the behavior for both
> interactive and Lisp calls?
>
> As examples,
>
>   * M-1 M-x org-sort-list is currently interpreted as a non-nil value
>     for WITH-CASE.  Instead, it would be indistinguishable from M-x
>     org-sort-list.
>
>   * A Lisp caller can currently set WITH-CASE to any non-nil value.
>     Using (interactive "p") for WITH-CASE, how do we distinguish a
>     numeric argument passed for WITH-CASE from an interactive call?

OK. It seems I was pretty much confused.

> Using an additional argument whose only purpose is to serve as a
> interactive flag, which is what called-interactively-p's docstring
> suggests, avoids these issues.

I'd rather avoid this. What about using

  (not (or executing-kbd-macro noninteractive))

then, and not touch to signature?

Regards,

Re: [PATCH v2] org-sort: Read compare-func in interactive calls

[Kyle Meyer]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Kyle Meyer <kyle@kyleam.com> writes:

[...]

>> Using an additional argument whose only purpose is to serve as a
>> interactive flag, which is what called-interactively-p's docstring
>> suggests, avoids these issues.
>
> I'd rather avoid this. What about using
>
>   (not (or executing-kbd-macro noninteractive))
>
> then, and not touch to signature?

I'm confused why called-interactively-p's docstring suggests that form.
At any rate, it won't do here:

    (funcall (lambda ()
               (interactive)
               (not (or executing-kbd-macro noninteractive))))
    ⇒ t

    (call-interactively (lambda ()
                          (interactive)
                          (not (or executing-kbd-macro noninteractive))))
    ⇒ t

-- 
Kyle

Re: [PATCH v2] org-sort: Read compare-func in interactive calls

[Nicolas Goaziou]

Hello,

Kyle Meyer <kyle@kyleam.com> writes:

> I'm confused why called-interactively-p's docstring suggests that
> form.

I'm also confused. The more I read the docstring, the less I understand
it. Ah well.

> At any rate, it won't do here:
>
>     (funcall (lambda ()
>                (interactive)
>                (not (or executing-kbd-macro noninteractive))))
>     ⇒ t
>
>     (call-interactively (lambda ()
>                           (interactive)
>                           (not (or executing-kbd-macro noninteractive))))
>     ⇒ t

Ok. Let me drop the ball here, we both lost too much time on this
already. If your patch with the signature change is functional, just go
ahead.

Thank you.

Regards,

-- 
Nicolas Goaziou