update CClientUIInterface and remove orphan Wx stuff #1988

Diapolo commented at 7:10 AM on November 5, 2012: none

fix ThreadSafeMessageBox always displays error icon
allow to specify MSG_ERROR / MSG_WARNING or MSG_INFORMATION without a custom caption / title
allow to specify CClientUIInterface::ICON_ERROR / ICON_WARNING and ICON_INFORMATION (which is default) as message box icon
remove CClientUIInterface::OK from ThreadSafeMessageBox-calls, as the OK button will be set as default, if none is specified
prepend "Bitcoin - " to used captions
rename BitcoinGUI::error() -> BitcoinGUI::message() and add function documentation
change all style parameters and enum flags to unsigned
update code to use that new API
update Client- and WalletModel to use new BitcoinGUI::message() and rename the classes error() method into message()
include the possibility to supply the wanted icon for messages from Client- and WalletModel via "style" parameter

Diapolo commented at 7:13 AM on November 5, 2012: none

@laanwj I suppose ui_interface.h - MessageBoxFlags could be cleaned up or the GUI part needs some reworking to make this options available (which seems not needed, as we only use MessageBoxFlags for displaying warnings / errors currently).

laanwj commented at 7:32 AM on November 5, 2012: member

ACK! This needed to be done badly :)

Indeed, the UI message interface has a lot of unused flags (inherited from Wx). Some of them could be implemented and used, others are completely useless and can be removed. Feel free to do so.

Diapolo commented at 10:50 AM on November 5, 2012: none

If you are fine with the code please first merge this and I'll create another cleanup-pull afterwards?

Edit: I also thought about changing the message box caption for InitError() and InitWarning() to Bitcoin - Error and Bitcoin - Warning.

Diapolo commented at 9:54 PM on November 5, 2012: none

I'll update this pull tomorrow :) did a litle more work here!

BitcoinPullTester commented at 1:53 AM on November 6, 2012: none

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/75ba0be11aef6bff3abebb667564a0671533cd81 for binaries and test log.

This could happen for one of several reasons:

It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

in src/qt/bitcoingui.h:None in 8543de9c8b outdated

 118 | @@ -119,8 +119,8 @@ class BitcoinGUI : public QMainWindow
 119 |      */
 120 |      void setEncryptionStatus(int status);
 121 |  
 122 | -    /** Notify the user of an error in the network or transaction handling code. */
 123 | -    void error(const QString &title, const QString &message, bool modal);
 124 | +    /** Notify the user of an event from the core network or transaction handling code. */
 125 | +    void error(const QString &title, const QString &message, bool modal, int style);

Diapolo commented at 7:02 AM on November 6, 2012:

@laanwj I would love to rename that function to coreEvent() or something as error() is not suitable IMHO. What do you think?

laanwj commented at 1:24 PM on November 8, 2012:

Maybe just call it message() (event was dumb, I thought this referred to some other place in the source :-).

Diapolo commented at 6:50 PM on November 8, 2012:

Yeah, that makes sense :).

in src/ui_interface.h:None in 8543de9c8b outdated

  73 | +        BTN_APPLY           = 0x02000000, // QMessageBox::Apply
  74 | +        BTN_RESET           = 0x04000000, // QMessageBox::Reset
  75 | +
  76 |          // Force blocking, modal message box dialog (not just OS notification)
  77 | -        MODAL                 = 0x00040000
  78 | +        MODAL               = 0x10000000

Diapolo commented at 7:02 AM on November 6, 2012:

That value was chosen to be greater than what is used as max. in qmessagebox.h for StandardButton!

Diapolo commented at 7:03 AM on November 6, 2012: none

Updated to not only fix the bug with always having an error icon, but to also extend the CClientUIInterface.

in src/qt/bitcoin.cpp:None in de57cbf0ce outdated

  39 | @@ -41,12 +40,17 @@ static void ThreadSafeMessageBox(const std::string& message, const std::string&
  40 |      if(guiref)
  41 |      {
  42 |          bool modal = (style & CClientUIInterface::MODAL);
  43 | +        // remove modal flag from style variable after we have the state
  44 | +        if (modal)
  45 | +            style -= CClientUIInterface::MODAL;

laanwj commented at 1:31 PM on November 8, 2012:

I really don't like using - on flags. Please use a mask with & if you need a subset of the bits, instead of subtracting them one by one.

Diapolo commented at 1:30 PM on November 9, 2012:

This should work, or?

style ^= CClientUIInterface::MODAL;

laanwj commented at 2:30 PM on November 9, 2012:

It certainly works (both with - and ^), my point was that removing the flags one by one when set gives needlessly verbose code, when you can simply select a subset of the flags with a mask.

Diapolo commented at 6:31 PM on November 9, 2012:

I was not aware of that mask stuff, update for the pull is on it's way now :).

in src/qt/bitcoingui.cpp:None in de57cbf0ce outdated

 606 | +    }
 607 | +    else if (style & CClientUIInterface::ICON_INFORMATION)
 608 | +        style -= CClientUIInterface::ICON_INFORMATION;
 609 | +
 610 | +    // If no button was specified use default OK button
 611 | +    if (!style)

laanwj commented at 1:34 PM on November 8, 2012:

So what I mean is:

QMessageBox::StandardButton buttons = (QMessageBox::StandardButton) (style & BTN_MASK);
if(!buttons)
    buttons = QMessageBox::Ok;
....
QMessageBox mBox((QMessageBox::Icon)nMBoxIcon, title, message, (QMessageBox::StandardButton)buttons);

where BTN_MASK is defined as BTN_OK | BTN_CANCEL | ... | BTN_RESET

Then you can remove all the -= lines!

Diapolo commented at 6:52 PM on November 8, 2012:

Of course a much nicer approach, thanks.

Diapolo commented at 7:01 PM on November 9, 2012: none

@laanwj Updated to just use masks, can you please take another look. Thanks for your time :).

in src/qt/clientmodel.cpp:None in 2b5a0ee86c outdated

  83 | @@ -84,7 +84,7 @@ void ClientModel::updateAlert(const QString &hash, int status)
  84 |          CAlert alert = CAlert::getAlertByHash(hash_256);
  85 |          if(!alert.IsNull())
  86 |          {
  87 | -            emit error(tr("Network Alert"), QString::fromStdString(alert.strStatusBar), false);
  88 | +            emit message(tr("Bitcoin - Network Alert"), QString::fromStdString(alert.strStatusBar), false, CClientUIInterface::ICON_ERROR);

laanwj commented at 7:41 PM on November 12, 2012:

If you really want to prefix tr("Bitcoin - ") to all titles, why not do it in message or some other inner function? This prevents having to change all these messages, and translators from having to translate Bitcoin a zillion times :)

Diapolo commented at 7:25 AM on November 13, 2012:

Didn't see that low hanging fruit, but will update :).

in src/ui_interface.h:None in 2b5a0ee86c outdated

  21 | @@ -22,6 +22,21 @@ enum ChangeType
  22 |      CT_DELETED
  23 |  };
  24 |  
  25 | +/**
  26 | + * Mask of all available icons in CClientUIInterface::MessageBoxFlags
  27 | + * This needs to be updated, when icons are changed there!
  28 | + */
  29 | +#define ICON_MASK (CClientUIInterface::ICON_INFORMATION | CClientUIInterface::ICON_WARNING | CClientUIInterface::ICON_ERROR)

laanwj commented at 7:43 PM on November 12, 2012:

Please add this to the enum, instead of defining a macro (same for BTN_MASK below).

Diapolo commented at 7:50 AM on November 13, 2012: none

Updated to make the MASKs part of the enums and now prepends "Bitcoin - " to the captions.

in src/init.cpp:None in efe74ada64 outdated

 210 | @@ -209,16 +211,22 @@ int main(int argc, char* argv[])
 211 |  
 212 |  bool static InitError(const std::string &str)
 213 |  {
 214 | -    uiInterface.ThreadSafeMessageBox(str, _("Bitcoin"), CClientUIInterface::OK | CClientUIInterface::MODAL);
 215 | +    uiInterface.ThreadSafeMessageBox(str, _("Error"), CClientUIInterface::ICON_ERROR | CClientUIInterface::MODAL);

laanwj commented at 8:01 AM on November 13, 2012:

I see another pattern here :-)

uiInterface.ThreadSafeMessageBox(str, _("Error"), CClientUIInterface::ICON_ERROR | CClientUIInterface::MODAL);
uiInterface.ThreadSafeMessageBox(str, _("Warning"), CClientUIInterface::ICON_WARNING | CClientUIInterface::MODAL);
....

Wonder if we can do anything with that :)

Diapolo commented at 8:05 AM on November 13, 2012:

I don't get it? Standing on the line I guess :-D.

Edit: Something like this in the enum?

<pre> /** Predefined combinations for certain usage cases */ MSG_INFORMATION = (ICON_INFORMATION | BTN_OK), MSG_WARNING = (ICON_WARNING | BTN_OK | MODAL), MSG_ERROR = (ICON_ERROR | BTN_OK | MODAL) </pre>

laanwj commented at 11:49 AM on November 13, 2012:

Messages with flag WARNING have "Warning" as title, same for error. If we aren't using title to summarize the message we could leave it out entirely and base it on the flags too. Another simplification of the interface.

laanwj commented at 8:04 AM on November 13, 2012: member

Sorry for being picky here, but I'm trying to get this "perfect" in one go.

Diapolo commented at 8:07 AM on November 13, 2012: none

@laanwj I'm the one who loves to do things perfect, so I'm really fine with getting constructive feedback!

Diapolo commented at 2:28 PM on November 13, 2012: none

Updated to allow more efficient usage of ThreadSafeMessageBox with predefined MSG_ERROR, MSG_WARNING or MSG_INFORMATION, which don't need a string for title/caption anymore.

Example before and after: uiInterface.ThreadSafeMessageBox(strerr, _("Error"), CClientUIInterface::ICON_ERROR | CClientUIInterface::BTN_OK | CClientUIInterface::MODAL); uiInterface.ThreadSafeMessageBox(strerr, "", CClientUIInterface::MSG_ERROR);

in src/util.cpp:None in 73419902c5 outdated

1247 | @@ -1248,7 +1248,7 @@ void AddTimeData(const CNetAddr& ip, int64 nTime)
1248 |                      string strMessage = _("Warning: Please check that your computer's date and time are correct! If your clock is wrong Bitcoin will not work properly.");
1249 |                      strMiscWarning = strMessage;
1250 |                      printf("*** %s\n", strMessage.c_str());
1251 | -                    uiInterface.ThreadSafeMessageBox(strMessage+" ", string("Bitcoin"), CClientUIInterface::OK | CClientUIInterface::ICON_EXCLAMATION);
1252 | +                    uiInterface.ThreadSafeMessageBox(strMessage, _("Warning"), CClientUIInterface::ICON_WARNING);

Diapolo commented at 2:30 PM on November 13, 2012:

Is this intended to be non-modal?

laanwj commented at 2:45 PM on November 13, 2012:

I suppose you could make it modal.

Diapolo commented at 2:50 PM on November 13, 2012:

Done...

Diapolo commented at 7:22 AM on November 14, 2012: none

@laanwj Would it be better (typesafe) to directly use CClientUIInterface::MessageBoxFlags style instead of int style in the modified functions? Seems like a not well thought idea, as (ICON_ERROR | BTN_OK) are not recognized as enum but as integer...

Diapolo commented at 11:24 AM on November 14, 2012: none

@laanwj I'll merge the last commit if you ACKed all changes and are fine with the pull as is now :).

laanwj commented at 12:02 PM on November 14, 2012: member

Code/design ack - still need to test

Diapolo commented at 12:39 PM on November 14, 2012: none

Good, merged commits, no code changes.

Diapolo commented at 4:34 PM on November 14, 2012: none

Had to fix the connect() calls in bitcoingui.cpp as they still were using int instead of unsigned int.

Diapolo commented at 9:31 PM on November 19, 2012: none

@laanwj I would love to replace all direct notificator->notify() with message() in another pull. Are you fine with using message(tr("foo"), tr("foo"), false, CClientUIInterface::ICON_WARNING); to achieve this :)?

Did you have the time try to test out this pull yet?

Diapolo commented at 7:57 AM on November 20, 2012: none

Added a detailed comment for ::message() and added a missing unsigned ;).

BitcoinPullTester commented at 1:11 PM on November 23, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/60e82f82e638b9059de82e8f093f3ab45a11ed94 for binaries and test log.

Diapolo commented at 11:17 AM on November 25, 2012: none

@laanwj I need your help getting this in, any time-window :)?

update CClientUIInterface and remove orphan Wx stuff

- fix ThreadSafeMessageBox always displays error icon
- allow to specify MSG_ERROR / MSG_WARNING or MSG_INFORMATION without a
  custom caption / title
- allow to specify CClientUIInterface::ICON_ERROR / ICON_WARNING and
  ICON_INFORMATION (which is default) as message box icon
- remove CClientUIInterface::OK from ThreadSafeMessageBox-calls, as
  the OK button will be set as default, if none is specified
- prepend "Bitcoin - " to used captions
- rename BitcoinGUI::error() -> BitcoinGUI::message() and add function
  documentation
- change all style parameters and enum flags to unsigned
- update code to use that new API

- update Client- and WalletModel to use new BitcoinGUI::message() and
  rename the classes error() method into message()
- include the possibility to supply the wanted icon for messages from
  Client- and WalletModel via "style" parameter

5350ea4171

in src/init.cpp:None in 60e82f82e6 outdated

 221 | -    uiInterface.ThreadSafeMessageBox(str, _("Bitcoin"), CClientUIInterface::OK | CClientUIInterface::ICON_EXCLAMATION | CClientUIInterface::MODAL);
 222 | +    uiInterface.ThreadSafeMessageBox(str, "", CClientUIInterface::MSG_WARNING);
 223 |      return true;
 224 |  }
 225 |  
 226 | +bool static InitInformation(const std::string &str)

laanwj commented at 6:05 AM on November 26, 2012:

It complains that this function is unused...

Diapolo commented at 6:52 AM on November 26, 2012:

Yes, because it IS unused ;). I thought it could be useful for others / later, that's why I added it.

laanwj commented at 7:09 AM on November 26, 2012:

Ok, makes sense. I don't really like the idea of introducing dead code in the core though just in case it is useful later (especially as it is just a wrapper around the ui interface). We should try to at least use it somewhere. Shouldn't be hard if there is a clear use case. I can't think of one right now. Issues that arise during initialization are either:

fatal: modal dialog and exit
important warning that requires immediate attention: modal dialog and proceed
warning affecting usage of the program, ie upgrade alerts: persistent warning in status bar

Transient notifications are most useful for events that are signaled to the user but do not require immediate attention. I don't see any of those arising in init (but maybe you do?).

Diapolo commented at 12:36 PM on November 26, 2012:

No need to discuss in the end, I just removed if for now :).

Diapolo commented at 8:18 AM on November 27, 2012: none

@laanwj I would like to use the new interface for other pulls, so it would be great if you could prioritize the testing of this pull :).

laanwj commented at 8:20 AM on November 27, 2012: member

I intended to merge it this morning but forgot. I'll try to do it in the evening today.

Diapolo commented at 8:27 AM on November 27, 2012: none

That's fine, thanks.

BitcoinPullTester commented at 6:30 PM on November 27, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/5350ea4171be69d16a94e6f40c5e7fa41e00ac13 for binaries and test log.

laanwj referenced this in commit 97c8e6389e on Nov 27, 2012

laanwj merged this on Nov 27, 2012

laanwj closed this on Nov 27, 2012

laudney referenced this in commit 6decedf7f8 on Mar 19, 2014

KolbyML referenced this in commit 2f1dcfce44 on Dec 5, 2020

DrahtBot locked this on Sep 8, 2021