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.el (ivy--filter): allow filter to be interruptible with input. #2567

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

kiennq
Copy link
Contributor

@kiennq kiennq commented May 17, 2020

This is an attempt to fix the issue in ba37ec22, while still allow ivy--filter to be interruptible by keyboard.

Test with and the issue reported in ba37ec221ce7a0fce6f881d83895b8f971636cc05 doesn't happens

(let ((ivy-expr '(counsel-describe-symbol))
      ivy-test-inhibit-message)
  (dotimes (i 50)
    (message "%s" i)
    (execute-kbd-macro (vconcat (kbd "C-c e") (kbd "RET")))
    (sleep-for 0.1)))

Note: I've test with both ivy-switch-buffer and counsel-describe-symbol

@kiennq kiennq changed the title ivy.el (ivy--filter): allow filter to be interruptable with input. ivy.el (ivy--filter): allow filter to be interruptible with input. May 17, 2020
@abo-abo
Copy link
Owner

abo-abo commented May 18, 2020

Why not just change ivy--filter and ivy--recompute-index to have the "interruptible" behavior by default?

@kiennq
Copy link
Contributor Author

kiennq commented May 18, 2020

Why not just change ivy--filter and ivy--recompute-index to have the "interruptible" behavior by default?

I think ivy--filter and ivy--recompute-index may be used for other purpose than filtering items on minibuffer, to minimize the risk, I only allow ivy--filter which is called from ivy--update-minibuffer to be interruptible.

@kiennq kiennq force-pushed the bug/update-minibuffer-slow branch from 8214c13 to b689d9d Compare May 18, 2020 10:46
@abo-abo
Copy link
Owner

abo-abo commented May 19, 2020

Thanks for the update. I think this PR complicates the code too much.

Ideally, I would like to have a single while-no-input in the whole code, located probably in ivy--exhibit.

Then we need to investigate the cause of the ivy-switch-buffer bug that I discovered before.

I think it might be because in C-x b RET, the RET has interrupted the initial filter. So maybe we can check that in ivy-done and perform the filter if the initial one wasn't finished.

@kiennq
Copy link
Contributor Author

kiennq commented May 19, 2020

I think it might be because in C-x b RET, the RET has interrupted the initial filter. So maybe we can check that in ivy-done and perform the filter if the initial one wasn't finished.

The problem is the RET is even interrupt the collection creation. And special case handling in ivy-done, seems more like a hack to me :sad:
My PR although while it makes the code more complicate, it's more straight forward and no hacking, special checking anywhere. It just allow a potentially long running function ivy--filter the ability to be interrupted by keyboard, not at all time but under safe time, when all the related variable has been set to correct value.

@kiennq
Copy link
Contributor Author

kiennq commented May 22, 2020

@abo-abo Friendly ping

@kiennq kiennq force-pushed the bug/update-minibuffer-slow branch from b689d9d to f0b0480 Compare June 14, 2020 01:25
@abo-abo
Copy link
Owner

abo-abo commented Jun 15, 2020

@kiennq Thanks for the code. But I feel it complicates things too much.

I think we should instead update/simplify ivy--update-minibuffer to extend the existing while-no-input expression to envelop the ivy--filter expression.

@kiennq
Copy link
Contributor Author

kiennq commented Jun 16, 2020

Let me see if I can do something.

This fixes the way the --pcre2 switch is appended when using a PCRE expression
for ivy-occur.

Fixes abo-abo#2635
@kiennq kiennq force-pushed the bug/update-minibuffer-slow branch 2 times, most recently from 50c2826 to 6e8a847 Compare July 23, 2020 10:33
@kiennq
Copy link
Contributor Author

kiennq commented Jul 23, 2020

@abo-abo Sorry for late update. I've push a new update with much smaller size that's using non-essential variable as a cue to interrupt calculation when there's keyboard input or not.
Can you see if this is ok?

ivy.el Outdated Show resolved Hide resolved
@kiennq
Copy link
Contributor Author

kiennq commented Aug 29, 2020

@abo-abo Friendly ping

Copy link
Collaborator

@basil-conto basil-conto left a comment

Choose a reason for hiding this comment

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

Sorry, I can't usefully review this, since I'm not and don't intend to become more familiar with Ivy's low-level architecture. And given @abo-abo's concerns, I especially can't merge this.

BTW: what if the BODY in ivy--while-no-input returns t? It seems like this isn't currently supported, so it might be useful to point this out in the docstring, and make sure that none of the callers can end up returning t.

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.

4 participants