Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

qt: Introduce PlatformStyle #6487

Merged
merged 1 commit into from Jul 31, 2015
Merged

Conversation

laanwj
Copy link
Member

@laanwj laanwj commented Jul 28, 2015

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 laanwj added the GUI label Jul 28, 2015
@jonasschnelli
Copy link
Contributor

jonasschnelli commented Jul 28, 2015

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
Copy link
Member Author

laanwj commented Jul 28, 2015

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
Copy link
Contributor

jonasschnelli commented Jul 28, 2015

Fair enough. Better pass – like you introduce it in this PR – than a singleton.

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

@Diapolo Diapolo Jul 29, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Misses a space after :.

Copy link
Member Author

@laanwj laanwj Jul 29, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qt adds that, somehow

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Jul 29, 2015

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):
bildschirmfoto 2015-07-29 um 14 59 41

No parameter passed:
bildschirmfoto 2015-07-29 um 15 14 38

Screens Ubuntu:
OSX:
bildschirmfoto 2015-07-29 um 15 17 15

No parameter passed:
bildschirmfoto 2015-07-29 um 15 17 28

Screens Windows:
OSX on Windows (no icons):
bildschirmfoto 2015-07-29 um 15 12 09

Other Platform on Windows (colored icons):
bildschirmfoto 2015-07-29 um 15 11 51

No -uiplatform parameter passed (standard):
bildschirmfoto 2015-07-29 um 15 11 36

@laanwj laanwj force-pushed the 2015_07_qt_platformstyle branch from 3b7e2e7 to 07810e4 Compare Jul 29, 2015
@laanwj
Copy link
Member Author

laanwj commented Jul 29, 2015

Again thanks for the extensive testing @jonasschnelli !

Squashed into one commit.

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 bitcoin#6328.
@laanwj laanwj force-pushed the 2015_07_qt_platformstyle branch from 07810e4 to eec7757 Compare Jul 31, 2015
@laanwj
Copy link
Member Author

laanwj commented Jul 31, 2015

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 laanwj merged commit eec7757 into bitcoin:master Jul 31, 2015
laanwj added a commit that referenced this pull request Jul 31, 2015
eec7757 qt: Introduce PlatformStyle (Wladimir J. van der Laan)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants