Adopt style colour for button icons #5493

luke-jr · 2014-12-16T20:11:23Z

No description provided.

jonasschnelli · 2014-12-16T21:03:52Z

Looks somehow strange (i would say unusable) on OSX 10.10.

I better liked the black icons.

Diapolo · 2014-12-17T08:53:03Z

I also don't like what @jonasschnelli posted...

laanwj · 2014-12-17T09:27:02Z

Christmas bitcoin

I love playing around with this, make the color selectable, go crazy, but please default to neutral black.

Or at least the font color for dark themes, if black has too little contrast with the background?

However, it looks like your current code ignores the theme by using static QColor(palette), and uses the silly Qt default instead. This must pass through QApplication: ie

const QColor colourbase(QApplication::palette().text().color());

Nit: s/colour/color everywhere. US English is what Qt uses so should we.

laanwj · 2014-12-17T09:33:44Z

src/qt/scicon.cpp

+            const int gray = 100 + qGray(rgb);
+            QColor colour = colourbase.lighter(gray);
+            colour.setAlpha(qAlpha(rgb));
+            img.setPixel(x, y, colour.rgba());


No need for this construction with lighter(). Just substitute the color, keep the alpha:

const QRgb rgb = img.pixel(x, y); img.setPixel(x, y, qRgba(base.red(), base.green(), base.blue(), qAlpha(rgb)));

If I drop .lighter, won't everything become the same colour shade (ignoring alpha)?

I haven't analyzed @jonasschnelli 's icons, but my observation was that everything already is in the same shade of black, the deepest shade of black available, and it is just alpha that varies.

laanwj · 2014-12-17T09:38:49Z

Less funny, but using font color as above results in much better:

BTW: the icons in the menu are still the original black, they need to be color-mangled too.

Diapolo · 2014-12-17T09:56:44Z

src/qt/scicon.h

+extern QIcon SingleColourIcon(const QIcon&);
+extern QIcon SingleColourIcon(const QString& filename);
+
+#endif


Nit: Missing header end comment.

luke-jr · 2014-12-17T10:34:04Z

Text colour (black, in my case) is exactly the boring/ugly I'm trying to avoid here... How's this new version look on your platforms?

The logic now is:
Buttons and such use the highlight colour or highlighted-text colour, depending on whichi one contrasts less with the window text (which is reasonably assumed to itself contrast with the background). Menus and the transaction list use text colour, to avoid becoming invisible when selected (which uses the highlight colour).

laanwj · 2014-12-17T10:36:31Z

src/qt/scicon.cpp

+    for (int x = img.width(); x--; )
+    {
+        for (int y = img.height(); y--; )
+        {


Can you at least take my comment into account? Does this code make any difference with what I gave?

From a basic run:

2 gray:120 2 gray:127 2 gray:128 2 gray:129 4 gray:118 4 gray:122 4 gray:124 6 gray:121 6 gray:123 8 gray:117 10 gray:115 10 gray:119 12 gray:114 12 gray:125 16 gray:116 42 gray:113 54 gray:111 76 gray:112 106 gray:107 110 gray:109 124 gray:105 174 gray:108 176 gray:110 216 gray:104 232 gray:106 434 gray:103 454 gray:101 572 gray:102 263287 gray:100

(that's approximately 1% of sampled pixels that are lighter than solid block)

I'm sure the base level is 0 i.e. absolute black, not 100.

Are you maybe counting the color of pixels with zero alpha (which you can't see anyway)? Or feeding the odd non-onecolor old icon through it?

In any case this is not a lot of variability. I leave it up to @jonasschnelli to confirm this, but I'd say it looks accidental and they're supposed to be 100% one-color.

I was printing the gray variable, which is qGray + 100

I was not counting the zero-alpha pixels, though I have no idea what icons were being fed through it...

Looks like it's just /icons/about

laanwj · 2014-12-17T13:02:52Z

src/qt/scicon.cpp

+            const QRgb rgb = img.pixel(x, y);
+            QColor color = colorbase;
+            color.setAlpha(qAlpha(rgb));
+            img.setPixel(x, y, color.rgba());


Can be reduced to two lines, you don't have to go through QColor at all:

const QRgb rgb = img.pixel(x, y); img.setPixel(x, y, qRgba(base.red(), base.green(), base.blue(), qAlpha(rgb)));

laanwj · 2014-12-17T13:06:11Z

I like it, it still looks serious but less boring.
I still think MacOSX users prefer a #ifdef MACOSX color=DeepestBlack() #endif but for weird themes like mine this is a definite improvement.

jonasschnelli · 2014-12-17T13:17:10Z

Now it looks like this on OSX (see below).
I would say it looks the same as it looked before this PR.

On @laanwj screen it looks really nice with the white icons!

Diapolo · 2014-12-17T18:11:32Z

src/qt/coincontroldialog.cpp

@@ -14,6 +14,7 @@

 #include "coincontrol.h"
 #include "main.h"
+#include "scicon.h"


Nit: This include belongs into the Qt include block. This here is for core includes :).

luke-jr · 2014-12-18T04:34:02Z

Changes made. Per @jonasschnelli's review, I think the Mac-specific change @laanwj requested is unnecessary?

laanwj · 2014-12-18T08:24:31Z

It looks like that is already automatic, which is better than making a special exception.

jonasschnelli · 2014-12-18T08:47:58Z

Now it look like this on my environments (see below).

On OSX the patch does not show any (visible) changes to the icons. Should we then not disable the manipulations to save ticks?
Switching the theme (Ubuntu) will not update the Icon style (IMO this would be a nice luxury feature but not a must)
Menu icons color change is missing
Needs windows testing and some posted screens

OSX 10.10

OSX 10.9

Ubuntu

luke-jr · 2014-12-18T09:07:22Z

Does OS X not permit users to customise their style? In any case, IMO it'd be ugly to #ifdef just to save some ticks we assume won't ever be needed...
I don't know if there's a way to even detect this?
Menu icons intentionally use a different colour (text colour) than buttons (highlight colour). Otherwise, they would become HighlightColour-on-HighlightColour, which is invisible/ugly.

Is the grey-on-grey menu icons with a case of style change going on? Or is Qt misreporting text colour somehow in that case?

jonasschnelli · 2014-12-18T09:14:14Z

Does OS X not permit users to customise their style? In any case, IMO it'd be ugly to #ifdef just to save some ticks we assume won't ever be needed...

I just tried. Could not manage to get some other icon color than the black one on my OSX 10.10.

I don't know if there's a way to even detect this?

You could re-test and compare the text-color when the main window becomes focus?

Menu icons intentionally use a different colour (text colour) than buttons (highlight colour). Otherwise, they would become HighlightColour-on-HighlightColour, which is invisible/ugly.

Is there no way to detect the menu background color and do a HLS change on 180° so we can get the "opposite color"? Or maybe this looks bad. Maybe only detect the black-value/threshold and decide to color the icons black or white?

laanwj · 2014-12-18T09:14:25Z

The standard Ubuntu style is nice. I like how it picks up the orange.

I first was afraid of the overhead too but then remembered that this only happens at window construction time, not every time the icons are rendered. So if this doesn't visibly affect start-up time, I'd prefer a ifdeff-less solution.
Don't bother. I don't think it's possible to detect this without platform-specific hacks. And we don't even support switching languages at runtime, let alone switching themes. This gets into waste-of-developer-time territory. Let them just restart the application.
On Ubuntu you can't get the color of the menu, as the menu is not part of your application but external. It will also use a different them than your application.
Yes, needs windows testing.

9b7d3fb Adopt style colour for button icons (Luke Dashjr)

laanwj · 2014-12-27T18:19:52Z

Merged via e515309, further fixes can be applied later, but merging this before it runs too much out of sync

Diapolo · 2014-12-28T10:56:35Z

I don't understand why the icons now look blueish and why that "adopts style colour for button icons"? The commit-msg doesn't tell me what this does ;). Anyhow this is now on Win8.1:

laanwj · 2014-12-29T09:47:36Z

Yes, definitely shouldn't be light blue on windows. Thanks for testing.

luke-jr · 2014-12-29T09:59:44Z

It's light-blue to match the highlight colour - looks fine to me?

laanwj · 2014-12-29T10:01:53Z

@luke-jr too low contrast with the grey

jonasschnelli · 2014-12-29T10:02:52Z

light-blue is not a good choice for windows. @Diapolo could you post some windows screens when manipulating your Theme or color-schema?

Diapolo · 2014-12-31T09:24:09Z

This was a high contrast theme, all other normal default Win81 themes didn't change the light-blue:

luke-jr · 2014-12-31T09:30:42Z

The blue is essentially what I get here in KDE. I think it looks good both with the blue and your high contrast screenshot. Please suggest an alternative, if you don't like it...

luke-jr force-pushed the match_colour branch from c2c2e76 to c627c02 Compare December 16, 2014 20:12

laanwj reviewed Dec 17, 2014
View reviewed changes

laanwj added the GUI label Dec 17, 2014

Diapolo reviewed Dec 17, 2014
View reviewed changes

luke-jr force-pushed the match_colour branch 2 times, most recently from cb3361b to d6b5cd6 Compare December 17, 2014 10:30

laanwj reviewed Dec 17, 2014
View reviewed changes

luke-jr force-pushed the match_colour branch from d6b5cd6 to 880e7aa Compare December 17, 2014 11:06

laanwj reviewed Dec 17, 2014
View reviewed changes

Diapolo reviewed Dec 17, 2014
View reviewed changes

Adopt style colour for button icons

7853535

luke-jr force-pushed the match_colour branch from 880e7aa to 7853535 Compare December 18, 2014 04:32

laanwj added a commit that referenced this pull request Dec 27, 2014

Merge pull request #5493

e515309

9b7d3fb Adopt style colour for button icons (Luke Dashjr)

sipa closed this Dec 28, 2014

jonasschnelli mentioned this pull request Jan 9, 2015

[QT] Win: icon coloring issue tray-menu #5625

Closed

jonasschnelli mentioned this pull request Dec 17, 2020

qt: Support macOS Dark mode bitcoin-core/gui#154

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Adopt style colour for button icons #5493

Adopt style colour for button icons #5493

Uh oh!

Conversation

luke-jr commented Dec 16, 2014

Uh oh!

jonasschnelli commented Dec 16, 2014

Uh oh!

Diapolo commented Dec 17, 2014

Uh oh!

laanwj commented Dec 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Dec 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Dec 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Dec 17, 2014

Uh oh!

jonasschnelli commented Dec 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Dec 18, 2014

Uh oh!

laanwj commented Dec 18, 2014

Uh oh!

jonasschnelli commented Dec 18, 2014

OSX 10.10

OSX 10.9

Ubuntu

Uh oh!

luke-jr commented Dec 18, 2014

Uh oh!

jonasschnelli commented Dec 18, 2014

Uh oh!

laanwj commented Dec 18, 2014

Uh oh!

laanwj commented Dec 27, 2014

Uh oh!

Diapolo commented Dec 28, 2014

Uh oh!

laanwj commented Dec 29, 2014

Uh oh!

luke-jr commented Dec 29, 2014

Uh oh!

laanwj commented Dec 29, 2014

Uh oh!

jonasschnelli commented Dec 29, 2014

Uh oh!

Diapolo commented Dec 31, 2014

Uh oh!

luke-jr commented Dec 31, 2014

Uh oh!

Uh oh!