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

Make counsel-yank-pop ivy-height configurable #1365

Merged
merged 1 commit into from Dec 10, 2017
Merged

Conversation

jojojames
Copy link
Contributor

@jojojames jojojames commented Dec 9, 2017

From #1362

counsel.el Outdated
@@ -3133,6 +3133,11 @@ A is the left hand side, B the right hand side."
:group 'ivy
:type 'string)

(defcustom counsel-yank-pop-ivy-height 5
Copy link
Collaborator

@basil-conto basil-conto Dec 9, 2017

Choose a reason for hiding this comment

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

Maybe counsel-yank-pop-height would be clear enough?

Copy link
Contributor Author

@jojojames jojojames Dec 9, 2017

Choose a reason for hiding this comment

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

Changed.

counsel.el Outdated
(defcustom counsel-yank-pop-ivy-height 5
"The `ivy-height' of `counsel-yank-pop'."
:group 'ivy
:type 'number)
Copy link
Collaborator

@basil-conto basil-conto Dec 9, 2017

Choose a reason for hiding this comment

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

This should be integer instead of number.

Copy link
Contributor Author

@jojojames jojojames Dec 9, 2017

Choose a reason for hiding this comment

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

Thanks, updated.

@abo-abo abo-abo merged commit a6375eb into abo-abo:master Dec 10, 2017
1 check passed
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Dec 10, 2017

Thanks.

seagle0128
Copy link
Contributor

@seagle0128 seagle0128 commented on a6375eb Jul 31, 2021

Choose a reason for hiding this comment

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

Currently the height is hardcoded to 5.

basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented on a6375eb Jul 31, 2021

Choose a reason for hiding this comment

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

@seagle0128

Currently the height is hardcoded to 5.

WDYM? AFAICT it currently defaults to 5:

swiper/counsel.el

Lines 4591 to 4593 in 20d78ae

(ivy-configure 'counsel-yank-pop
:height 5
:format-fn #'counsel--yank-pop-format-function)

Which means you should be able to customise it like so:

(with-eval-after-load 'counsel
  (ivy-configure #'counsel-yank-pop :height (round float-pi)))

seagle0128
Copy link
Contributor

@seagle0128 seagle0128 commented on a6375eb Jul 31, 2021

Choose a reason for hiding this comment

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

That's exactly what I mean. It's hardcoded, while not an option like counsel-yank-pop-height in this PR. I don't think users like to customize with ivy-configure.

basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented on a6375eb Jul 31, 2021

Choose a reason for hiding this comment

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

It's hardcoded

Well, hardcoded implies that it can't be changed, but...

while not an option like counsel-yank-pop-height in this PR

...I agree that simply loading counsel.el should not overwrite a manually modified ivy-height-alist like it currently does.

But at least ivy-configure does not modify ivy-height-alist if the user has customised it via the Customize interface, e.g. via customize-set-variable.

I don't think users like to customize with ivy-configure.

I don't either, in fact I think the way it's currently used is a terrible idea that breaks several Emacs conventions and user expectations. But I suspect that ship has largely sailed, unless someone is willing to work together with @abo-abo on #2442 and related issues (I lack both the time and interest to).

Either way, this is something that would preferably be discussed in an issue or PR, not in a commit comments section. Thanks.

seagle0128
Copy link
Contributor

@seagle0128 seagle0128 commented on a6375eb Jul 31, 2021

Choose a reason for hiding this comment

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

Well, hardcoded implies that it can't be changed, but...

In fact, in Emacs everything can be changed easily, but...

BTW, I found this post: https://www.gitmemory.com/issue/abo-abo/swiper/2854/826066579.
Thanks anyway!

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

4 participants