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

Add ivy-display-schemes and use it. #1443

Closed
wants to merge 3 commits into from

Conversation

@tumashu
Copy link
Contributor

commented Feb 9, 2018

  • ivy.el (ivy-display-schemes): New variable.
    (ivy-read): Use ivy-dispatching-call to clean up.
    (ivy-display-scheme-call): New function.
@tumashu

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2018

I want to create a ivy-posframe which is very similar ivy-overlay, so I need this ivy feature :-)

@tumashu

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2018

This is a init version of ivy-posframe: https://github.com/tumashu/ivy-posframe/blob/master/ivy-posframe.el, you can as a test example,

8

ivy.el Outdated
'((ivy-overlay
:display ivy-display-function-overlay
:cleanup ivy-overlay-cleanup))
"An alist of display schemes.")

This comment has been minimized.

Copy link
@basil-conto

basil-conto Feb 9, 2018

Collaborator

How about providing a bit more information with something like "Map display schemes to their property list."

ivy.el Outdated
@@ -1661,7 +1667,8 @@ customizations apply to the current completion session."
collection))))
(ivy-display-function
(unless (window-minibuffer-p)
(cdr (assoc caller ivy-display-functions-alist)))))
(cdr (or (assoc caller ivy-display-functions-alist)
(assoc t ivy-display-functions-alist))))))

This comment has been minimized.

Copy link
@basil-conto

basil-conto Feb 9, 2018

Collaborator

You can use assq instead of assoc for both of these calls.

ivy.el Outdated
ivy-display-function))
(setq func (plist-get plist action)))))
(when (functionp func)
(funcall func))))

This comment has been minimized.

Copy link
@basil-conto

basil-conto Feb 9, 2018

Collaborator

Why not

(defun ivy-display-scheme-call (action)
  "Call ACTION property associated with `ivy-display-function'."
  (when ivy-display-function
    (let* ((scheme (cl-rassoc-if (lambda (plist)
                                   (eq (plist-get plist :display)
                                       ivy-display-function))
                                 ivy-display-schemes))
           (action (plist-get (cdr scheme) action)))
      (when (functionp action)
        (funcall action)))))

Guaranteed 100% untested.

@tumashu tumashu force-pushed the tumashu:master branch Feb 9, 2018

@tumashu

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2018

Fixed, please check again :-)

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Feb 9, 2018

Thanks. Wouldn't the following be slightly simpler and cleaner, without any loss of generality?

diff --git a/ivy.el b/ivy.el
index 4e91f7f..1f3e5b2 100644
--- a/ivy.el
+++ b/ivy.el
@@ -1661,7 +1661,8 @@ ivy-read
                                     collection))))
         (ivy-display-function
          (unless (window-minibuffer-p)
-           (cdr (assoc caller ivy-display-functions-alist)))))
+           (cdr (or (assq caller ivy-display-functions-alist)
+                    (assq t ivy-display-functions-alist))))))
     (setq ivy-last
           (make-ivy-state
            :prompt prompt
@@ -1721,8 +1722,9 @@ ivy-read
                                                (cdr (symbol-value hist))))))))
                  (ivy-state-current ivy-last)))
           (remove-hook 'post-command-hook #'ivy--queue-exhibit)
-          (when (eq ivy-display-function 'ivy-display-function-overlay)
-            (ivy-overlay-cleanup))
+          (let ((cleanup (ivy--display-function-prop :cleanup)))
+            (when (functionp cleanup)
+              (funcall cleanup)))
           (when (setq unwind (ivy-state-unwind ivy-last))
             (funcall unwind))
           (unless (eq ivy-exit 'done)
@@ -2398,6 +2400,15 @@ ivy-fixed-height-minibuffer
   :type 'boolean)
 
 ;;** Rest
+(defvar ivy--display-function-props
+  '((ivy-display-function-overlay :cleanup ivy-overlay-cleanup))
+  "Map `ivy-display-function' values to their property lists.")
+
+(defun ivy--display-function-prop (prop)
+  "Return PROP associated with current `ivy-display-function'."
+  (plist-get (cdr (assq ivy-display-function ivy--display-function-props))
+             prop))
+
 (defcustom ivy-truncate-lines t
   "Minibuffer setting for `truncate-lines'."
   :type 'boolean)
@abo-abo

This comment has been minimized.

Copy link
Owner

commented Feb 9, 2018

I like the plist-get version more. @tumashu could you please incorporate this change?

Allow ivy-display-functions-alist adding key: t as fallback
* ivy.el (ivy-read): Using function associated to t
  of ivy-display-functions-alist as fallback function.

@tumashu tumashu force-pushed the tumashu:master branch Feb 9, 2018

@tumashu

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2018

Changed, the different is:
use ivy-display-functions-props instead of ivy--display-function-props, for I think this variable will be used by other package, should not be a internal variable.

@basil-conto
Copy link
Collaborator

left a comment

Apart from these wording nitpicks, LGTM.

@@ -196,6 +196,12 @@ See https://github.com/abo-abo/swiper/wiki/ivy-display-function."
(const :tag "Popup" ivy-display-function-popup)
(const :tag "Overlay" ivy-display-function-overlay)))

(defvar ivy-display-functions-props

This comment has been minimized.

Copy link
@basil-conto

basil-conto Feb 9, 2018

Collaborator

I think ivy-display-function-props (singular) is better grammar than ivy-display-functions-props here.

ivy.el Outdated
'((ivy-display-function-overlay :cleanup ivy-overlay-cleanup))
"Map display functions of ivy to theirs action property lists.
These actions are functions which work closely with display functions,
for example: cleanup action functions.")

This comment has been minimized.

Copy link
@basil-conto

basil-conto Feb 9, 2018

Collaborator

I think it's better to write:

 "Map Ivy display functions to their property lists.
Examples of properties include associated `:cleanup' functions."

There's no reason to limit this alist to "action" functions.

@tumashu

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2018

I prefer to functions is:

  1. we have ivy-display-functions-alist :-)
  2. we want to use this variable to record all display-functions' s props.
@tumashu

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2018

There's no reason to limit this alist to "action" functions.

Right

@tumashu tumashu force-pushed the tumashu:master branch Feb 10, 2018

ivy.el Outdated
Note: this variable is used to config global display function,
If user want to set a command's display function, for example:
the display function used by ivy-completion-in-region, please
use `ivy-display-functions-alist' instead."

This comment has been minimized.

Copy link
@basil-conto

basil-conto Feb 10, 2018

Collaborator

This is a good opportunity to improve the whole docstring:

  "Determine where to display candidates.
When nil (the default), candidates are shown in the minibuffer.
Otherwise, this can be set to a function which takes a string
argument comprising the current matching candidates and displays
it somewhere.

This user option acts as a global default for Ivy-based
completion commands.  You can customize the display function on a
per-command basis via `ivy-display-functions-alist', which see.
See also URL
`https://github.com/abo-abo/swiper/wiki/ivy-display-function'."

This comment has been minimized.

Copy link
@tumashu

tumashu Feb 10, 2018

Author Contributor

cool

@tumashu tumashu force-pushed the tumashu:master branch Feb 10, 2018

(cdr (assoc caller ivy-display-functions-alist)))))
(or ivy-display-function
(cdr (or (assq caller ivy-display-functions-alist)
(assq t ivy-display-functions-alist)))))))

This comment has been minimized.

Copy link
@basil-conto

basil-conto Feb 10, 2018

Collaborator

Doesn't the first operand in (or ivy-display-function ...) make it impossible to customise ivy-display-function on a per-command basis?

This comment has been minimized.

Copy link
@tumashu

tumashu Feb 10, 2018

Author Contributor

If user want to config per-command function, they should use ivy-display-functions-alist and set ivy-display-function to nil.

This comment has been minimized.

Copy link
@basil-conto

basil-conto Feb 10, 2018

Collaborator

That is a bad idea for two important reasons:

  1. It is a breaking change.
  2. It makes ivy-display-function redundant for any setting other than nil, as the user will have to customise ivy-display-functions-alist for every single command they use.

This comment has been minimized.

Copy link
@basil-conto

basil-conto Feb 10, 2018

Collaborator

On further inspection, I'm not quite sure what the intended interplay between ivy-display-function and ivy-display-functions-alist is at the moment, so I retract my last comment and defer to @abo-abo for comment.

Some comments/questions/observations:

  • ivy-display-function is also used in ivy--insert-minibuffer.
  • Why do we check for window-minibuffer-p before let-binding ivy-display-function? Could this be simplified as a lookup into ivy-display-functions-alist, in which ivy-display-function is mapped to by the fallback key t?
  • If window-minibuffer-p is necessary, shouldn't it be (minibuffer-window-active-p (selected-window)) instead, seeing as there can be multiple minibuffer windows?

This comment has been minimized.

Copy link
@tumashu

tumashu Feb 10, 2018

Author Contributor

maybe ivy-display-functions-alist should override ivy-display-function :-)

but this seem to have other problem.

This comment has been minimized.

Copy link
@abo-abo

abo-abo Feb 11, 2018

Owner

We check window-minibuffer-p because the only viable display function (other than nil) at the time was ivy-display-function-overlay, which doesn't make sense to use when you press TAB while in the minibuffer.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Feb 10, 2018

I prefer to functions is:

  1. we have ivy-display-functions-alist :-)

Fair enough, I guess it can't be helped (except via varaliases, but that would be overkill).

  1. we want to use this variable to record all display-functions' s props.

Sure, but "display function properties" is correct grammar even when talking about more than one display function, whereas "display functions properties" is technically incorrect. Obviously this doesn't matter much in a programming context, I'm just pedantic. :)

@tumashu

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2018

Fair enough, I guess it can't be helped (except via varaliases, but that would be overkill).
...
Sure, but "display function properties" is correct grammar even when talking about more than one display function, whereas "display functions properties" is technically incorrect.

If no the problem I said, function is good too:-), for me, functions or function is no different for my poor English :-)

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Feb 10, 2018

If no the problem I said, function is good too

No I agree with you that it is more important to be consistent with ivy-display-functions-alist, as the issue is not important at all; I had just forgotten about ivy-display-functions-alist when I made my nitpick comment.

This is why I said "fair enough, I guess it can't be helped."

@tumashu tumashu force-pushed the tumashu:master branch Feb 10, 2018

@tumashu

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2018

...

tumashu added 2 commits Feb 9, 2018
Add ivy-display-functions-props and use it
* ivy.el (ivy--display-function-prop): New function.
(ivy-display-functions-props): New variable.
(ivy-read): Use ivy--display-function-prop to get cleanup function.
ivy-display-function will be used to set global display function.
* ivy.el (ivy-display-function): Update docstring, ivy-display-function is
used set global display function now.
(ivy-read): Use ivy-display-function first if it is non-nil.

@tumashu tumashu force-pushed the tumashu:master branch to 079fb3b Feb 10, 2018

@tumashu

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2018

Thanks god, magit-rebase save me ...

@tumashu

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2018

https://github.com/abo-abo/swiper/wiki/ivy-display-function is incorrect, the fact is that if you want to use a display-function, you should set ivy-display-function and add your commands to ivy-display-functions-alist. :-(

It seem to be a bug ...

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Feb 10, 2018

It seem to be a bug

Yeah, that's why I'm not so certain about the correct solution, as described here: #1443 (comment).

@abo-abo abo-abo closed this in 02bdc7f Feb 11, 2018

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Feb 11, 2018

Merged, thanks. Refinement PRs are welcome, as usual.

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.