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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f67a2fe
Allow ivy-display-functions-alist adding key: t as fallback
tumashu Feb 9, 2018
48ee247
Add ivy-display-functions-props and use it
tumashu Feb 9, 2018
079fb3b
ivy-display-function will be used to set global display function.
tumashu Feb 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 28 additions & 7 deletions ivy.el
View file
Open in desktop
Expand Up @@ -186,16 +186,28 @@ Only \"./\" and \"../\" apply here. They appear in reverse order."
:type 'boolean)

(defcustom ivy-display-function nil
"Decide where to display the candidates.
This function takes a string with the current matching candidates
and has to display it somewhere.
See https://github.com/abo-abo/swiper/wiki/ivy-display-function."
"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'."
:type '(choice
(const :tag "Minibuffer" nil)
(const :tag "LV" ivy-display-function-lv)
(const :tag "Popup" ivy-display-function-popup)
(const :tag "Overlay" ivy-display-function-overlay)))

(defvar ivy-display-functions-props
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'((ivy-display-function-overlay :cleanup ivy-overlay-cleanup))
"Map Ivy display functions to their property lists.
Examples of properties include associated `:cleanup' functions.")

(defvar ivy-display-functions-alist
'((ivy-completion-in-region . ivy-display-function-overlay))
"An alist for customizing `ivy-display-function'.")
Expand Down Expand Up @@ -1661,7 +1673,9 @@ customizations apply to the current completion session."
collection))))
(ivy-display-function
(unless (window-minibuffer-p)
(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)))))))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@tumashu tumashu Feb 10, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

but this seem to have other problem.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

(setq ivy-last
(make-ivy-state
:prompt prompt
Expand Down Expand Up @@ -1721,8 +1735,9 @@ customizations apply to the current completion session."
(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)
Expand All @@ -1732,6 +1747,12 @@ customizations apply to the current completion session."
(remove-list-of-text-properties
0 1 '(idx) (ivy-state-current ivy-last))))))

(defun ivy--display-function-prop (prop)
"Return PROP associated with current `ivy-display-function'."
(plist-get (cdr (assq ivy-display-function
ivy-display-functions-props))
prop))

(defun ivy--reset-state (state)
"Reset the ivy to STATE.
This is useful for recursive `ivy-read'."
Expand Down