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.

Sign up for GitHub

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

Jump to bottom

Set prompt color #13

Closed
wants to merge 4 commits into from
Closed

Set prompt color #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c3913f1
support prompt color
akerl Feb 20, 2020
8052a64
fix typo
akerl Feb 20, 2020
2e90b47
Merge pull request #1 from akerl-unpriv/feature/prompt-color
akerl Feb 20, 2020
42d7f65
fix WithPromptColor comment
akerl Feb 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion fuzzyfinder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ func (f *finder) _draw() {
// prompt line
var promptLinePad int
for _, r := range []rune(f.opt.promptString) {
f.term.setCell(promptLinePad, height-1, r, termbox.ColorBlue, termbox.ColorDefault)
f.term.setCell(promptLinePad, height-1, r, f.opt.promptColor, termbox.ColorDefault)
promptLinePad++
}
var r rune
Expand Down
13 changes: 13 additions & 0 deletions option.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
package fuzzyfinder

import (
"github.com/gdamore/tcell/termbox"
)

type opt struct {
mode mode
previewFunc func(i, width, height int) string
multi bool
hotReload bool
promptString string
promptColor termbox.Attribute
Copy link
Owner

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
)

Copy link
Author

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.

Copy link
Owner

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.

Copy link
Owner

Choose a reason for hiding this comment

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

@martinbaillie How about you?

Copy link
Contributor

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.

}

type mode int
Expand All @@ -23,6 +28,7 @@ const (

var defaultOption = opt{
promptString: "> ",
promptColor: termbox.ColorBlue,
}

// Option represents available fuzzy-finding options.
Expand Down Expand Up @@ -64,6 +70,13 @@ func WithPromptString(s string) Option {
}
}

// WithPromptColor changes the prompt color. The default value is termbox.ColorBlue.
func WithPromptColor(c termbox.Attribute) Option {
return func(o *opt) {
o.promptColor = c
}
}

// withMulti enables to select multiple items by tab key.
func withMulti() Option {
return func(o *opt) {
Expand Down