| ngraham | |
| davidedmundson |
| Plasma | |
| VDG |
Old code had no way to adapt icon colors based on background so this patch uses different components to color the icons black with light backgrounds, and white with dark backgrounds.
Because Plasma Components 2.0 ToolButton doesn't support icon colors and because with 3.0 the code doesn't work, we have to port the affected buttons to QQC2 ToolButton.
BUG: 353819
FIXED-IN: 5.17
Before:
After:
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
Seems like that could be one of the solutions: https://bugs.kde.org/show_bug.cgi?id=353819#c2
But might also be less precise; the icon color values are grabbed from textColor so this way we know they'll match text color.
I don't like hardcoding the things we're comparing against. If we ever add more colors besides "black" or "translucent-light", or of the actual color values of those change (unlikely, but possible), all these buttons will need their code to be adjusted and that'll be easy to forget. I feel like we should be programmatically sampling the background color itself.
Also, if you're ever tempted to duplicate the same logic several times, that's a sure sign it should be in a function. Then each button can just call that function.
Finally porting from PlasmaComponents to QQC2 seems unrelated (or at least the relationship isn't clear from the description). Maybe we should first do that separately.
I get what you're saying but that's just how the applet is designed. It's preconfigured SVG backgrounds (you can find them in /usr/share/plasma/desktoptheme/default/widgets/notes.svgz). I'm not really introducing any hardcoding that isn't already there. See the hardcoding of text color:
style: PlasmaStyle.TextAreaStyle {
//this is deliberately _NOT_ the theme color as we are over a known bright background
//an unknown colour over a known colour is a bad move as you end up with white on yellow
textColor: (plasmoid.configuration.color === "black" || plasmoid.configuration.color === "translucent-light") ? "#dfdfdf" : "#202020"
}So one thing is if we'd want to rewrite the whole thing to be a rectangle and then you could select a huge amount of colors as the background I guess, as well as have a spinbox for opacity. Also tagging @broulik as he might know better why it's not like that in the first place.
The other thing about components is that Plasma Components doesn't support icon.color. I'd have kept using it, but PC2 spews out an error, while with PC3 it didn't complain but didn't work anyway.
Gotcha. At least use a function instead of duplicating the same code in each button.
The other thing about components is that Plasma Components doesn't support icon.color. I'd have kept using it, but PC2 spews out an error, while with PC3 it didn't complain but didn't work anyway.
OK, that makes sense. Can you make it a bit more clear in the Description?
Ah yeah, rereading your original comment I now see that bit went over my head, that would be much better so I will make those changes.
The other thing about components is that Plasma Components doesn't support icon.color. I'd have kept using it, but PC2 spews out an error, while with PC3 it didn't complain but didn't work anyway.
OK, that makes sense. Can you make it a bit more clear in the Description?
Yep!
| applets/notes/package/contents/ui/main.qml | ||
|---|---|---|
| 60 | Add a space between the slashes and the text. Also since each line is a sentence here, some punctuation might make that more clear! | |
| applets/notes/package/contents/ui/main.qml | ||
|---|---|---|
| 63 | Make that readonly property color textIconColor: {
if (plasmoid....) {
...
}
...
} | |
| 261–265 | Not too happy of using desktop buttons here instead of plasma components but custom color stuff doesn't work in Plasma. Can you make the button follow the size of a normal Plasma ToolButton or the units.iconSizes or something at least? They are a bit tiny here because they rely on Qt scaling which Plasma doesn't do | |
| applets/notes/package/contents/ui/main.qml | ||
|---|---|---|
| 261–265 | I see what you mean, they really are tiny. The problem is the button itself can be made bigger, but the icons remain the same. I tried to set icon.width and icon.height but it doesn't do anything. I then tried patching the PC3 ToolButton and it was only a matter of replacing the PlasmaCore.IconItem with Kirigami.Icon and adding color: control.icon.color. Then we get a working icon.color property in PC3, although since it picks up on the Plasma this might happen:  Thoughts on how to proceed? | |
I tried to set icon.width and icon.height but it doesn't do anything.
In desktop style that should work since D20418
Tried it in Neon unstable as well but the icons always remained the same. Does it work for you if you modify this patch?
@broulik I tried again in master and icon.width and icon.height aren't working for me with this patch.