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

Add selectrum-default-value-format customizable variable #445

Merged
merged 1 commit into from
Feb 16, 2021
Merged

Add selectrum-default-value-format customizable variable #445

merged 1 commit into from
Feb 16, 2021

Conversation

minad
Copy link
Contributor

@minad minad commented Feb 16, 2021

  • This variable allows the user to customize the format
    of the default value indicator
  • The variable can also be nil in order to hide the indicator
  • Do not remove the properties from the candidate string,
    for example Consult uses candidate strings with the invisible
    property
  • See also Prompt shows [default value: ...] for Consult narrowing #444

@minad
Copy link
Contributor Author

minad commented Feb 16, 2021

For simplicity I went with the format string approach for now. This is also more aligned with selectrum-multiline-display-settings which also uses a format string.

I find the current candidate highlighting a bit questionable. Maybe it would be okay to always use the minibuffer-prompt face?

                          (if (and selectrum--current-candidate-index
                                   (< selectrum--current-candidate-index
                                      0))
                              'selectrum-current-candidate
                            'minibuffer-prompt)))))

Copy link
Collaborator

@clemera clemera left a comment

Choose a reason for hiding this comment

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

Great, thanks!

selectrum.el Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@minad
Copy link
Contributor Author

minad commented Feb 16, 2021

What about this question:

I find the current candidate highlighting a bit questionable. Maybe it would be okay to always use the minibuffer-prompt face?

One more question - I am not sure if nil is part of the defcustom string type spec. Sometimes nil is coerced to a string, sometimes not. So it could also be (choice (const nil) string) (FIXED, should be choice).

* This variable allows the user to customize the format
  of the default value indicator
* The variable can also be nil in order to hide the indicator
* Do not remove the properties from the candidate string,
  for example Consult uses candidate strings with the invisible
  property
* PR #445
* Issue #444
@clemera
Copy link
Collaborator

clemera commented Feb 16, 2021

I find the current candidate highlighting a bit questionable. Maybe it would be okay to always use the minibuffer-prompt face?

I don't know I don't think I would like that, using the same face as for candidates makes it look like a usual candidate which I like.

One more question - I am not sure if nil is part of the defcustom string type spec. Sometimes nil is coerced to a string, sometimes not. So it could also be (choice (const nil) string).

I'm not picky about it but if its not part of the spec than why not use (choice (const nil) string)?

@minad
Copy link
Contributor Author

minad commented Feb 16, 2021

I don't know I don't think I would like that, using the same face as for candidates makes it look like a usual candidate which I like.

Okay, so keep it as is? Then I am finished with the PR - ready to merge.

@clemera clemera merged commit f27819b into radian-software:master Feb 16, 2021
@clemera
Copy link
Collaborator

clemera commented Feb 16, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants