update CClientUIInterface and remove orphan Wx stuff #1988

Merged
merged 1 commit into from Nov 27, 2012

3 participants

@Diapolo
  • 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

@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
Bitcoin 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

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

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

@BitcoinPullTester

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

@Diapolo Diapolo and 1 other commented on an outdated diff Nov 6, 2012
src/qt/bitcoingui.h
@@ -119,8 +119,8 @@ class BitcoinGUI : public QMainWindow
*/
void setEncryptionStatus(int status);
- /** Notify the user of an error in the network or transaction handling code. */
- void error(const QString &title, const QString &message, bool modal);
+ /** Notify the user of an event from the core network or transaction handling code. */
+ void error(const QString &title, const QString &message, bool modal, int style);
@Diapolo
Diapolo added a note Nov 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
Bitcoin member
laanwj added a note Nov 8, 2012

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

@Diapolo
Diapolo added a note Nov 8, 2012

Yeah, that makes sense :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Diapolo Diapolo commented on an outdated diff Nov 6, 2012
src/ui_interface.h
// Force blocking, modal message box dialog (not just OS notification)
- MODAL = 0x00040000
+ MODAL = 0x10000000
@Diapolo
Diapolo added a note Nov 6, 2012

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Diapolo

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

@laanwj laanwj and 1 other commented on an outdated diff Nov 8, 2012
src/qt/bitcoin.cpp
@@ -41,12 +40,17 @@ static void ThreadSafeMessageBox(const std::string& message, const std::string&
if(guiref)
{
bool modal = (style & CClientUIInterface::MODAL);
+ // remove modal flag from style variable after we have the state
+ if (modal)
+ style -= CClientUIInterface::MODAL;
@laanwj
Bitcoin member
laanwj added a note Nov 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
Diapolo added a note Nov 9, 2012

This should work, or?

style ^= CClientUIInterface::MODAL;

@laanwj
Bitcoin member
@Diapolo
Diapolo added a note Nov 9, 2012

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@laanwj laanwj and 1 other commented on an outdated diff Nov 8, 2012
src/qt/bitcoingui.cpp
}
+ else if (style & CClientUIInterface::ICON_WARNING) {
+ nMBoxIcon = QMessageBox::Warning;
+ nNotifyIcon = Notificator::Warning;
+ style -= CClientUIInterface::ICON_WARNING;
+ }
+ else if (style & CClientUIInterface::ICON_INFORMATION)
+ style -= CClientUIInterface::ICON_INFORMATION;
+
+ // If no button was specified use default OK button
+ if (!style)
@laanwj
Bitcoin member
laanwj added a note Nov 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
Diapolo added a note Nov 8, 2012

Of course a much nicer approach, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Diapolo

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

@laanwj laanwj and 1 other commented on an outdated diff Nov 12, 2012
src/qt/clientmodel.cpp
@@ -84,7 +84,7 @@ void ClientModel::updateAlert(const QString &hash, int status)
CAlert alert = CAlert::getAlertByHash(hash_256);
if(!alert.IsNull())
{
- emit error(tr("Network Alert"), QString::fromStdString(alert.strStatusBar), false);
+ emit message(tr("Bitcoin - Network Alert"), QString::fromStdString(alert.strStatusBar), false, CClientUIInterface::ICON_ERROR);
@laanwj
Bitcoin member
laanwj added a note Nov 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
Diapolo added a note Nov 13, 2012

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@laanwj laanwj commented on an outdated diff Nov 12, 2012
src/ui_interface.h
@@ -22,6 +22,21 @@ enum ChangeType
CT_DELETED
};
+/**
+ * Mask of all available icons in CClientUIInterface::MessageBoxFlags
+ * This needs to be updated, when icons are changed there!
+ */
+#define ICON_MASK (CClientUIInterface::ICON_INFORMATION | CClientUIInterface::ICON_WARNING | CClientUIInterface::ICON_ERROR)
@laanwj
Bitcoin member
laanwj added a note Nov 12, 2012

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Diapolo

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

@laanwj laanwj and 1 other commented on an outdated diff Nov 13, 2012
src/init.cpp
@@ -209,16 +211,22 @@ int main(int argc, char* argv[])
bool static InitError(const std::string &str)
{
- uiInterface.ThreadSafeMessageBox(str, _("Bitcoin"), CClientUIInterface::OK | CClientUIInterface::MODAL);
+ uiInterface.ThreadSafeMessageBox(str, _("Error"), CClientUIInterface::ICON_ERROR | CClientUIInterface::MODAL);
@laanwj
Bitcoin member
laanwj added a note Nov 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
Diapolo added a note Nov 13, 2012

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

Edit: Something like this in the enum?

        /** 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)
@laanwj
Bitcoin member
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@laanwj
Bitcoin member

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

@Diapolo

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

@Diapolo

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);

@Diapolo Diapolo and 1 other commented on an outdated diff Nov 13, 2012
src/util.cpp
@@ -1248,7 +1248,7 @@ void AddTimeData(const CNetAddr& ip, int64 nTime)
string strMessage = _("Warning: Please check that your computer's date and time are correct! If your clock is wrong Bitcoin will not work properly.");
strMiscWarning = strMessage;
printf("*** %s\n", strMessage.c_str());
- uiInterface.ThreadSafeMessageBox(strMessage+" ", string("Bitcoin"), CClientUIInterface::OK | CClientUIInterface::ICON_EXCLAMATION);
+ uiInterface.ThreadSafeMessageBox(strMessage, _("Warning"), CClientUIInterface::ICON_WARNING);
@Diapolo
Diapolo added a note Nov 13, 2012

Is this intended to be non-modal?

@laanwj
Bitcoin member
@Diapolo
Diapolo added a note Nov 13, 2012

Done...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Diapolo

@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

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

@laanwj
Bitcoin member
@Diapolo

Good, merged commits, no code changes.

@Diapolo

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

@Diapolo

@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

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

@BitcoinPullTester

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

@Diapolo

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

@laanwj laanwj and 1 other commented on an outdated diff Nov 26, 2012
return true;
}
+bool static InitInformation(const std::string &str)
@laanwj
Bitcoin member
laanwj added a note Nov 26, 2012

It complains that this function is unused...

@Diapolo
Diapolo added a note Nov 26, 2012

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

@laanwj
Bitcoin member
@Diapolo
Diapolo added a note Nov 26, 2012

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Philip Kaufmann 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
5350ea4
@Diapolo

@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
Bitcoin member
@Diapolo

That's fine, thanks.

@BitcoinPullTester

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

@laanwj laanwj merged commit 97c8e63 into bitcoin:master Nov 27, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment