Skip to content

Conversation

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Jun 23, 2015

A cleaner alternative to #6327

@Diapolo
Copy link

Diapolo commented Jun 24, 2015

+9 / -3 yours
+7 / -7 mine :-P

Well I like mine better, as I removed an #ifdef ;).

@jonasschnelli
Copy link
Contributor

ACK

@laanwj
Copy link
Member

laanwj commented Jun 25, 2015

We had this discussion before, shortly ago: #6143

For the sake of making sure as much code gets compiled for every platform, logic that does not use specific platform-dependent APIs (and thus potentially compiles on other platforms) should not be in platform-specific #ifdefs.

E.g. use MACOSX/WINDOWS to determine a UIPlatformStyle enum once, then pass that on. This could be overridden for testing. This would also apply to things such as "display images on buttons" which are currently #ifdefed.

(Or instead of an enum: this could work like NetworkStyle, where the class itself contains a list of platform specific customization flags, e.g. showImagesOnButton, colorizeIcons, etc)

@laanwj laanwj added the GUI label Jun 25, 2015
@laanwj
Copy link
Member

laanwj commented Jul 28, 2015

Closing in favor of #6487, which implements the approach outlined in my above post.

@laanwj laanwj closed this Jul 28, 2015
laanwj added a commit to laanwj/bitcoin that referenced this pull request Jul 31, 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 bitcoin#6328.
@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.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants