-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
Set prompt color #13
Set prompt color #13
Conversation
support prompt color
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for submitting a Pull Request!
I suggested some points. Could you confirm it?
And could you merge the latest changes from master? It fixes CI errors.
option.go
Outdated
@@ -64,6 +70,13 @@ func WithPromptString(s string) Option { | |||
} | |||
} | |||
|
|||
// WithPromptString changes the prompt string. The default value is "> ". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is the same as WithPromptString
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
type opt struct { | ||
mode mode | ||
previewFunc func(i, width, height int) string | ||
multi bool | ||
hotReload bool | ||
promptString string | ||
promptColor termbox.Attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it is not preferred to depend on a specific library in the aspect of backward-compatibility. We cannot change termbox
to another one because it breaks go-fuzzyfinder's backward-compatibility.
Could you define own Color
type and each color?
For example:
type Color int32
const (
ColorBlack Color = iota
ColorRed
ColorGreen
ColorYellow
ColorBlue
ColorMagenta
ColorCyan
ColorWhite
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think I agree. Because go-fuzzyfinder passes through the defined color code, redefining iotas like this would still involve breaking backwards-compatibility if the termbox dep was replaced later (because there’s no guarantee that the new library defines constant uint16 color codes the same way).
If the goal is to avoid locking the public go-fuzzyfinder API to a specific backing terminal library, I think the code would need to translate everywhere colors are used, so that fuzzyfinder.ColorBlack is converted to termbox.ColorBlack when used (and the same for the other colors), so that it doesn’t rely on the iota layout being permanently consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least now the compatibility between I suggested enums and termbox
's one will be kept as long as we use Go modules. And if termbox
breaks Color
backward-compatibility in a new version, we can remap enums we defined when updating go.mod
.
However, this opinion is definitely correct. Also, we may replace termbox
to tcell
in the near future because nsf/go-termbox
is not now maintained.
So, could you remap defined colors to termbox
's one?
I think the code would need to translate everywhere colors are used, so that fuzzyfinder.ColorBlack is converted to termbox.ColorBlack when used (and the same for the other colors), so that it doesn’t rely on the iota layout being permanently consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martinbaillie How about you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion on this either way. I'm still making use of this library internally at work for some CLIs and flexible prompt colour would be nice.
@akerl ping |
@ktr0731 I think I agree about the change described above. It’s probable I won’t have the bandwidth to revise this PR for another week or so. |
Could you reopen the PR If it is ready? |
This PR adds support for setting Prompt Color to a custom value. I added it because getting the “blue” color to be readable on my dark-background terminal was an exercise in futility.