Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ivy provides wrong candidate to :action with custom sort #1680

Closed
raxod502 opened this issue Jul 23, 2018 · 3 comments

Comments

@raxod502
Copy link
Contributor

commented Jul 23, 2018

This is a hairy one. Consider the following code:

;; -*- lexical-binding: t -*-

(defvar bootstrap-version)
(let ((bootstrap-file
       (expand-file-name "straight/repos/straight.el/bootstrap.el" user-emacs-directory))
      (bootstrap-version 5))
  (unless (file-exists-p bootstrap-file)
    (with-current-buffer
        (url-retrieve-synchronously
         "https://raw.githubusercontent.com/raxod502/straight.el/develop/install.el"
         'silent 'inhibit-cookies)
      (goto-char (point-max))
      (eval-print-last-sexp)))
  (load bootstrap-file nil 'nomessage))

(straight-use-package 'org)

(find-file (expand-file-name "test.org" temporary-file-directory))
(erase-buffer)
(insert
 "
* Headline 1
* Headline 2
* Headline 3
* Headline 4
* Headline 5
")
(save-buffer)

(straight-use-package 'ivy)
(ivy-mode +1)

(defun my-ivy-sort-function (x y)
  (if (consp x)
      (string> (car x) (car y))
    (string> x y)))

(setq ivy-sort-functions-alist '((t . my-ivy-sort-function)))

(require 'cl-lib)

(cl-defun my-ivy-advice (orig-fun prompt collection &rest kwargs &key action
                                  &allow-other-keys)
  (let* ((new-action
          (if (or (null action) (functionp action))
              (lambda (result)
                (message "Action result was: %S" result)
                (when action
                  (funcall action result)))
            ;; Don't bother handling case where action is a list.
            action))
         (result (apply orig-fun prompt collection :action new-action kwargs)))
    (message "Actual result was: %S" result)
    result))

(advice-add #'ivy-read :around #'my-ivy-advice)

(org-refile-get-location "Prompt")

(switch-to-buffer (messages-buffer))

[You can use this template in make plain from this repository, if you wish. I used straight.el because it was easier, but either method offers complete reproducibility.]

Run with:

$ emacs -Q -l org-capture-repro.el

and press RET. It then switches to *Messages*. Output:

For information about GNU Emacs and the GNU system, type C-h C-a.
Saving file /var/folders/z2/2q4hmwtn0kl4_9rkqqzg8zs00000gn/T/test.org...
Wrote /var/folders/z2/2q4hmwtn0kl4_9rkqqzg8zs00000gn/T/test.org
Getting targets...done
Action result was: (#("Headline 1" 0 1 (idx 4)) "/var/folders/z2/2q4hmwtn0kl4_9rkqqzg8zs00000gn/T/test.org" "^\\(\\*+\\)\\(?: +\\(DONE\\|TODO\\)\\)?\\(?: +\\(\\[#.\\]\\)\\)?\\(?: +\\(?:\\[[0-9%/]+\\] *\\)*\\(Headline 1\\)\\(?: *\\[[0-9%/]+\\]\\)*\\)\\(?:[ 	]+\\(:[[:alnum:]_@#%:]+:\\)\\)?[ 	]*$" 2)
Actual result was: "Headline 5"

Note that the candidate displayed after Action result is different from the one displayed after Actual result. This means that the candidate passed to the :action function is different from the one returned from ivy-read. The one returned from ivy-read is correct; however, the one passed to :action is wrong. It looks like Ivy sorts the collection using the custom sort function, assigns unique indexes to the candidates, and then (after a candidate is selected) looks up the index in the unsorted collection. Thus passing the wrong candidate to :action.

I spent two hours trying to untangle this, with no success. What is going on?

Revisions:

(("elpa" . "d9d04d151c636b4836d3789c445dc3cdf95621d4")
 ("epkgs" . "59ca0b0ae4b09d45579232ed4e729d514ab49747")
 ("melpa" . "2304869fc297723b84505b8a80d6d05854ea69c4")
 ("org" . "5057e398911daada95958a907cc75285ceb09729")
 ("straight.el" . "8bd2caac3ef363abc55a40d47cd38c7e71293481")
 ("swiper" . "92efd629b04c0e46614129d4408fee95eeee0cd5"))

Emacs version: 26.1 from Homebrew

raxod502 added a commit to raxod502/swiper that referenced this issue Jul 23, 2018
@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jul 23, 2018

Here's the same recipe, but simplified to be less intrusive and independent of package managers:

;;; foo.el -*- lexical-binding: t -*-

(require 'cl-lib)
(require 'org)

(require 'ivy)

(defun my-ivy-sort-function (x y)
  (if (consp x)
      (string> (car x) (car y))
    (string> x y)))

(cl-defun my-ivy-advice (orig-fun prompt collection &rest kwargs &key action
                                  &allow-other-keys)
  (let* ((ivy-sort-functions-alist '((t . my-ivy-sort-function)))
         (new-action
          (if (or (null action) (functionp action))
              (lambda (result)
                (message "Action result was: %S" result)
                (when action
                  (funcall action result)))
            ;; Don't bother handling case where action is a list.
            action))
         (result (apply orig-fun prompt collection :action new-action kwargs)))
    (message "Actual result was: %S" result)
    result))

(unwind-protect
    (with-temp-buffer
      (insert "
* Headline 1
* Headline 2
* Headline 3
* Headline 4
* Headline 5\n")
      (org-mode)
      (ivy-mode)
      (advice-add #'ivy-read :around #'my-ivy-advice)
      (ignore-errors (org-refile-get-location "Prompt")))
  (advice-remove #'ivy-read #'my-ivy-advice)
  (pop-to-buffer (messages-buffer)))

;;; foo.el ends here

This can be tested via emacs -Q -L /path/to/swiper/ -l foo.el followed by RET at the prompt.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jul 23, 2018

Judging from your description and workaround, it sounds like ivy--reset-state or similar is indexing candidates before sorting them. Sorry, I don't currently have time to look any deeper.

@abo-abo abo-abo closed this in 7f330fc Jul 23, 2018

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jul 23, 2018

Thanks, please test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.