qt: add network-specific style object #5066

laanwj commented at 9:15 AM on October 9, 2014: member

Mainly cleanups: Gets rid of isTestNet everywhere, by keeping track of network-specific theming in a central place.

Also makes GUI no longer dependent on the network ID enumeration, which alleviates concerns about #4802.

laanwj force-pushed on Oct 9, 2014

in src/qt/networkstyle.cpp:None in b42db95fe6 outdated

   8 | +    const char *appIcon;
   9 | +    const char *titleAddText;
  10 | +    const char *splashImage;
  11 | +} network_styles[] = {
  12 | +    {"main", QAPP_APP_NAME_DEFAULT, ":/icons/bitcoin", "", ":/images/splash"},
  13 | +    {"test", QAPP_APP_NAME_TESTNET, ":/icons/bitcoin_testnet", "[testnet]", ":/images/splash_testnet"},

Diapolo commented at 9:22 AM on October 9, 2014:

As is, this removes the possibility to translate testnet or regtest. Is this intended?

laanwj commented at 9:30 AM on October 9, 2014:

Do we really want people to translate the (developer jargon abbreviation) network names? I don't think so.

laanwj commented at 10:05 AM on October 9, 2014:

Hmm well on the other hand "[testnet]" is already a translation message. So there is no extra work in keeping it. And people already know (or should know) about the testnet. Not sure about regtest though, that's just too obscure.

in src/qt/networkstyle.h:None in b42db95fe6 outdated

   0 | @@ -0,0 +1,31 @@
   1 | +#ifndef H_NETWORKSTYLE

Diapolo commented at 9:23 AM on October 9, 2014:

missing license

in src/qt/splashscreen.cpp:None in b42db95fe6 outdated

  73 | @@ -78,23 +74,20 @@ SplashScreen::SplashScreen(Qt::WindowFlags f, bool isTestNet) :
  74 |      pixPaint.setFont(QFont(font, 10*fontFactor));
  75 |      pixPaint.drawText(pixmap.width()-titleTextWidth-paddingRight,paddingTop+titleCopyrightVSpace,copyrightText);
  76 |  
  77 | -    // draw testnet string if testnet is on
  78 | -    if(isTestNet) {
  79 | +    // draw title add string if special network

Diapolo commented at 9:24 AM on October 9, 2014:

Nit: grammar?

in src/qt/networkstyle.h:None in b42db95fe6 outdated

  26 | +    const QIcon &getAppIcon() const { return appIcon; }
  27 | +    const QString &getTitleAddText() const { return titleAddText; }
  28 | +    const QPixmap &getSplashImage() const { return splashImage; }
  29 | +};
  30 | +
  31 | +#endif

Diapolo commented at 9:25 AM on October 9, 2014:

missing header end comment // H_NETWORKSTYLE

Diapolo commented at 9:26 AM on October 9, 2014: none

Seems like an ACK-worthy idea, appart from the tr() thing and the nits.

laanwj commented at 10:22 AM on October 9, 2014: member

There you go, translated [testnet] again: untitled

laanwj force-pushed on Oct 9, 2014

Diapolo commented at 11:20 AM on October 9, 2014: none

Thanks for the fixes, utACK.

qt: add network-specific style object

Mainly cleanups: Gets rid of isTestNet everywhere, by keeping track
of network-specific theming in a central place.

Also makes GUI no longer dependent on the network ID enumeration, which
alleviates concerns about #4802.

99a814342b

laanwj force-pushed on Oct 9, 2014

in src/qt/bitcoin.cpp:None in 99a814342b outdated

 634 | @@ -636,6 +635,8 @@ int main(int argc, char *argv[])
 635 |          PrintExceptionContinue(NULL, "Runaway exception");
 636 |          app.handleRunawayException(QString::fromStdString(strMiscWarning));
 637 |      }
 638 | +    delete networkStyle;

theuni commented at 5:23 PM on October 9, 2014:

I think a NetworkStyle::destroy(networkStyle) or networkStyle->destroy() or so would be a bit more clear here? With just a delete, it's hard to tell where it came from.

laanwj commented at 5:54 PM on October 9, 2014:

Does it matter where it came from? In principle any wrap-up code could be done in the destructor. I'm fine with adding a NetworkStyle::destroy(obj) function if you think it's more symmetric, but unlike instantiate there is no reason to do anything special based on the network, it would just call delete anyway. I don't like the ->destroy() style as it involves delete self.

theuni commented at 7:06 PM on October 9, 2014:

It doesn't really matter, it was just a suggestion for readability.

Out of curiosity though, why not just create a normal scoped object in main and pass around a pointer to it? I suspect I'm missing something obvious.

makhmet commented at 7:25 PM on October 9, 2014:

Здравствуйте друзья!Я не разбираюсь в этом.С уважением Махмет!

2014-10-09 23:07 GMT+04:00 Cory Fields notifications@github.com:

In src/qt/bitcoin.cpp:

@@ -636,6 +635,8 @@ int main(int argc, char *argv[]) PrintExceptionContinue(NULL, "Runaway exception"); app.handleRunawayException(QString::fromStdString(strMiscWarning)); }

delete networkStyle;

It doesn't really matter, it was just a suggestion for readability.

Out of curiosity though, why not just create a normal scoped object in main and pass around a pointer to it? I suspect I'm missing something obvious.

— Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/5066/files#r18666838.

laanwj commented at 6:12 AM on October 10, 2014:

@makhmet Graag alleen Engels hier. @theuni Well - I initially started from the approach that there was a Style class per network, but that didn't turn out to be needed. The current interface (with a constructor-independent instantiate) leaves some freedom to return to that later if it becomes necessary (ie, if you'd want to paint the background in a network-specific way and offer a method for that). Also the function can return 0 if the network is unknown, unlike a constructor which would have to mess around with exceptions.

Good idea about the RAII though. I'll change the networkStyle in main to a ~~boost::scoped_ptr~~ QScopedPointer.

makhmet commented at 11:12 PM on November 9, 2014:

mirbitcoin21000000 - https://MirBitcoin21000000 https://sites.google.com/site/mirbitcoin21000000/. google.com/site/mirbitcoin21000000/

theuni commented at 5:28 PM on October 9, 2014: member

utACK, much cleaner.

squashme: Use a scoped pointer for the style instead of an explicit delete afd2a6bbfc

laanwj commented at 6:44 AM on October 10, 2014: member

Closing as it's now part of #4802

laanwj closed this on Oct 10, 2014

MarcoFalke locked this on Sep 8, 2021