Allow changing the foreground/text color of buttons
Needs ReviewPublic

Authored by thiagoneves on Feb 8 2019, 4:09 PM.

Details

Reviewers
cfeck
tcanabrava

Diff Detail

Repository
R353 KCalc
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8063
Build 8081: arc lint + arc unit
thiagoneves created this revision.Feb 8 2019, 4:09 PM
Restricted Application added a subscriber: kde-utils-devel. · View Herald TranscriptFeb 8 2019, 4:09 PM
thiagoneves requested review of this revision.Feb 8 2019, 4:09 PM
cfeck added a comment.Feb 8 2019, 5:03 PM

Could you change the UI to have three columns? It already uses a grid layout, so you can insert a third column.

Button colors
Functions: [FG] [BG]
Numbers:  [FG] {BG]

etc.

This way it is easier to see the color combinations.

kcalc.cpp
2076

No need to create a color scheme twice. Please reuse schemeButtons for button and text color.

2082

Please change casts to qobject_cast (here and below).

kcalc_button.cpp
168

This change can be reverted. The palette.setColor() call below is sufficient.

171

textColor() returns a QColor, so the cast is not needed.

232

Color does not affect the size. Can be removed.

kcalc_button.h
70

QColor textColor() const

93

Please rename to text_color_ to make it consistent with other member variables.

cfeck requested changes to this revision.EditedFeb 8 2019, 5:15 PM

There is a usability issue with the MR button when the memory is empty. Setting the text color overrides the lighter color that is used for disabled buttons.

I suggest to use something like setAlphaF(0.7) on the color for disabled() buttons before modifying the context's palette.

This revision now requires changes to proceed.Feb 8 2019, 5:15 PM
thiagoneves updated this revision to Diff 51249.Feb 9 2019, 1:24 PM

Updating D18854: Allow changing the foreground/text color of buttons

cfeck added inline comments.Feb 23 2019, 2:28 PM
kcalc.cpp
2076

I didn't want you to remove the complete code. If the colors are not changed, no style sheets should be set. See bug 237513.

kcalc_button.cpp
171

Better modify a local QColor variable before setting. This way you can remove the setEnabled() overload.