I-Dream-in-Code / kde-arch-update-plasmoid

30 stars 9 forks source link

Configuration dialog text invisible if plasma color theme and application color theme are not the same #14

Closed Xabre666 closed 6 years ago

Xabre666 commented 6 years ago

I'm using Breeze Dark plasma theme but Breeze color scheme for applications. Which results in a white text on white background in configuration dialog.

https://i.imgur.com/hyqzcQb.png

Cause of the problem is that in ConfigGeneral.qml text color for items is defined as color: theme.TextColor, but configuration dialog is not a qml widget but a proper window, which uses color scheme for applications, not colors for the plasma theme. Is it possible to change it so that correct color scheme is used? Thank you.

I-Dream-in-Code commented 6 years ago

I'm out of town for for 3 days with no access to my computer so I'll get to it when I get back

I-Dream-in-Code commented 6 years ago

AFAIK there's no "color scheme.textColor" option in qml.

Are you saying the base root item on ConfigGeneral.qmlshould be changed Item to some other type?

Xabre666 commented 6 years ago

OK, first disclaimer: I'm not a coder, I'm just a guy who likes to tweak & break stuff, so I have no idea what I'm talking about, :)

However, these are my observations. Currently, in ConfigGeneral.qml, you have this:

    Text {
        text: i18n("Show upgrade on konsole")
        color: theme.textColor
    }
    CheckBox {
        id: konsoleCheckBox
    }

Result: text color is wrong if desktoptheme color scheme is different than application color scheme.

But, if instead you use this:

    CheckBox {
        id: konsoleCheckBox
        text: i18n("Show upgrade on konsole")
    }

and just like that, text is OK and automatically adjusted according to color scheme, no need to touch text color at all.

Another way to fix it is to use labels without need to define text color at all, ie substitute

    Text {
        text: i18n("Hide the updates version number")
        color: theme.textColor
    }

With

    Label {
        text: i18n("Hide the updates version number")
    }

Again, I'm not a coder so I have no idea if any of what I'm suggesting is sane/may break stuff.

Xabre666 commented 6 years ago

I've actually tested substituting Text {} with Label {} and it seems to work perfectly here

https://i.imgur.com/oTWOrTn.png (edit: fixed image link)

I-Dream-in-Code commented 6 years ago

Good call on the label.

One final question before I merge, does the package list suffer from this issue too or just the settings?

Xabre666 commented 6 years ago

No, package list is fine, text color is adjusted according to desktop theme that's currently used. With Breeze Plasma theme it's black text on white background, and with Breeze Dark it's white text on black background. Issue appeared simply because settings window is a separate window, not not exactly a plasmoid , if that makes any sense.

I-Dream-in-Code commented 6 years ago

That's what I figured. It's a plasmoid so it defaultly uses theme vs color scheme.

I'll merge it right now and update AUR package and master

I-Dream-in-Code commented 6 years ago

both are updated

please confirm that it works

Xabre666 commented 6 years ago

Rebuild & now everything looks great. Thank you.

I-Dream-in-Code commented 6 years ago

Cool

I would never has figured out Label vs text so thank you for solving it. :)

I-Dream-in-Code commented 6 years ago

I also fixed the alignment on the interval spinbox