Skip to content

Finer-grained UI updates, move UI interface to boost::signals #1205

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.

Sign up for GitHub

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

Jump to bottom
Merged
laanwj merged 4 commits into from
May 20, 2012
Merged

Finer-grained UI updates, move UI interface to boost::signals #1205

laanwj merged 4 commits into from
May 20, 2012

Conversation

@laanwj
Copy link
Member

@laanwj laanwj commented May 5, 2012

Gets rid of MainFrameRepaint in favor of specific update functions that tell the UI exactly what changed.

  • This improves the efficiency of various handlers (no unnecessary balance recalculations).
  • Also fixes the problem with mined transactions not showing up until restart.
  • As this finally makes it possible for the UI to know when a new alert arrived, it can be shown as OS notification.
  • New addresses are selected again in address book (was a regression in 0.6.1)

The following notifications were added:

  • NotifyBlocksChanged: Block chain changed
  • NotifyKeyStoreStatusChanged: Wallet status (encrypted, locked) changed.
  • NotifyAddressBookChanged: Address book entry changed.
  • NotifyTransactionChanged: Wallet transaction added, removed or updated.
  • NotifyNumConnectionsChanged: Number of connections changed.
  • NotifyAlertChanged: New, updated or cancelled alert.

These notifications could also be useful for RPC clients. However, currently, they are ignored in bitcoind.

Also brings back polling with timer for numBlocks in ClientModel. This value updates so frequently during initial download that the number of signals clogs the UI thread and causes heavy CPU usage. And after initial block download, the value changes so rarely that a delay of half a second until the UI updates is unnoticable.

Second commit converts UI interface to boost::signals:

  • Core no longer links to any UI functions directly, but the UI subscribes to notifications through boost::signals.
  • Signals now go directly from the core to WalletModel/ClientModel.
    • WalletModel subscribes to signals on CWallet: Prepares for multi-wallet support, by no longer assuming an implicit global wallet.
  • Gets rid of noui.cpp, the few lines that were left are merged into init.cpp
  • Rename wxXXX message flags to MF_XXX, to make them UI indifferent.
  • ThreadSafeMessageBox converted to void, no longer returns the value 4 which was never used

@laanwj laanwj mentioned this pull request May 6, 2012
Merged
@Diapolo
Copy link

Diapolo commented May 7, 2012

I looked around in your changes and there is much code in, I can't really say anything to most parts ^^. Does it compile fine on Windows? This takes more time for me to check this, sorry.

@laanwj
Copy link
Member Author

laanwj commented May 7, 2012

It mostly needs sanity testing. The code should compile find on windows, as there's no OS specific changes. There should be no user-visible changes to behaviour (except for the small fixed problems) but this improves efficiency, prepares for multiple-wallet support and makes it easier to split the UI into a separate process at some point.

@laanwj laanwj mentioned this pull request May 8, 2012
Closed
@Diapolo
Copy link

Diapolo commented May 10, 2012

This should help with lowering the current CPU usage of utilizing a full core, while initial block chain download is in progress, right? Is this an issue on non-Windows versions, too and have you verified this?

@laanwj
Copy link
Member Author

laanwj commented May 10, 2012

Well, this is not really meant as a performance fix, although it probably has that effect. AFAIK there are no pressing UI performance issues in 0.6.2.

The idea is to further formalize the interface between the UI and the core.

Instead of a sledgehammer "mainFrameRepaint" event, the UI gets messages such as AddressBookChanged, TransactionChanged that tell the UI directly what has changed, so it can update its internal model (and display) without further checks. This has a positive effect on performance and lock contention.

Eventually I want to remove all locks and direct access in the UI to the core memory space, and handle everything through function calls. This is much safer, and from that point, it is easy(ish) to completely separate the processes by using message passing.

@Diapolo Diapolo mentioned this pull request May 11, 2012
Closed
@laanwj laanwj mentioned this pull request May 15, 2012
Closed
@sipa
Copy link
Member

sipa commented May 17, 2012

ACK on changed to core

@luke-jr
Copy link
Member

luke-jr commented May 19, 2012

test_bitcoin fails to link:

obj/main.o: In function `InvalidChainFound':
/home/luke-jr/Projects/Education/Tonal/BitCoin/bitcoin/src/main.cpp:949: undefined reference to `uiInterface'
obj/main.o: In function `CBlock::AddToBlockIndex(unsigned int, unsigned int)':
/home/luke-jr/Projects/Education/Tonal/BitCoin/bitcoin/src/main.cpp:1650: undefined reference to `uiInterface'
obj/main.o: In function `CheckDiskSpace(unsigned long long)':
/home/luke-jr/Projects/Education/Tonal/BitCoin/bitcoin/src/main.cpp:1861: undefined reference to `uiInterface'
/home/luke-jr/Projects/Education/Tonal/BitCoin/bitcoin/src/main.cpp:1862: undefined reference to `uiInterface'
obj/main.o: In function `CAlert::ProcessAlert()':
/home/luke-jr/Projects/Education/Tonal/BitCoin/bitcoin/src/main.cpp:2207: undefined reference to `uiInterface'
obj/main.o:/home/luke-jr/Projects/Education/Tonal/BitCoin/bitcoin/src/main.cpp:2213: more undefined references to `uiInterface' follow

@luke-jr
Copy link
Member

luke-jr commented May 19, 2012

Fix for test_bitcoin issue in 315fa37

luke-jr
luke-jr reviewed May 19, 2012
View reviewed changes
src/ui_interface.h Outdated
Copy link
Member

@luke-jr luke-jr May 19, 2012

Choose a reason for hiding this comment

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

http://msdn.microsoft.com/en-us/library/windows/desktop/ms647581(v=vs.85).aspx
MF_HELP = 0x00004000L -- Indicates that the menu item has a vertical separator to its left.

@laanwj
Copy link
Member Author

laanwj commented May 19, 2012

Thanks for testing sipa and luke-jr.

I've pushed a commit that should resolve the test build and windows build issues.

laanwj added 4 commits May 20, 2012 10:41
@laanwj
Fine-grained UI updates
fe4a655 
Gets rid of `MainFrameRepaint` in favor of specific update functions that tell the UI exactly what changed.

This improves the efficiency of various handlers. Also fixes problems with mined transactions not showing up until restart.

The following notifications were added:

- `NotifyBlocksChanged`: Block chain changed
- `NotifyKeyStoreStatusChanged`: Wallet status (encrypted, locked) changed.
- `NotifyAddressBookChanged`: Address book entry changed.
- `NotifyTransactionChanged`: Wallet transaction added, removed or updated.
- `NotifyNumConnectionsChanged`: Number of connections changed.
- `NotifyAlertChanged`: New, updated or cancelled alert. As this finally makes it possible for the UI to know when a new alert arrived, it can be shown as OS notification.

These notifications could also be useful for RPC clients. However, currently, they are ignored in bitcoind (in noui.cpp).

Also brings back polling with timer for numBlocks in ClientModel. This value updates so frequently during initial download that the number of signals clogs the UI thread and causes heavy CPU usage. And after initial block download, the value changes so rarely that a delay of half a second until the UI updates is unnoticable.
@laanwj
Convert UI interface to boost::signals2.
ab1b288 
- Signals now go directly from the core to WalletModel/ClientModel.
  - WalletModel subscribes to signals on CWallet: Prepares for multi-wallet support, by no longer assuming an implicit global wallet.
- Gets rid of noui.cpp, the few lines that were left are merged into init.cpp
- Rename wxXXX message flags to MF_XXX, to make them UI indifferent.
- ThreadSafeMessageBox no longer returns the value `4` which was never used, converted to void.
@laanwj
Process address book updates incrementally
0832c0d 
- No longer invalidates selection model, thus retains selection on address book changes
- Fixes selection of new address when added
@laanwj
Make testcases build, prevent windows symbol collision
239c11d
laanwj added a commit that referenced this pull request May 20, 2012
@laanwj
Merge pull request #1205 from laanwj/2012_05_granular_ui_notifications
5a8398e 
Finer-grained UI updates, move UI interface to boost::signals
@laanwj laanwj merged commit 5a8398e into bitcoin:master May 20, 2012
coblee pushed a commit to litecoin-project/litecoin that referenced this pull request Jul 17, 2012
@laanwj
Merge pull request bitcoin#1205 from laanwj/2012_05_granular_ui_notif…
87549bc 
…ications

Finer-grained UI updates, move UI interface to boost::signals
@laanwj laanwj deleted the 2012_05_granular_ui_notifications branch April 9, 2014 14:15
suprnurd pushed a commit to chaincoin-legacy/chaincoin that referenced this pull request Dec 5, 2017
@tgflynn @UdjinM6
Deletion time fix (bitcoin#1205)
711a5fb 
* Fix deletion time bug
* Only set deletion time if it has not already been set
lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Jan 22, 2019
@gandrewstone
Merge pull request bitcoin#1205 from ptschip/dev_mem_easy
cc695d6 
Fix leak in getblocktemplate
lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Dec 25, 2019
@furszy
Merge bitcoin#1205: [Cleanup] Remove unnecessary QtCreator files
4f19cd0 
b578675 [Cleanup] Remove unnecessary QtCreator files (Fuzzbawls)

Pull request description:

  We don't need to be including these IDE files in the source repo

ACKs for top commit:
  random-zebra:
    utACK b578675
  furszy:
    utACK b578675
  Warrows:
    ACK b578675

Tree-SHA512: 8d14634ef9e62291242c9a7ee2c4e0b680233980389e077dfdbdb4bf546dddefcb0da99d664153ca2144039e6938341d73ede868abe5432345edd3ef011d3fcb
@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.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.7.0

Development

Successfully merging this pull request may close these issues.

4 participants

@laanwj @Diapolo @sipa @luke-jr