Skip to content

use new message() function in BitcoinGUI#2045

Merged
laanwj merged 1 commit intobitcoin:masterfrom
Diapolo:use_message
Dec 13, 2012
Merged

use new message() function in BitcoinGUI#2045
laanwj merged 1 commit intobitcoin:masterfrom
Diapolo:use_message

Conversation

@Diapolo
Copy link
Copy Markdown

@Diapolo Diapolo commented Nov 27, 2012

  • use it for displaying URI parsing warnings
  • use it for displaying error and information in backup wallet function
    (the information display is new and the error was a warning before)
  • cleanup BitcoinGUI::incomingTransaction()
    -- use message() + the information icon from message
    -- comment out an unused parameter in the function definition and
    declaration
    -- move all pre-checks at the beginning of the function

@laanwj There is this comment in qmessagebox.h (// the following functions are obsolete) that lists the static QMessageBox functions (warning, information and so on) as obsolete, so I think we should talk about replacing them over the next months.

@BitcoinPullTester
Copy link
Copy Markdown

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

@BitcoinPullTester
Copy link
Copy Markdown

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

Comment thread src/qt/bitcoingui.cpp Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder why we still have a mysterious boolean parameter for the 'modal' state, even though that's one of the CClientUIInterface flags? :-)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think the reason is in bitcoin.cpp ThreadSafeMessageBox(), where we first use the modal flag. I'm sure it could be removed, when I have time I'll take a look. Do you think this should block merging this pull then?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Possibly. I think the current semantics is pretty vague. Some of the flags are usable, but not modal, because it's separated out to a different parameter. A pull request to change this would affect exactly the same areas as this one. So let's try to do it right in one go.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

See #2065

- use it for displaying URI parsing warnings
- use it for displaying error and information in backup wallet function
  (the information display is new and the error was a warning before)

- cleanup BitcoinGUI::incomingTransaction()
-- use message() + the information icon from message
-- comment out an unused parameter in the function definition and
   declaration
-- move all pre-checks at the beginning of the function
@BitcoinPullTester
Copy link
Copy Markdown

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

@Diapolo
Copy link
Copy Markdown
Author

Diapolo commented Dec 4, 2012

@laanwj This should be sane now, it was rebased after the removal of the modal flag.

Comment thread src/qt/bitcoingui.cpp
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO this one should be modal

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It IS :), as MSG_ERROR = (ICON_ERROR | BTN_OK | MODAL).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, cool :)

@Diapolo
Copy link
Copy Markdown
Author

Diapolo commented Dec 7, 2012

@laanwj Considered ready to get merged?

@Diapolo
Copy link
Copy Markdown
Author

Diapolo commented Dec 12, 2012

@laanwj ping

laanwj added a commit that referenced this pull request Dec 13, 2012
use new message() function in BitcoinGUI
@laanwj laanwj merged commit 07c3f84 into bitcoin:master Dec 13, 2012
laudney pushed a commit to reddcoin-project/reddcoin-3.10 that referenced this pull request Mar 19, 2014
use new message() function in BitcoinGUI
@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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants