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--select-occur-buffer): Support frames #1855

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

Conversation

basil-conto
Copy link
Collaborator

(ivy--find-occur-buffer): Save buffer once.
Use eq instead of equal for buffer objects.
(ivy-occur-next-line, ivy-occur-previous-line):
Reword docstrings without contractions.

Please check that these changes make sense, as I haven't actually tested them properly.

@basil-conto
Copy link
Collaborator Author

ivy.el (ivy--select-occur-buffer): Support frames

Sorry, just in case it's not clear: window-list doesn't consider other frames, whereas get-buffer-window, get-buffer-window-list, display-buffer, etc. do.

@abo-abo
Copy link
Owner

abo-abo commented Dec 12, 2018

Sorry, but your commits break performance.

Here's how I benchmark it: in ivy.el, swiper for def and C-c C-o.

Please use this code for a benchmark:

(defhydra hydra-grep-jk (global-map "β")
  ("j" ivy-occur-next-line "up")
  ("k" ivy-occur-previous-line "dn")
  ("q" nil))

Then try to cycle though candidates with βjjjjjjjjjjjkkkkkkk. With the current master, it's smooth. But with your change there's no redisplay and Emacs hangs for a while. This is likely because pop-to-buffer call is expensive even if the window is reused, while my custom code to do the same is fast.

@basil-conto
Copy link
Collaborator Author

I can't reproduce any slowdown or hanging with your recipe - it's just as fast either way. Perhaps there's something in your display-buffer-alist or related configuration that's causing the slowdown? Anyway, I'll update the patch to use get-buffer-window then.

@basil-conto
Copy link
Collaborator Author

Are you able to reproduce the slowdown from make plain and emacs -Q?

@abo-abo
Copy link
Owner

abo-abo commented Dec 12, 2018

Are you able to reproduce the slowdown from make plain?

Yes, on Emacs 26.1 it's reproducible.

@basil-conto
Copy link
Collaborator Author

Are you able to reproduce the slowdown from make plain?

Yes, on Emacs 26.1 it's reproducible.

My full question also asked about emacs -Q ;) The package-initialize that make plain performs can potentially be just as intrusive as loading user-init-file, if not more so.

So, without a more detailed recipe I can't figure out why pop-to-buffer is causing issues for you, but as I said I'll try to work around it. I'm looking into some other parts of the occur logic as well which may need tweaking to support frames properly.

@abo-abo
Copy link
Owner

abo-abo commented Dec 12, 2018

The package-initialize that make plain performs can potentially be just as intrusive as loading user-init-file, if not more so.

Please elaborate, I wasn't aware of this.

@basil-conto
Copy link
Collaborator Author

basil-conto commented Dec 12, 2018

package-initialize either loads package-quickstart-file or activates all packages under package-user-dir and package-directory-list. Either option can run arbitrary Lisp which deviates from emacs -Q. There are many third-party packages which autoload things they shouldn't be doing at top-level, and users are free to place their personal packages in this search path. This may be a bit of a long shot in the majority of cases, but don't forget that there are well-intentioned packages out there which autoload things like alternative key bindings or function advice. Even seemingly innocuous settings can make a difference between emacs -Q and make plain.

A good example of such a difference within Counsel itself is the change in behaviour of counsel-M-x depending on whether package-initialize found amx or smex in the search path.

@abo-abo
Copy link
Owner

abo-abo commented Dec 12, 2018

Would doing (package-initialize t) instead change anything?

@basil-conto
Copy link
Collaborator Author

Would doing (package-initialize t) instead change anything?

Sure, I think that would be much closer to emacs -Q. But what's the benefit of calling package-initialize at all in that case?

How about providing both: one target which just loads Ivy, Swiper, and Counsel, and another which additionally activates packages?

@abo-abo
Copy link
Owner

abo-abo commented Dec 13, 2018

another which additionally activates packages?

I think we don't actually need that one. I've turned it off. Maybe I wanted it for something like (require 'ivy-hydra), or avy integration.

(ivy--find-occur-buffer): Save buffer once.
Use eq instead of equal for buffer objects.
(ivy-occur-next-line, ivy-occur-previous-line):
Reword docstrings without contractions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants