Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
43 changes: 41 additions & 2 deletions theme/icons.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
package theme

import (
"bytes"
"image"
"image/color"
_ "image/jpeg" // register JPEG decoder so DisabledResource can desaturate JPEG icons
"image/png"

"fyne.io/fyne/v2"
"fyne.io/fyne/v2/internal/svg"
Expand Down Expand Up @@ -838,9 +842,44 @@
return "disabled_" + unwrapResource(res.source).Name()
}

// Content returns the disabled style content of the correct resource for the current theme
// Content returns the disabled style content of the correct resource for the current theme.
// SVG resources are recolored with the theme's disabled color; bitmap resources (PNG, JPEG, ...)
// are desaturated to greyscale since they cannot be recolored.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think that "since they cannot be recolored" in the docs is required - focus on behaviour in public docs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in e558fb3 β€” dropped the implementation reason, kept only the behaviour.

func (res *DisabledResource) Content() []byte {
return colorizeLogError(unwrapResource(res.source).Content(), Color(ColorNameDisabled))
src := unwrapResource(res.source)
content := src.Content()
if svg.IsResourceSVG(src) {
return colorizeLogError(content, Color(ColorNameDisabled))
}
return desaturateLogError(content)
}

// desaturateLogError returns a PNG-encoded greyscale copy of the given image bytes,
// preserving the alpha channel. If decoding or encoding fails, the original bytes are
// returned so the caller can still render something.
func desaturateLogError(src []byte) []byte {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As an internal API I would think that returning the error instead of logging it makes more sense.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in e558fb3 β€” desaturate now returns ([]byte, error); Content() logs and falls back to the original bytes.

img, _, err := image.Decode(bytes.NewReader(src))
if err != nil {
fyne.LogError("Failed to decode bitmap for disabled state", err)
return src
}
bounds := img.Bounds()
gray := image.NewNRGBA(bounds)
for y := bounds.Min.Y; y < bounds.Max.Y; y++ {
for x := bounds.Min.X; x < bounds.Max.X; x++ {
// Convert via NRGBA so the luminance is computed on unpremultiplied
// channels β€” otherwise partially-transparent pixels go too dark.
n := color.NRGBAModel.Convert(img.At(x, y)).(color.NRGBA)
lum := uint8((299*uint32(n.R) + 587*uint32(n.G) + 114*uint32(n.B)) / 1000)

Check failure on line 873 in theme/icons.go

View workflow job for this annotation

GitHub Actions / static_analysis (macos-latest)

G115: integer overflow conversion uint32 -> uint8 (gosec)

Check failure on line 873 in theme/icons.go

View workflow job for this annotation

GitHub Actions / static_analysis (ubuntu-latest)

G115: integer overflow conversion uint32 -> uint8 (gosec)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What are these magic numbers?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ITU-R BT.601 luma coefficients. Replaced with named constants (lumaWeightR/G/B, lumaScale) in e558fb3. Same commit also resolves the gosec G115 lint by clamping before the uint8 cast.

gray.SetNRGBA(x, y, color.NRGBA{R: lum, G: lum, B: lum, A: n.A})
}
}
var buf bytes.Buffer
if err := png.Encode(&buf, gray); err != nil {
fyne.LogError("Failed to encode desaturated bitmap", err)
return src
}
return buf.Bytes()
}

// ThemeColorName returns the fyne.ThemeColorName that is used as foreground color.
Expand Down
4 changes: 1 addition & 3 deletions widget/button.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"fyne.io/fyne/v2/canvas"
"fyne.io/fyne/v2/driver/desktop"
col "fyne.io/fyne/v2/internal/color"
"fyne.io/fyne/v2/internal/svg"
"fyne.io/fyne/v2/internal/widget"
"fyne.io/fyne/v2/layout"
"fyne.io/fyne/v2/theme"
Expand Down Expand Up @@ -419,8 +418,7 @@ func (r *buttonRenderer) updateIconAndText() {
r.icon.FillMode = canvas.ImageFillContain
r.SetObjects([]fyne.CanvasObject{r.background, r.tapBG, r.label, r.icon})
}
// TODO support disabling bitmap resource not just SVG
if r.button.Disabled() && svg.IsResourceSVG(icon) {
if r.button.Disabled() {
icon = theme.NewDisabledResource(icon)
}
r.icon.Resource = icon
Expand Down
19 changes: 19 additions & 0 deletions widget/button_internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,25 @@ func TestButton_DisabledIconChangedDirectly(t *testing.T) {
assert.Equal(t, render.icon.Resource.Name(), fmt.Sprintf("disabled_%v", searchBaseName))
}

func TestButton_DisabledBitmapIcon(t *testing.T) {
pngIcon := fyne.NewStaticResource("fyne.png", iconData)
button := NewButtonWithIcon("Test", pngIcon, nil)
render := test.TempWidgetRenderer(t, button).(*buttonRenderer)

// While enabled the original bitmap resource is rendered untouched.
assert.Equal(t, pngIcon, render.icon.Resource)

// When disabled the resource is wrapped so DisabledResource.Content()
// returns a desaturated PNG copy β€” the icon is recolored, not faded.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This states the expected behaviour but does not test it.

Please expand this test to confirm that pixels (at least some of them) have been desaturated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 372597b β€” the test now decodes DisabledResource.Content() and asserts every opaque pixel is greyscale (R==G==B). A sanity check on the source PNG confirms it contains coloured pixels so the assertion can't pass trivially.

button.Disable()
assert.True(t, strings.HasPrefix(render.icon.Resource.Name(), "disabled_"),
"disabled icon should be wrapped: %s", render.icon.Resource.Name())

// Re-enabling restores the original resource reference.
button.Enable()
assert.Equal(t, pngIcon, render.icon.Resource)
}

func TestButton_Focus(t *testing.T) {
tapped := false
button := NewButton("Test", func() {
Expand Down
Loading