Adopt style colour for button icons #5493

luke-jr commented at 8:11 PM on December 16, 2014: member

No description provided.

luke-jr force-pushed on Dec 16, 2014

jonasschnelli commented at 9:03 PM on December 16, 2014: contributor

Looks somehow strange (i would say unusable) on OSX 10.10. bildschirmfoto 2014-12-16 um 22 01 46

I better liked the black icons.

Diapolo commented at 8:53 AM on December 17, 2014: none

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

laanwj commented at 9:27 AM on December 17, 2014: member

christmas_bitcoin 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.

in src/qt/scicon.cpp:None in c627c02c5e outdated

  20 | +        {
  21 | +            const QRgb rgb = img.pixel(x, y);
  22 | +            const int gray = 100 + qGray(rgb);
  23 | +            QColor colour = colourbase.lighter(gray);
  24 | +            colour.setAlpha(qAlpha(rgb));
  25 | +            img.setPixel(x, y, colour.rgba());

laanwj commented at 9:33 AM on December 17, 2014:

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)));

luke-jr commented at 9:45 AM on December 17, 2014:

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

laanwj commented at 9:51 AM on December 17, 2014:

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 commented at 9:38 AM on December 17, 2014: member

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

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

laanwj added the label GUI on Dec 17, 2014

in src/qt/scicon.h:None in c627c02c5e outdated

  10 | +
  11 | +extern QImage SingleColourImage(const QString& filename);
  12 | +extern QIcon SingleColourIcon(const QIcon&);
  13 | +extern QIcon SingleColourIcon(const QString& filename);
  14 | +
  15 | +#endif

Diapolo commented at 9:56 AM on December 17, 2014:

Nit: Missing header end comment.

in src/qt/scicon.cpp:None in c627c02c5e outdated

   9 | +#include <QImage>
  10 | +#include <QPalette>
  11 | +#include <QPixmap>
  12 | +
  13 | +static
  14 | +void MakeSingleColourImage(QImage& img, const QColor& colourbase)

Diapolo commented at 9:57 AM on December 17, 2014:

Can you stop doing that? We already had this discussion. static void MakeSingleColourImage(QImage& img, const QColor& colourbase)

in src/qt/scicon.cpp:None in c627c02c5e outdated

   0 | @@ -0,0 +1,61 @@
   1 | +// Copyright (c) 2014 The Bitcoin developers
   2 | +// Distributed under the MIT software license, see the accompanying
   3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +#include "scicon.h"
   6 | +
   7 | +#include <QColor>
   8 | +#include <QIcon>

Diapolo commented at 9:58 AM on December 17, 2014:

Already included in the header...

luke-jr force-pushed on Dec 17, 2014

luke-jr commented at 10:34 AM on December 17, 2014: member

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).

in src/qt/scicon.cpp:None in d6b5cd6b52 outdated

  15 | +{
  16 | +    img = img.convertToFormat(QImage::Format_ARGB32);
  17 | +    for (int x = img.width(); x--; )
  18 | +    {
  19 | +        for (int y = img.height(); y--; )
  20 | +        {

laanwj commented at 10:36 AM on December 17, 2014:

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

luke-jr commented at 10:45 AM on December 17, 2014:

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

luke-jr commented at 10:47 AM on December 17, 2014:

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

laanwj commented at 10:55 AM on December 17, 2014:

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?

laanwj commented at 10:57 AM on December 17, 2014:

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.

luke-jr commented at 10:58 AM on December 17, 2014:

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...

luke-jr commented at 11:02 AM on December 17, 2014:

Looks like it's just /icons/about

luke-jr force-pushed on Dec 17, 2014

in src/qt/scicon.cpp:None in 880e7aaa13 outdated

  19 | +        for (int y = img.height(); y--; )
  20 | +        {
  21 | +            const QRgb rgb = img.pixel(x, y);
  22 | +            QColor color = colorbase;
  23 | +            color.setAlpha(qAlpha(rgb));
  24 | +            img.setPixel(x, y, color.rgba());

laanwj commented at 1:02 PM on December 17, 2014:

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 commented at 1:06 PM on December 17, 2014: member

untitled

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 commented at 1:17 PM on December 17, 2014: contributor

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!

bildschirmfoto 2014-12-17 um 14 15 07

Adopt style colour for button icons 78535359d3

in src/qt/coincontroldialog.cpp:None in 880e7aaa13 outdated

  13 | @@ -14,6 +14,7 @@
  14 |  
  15 |  #include "coincontrol.h"
  16 |  #include "main.h"
  17 | +#include "scicon.h"

Diapolo commented at 6:11 PM on December 17, 2014:

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

in src/qt/rpcconsole.cpp:None in 880e7aaa13 outdated

  12 | @@ -13,6 +13,7 @@
  13 |  #include "chainparams.h"
  14 |  #include "rpcserver.h"
  15 |  #include "rpcclient.h"
  16 | +#include "scicon.h"

Diapolo commented at 6:12 PM on December 17, 2014:

Nit: Same here, this include belongs into the Qt include block. This here is for core includes :). Btw. thanks for sticking with the alphabetical ordering scheme :).

in src/qt/scicon.cpp:None in 880e7aaa13 outdated

   0 | @@ -0,0 +1,86 @@
   1 | +// Copyright (c) 2014 The Bitcoin developers
   2 | +// Distributed under the MIT software license, see the accompanying
   3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +#include "scicon.h"
   6 | +
   7 | +#include <QApplication>
   8 | +#include <QColor>

Diapolo commented at 6:13 PM on December 17, 2014:

QColor and QIcon are still included in the header already, so no need for them to be included here again.

in src/qt/signverifymessagedialog.cpp:None in 880e7aaa13 outdated

  10 | @@ -11,6 +11,7 @@
  11 |  
  12 |  #include "base58.h"
  13 |  #include "init.h"
  14 | +#include "scicon.h"

Diapolo commented at 6:14 PM on December 17, 2014:

Nit: Also wrong include block...

in src/qt/scicon.h:None in 880e7aaa13 outdated

   0 | @@ -0,0 +1,20 @@
   1 | +// Copyright (c) 2014 The Bitcoin developers
   2 | +// Distributed under the MIT software license, see the accompanying
   3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +#ifndef BITCOIN_QT_SCICON_H
   6 | +#define BITCOIN_QT_SCICON_H
   7 | +
   8 | +#include <QColor>

laanwj commented at 7:20 PM on December 17, 2014:

You don't have to include these here, you could get away with predeclarations, ie.

QT_BEGIN_NAMESPACE
class QString
class QColor
class QIcon
QT_END_NAMESPACE

in src/qt/scicon.h:None in 880e7aaa13 outdated

   7 | +
   8 | +#include <QColor>
   9 | +#include <QString>
  10 | +#include <QIcon>
  11 | +
  12 | +extern QImage SingleColorImage(const QString& filename, const QColor&);

laanwj commented at 7:21 PM on December 17, 2014:

No need to use extern in C++. We don't use it anywhere in the GUI code.

luke-jr force-pushed on Dec 18, 2014

luke-jr commented at 4:34 AM on December 18, 2014: member

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

laanwj commented at 8:24 AM on December 18, 2014: member

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

jonasschnelli commented at 8:47 AM on December 18, 2014: contributor

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

bildschirmfoto 2014-12-18 um 08 59 28

OSX 10.9

bildschirmfoto 2014-12-18 um 09 03 49

Ubuntu

bildschirmfoto 2014-12-18 um 09 36 17

bildschirmfoto 2014-12-18 um 09 36 49

bildschirmfoto 2014-12-18 um 09 42 31

bildschirmfoto 2014-12-18 um 09 45 09

luke-jr commented at 9:07 AM on December 18, 2014: member

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 commented at 9:14 AM on December 18, 2014: contributor

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 commented at 9:14 AM on December 18, 2014: member

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.

laanwj referenced this in commit e5153095ea on Dec 27, 2014

laanwj commented at 6:19 PM on December 27, 2014: member

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

Diapolo commented at 10:56 AM on December 28, 2014: none

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: overview

sipa closed this on Dec 28, 2014

laanwj commented at 9:47 AM on December 29, 2014: member

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

luke-jr commented at 9:59 AM on December 29, 2014: member

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

laanwj commented at 10:01 AM on December 29, 2014: member

@luke-jr too low contrast with the grey

jonasschnelli commented at 10:02 AM on December 29, 2014: contributor

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

Diapolo commented at 9:24 AM on December 31, 2014: none

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

luke-jr commented at 9:30 AM on December 31, 2014: member

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...

MarcoFalke locked this on Sep 8, 2021