org-ql icon indicating copy to clipboard operation
org-ql copied to clipboard

Transient "Sort by" fails with custom sort function

Open SterlingHooten opened this issue 3 years ago • 2 comments

Calling a transient menu with "v" in an org-select buffer and inputting a function name for a non-defined sorting method gives the error:

user-error: SORT must be either nil, one or a list of the defined sorting methods (see documentation), or a comparison function of two arguments.

I believe this is a problem with org-ql-view--complete-sort and specifically completing-read-multiple /always/ returning a list, so that the pcase (and (pred listp) sort) always returns t.

This then returns a list which is not entirely comprised of default sorting methods and the caller throws an error.

Below is an extremely hacky fix. Better might be for the caller to filter out the "defined sorting methods" and then try to work with whatever remains.

This also speaks to the inability of combining multiple user-defined sorting methods. Although the "defined sorting methods" can be combined in a list together, only one user-defined function is allowed (from my understanding). If the caller accepted a list, and could call arbitrary function names contained within that list, you wouldn't need the sorting logic in the pcase (I think).

(defun org-ql-view--complete-sort ()
  "Return value for `org-ql-view-sort' using completion."
  ;; Use space to separate sorting predicates, not comma.
  (let* ((crm-separator (rx space))
         (crm-local-completion-map (let ((map (copy-keymap crm-local-completion-map)))
                                     (define-key map (kbd "SPC") nil)
                                     map))
;; SWH put defined methods into a list to call in multiple locations
	 (default-sorts (list "buffer-order"
                                                     "date"
                                                     "deadline"
                                                     "priority"
                                                     "random"
                                                     "reverse"
                                                     "scheduled"
                                                     "todo"))
         (input (->> (completing-read-multiple "Sort by: "
                                               default-sorts
                                               nil nil (when org-ql-view-sort
                                                         (prin1-to-string org-ql-view-sort)))
            (--remove (equal "buffer-order" it)))
	  ))
    (pcase input
      ('nil nil)
      ((and
;; SWH Check if the list is longer than 1, which in present version is only acceptable if all are defined methods 
	(pred (lambda (x) (> (list-length x) 1)))
	sort)
       (mapcar #'intern sort))
      ((and
;; SWH Check if all the elements of the list were defined methods (I don't know a better way to do this)
	(pred (lambda (x) (seq-set-equal-p (seq-uniq (cl-union default-sorts x)) default-sorts)))
	    sort)
       (mapcar #'intern sort))
      (sort ;; One sorter.
       (intern (car sort))))))

SterlingHooten avatar Sep 07 '22 23:09 SterlingHooten

Yes, the handling of sort functions should be improved. Thanks for writing this up. I probably won't have time to work on this soon, but I intend to solve this well.

alphapapa avatar Sep 09 '22 18:09 alphapapa

Is this why sorting by just buffer-order alone works, but buffer-order priority date gets turned into s Sort By: (priority date)?

I thought maybe it was because I'm using vertico, but I disabled vertico-mode and got the same result.

ParetoOptimalDev avatar May 09 '23 15:05 ParetoOptimalDev