Skip to content

Add flag selectedReverseColor to reverse style of selected item#598

Closed
mdom wants to merge 5 commits intorivo:masterfrom
mdom:selectedReverseColor
Closed

Add flag selectedReverseColor to reverse style of selected item#598
mdom wants to merge 5 commits intorivo:masterfrom
mdom:selectedReverseColor

Conversation

@mdom
Copy link
Copy Markdown

@mdom mdom commented May 11, 2021

I'm working on xterm with disabled color support. This patch allows me to see the currently selected item in a list. This is probably not a good solution, there are surley be other widgets with similar problems. But i just wanted to see if the reverse code worked.

What do you think about adding a api, where i can just set styles directly for elements? Like SetSelectedStyle, SetBorderStyle, etc?

@mdom
Copy link
Copy Markdown
Author

mdom commented May 11, 2021

Oh, I forget to write that i would be willing to add the code if you like the idea... :)

Copy link
Copy Markdown
Owner

@rivo rivo left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. This is generally a good idea and one step closer to resolving #611. I have a few review comments and requests for changes. Please have a look.

Comment thread README.md
- [Fast disk usage analyzer written in Go](https://github.com/dundee/gdu)
- [Multiplayer Chess On Terminal](https://github.com/qnkhuat/gochess)
- [Scriptable TUI music player](https://github.com/issadarkthing/gomu)
- [MangaDesk : TUI Client for downloading manga to your computer](https://github.com/darylhjd/mangadesk)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It's ok to include this here but if you want this to be accepted faster, you may want to submit a separate PR for your project.

Comment thread go.mod

require (
github.com/gdamore/tcell/v2 v2.2.1
github.com/gdamore/tcell/v2 v2.3.3
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can you remove the go.mod changes from this PR? I'd like to keep upgrades separate.

Comment thread go.sum
github.com/gdamore/tcell/v2 v2.2.0/go.mod h1:cTTuF84Dlj/RqmaCIV5p4w8uG1zWdk0SF6oBpwHp4fU=
github.com/gdamore/tcell/v2 v2.2.1 h1:Gt8wk0jd5pIK2CyXNo/fqwxNWf726j1lQjEDdfbnqTc=
github.com/gdamore/tcell/v2 v2.2.1/go.mod h1:cTTuF84Dlj/RqmaCIV5p4w8uG1zWdk0SF6oBpwHp4fU=
github.com/gdamore/tcell/v2 v2.3.3 h1:RKoI6OcqYrr/Do8yHZklecdGzDTJH9ACKdfECbRdw3M=
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can you remove the go.sum changes from this PR? I'd like to keep upgrades separate.

Comment thread list.go
// The item main text color.
mainTextColor tcell.Color
// The item main text style.
mainTextStyle tcell.Style
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is not indented like the others. Did this go through gofmt? Is this maybe just an issue with GitHub?

Comment thread list.go

// NewList returns a new form.
func NewList() *List {
base := tcell.StyleDefault.Background(Styles.PrimitiveBackgroundColor)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

While I appreciate this PR (it's something that needs to be done anyway, see #611), I'm not sure setting this background colour for all styles is the correct way to do this (unless it's ignored, see further below). Have you tried setting a list's background colour using Box.SetBackgroundColor() to something else, say, blue? What do the list items look like when you do that?

Comment thread list.go
}

// SetMainTextStyle sets the style of the items' main text.
func (l *List) SetMainTextStyle(style tcell.Style) *List {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can you also add getters for all of these new setters, please?

Comment thread list.go
// Shortcuts.
if showShortcuts && item.Shortcut != 0 {
Print(screen, fmt.Sprintf("(%s)", string(item.Shortcut)), x-5, y, 4, AlignRight, l.shortcutColor)
printWithStyle(screen, fmt.Sprintf("(%s)", string(item.Shortcut)), x-5, y, 0, 4, AlignRight, l.shortcutStyle, false)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Referring to my comment above and relating to all of the printWithStyle() calls: Doesn't this override the list's overall background colour? You might want to set the last argument (maintainBackground) to true if the style's background colour is tcell.ColorDefault.

@mdom mdom closed this by deleting the head repository Nov 10, 2022
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.

3 participants