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

Fix ivy-read-action #2082

Closed
wants to merge 1 commit into from
Closed

Fix ivy-read-action #2082

wants to merge 1 commit into from

Conversation

@ghost
Copy link

@ghost ghost commented May 28, 2019

The two action dispatching commands rely on the return value of ivy-read-action (ivy.el#L840), so #2079 makes those commands not working any more.

By the way, I do not have an Emacs CA yet, but I think this is a trivial change and does not require one?

Thanks!

Fix a bug introduced in #2079, which made action dispatching stop
working because those commands rely on the return value of
`ivy-read-action'.
(setcar actions (1+ action-idx))
(ivy-set-action actions))))))
(ivy-shrink-after-dispatching))
(prog1
Copy link
Collaborator

@basil-conto basil-conto May 28, 2019

Nitpick: I think it's pretty distasteful passing the entire function body as the first argument to prog1. Why not introduce a new local variable to hold the result, and return that after ivy-shrink-after-dispatching?

Copy link
Collaborator

@basil-conto basil-conto May 28, 2019

Actually, it's even simpler than that:

diff --git a/ivy.el b/ivy.el
index 04820e6..1f4edbb 100644
--- a/ivy.el
+++ b/ivy.el
@@ -819,16 +819,16 @@ ivy-read-action
                                       (cdr actions)))
                     (not (string= key (car (nth action-idx (cdr actions))))))
           (setq key (concat key (string (read-key hint)))))
-        (cond ((member key '("�" "�"))
-               nil)
-              ((null action-idx)
-               (message "%s is not bound" key)
-               nil)
-              (t
-               (message "")
-               (setcar actions (1+ action-idx))
-               (ivy-set-action actions))))))
-  (ivy-shrink-after-dispatching))
+        (prog1 (cond ((member key '("�" "�"))
+                      nil)
+                     ((null action-idx)
+                      (message "%s is not bound" key)
+                      nil)
+                     (t
+                      (message "")
+                      (setcar actions (1+ action-idx))
+                      (ivy-set-action actions)))
+          (ivy-shrink-after-dispatching))))))
 
 (defun ivy-shrink-after-dispatching ()
   "Shrink the window after dispatching when action list is too large."

Copy link
Collaborator

@basil-conto basil-conto May 28, 2019

Actually, it's even simpler than the simple version:

diff --git a/ivy.el b/ivy.el
index 04820e6..9bf357d 100644
--- a/ivy.el
+++ b/ivy.el
@@ -819,6 +819,7 @@ ivy-read-action
                                       (cdr actions)))
                     (not (string= key (car (nth action-idx (cdr actions))))))
           (setq key (concat key (string (read-key hint)))))
+        (ivy-shrink-after-dispatching)
         (cond ((member key '("�" "�"))
                nil)
               ((null action-idx)
@@ -827,8 +828,7 @@ ivy-read-action
               (t
                (message "")
                (setcar actions (1+ action-idx))
-               (ivy-set-action actions))))))
-  (ivy-shrink-after-dispatching))
+               (ivy-set-action actions)))))))
 
 (defun ivy-shrink-after-dispatching ()
   "Shrink the window after dispatching when action list is too large."

@abo-abo abo-abo closed this in 7e8622f May 29, 2019
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented May 29, 2019

Thanks all, I applied the final simple version of the patch.

astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
Fix a bug introduced in abo-abo#2079, which made action dispatching stop
working because those commands rely on the return value of
`ivy-read-action'.

Fixes abo-abo#2082
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants