Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Streamline UI ↔ Core interface #1019

Merged
merged 10 commits into from
Apr 4, 2012
Merged

Conversation

laanwj
Copy link
Member

@laanwj laanwj commented Mar 31, 2012

This is a grab-bag of commits designed to streamline the UI↔Core interface. It contains various small improvements as well as code cleanups:

Improvements:

  • Allow other threads to queue shutdown in UI thread (functionality in case of bitcoind is unchanged). RPC stop and encryptwallet will now work with UI, and this also fixes segfault part of v0.6.0rc5 segfault on initial blockchain download when out of diskspace #999.
  • update UI only when needed, instead of polling with a timer (core notifies UI using AddressBookRepaint / MainFrameRepaint functions)
    • This fixes the issue with address created using RPC 'getnewaddress' not appearing in UI address book
  • use a messagebox to display the error when -server is provided without providing rpcpassword, this doesn't get lost like a message to the console (functionality in case of bitcoind is unchanged)
  • allow threads to request a modal dialog box for fatal errors, so that they can wait with shutting down until the user clicked OK (disk space error, etc)

Code cleanups:

  • move QT_PLUGINS stuff to qt main file, where it belongs
  • move dummy translation function _ to qtui.h/noui.h instead of util.h
  • remove dependency for SecureString on serialize.h and util.h (and the rest of the knot of .h files) by moving allocators to new include file allocators.h
  • remove broken and/or unnecessary functions from qtui.h/noui.h, clarify names, and leave only one MessageBox function that can be used both from AppInit2 and other threads

In principle, these commits are fairly independent and could be separate pull requests if necessary.

@sipa
Copy link
Member

sipa commented Apr 1, 2012

It looks all sane and functional to me.

@laanwj
Copy link
Member Author

laanwj commented Apr 2, 2012

I have to think about point 2 a bit more though: do changes in statistics like the # of connections or the number of blocks trigger a MainFrameRepaint? I don't think so, and doing that could potentially result in a deluge of cross-thread notifications.

It does make sense to poll those statistics with a timer, just not the well-delineated updates to the transaction list (and balance), address book.

@laanwj
Copy link
Member Author

laanwj commented Apr 2, 2012

To answer my own question: yes, MainFrameRepaint is called when the number of connections changes / number of blocks count. This is safe.

@sipa
Copy link
Member

sipa commented Apr 3, 2012

ACK on the code changes to core, but needs rebasing.

@laanwj
Copy link
Member Author

laanwj commented Apr 4, 2012

Ok, this is all rebased and ready now.

… instead of a timer.

- Overall, this is better design
- This fixes problems with the address book UI not updating when the address book is changed through RPC
- Move Statusbar change detection responsibility to ClientModel
- rename wxMessageBox, remove redundant arguments to noui/qtui calls
- also, add flag to force blocking, modal dialog box for disk space warning etc
- clarify function naming
- no more special MessageBox needed from AppInit2, as window object is created before calling AppInit2
@Diapolo
Copy link

Diapolo commented Apr 4, 2012

Small question in qtgui.h and nogui.h I saw "#define wxMessageBox MyMessageBox" , is there some old wx code in, that can be removed, too?

Edit: Ah you removed that already :), nice and ACK!

@laanwj
Copy link
Member Author

laanwj commented Apr 4, 2012

Yep! the only wx* definitions left are the flags to signal the message kind. They can be renamed, and a lot of can go, too, but I haven't determined yet which ones are useful (also in the future) and what to rename them to. Let's leave that to a later decision.

Edit: and can also be deduplicated and defined in a common header; there is no need to define them differently in qtui/noui.h.
Edit.2: thinking about it, there is no need for separate qtui.h / noui.h headers at all if we don't define implementations in the headers, which would be a sane thing to do anyway. The distinction could be made by linking bitcoind against a different implementation file.

@Diapolo
Copy link

Diapolo commented Apr 4, 2012

Sounds pretty good.

@laanwj
Copy link
Member Author

laanwj commented Apr 4, 2012

I've added another code cleanup / deduplication commit: Move from noui.h / qtui.h to one ui_interface.h with dummy implementation for the daemon.

sipa added a commit that referenced this pull request Apr 4, 2012
Streamline UI ↔ Core interface
@sipa sipa merged commit b0a7e05 into bitcoin:master Apr 4, 2012
coblee pushed a commit to litecoin-project/litecoin that referenced this pull request Jul 17, 2012
@laanwj laanwj deleted the 2012_03_uirefactor branch April 9, 2014 14:14
suprnurd pushed a commit to chaincoin-legacy/chaincoin that referenced this pull request Dec 5, 2017
* refactor CalculateScore (remove mod, remove extra cs_main lock)

* remove GetCurrentMasternode, use GetNextMasternodeInQueueForPayment instead

* fix masternode rpc "current" (calculate node to pay next block), add rpc "winner" (calculate node to vote for)

* remove "calcscore" rpc
ptschip pushed a commit to ptschip/bitcoin that referenced this pull request Apr 8, 2018
Re-enable Op codes for May 15 hardfork
Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Oct 8, 2019
799c332 Set fSuccess true on valid wtxNew (Peter Bushnell)

Pull request description:

Tree-SHA512: 1580fa19f9e638de5f59ea180d3ae1b77cfac43a0b96770904b75468e0c75192a87fca68995e3a93e0bafb3848ab7c7c189243ef71b673736fd466b89744cae9
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
@maflcko maflcko removed the CI failed label Apr 11, 2023
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.

5 participants