Fade disabled button icon when resource is a bitmap (#6217)#6381
Fade disabled button icon when resource is a bitmap (#6217)#6381mohsenm4 wants to merge 3 commits into
Conversation
f4c155b to
92ef4b5
Compare
Buttons with SVG icons were already grayed out on Disable() via NewDisabledResource, but bitmap resources (PNG, JPEG, ...) cannot be recolored that way and stayed at full opacity, leaving no visual cue that the button is disabled. Apply a translucency to the icon image when the button is disabled and the resource is not an SVG; reset it when the button is enabled again. The chosen translucency is a fixed value and documented inline — the emoji-text rendering aspect from the original report is tracked separately and is not addressed here. Refs fyne-io#6217 (partial — Button.Icon only) Refs fyne-io#6383 (follow-up for emoji text)
92ef4b5 to
008f894
Compare
andydotxyz
left a comment
There was a problem hiding this comment.
Thanks for looking at this. However translucency feels like a workaround. bitmap images absolutely can be recoloured. In this case it should be sufficient to reduce saturation to make it appear greyscale.
| ButtonIconTrailingText | ||
| ) | ||
|
|
||
| // disabledIconTranslucency is the alpha-fade applied to bitmap icons (PNG, JPEG, ...) |
There was a problem hiding this comment.
Please don't commit walls of comment. A comment should be added when its public API or where the code is not clear without it.
| if svg.IsResourceSVG(icon) { | ||
| icon = theme.NewDisabledResource(icon) | ||
| } else { | ||
| // Bitmap resources cannot be recolored, so fade them instead |
There was a problem hiding this comment.
Yes, bitmaps can be recoloured. Not in the same way but it is not impossible.
Remove the colour saturation to be greyscale and then apply colour (if the desired outcome is not grey)
Per review feedback on fyne-io#6381: translucency was a workaround. Bitmap resources can be recoloured by reducing saturation so the icon appears greyscale, matching how SVGs are recoloured to ColorNameDisabled. Extend theme.DisabledResource.Content() to decode non-SVG resources, convert each pixel to its ITU-R BT.601 luminance (alpha preserved), and return the result as PNG. SVG content keeps its existing colorize path. Drop the IsResourceSVG branching in widget/button.go — NewDisabledResource now handles both kinds. Removes the disabledIconTranslucency constant and the Translucency field manipulation introduced earlier on this branch.
|
Thanks for the feedback — you're right, translucency was hiding the real fix. In Side effect: every other call site of Pushed as a follow-up commit, no force-push, so the diff against your previous review should be just |
andydotxyz
left a comment
There was a problem hiding this comment.
A couple of notes inline. Plus the static check errors would need to be addressed
| // 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. |
There was a problem hiding this comment.
I don't think that "since they cannot be recolored" in the docs is required - focus on behaviour in public docs.
There was a problem hiding this comment.
Fixed in e558fb3 — dropped the implementation reason, kept only the behaviour.
| // 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 { |
There was a problem hiding this comment.
As an internal API I would think that returning the error instead of logging it makes more sense.
There was a problem hiding this comment.
Done in e558fb3 — desaturate now returns ([]byte, error); Content() logs and falls back to the original bytes.
| // 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) |
There was a problem hiding this comment.
What are these magic numbers?
There was a problem hiding this comment.
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.
Summary
Disable()d, even though SVG-icon buttons were correctly grayed out viaNewDisabledResource. There was no visual cue that the button was disabled.Translucencyfade to thecanvas.Imageto give a visual disabled cue. SVG behaviour is unchanged.// TODO support disabling bitmap resource not just SVGinwidget/button.go.Scope / not closing #6217
The original report (#6217) mixes two related problems:
Button.Icon) not faded onDisable()— fixed here.🔝) not faded onDisable()— not addressed here, the fix would live in the text/painter pipeline rather than the button renderer. Tracked as #6383.So this PR intentionally uses
Refs #6217rather thanFixes #6217, to avoid auto-closing the original report before the emoji part is solved.Note on the translucency constant
disabledIconTranslucency = 0.5is a fixed value because we can't recolor a raster image withtheme.ColorNameDisabledthe way SVGs are recolored. The choice is documented inline — happy to switch to a theme-derived value (e.g. derived from the alpha ratio betweenColorNameDisabledandColorNameForeground) if you prefer.Test plan
go build ./...go test ./widget/ -run TestButton -count=1— all existing button tests pass plus the newTestButton_DisabledBitmapIcon(covers enable → disable → re-enable for a PNG resource).Iconset that the icon now fades onDisable()and restores onEnable().Refs #6217
Refs #6383