fix(palette): address style review feedback
- commit
- 053d9e66fde92bc03476a29bbb6bfa23047a0c5d
- author
- brice · · 2026-07-07 15:39 UTC
- parent
- 32173a3
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
3 files · +77 -36
M internal/usecase/palette/model.go +2 -3
| @@ -117,7 +117,6 @@ func (m *Model) Render(inner domain.Size, styles RenderStyles) renderer.Frame { | |||
| 117 | 117 | base := renderer.DefaultStyle() | |
| 118 | 118 | selection := styles.Selection | |
| 119 | 119 | desc := styles.Description | |
| 120 | - | desc.Italic = true | |
| 121 | 120 | ui.FillRect(frame, domain.Rect{Width: frame.Width, Height: frame.Height}, renderer.Cell{Rune: ' ', Style: base}) | |
| 122 | 121 | if m == nil { | |
| 123 | 122 | ui.DrawInputLine(frame, 0, "> ", "", base, selection) | |
| @@ -183,11 +182,11 @@ func (m *Model) Render(inner domain.Size, styles RenderStyles) renderer.Frame { | |||
| 183 | 182 | ||
| 184 | 183 | func mergePaletteDescStyle(line, desc renderer.Style) renderer.Style { | |
| 185 | 184 | out := line | |
| 186 | - | out.Italic = true | |
| 185 | + | out.Italic = desc.Italic | |
| 187 | 186 | if desc.HasForegroundRGB { | |
| 188 | 187 | out.HasForegroundRGB = true | |
| 189 | 188 | out.ForegroundRGB = desc.ForegroundRGB | |
| 190 | - | out.Foreground = desc.Foreground | |
| 189 | + | out.Foreground = -1 | |
| 191 | 190 | } else if desc.Foreground >= 0 { | |
| 192 | 191 | out.HasForegroundRGB = false | |
| 193 | 192 | out.Foreground = desc.Foreground | |
| @@ -117,7 +117,6 @@ func (m *Model) Render(inner domain.Size, styles RenderStyles) renderer.Frame { | |||||
| 117 | base := renderer.DefaultStyle() | 117 | base := renderer.DefaultStyle() | ||
| 118 | selection := styles.Selection | 118 | selection := styles.Selection | ||
| 119 | desc := styles.Description | 119 | desc := styles.Description | ||
| 120 | - | desc.Italic = true | |||
| 121 | ui.FillRect(frame, domain.Rect{Width: frame.Width, Height: frame.Height}, renderer.Cell{Rune: ' ', Style: base}) | 120 | ui.FillRect(frame, domain.Rect{Width: frame.Width, Height: frame.Height}, renderer.Cell{Rune: ' ', Style: base}) | ||
| 122 | if m == nil { | 121 | if m == nil { | ||
| 123 | ui.DrawInputLine(frame, 0, "> ", "", base, selection) | 122 | ui.DrawInputLine(frame, 0, "> ", "", base, selection) | ||
| @@ -183,11 +182,11 @@ func (m *Model) Render(inner domain.Size, styles RenderStyles) renderer.Frame { | |||||
| 183 | 182 | ||||
| 184 | func mergePaletteDescStyle(line, desc renderer.Style) renderer.Style { | 183 | func mergePaletteDescStyle(line, desc renderer.Style) renderer.Style { | ||
| 185 | out := line | 184 | out := line | ||
| 186 | - | out.Italic = true | 185 | + | out.Italic = desc.Italic |
| 187 | if desc.HasForegroundRGB { | 186 | if desc.HasForegroundRGB { | ||
| 188 | out.HasForegroundRGB = true | 187 | out.HasForegroundRGB = true | ||
| 189 | out.ForegroundRGB = desc.ForegroundRGB | 188 | out.ForegroundRGB = desc.ForegroundRGB | ||
| 190 | - | out.Foreground = desc.Foreground | 189 | + | out.Foreground = -1 |
| 191 | } else if desc.Foreground >= 0 { | 190 | } else if desc.Foreground >= 0 { | ||
| 192 | out.HasForegroundRGB = false | 191 | out.HasForegroundRGB = false | ||
| 193 | out.Foreground = desc.Foreground | 192 | out.Foreground = desc.Foreground | ||
M internal/usecase/palette/model_test.go +68 -33
| @@ -92,37 +92,72 @@ func TestRenderDrawsInputCaretSelectedLineAndCodeHighlight(t *testing.T) { | |||
| 92 | 92 | require.True(t, frame.At(13, 1).Style.Italic, "description is italic") | |
| 93 | 93 | } | |
| 94 | 94 | ||
| 95 | - | func TestRenderUsesSelectionStyleForFuzzyHighlights(t *testing.T) { | |
| 96 | - | m := New([]command.Command{cmd("CPY", "Copy", "Enter copy mode")}) | |
| 97 | - | m.Insert('c') | |
| 98 | - | m.Insert('y') | |
| 99 | - | accent := renderer.DefaultStyle() | |
| 100 | - | accent.HasBackgroundRGB = true | |
| 101 | - | accent.BackgroundRGB = renderer.RGB{R: 1, G: 2, B: 3} | |
| 102 | - | ||
| 103 | - | styles := DefaultRenderStyles() | |
| 104 | - | styles.Selection = accent | |
| 105 | - | frame := m.Render(domain.Size{Cols: 28, Rows: 3}, styles) | |
| 106 | - | ||
| 107 | - | matched := frame.At(0, 1).Style | |
| 108 | - | require.True(t, matched.Bold) | |
| 109 | - | require.False(t, matched.Inverse) | |
| 110 | - | require.True(t, matched.HasBackgroundRGB) | |
| 111 | - | require.Equal(t, renderer.RGB{R: 1, G: 2, B: 3}, matched.BackgroundRGB) | |
| 112 | - | } | |
| 113 | - | ||
| 114 | - | func TestRenderUsesMutedItalicDescriptionStyle(t *testing.T) { | |
| 115 | - | m := New([]command.Command{cmd("CPY", "Copy", "Enter copy mode")}) | |
| 116 | - | muted := renderer.DefaultStyle() | |
| 117 | - | muted.HasForegroundRGB = true | |
| 118 | - | muted.ForegroundRGB = renderer.RGB{R: 10, G: 20, B: 30} | |
| 119 | - | ||
| 120 | - | styles := DefaultRenderStyles() | |
| 121 | - | styles.Description = muted | |
| 122 | - | frame := m.Render(domain.Size{Cols: 28, Rows: 3}, styles) | |
| 123 | - | ||
| 124 | - | desc := frame.At(13, 1).Style | |
| 125 | - | require.True(t, desc.Italic) | |
| 126 | - | require.True(t, desc.HasForegroundRGB) | |
| 127 | - | require.Equal(t, renderer.RGB{R: 10, G: 20, B: 30}, desc.ForegroundRGB) | |
| 95 | + | func TestRenderUsesConfiguredStyles(t *testing.T) { | |
| 96 | + | tests := []struct { | |
| 97 | + | name string | |
| 98 | + | styles func() RenderStyles | |
| 99 | + | x int | |
| 100 | + | assert func(t *testing.T, style renderer.Style) | |
| 101 | + | }{ | |
| 102 | + | { | |
| 103 | + | name: "selection style for fuzzy highlights", | |
| 104 | + | styles: func() RenderStyles { | |
| 105 | + | accent := renderer.DefaultStyle() | |
| 106 | + | accent.HasBackgroundRGB = true | |
| 107 | + | accent.BackgroundRGB = renderer.RGB{R: 1, G: 2, B: 3} | |
| 108 | + | styles := DefaultRenderStyles() | |
| 109 | + | styles.Selection = accent | |
| 110 | + | return styles | |
| 111 | + | }, | |
| 112 | + | x: 0, | |
| 113 | + | assert: func(t *testing.T, style renderer.Style) { | |
| 114 | + | require.True(t, style.Bold) | |
| 115 | + | require.False(t, style.Inverse) | |
| 116 | + | require.True(t, style.HasBackgroundRGB) | |
| 117 | + | require.Equal(t, renderer.RGB{R: 1, G: 2, B: 3}, style.BackgroundRGB) | |
| 118 | + | }, | |
| 119 | + | }, | |
| 120 | + | { | |
| 121 | + | name: "muted italic description style", | |
| 122 | + | styles: func() RenderStyles { | |
| 123 | + | muted := renderer.DefaultStyle() | |
| 124 | + | muted.Italic = true | |
| 125 | + | muted.HasForegroundRGB = true | |
| 126 | + | muted.ForegroundRGB = renderer.RGB{R: 10, G: 20, B: 30} | |
| 127 | + | styles := DefaultRenderStyles() | |
| 128 | + | styles.Description = muted | |
| 129 | + | return styles | |
| 130 | + | }, | |
| 131 | + | x: 13, | |
| 132 | + | assert: func(t *testing.T, style renderer.Style) { | |
| 133 | + | require.True(t, style.Italic) | |
| 134 | + | require.True(t, style.HasForegroundRGB) | |
| 135 | + | require.Equal(t, -1, style.Foreground) | |
| 136 | + | require.Equal(t, renderer.RGB{R: 10, G: 20, B: 30}, style.ForegroundRGB) | |
| 137 | + | }, | |
| 138 | + | }, | |
| 139 | + | { | |
| 140 | + | name: "description italic is configurable", | |
| 141 | + | styles: func() RenderStyles { | |
| 142 | + | styles := DefaultRenderStyles() | |
| 143 | + | styles.Description.Italic = false | |
| 144 | + | return styles | |
| 145 | + | }, | |
| 146 | + | x: 13, | |
| 147 | + | assert: func(t *testing.T, style renderer.Style) { | |
| 148 | + | require.False(t, style.Italic) | |
| 149 | + | }, | |
| 150 | + | }, | |
| 151 | + | } | |
| 152 | + | ||
| 153 | + | for _, tt := range tests { | |
| 154 | + | t.Run(tt.name, func(t *testing.T) { | |
| 155 | + | m := New([]command.Command{cmd("CPY", "Copy", "Enter copy mode")}) | |
| 156 | + | m.Insert('c') | |
| 157 | + | m.Insert('y') | |
| 158 | + | frame := m.Render(domain.Size{Cols: 28, Rows: 3}, tt.styles()) | |
| 159 | + | ||
| 160 | + | tt.assert(t, frame.At(tt.x, 1).Style) | |
| 161 | + | }) | |
| 162 | + | } | |
| 128 | 163 | } | |
| @@ -92,37 +92,72 @@ func TestRenderDrawsInputCaretSelectedLineAndCodeHighlight(t *testing.T) { | |||||
| 92 | require.True(t, frame.At(13, 1).Style.Italic, "description is italic") | 92 | require.True(t, frame.At(13, 1).Style.Italic, "description is italic") | ||
| 93 | } | 93 | } | ||
| 94 | 94 | ||||
| 95 | - | func TestRenderUsesSelectionStyleForFuzzyHighlights(t *testing.T) { | 95 | + | func TestRenderUsesConfiguredStyles(t *testing.T) { |
| 96 | - | m := New([]command.Command{cmd("CPY", "Copy", "Enter copy mode")}) | 96 | + | tests := []struct { |
| 97 | - | m.Insert('c') | 97 | + | name string |
| 98 | - | m.Insert('y') | 98 | + | styles func() RenderStyles |
| 99 | - | accent := renderer.DefaultStyle() | 99 | + | x int |
| 100 | - | accent.HasBackgroundRGB = true | 100 | + | assert func(t *testing.T, style renderer.Style) |
| 101 | - | accent.BackgroundRGB = renderer.RGB{R: 1, G: 2, B: 3} | 101 | + | }{ |
| 102 | - | 102 | + | { | |
| 103 | - | styles := DefaultRenderStyles() | 103 | + | name: "selection style for fuzzy highlights", |
| 104 | - | styles.Selection = accent | 104 | + | styles: func() RenderStyles { |
| 105 | - | frame := m.Render(domain.Size{Cols: 28, Rows: 3}, styles) | 105 | + | accent := renderer.DefaultStyle() |
| 106 | - | 106 | + | accent.HasBackgroundRGB = true | |
| 107 | - | matched := frame.At(0, 1).Style | 107 | + | accent.BackgroundRGB = renderer.RGB{R: 1, G: 2, B: 3} |
| 108 | - | require.True(t, matched.Bold) | 108 | + | styles := DefaultRenderStyles() |
| 109 | - | require.False(t, matched.Inverse) | 109 | + | styles.Selection = accent |
| 110 | - | require.True(t, matched.HasBackgroundRGB) | 110 | + | return styles |
| 111 | - | require.Equal(t, renderer.RGB{R: 1, G: 2, B: 3}, matched.BackgroundRGB) | 111 | + | }, |
| 112 | - | } | 112 | + | x: 0, |
| 113 | - | 113 | + | assert: func(t *testing.T, style renderer.Style) { | |
| 114 | - | func TestRenderUsesMutedItalicDescriptionStyle(t *testing.T) { | 114 | + | require.True(t, style.Bold) |
| 115 | - | m := New([]command.Command{cmd("CPY", "Copy", "Enter copy mode")}) | 115 | + | require.False(t, style.Inverse) |
| 116 | - | muted := renderer.DefaultStyle() | 116 | + | require.True(t, style.HasBackgroundRGB) |
| 117 | - | muted.HasForegroundRGB = true | 117 | + | require.Equal(t, renderer.RGB{R: 1, G: 2, B: 3}, style.BackgroundRGB) |
| 118 | - | muted.ForegroundRGB = renderer.RGB{R: 10, G: 20, B: 30} | 118 | + | }, |
| 119 | - | 119 | + | }, | |
| 120 | - | styles := DefaultRenderStyles() | 120 | + | { |
| 121 | - | styles.Description = muted | 121 | + | name: "muted italic description style", |
| 122 | - | frame := m.Render(domain.Size{Cols: 28, Rows: 3}, styles) | 122 | + | styles: func() RenderStyles { |
| 123 | - | 123 | + | muted := renderer.DefaultStyle() | |
| 124 | - | desc := frame.At(13, 1).Style | 124 | + | muted.Italic = true |
| 125 | - | require.True(t, desc.Italic) | 125 | + | muted.HasForegroundRGB = true |
| 126 | - | require.True(t, desc.HasForegroundRGB) | 126 | + | muted.ForegroundRGB = renderer.RGB{R: 10, G: 20, B: 30} |
| 127 | - | require.Equal(t, renderer.RGB{R: 10, G: 20, B: 30}, desc.ForegroundRGB) | 127 | + | styles := DefaultRenderStyles() |
| 128 | + | styles.Description = muted | |||
| 129 | + | return styles | |||
| 130 | + | }, | |||
| 131 | + | x: 13, | |||
| 132 | + | assert: func(t *testing.T, style renderer.Style) { | |||
| 133 | + | require.True(t, style.Italic) | |||
| 134 | + | require.True(t, style.HasForegroundRGB) | |||
| 135 | + | require.Equal(t, -1, style.Foreground) | |||
| 136 | + | require.Equal(t, renderer.RGB{R: 10, G: 20, B: 30}, style.ForegroundRGB) | |||
| 137 | + | }, | |||
| 138 | + | }, | |||
| 139 | + | { | |||
| 140 | + | name: "description italic is configurable", | |||
| 141 | + | styles: func() RenderStyles { | |||
| 142 | + | styles := DefaultRenderStyles() | |||
| 143 | + | styles.Description.Italic = false | |||
| 144 | + | return styles | |||
| 145 | + | }, | |||
| 146 | + | x: 13, | |||
| 147 | + | assert: func(t *testing.T, style renderer.Style) { | |||
| 148 | + | require.False(t, style.Italic) | |||
| 149 | + | }, | |||
| 150 | + | }, | |||
| 151 | + | } | |||
| 152 | + | ||||
| 153 | + | for _, tt := range tests { | |||
| 154 | + | t.Run(tt.name, func(t *testing.T) { | |||
| 155 | + | m := New([]command.Command{cmd("CPY", "Copy", "Enter copy mode")}) | |||
| 156 | + | m.Insert('c') | |||
| 157 | + | m.Insert('y') | |||
| 158 | + | frame := m.Render(domain.Size{Cols: 28, Rows: 3}, tt.styles()) | |||
| 159 | + | ||||
| 160 | + | tt.assert(t, frame.At(tt.x, 1).Style) | |||
| 161 | + | }) | |||
| 162 | + | } | |||
| 128 | } | 163 | } | ||
M internal/usecase/theme/theme_test.go +7 -0
| @@ -355,6 +355,9 @@ func TestStyleHelpersFallbackAndThemed(t *testing.T) { | |||
| 355 | 355 | if got := BorderStyle(Theme{Known: true}); !got.Equal(renderer.DefaultStyle()) { | |
| 356 | 356 | t.Fatalf("border fallback=%+v", got) | |
| 357 | 357 | } | |
| 358 | + | if got := MutedTextStyle(unknown); !got.Equal(renderer.DefaultStyle()) { | |
| 359 | + | t.Fatalf("muted fallback=%+v", got) | |
| 360 | + | } | |
| 358 | 361 | inverse := renderer.DefaultStyle() | |
| 359 | 362 | inverse.Inverse = true | |
| 360 | 363 | if got := AccentStyle(unknown); !got.Equal(inverse) { | |
| @@ -380,4 +383,8 @@ func TestStyleHelpersFallbackAndThemed(t *testing.T) { | |||
| 380 | 383 | if !border.HasForegroundRGB || border.ForegroundRGB != (renderer.RGB{R: 124, G: 128, B: 132}) { | |
| 381 | 384 | t.Fatalf("border themed=%+v", border) | |
| 382 | 385 | } | |
| 386 | + | muted := MutedTextStyle(theme) | |
| 387 | + | if !muted.HasForegroundRGB || muted.ForegroundRGB != (renderer.RGB{R: 115, G: 119, B: 124}) { | |
| 388 | + | t.Fatalf("muted themed=%+v", muted) | |
| 389 | + | } | |
| 383 | 390 | } | |
| @@ -355,6 +355,9 @@ func TestStyleHelpersFallbackAndThemed(t *testing.T) { | |||||
| 355 | if got := BorderStyle(Theme{Known: true}); !got.Equal(renderer.DefaultStyle()) { | 355 | if got := BorderStyle(Theme{Known: true}); !got.Equal(renderer.DefaultStyle()) { | ||
| 356 | t.Fatalf("border fallback=%+v", got) | 356 | t.Fatalf("border fallback=%+v", got) | ||
| 357 | } | 357 | } | ||
| 358 | + | if got := MutedTextStyle(unknown); !got.Equal(renderer.DefaultStyle()) { | |||
| 359 | + | t.Fatalf("muted fallback=%+v", got) | |||
| 360 | + | } | |||
| 358 | inverse := renderer.DefaultStyle() | 361 | inverse := renderer.DefaultStyle() | ||
| 359 | inverse.Inverse = true | 362 | inverse.Inverse = true | ||
| 360 | if got := AccentStyle(unknown); !got.Equal(inverse) { | 363 | if got := AccentStyle(unknown); !got.Equal(inverse) { | ||
| @@ -380,4 +383,8 @@ func TestStyleHelpersFallbackAndThemed(t *testing.T) { | |||||
| 380 | if !border.HasForegroundRGB || border.ForegroundRGB != (renderer.RGB{R: 124, G: 128, B: 132}) { | 383 | if !border.HasForegroundRGB || border.ForegroundRGB != (renderer.RGB{R: 124, G: 128, B: 132}) { | ||
| 381 | t.Fatalf("border themed=%+v", border) | 384 | t.Fatalf("border themed=%+v", border) | ||
| 382 | } | 385 | } | ||
| 386 | + | muted := MutedTextStyle(theme) | |||
| 387 | + | if !muted.HasForegroundRGB || muted.ForegroundRGB != (renderer.RGB{R: 115, G: 119, B: 124}) { | |||
| 388 | + | t.Fatalf("muted themed=%+v", muted) | |||
| 389 | + | } | |||
| 383 | } | 390 | } | ||