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

Bind mouse events #1279

Closed
wants to merge 11 commits into from
Closed

Bind mouse events #1279

wants to merge 11 commits into from

Conversation

mookid
Copy link
Contributor

@mookid mookid commented Nov 12, 2017

Fix #1278.

Probably a super dumb implementation: call ivy-next-line-and-call and then ivy-alt-done. I wonder if there is anything simpler; I guess that the tricky part here is to establish the bindings, then the implementation can evolve.

ivy.el Outdated
0 (length str)
`(mouse-face
ivy-minibuffer-match-highlight
help-echo (format "mouse-1: %s" ivy-mouse-action-tooltip))
Copy link
Collaborator

@basil-conto basil-conto Nov 12, 2017

Choose a reason for hiding this comment

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

Missing backquote comma?

Copy link
Contributor Author

@mookid mookid Nov 12, 2017

Choose a reason for hiding this comment

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

Indeed. It surprises me that it works without the comma...

Copy link
Collaborator

@basil-conto basil-conto Nov 13, 2017

Choose a reason for hiding this comment

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

Actually it should come as no surprise - the help-echo property can be evaluated as a function / form, in which case I suggest reverting to deferred evaluation (i.e. replace backquote with plain quote), since ivy-mouse-action-tooltip is a variable.

Copy link
Collaborator

@basil-conto basil-conto Nov 17, 2017

Choose a reason for hiding this comment

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

@mookid WDYT? (It just occurred to me that GitHub may not have notifed you of my replies on this and previous "outdated diffs". My apologies if you've already seen - I won't nag again.)

ivy.el Outdated
(let ((str
(funcall ivy--highlight-function (copy-sequence str))))

(add-text-properties
Copy link
Collaborator

@basil-conto basil-conto Nov 12, 2017

Choose a reason for hiding this comment

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

Why not propertize?

Copy link
Contributor Author

@mookid mookid Nov 12, 2017

Choose a reason for hiding this comment

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

Indeed, looks better with it, but adds an unnecessary copy.

I guess that the mouse-face property can be added for both branches of the if, what do you think?

Copy link
Collaborator

@basil-conto basil-conto Nov 13, 2017

Choose a reason for hiding this comment

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

but adds an unnecessary copy

True, unless you pass the result of propertize to ivy--highlight-function:

(defun ivy--format-minibuffer-line (str)
  "Format line STR for use in minibuffer."
  (if (eq ivy-display-style 'fancy)
      (funcall ivy--highlight-function
               (propertize str
                           'mouse-face 'ivy-minibuffer-match-highlight
                           'help-echo  '(format "mouse-1: %s"
                                                ivy-mouse-action-tooltip)))
    (copy-sequence str)))

Though this relies on ivy--highlight-function to not overwrite any of the propertize properties, so I don't support it too strongly.

Copy link
Collaborator

@basil-conto basil-conto Nov 13, 2017

Choose a reason for hiding this comment

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

I guess that the mouse-face property can be added for both branches of the if

Indeed, I think mouse-face should be set for any clickable results, whereas ivy-display-style seems to be more specific to highlighting regexp matches.

ivy.el Outdated
(- line-number-candidate
ivy--window-index)))
(ivy-next-line-and-call offset))
(ivy-alt-done)))
Copy link
Collaborator

@basil-conto basil-conto Nov 12, 2017

Choose a reason for hiding this comment

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

Maybe it's just me, but I find it clearer to write

(defun ivy-mouse-done (event)
  "Exit the minibuffer with the selected candidate."
  (interactive "@e")
  (when event
    (ivy-next-line-and-call
     (- (max 0 (- (line-number-at-pos (posn-point (event-start event)))
                  2))
        ivy--window-index))
    (ivy-alt-done)))

Out of curiosity (as I am not familiar with the code or its purpose), why is up to 2 subtracted from the line number?

Copy link
Contributor Author

@mookid mookid Nov 12, 2017

Choose a reason for hiding this comment

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

The purpose is to compute, given a click event in the minibuffer window, how many lines (offset) to adjust the selected candidate (to set it the the pointed-to candidate) before exiting.

As it is no so clear why the 2 is subtracted, I preferred to give meaningful names to the values.

line-number-at-point is the line number in the minibuffer window; line 1 is the prompt, line 2 the first candidate... as the click event designates a candidate, there is (max 2 ...) (a click on the prompt line is interpreted as selecting the first candidate).

line-number-candidate corresponds to the index of the selected candidate in the list of displayed candidates (which is indexed from 0).

We substract one because of the prompt, and one to convert to 0 based index.

Copy link
Collaborator

@basil-conto basil-conto Nov 13, 2017

Choose a reason for hiding this comment

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

Thanks, that makes sense. I'm not against naming intermediate results, I just find it clearer to subtract 2 and then ensure the result is a valid index, rather than doing (max 2 N) followed by a subtraction of 2.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Nov 13, 2017

Thanks, very cool. After a click I'm getting:

user-error: Minibuffer window is not active

Everything still works, but the error message is annoying. Reproducible with make plain.

@mookid
Copy link
Contributor Author

@mookid mookid commented Nov 13, 2017

Yes, I've seen that. I will investigate that later today or tomorrow.

A trickier event code to write is scrolling...

@mookid
Copy link
Contributor Author

@mookid mookid commented Nov 16, 2017

@abo-abo do you see anything more to be implemented/modified?

@mookid
Copy link
Contributor Author

@mookid mookid commented Nov 29, 2017

@abo-abo any hope that this lands on master?

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Nov 29, 2017

@mookid Your thoughts on my comments above re: unconditionally adding mouse-face and leaving help-echo unevaluated?

@mookid
Copy link
Contributor Author

@mookid mookid commented Nov 29, 2017

I added a binding for mouse-3. But of course, one can't really end the session with the mouse, as the implementation becomes (read-char). is there any reason why this is not instead an ivy session?

Also note that the ivy--format-minibuffer-line machinery prevents correct propertization of multiline candidates.

@basil-conto :

unconditionally adding mouse-face is

This is `Apply mouse face even when non fancy.'.

leaving help-echo unevaluated

I miss the point.

ivy.el Outdated

(defun ivy-mouse-done (event)
(interactive "@e")
(let* ((offet (ivy-mouse-offset event)))
Copy link
Collaborator

@basil-conto basil-conto Nov 30, 2017

Choose a reason for hiding this comment

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

offet -> offset
let* -> let
here and in ivy-mouse-dispatching-done.

Copy link
Contributor Author

@mookid mookid Nov 30, 2017

Choose a reason for hiding this comment

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

Indeed, thanks!

(- line-number-candidate
ivy--window-index)))
offset)
nil))
Copy link
Collaborator

@basil-conto basil-conto Nov 30, 2017

Choose a reason for hiding this comment

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

Nitpick: can also be

(and event
     (let* ((...))
       ...
       offset)

if you like.

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Nov 30, 2017

@mookid

unconditionally adding mouse-face is

This is `Apply mouse face even when non fancy.'.

leaving help-echo unevaluated

I miss the point.

Sorry, I hadn't seen your latest changes, which cover both counts. Apart from my latest nitpick review, LGTM.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Nov 30, 2017

@mookid Looks good to merge, except for ivy--window-index. That var is not clear to me: in some cases it tries to replace ivy--index, in others not. Can you explain why we need it? I tried replacing it with ivy--index and it doesn't quite work.

@mookid
Copy link
Contributor Author

@mookid mookid commented Nov 30, 2017

The key is in ivy--format. ivy--index is, as far as I understand/remember, the index of the selected candidate in the list.

ivy--window-index is the index of the highlighted candidate in the list of displayed candidates.

Previously, the two were not distinct, and the value of ivy--index was temporarily changed before calling ivy--format-minibuffer-line in ivy-format.

What do you think about replacing read-key with a recursive ivy session?

abo-abo added a commit that referenced this issue Nov 30, 2017
Previously, when completing file names, intermediate dired buffers
would pop up for directories.

Re #1279
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Nov 30, 2017

Thanks!

@mookid mookid deleted the bind-mouse-events branch Dec 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants