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

Latest update results in inacurate minibuffer size for small number of candidates #2118

Closed
gusbrs opened this issue Jul 3, 2019 · 9 comments
Closed

Comments

@gusbrs
Copy link
Contributor

@gusbrs gusbrs commented Jul 3, 2019

As requested in discussion at 7183491, I open an issue in this respect. And I reproduce what was initially reported there to put all info in the proper place.

I am experiencing a small glitch resulting from this commit. It was not happening until the latest update, and it happens now with ivy-20190701.1140. And it is gone with:

(advice-add 'ivy--minibuffer-setup :after
              (lambda () (ivy--exhibit)))

What happens here is that in ivy completions with small number of candidates (less than ivy-height), the last candidate is too close to the frame's edge. And when you navigate to any other candidate, the minibuffer increases in size a little to accommodate to what used to be the normal situation. So we get both a strange looking initial state and an unstable ivy minibuffer size.

I did not try to isolate it from my current configs, but if you need more info, I can try to narrow down things or explain further.

Some images, to be more precise. This is the state where counsel-bookmark starts for me (the bottom of the image is the frame's edge):

screen1

And after I navigate to the following candidate:

screen2

The same behaviour happens in other places I somehow call ivy-read with a small number of candidates.

As to your questions:

Can you reproduce it with make plain?

I'm not sure what you mean. Ivy? Emacs? I usually install ivy from MELPA. I wouldn't like to disrupt the workflow too much at the moment, if possible. But in the case this is the only way to diagnose the problem, I'm at your disposal to try things out.

What's your Emacs version?

The latest released version, namely GNU Emacs 26.2 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.30) of 2019-04-22. Besides that, I have ivy-20190701.1140, counsel-20190629.1203, swiper-20190627.1944.

Is replacing ivy--exhibit with ivy--resize-minibuffer-to-fit work in your advice?

No. The problem persists if I use instead:

(advice-add 'ivy--minibuffer-setup :after
              (lambda () (ivy--resize-minibuffer-to-fit)))
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jul 3, 2019

Thanks for the follow up. Running make plain means:

git clone https://github.com/abo-abo/swiper/
cd swiper
make plain

This will run Emacs with a minimal config, and Ivy loaded. If you can reproduce a bug with make plain, it means that I can likely do so as well. Really helps to narrow down issues related to config.

@gusbrs
Copy link
Contributor Author

@gusbrs gusbrs commented Jul 3, 2019

@abo-abo I think I could find the relevant interaction. As I tried to reproduce this with emacs -Q (and I couldn't), I noticed the bottom spacing I was used to is actually much larger than the default (as shown in the second image). So I suspected my line-spacing setting, which has already bitten me in the foot in the past.

So I actually have:

'(line-spacing 0.1)

In my custom-set-variables section. Removing it eliminates the problem. In this case, I'm not sure this is to be considered an Ivy issue... (you tell me). And if you think testing with make plain is still required, let me know, and I'll do it. Or, for that matter, any additional info.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jul 3, 2019

I still can't reproduce with:

(setq line-spacing 0.1)
(ivy-read "test: " '("one" "two" "three"))

on Emacs 26.1, GNU/Linux.

@gusbrs
Copy link
Contributor Author

@gusbrs gusbrs commented Jul 3, 2019

line-spacing becomes buffer-local when set, so that's what happens when you (setq line-spacing 0.1).

I can reproduce the issue with emacs -Q by setting this line-spacing value through the customize interface, and then running

(ivy-read "test: " '("one" "two" "three"))

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jul 3, 2019

Thanks, I can now reproduce with (setq-default line-spacing 0.1).

@gusbrs
Copy link
Contributor Author

@gusbrs gusbrs commented Jul 3, 2019

Well, I'm thinking here already that setting line-spacing globally is not really wise, so I personally moved it to run only on text-mode-hook and prog-mode-hook. I had already had problems with this with calc's trail, and now this. Anyway, if this is useful to make Ivy more robust to an eventual unwise setting (the doc string does say it can be customized...) I'm glad with that.

@abo-abo abo-abo closed this in 673c45e Jul 3, 2019
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jul 3, 2019

Thanks. Ivy will now set line-spacing to nil, but only in the minibuffer. If there's a solution that makes line-spacing work properly in the minibuffer, PRs are welcome.

@gusbrs
Copy link
Contributor Author

@gusbrs gusbrs commented Jul 3, 2019

@abo-abo Thank you very much.

Btw, let me ask you something. I only reported this today, because I could pin down a behaviour change to a particular commit, and it would be a lost opportunity not to do so. I do know that "hammering" anyone's issue tracker may be felt as impolite, and it may as well really be, depending on how it's done. And, besides this one, I have hit yours here twice recently. I do have some more stuff for you, which is kept here for an appropriate time. But I also know there are different attitudes towards the matter. So, do prefer a more "prompt" or a more "parsimonious" approach to bringing issues? I just don't mean to abuse, specially as I'm really thankful for the work being done here.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jul 3, 2019

I prefer prompt issues with a good description of what you want and a series of reproduction steps. You can open as many of these as you like :)
Issues like this one are good: in just a few minutes of work, we've caught a nasty edge case and fixed it. So thanks for the report.

astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
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 pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants