qt: Introduce PlatformStyle #6487

pull laanwj wants to merge 1 commits into bitcoin:master from laanwj:2015_07_qt_platformstyle changing 37 files +462 −318

laanwj commented at 2:22 PM on July 28, 2015: member

Introduce a PlatformStyle to handle platform-specific customization of the UI.

This replaces 'scicon', as well as #ifdefs to determine whether to place icons on buttons.

The selected PlatformStyle defaults to the platform that the application was compiled on, but can be overridden from the command line with -uiplatform=<x>.

Also fixes the warning from #6328.
laanwj added the label GUI on Jul 28, 2015
jonasschnelli commented at 2:31 PM on July 28, 2015: contributor

Very nice! Plans to test this in detail.

Wouldn't it be possible to crate a PlattformBase class inherited from QObject, instead of passing PlatformStyle in most of the ui based constructors? Or is this not possible because of different superclasses?
laanwj commented at 2:40 PM on July 28, 2015: member

Wouldn't it be possible to crate a PlattformBase class inherited from QObject, instead of passing PlatformStyle in most of the ui based constructors? Or is this not possible because of different superclasses?

I don't think that would work. There are various other ways to 'hide' the dependency, for example a singleton pattern, but I strongly prefer passing dependencies explicitly.

The only place it is unfortunate is in the WalletModel / TransactionTableModel. Ideally only the view cares about the style, not the model. But the decoration icon has to be in the platform style. Could be fixed later with a delegate but this is the most straightforward.
jonasschnelli commented at 2:41 PM on July 28, 2015: contributor

Fair enough. Better pass – like you introduce it in this PR – than a singleton.
laanwj added the label Refactoring on Jul 29, 2015

in src/qt/bitcoin.cpp:None in 3b7e2e7b21 outdated

 438 | @@ -421,6 +439,8 @@ void BitcoinApplication::initializeResult(int retval)
 439 |      returnValue = retval ? 0 : 1;
 440 |      if(retval)
 441 |      {
 442 | +        // Log this only after AppInit2 finishes, as then logging setup is guaranteed complete
 443 | +        qWarning() << "Platform customization:" << platformStyle->getName();

Diapolo commented at 9:50 AM on July 29, 2015:

Nit: Misses a space after :.

laanwj commented at 4:44 PM on July 29, 2015:

qt adds that, somehow

in src/qt/platformstyle.cpp:None in 3b7e2e7b21 outdated
```
  25 | +    {"macosx", false, false, true},
  26 | +    {"windows", true, false, false},
  27 | +    /* Other: linux, unix, ... */
  28 | +    {"other", true, true, false}
  29 | +};
  30 | +static const unsigned platform_styles_count = sizeof(platform_styles)/sizeof(*platform_styles);
```
Diapolo commented at 9:56 AM on July 29, 2015:

This is only used once, do we need this const here?

laanwj commented at 4:47 PM on July 29, 2015:

Why not? It's analogous to networkstyle.cpp, and it makes the code more readable by using a name instead of a long in-line expression.
jonasschnelli commented at 1:19 PM on July 29, 2015: contributor

Tested ACK. It works. Binaries: https://builds.jonasschnelli.ch/pulls/6487/

Screens OSX: Other platform on OSX, icons on button, colored (but black as expected): <img width="1062" alt="bildschirmfoto 2015-07-29 um 14 59 41" src="https://cloud.githubusercontent.com/assets/178464/8958315/6969ebd4-3604-11e5-914c-633cc89ee4d3.png">

No parameter passed: <img width="948" alt="bildschirmfoto 2015-07-29 um 15 14 38" src="https://cloud.githubusercontent.com/assets/178464/8958343/8ed2cdaa-3604-11e5-9e79-aa6748249f70.png">

Screens Ubuntu: OSX: <img width="419" alt="bildschirmfoto 2015-07-29 um 15 17 15" src="https://cloud.githubusercontent.com/assets/178464/8958407/fd5a97a8-3604-11e5-85f0-681ca9edd114.png">

No parameter passed: <img width="420" alt="bildschirmfoto 2015-07-29 um 15 17 28" src="https://cloud.githubusercontent.com/assets/178464/8958408/fd5ea942-3604-11e5-855f-a9a9d5f57157.png">

Screens Windows: OSX on Windows (no icons): <img width="450" alt="bildschirmfoto 2015-07-29 um 15 12 09" src="https://cloud.githubusercontent.com/assets/178464/8958297/48e4a8d6-3604-11e5-912f-3aaaa8aa6006.png">

Other Platform on Windows (colored icons): <img width="549" alt="bildschirmfoto 2015-07-29 um 15 11 51" src="https://cloud.githubusercontent.com/assets/178464/8958296/48e27fca-3604-11e5-86d4-117f87db47c5.png">

No -uiplatform parameter passed (standard): <img width="544" alt="bildschirmfoto 2015-07-29 um 15 11 36" src="https://cloud.githubusercontent.com/assets/178464/8958298/48e60c62-3604-11e5-9bef-56aea3619648.png">
laanwj force-pushed on Jul 29, 2015
laanwj commented at 4:54 PM on July 29, 2015: member

Again thanks for the extensive testing @jonasschnelli !

Squashed into one commit.

qt: Introduce PlatformStyle

Introduce a PlatformStyle to handle platform-specific customization of
the UI.

This replaces 'scicon', as well as #ifdefs to determine whether to place
icons on buttons.

The selected PlatformStyle defaults to the platform that the application
was compiled on, but can be overridden from the command line with
`-uiplatform=<x>`.

Also fixes the warning from #6328.

eec7757445

laanwj force-pushed on Jul 31, 2015

laanwj commented at 7:37 AM on July 31, 2015: member

Added a comment to per-platform initialization in BitcoinApplication::BitcoinApplication.

    // This must be done inside the BitcoinApplication constructor, or after it, because
    // PlatformStyle::instantiate requires a QApplication

laanwj merged this on Jul 31, 2015
laanwj closed this on Jul 31, 2015
laanwj referenced this in commit 9848d4297b on Jul 31, 2015
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#6328 qt/scicon: Avoid warnings about unused MakeSingleColorImage