Open andydotxyz opened 3 months ago
I believe it relates to the OnPrimary work @toaster did - I don't think it should be intended behavior and be treated as a bug to be fixed in 2.5.x, but curious to hear Tilo's thoughts.
As I attempted to create a "minimal" example to demonstrate and better understand the issue, I realized the title is not entirely correct and it is A LOT more complicated.
This with v2.5
This with v2.4.3
The Code
package main
import (
"fyne.io/fyne/v2"
"fyne.io/fyne/v2/app"
"fyne.io/fyne/v2/container"
"fyne.io/fyne/v2/theme"
"fyne.io/fyne/v2/widget"
)
func main() {
app := app.New()
window := app.NewWindow("")
successIcon := theme.NewSuccessThemedResource(theme.RadioButtonCheckedIcon())
successCol := container.NewVBox(
&widget.Label{Text: "success", Alignment: fyne.TextAlignCenter},
&widget.Button{Text: "low", Importance: widget.LowImportance, Icon: successIcon},
&widget.Button{Text: "mid", Importance: widget.MediumImportance, Icon: successIcon},
&widget.Button{Text: "high", Importance: widget.HighImportance, Icon: successIcon},
&widget.Button{Text: "dngr", Importance: widget.DangerImportance, Icon: successIcon},
&widget.Button{Text: "sccss", Importance: widget.SuccessImportance, Icon: successIcon},
&widget.Button{Text: "warn", Importance: widget.WarningImportance, Icon: successIcon},
)
warningIcon := theme.NewWarningThemedResource(theme.RadioButtonCheckedIcon())
warningCol := container.NewVBox(
&widget.Label{Text: "warning", Alignment: fyne.TextAlignCenter},
&widget.Button{Text: "low", Importance: widget.LowImportance, Icon: warningIcon},
&widget.Button{Text: "mid", Importance: widget.MediumImportance, Icon: warningIcon},
&widget.Button{Text: "high", Importance: widget.HighImportance, Icon: warningIcon},
&widget.Button{Text: "dngr", Importance: widget.DangerImportance, Icon: warningIcon},
&widget.Button{Text: "sccss", Importance: widget.SuccessImportance, Icon: warningIcon},
&widget.Button{Text: "warn", Importance: widget.WarningImportance, Icon: warningIcon},
)
errorIcon := theme.NewErrorThemedResource(theme.RadioButtonCheckedIcon())
errorCol := container.NewVBox(
&widget.Label{Text: "error", Alignment: fyne.TextAlignCenter},
&widget.Button{Text: "low", Importance: widget.LowImportance, Icon: errorIcon},
&widget.Button{Text: "mid", Importance: widget.MediumImportance, Icon: errorIcon},
&widget.Button{Text: "high", Importance: widget.HighImportance, Icon: errorIcon},
&widget.Button{Text: "dngr", Importance: widget.DangerImportance, Icon: errorIcon},
&widget.Button{Text: "sccss", Importance: widget.SuccessImportance, Icon: errorIcon},
&widget.Button{Text: "warn", Importance: widget.WarningImportance, Icon: errorIcon},
)
primaryIcon := theme.NewPrimaryThemedResource(theme.RadioButtonCheckedIcon())
primaryCol := container.NewVBox(
&widget.Label{Text: "primary", Alignment: fyne.TextAlignCenter},
&widget.Button{Text: "low", Importance: widget.LowImportance, Icon: primaryIcon},
&widget.Button{Text: "mid", Importance: widget.MediumImportance, Icon: primaryIcon},
&widget.Button{Text: "high", Importance: widget.HighImportance, Icon: primaryIcon},
&widget.Button{Text: "dngr", Importance: widget.DangerImportance, Icon: primaryIcon},
&widget.Button{Text: "sccss", Importance: widget.SuccessImportance, Icon: primaryIcon},
&widget.Button{Text: "warn", Importance: widget.WarningImportance, Icon: primaryIcon},
)
invertedIcon := theme.NewInvertedThemedResource(theme.RadioButtonCheckedIcon())
invertedCol := container.NewVBox(
&widget.Label{Text: "inverted", Alignment: fyne.TextAlignCenter},
&widget.Button{Text: "low", Importance: widget.LowImportance, Icon: invertedIcon},
&widget.Button{Text: "mid", Importance: widget.MediumImportance, Icon: invertedIcon},
&widget.Button{Text: "high", Importance: widget.HighImportance, Icon: invertedIcon},
&widget.Button{Text: "dngr", Importance: widget.DangerImportance, Icon: invertedIcon},
&widget.Button{Text: "sccss", Importance: widget.SuccessImportance, Icon: invertedIcon},
&widget.Button{Text: "warn", Importance: widget.WarningImportance, Icon: invertedIcon},
)
disabledIcon := theme.NewDisabledResource(theme.RadioButtonCheckedIcon())
disabledCol := container.NewVBox(
&widget.Label{Text: "disabled", Alignment: fyne.TextAlignCenter},
&widget.Button{Text: "low", Importance: widget.LowImportance, Icon: disabledIcon},
&widget.Button{Text: "mid", Importance: widget.MediumImportance, Icon: disabledIcon},
&widget.Button{Text: "high", Importance: widget.HighImportance, Icon: disabledIcon},
&widget.Button{Text: "dngr", Importance: widget.DangerImportance, Icon: disabledIcon},
&widget.Button{Text: "sccss", Importance: widget.SuccessImportance, Icon: disabledIcon},
&widget.Button{Text: "warn", Importance: widget.WarningImportance, Icon: disabledIcon},
)
focusIcon := theme.NewColoredResource(theme.RadioButtonCheckedIcon(), theme.ColorNameFocus)
focusCol := container.NewVBox(
&widget.Label{Text: "focus", Alignment: fyne.TextAlignCenter},
&widget.Button{Text: "low", Importance: widget.LowImportance, Icon: focusIcon},
&widget.Button{Text: "mid", Importance: widget.MediumImportance, Icon: focusIcon},
&widget.Button{Text: "high", Importance: widget.HighImportance, Icon: focusIcon},
&widget.Button{Text: "dngr", Importance: widget.DangerImportance, Icon: focusIcon},
&widget.Button{Text: "sccss", Importance: widget.SuccessImportance, Icon: focusIcon},
&widget.Button{Text: "warn", Importance: widget.WarningImportance, Icon: focusIcon},
)
window.SetContent(
container.NewHBox(
successCol,
warningCol,
errorCol,
primaryCol,
invertedCol,
disabledCol,
focusCol,
),
)
window.ShowAndRun()
}
Yes, this is related to my work on the colors. I’m not sure whether we can fix this for 2.5 without reverting the usage of the new colors for buttons at all.
For 2.6 there are further PRs in the pipeline which should streamline the “on” colors and which did not make it into the 2.5 release.
Might be, that even more work is necessary on the aforementioned PRs to make the *ThemedResource
consistent again.
Regarding the icon colors: I strongly believe that they should be the same as the text color. IMO, one should not create a button with a state (e.g. “success”) themed resource but a state button. If one does not want the background to show the state but the foreground, one could override the state colors in the theme to achieve this.
Regardless of their originally intended use, it has been possible to use ThemedResource to customize button icon colors since ThemedResource was introduced, and there are apps out there that depend on that behavior, so the change always seemed like a regression to me, though I feel like I had been overruled.
My app is also one that would be impacted since I use buttons for a navigation control:
Though for now I am OK since I only change icon colors in MediumImportance buttons. I wonder if we need more importance levels to make up for the lost functionality (like a "MediumHighImportance" that colors the icon and the text but leaves the button background the standard color?) - that one would solve my use case at least
Discussed in https://github.com/fyne-io/fyne/discussions/5020