Early subscribe core signals in transaction table model #121
+26
−24
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -96,25 +96,29 @@ class TransactionTablePriv | |||||||
| */ | ||||||||
| QList<TransactionRecord> cachedWallet; | ||||||||
|
|
||||||||
| /** True when model finishes loading all wallet transactions on start */ | ||||||||
| bool m_loaded = false; | ||||||||
| /** True when transactions are being notified, for instance when scanning */ | ||||||||
| bool m_loading = false; | ||||||||
| std::vector< TransactionNotification > vQueueNotifications; | ||||||||
|
|
||||||||
| void NotifyTransactionChanged(const uint256 &hash, ChangeType status); | ||||||||
| void DispatchNotifications(); | ||||||||
|
|
||||||||
|
|
||||||||
| /* Query entire wallet anew from core. | ||||||||
| */ | ||||||||
| void refreshWallet(interfaces::Wallet& wallet) | ||||||||
| { | ||||||||
promag marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| assert(!m_loaded); | ||||||||
| { | ||||||||
| for (const auto& wtx : wallet.getWalletTxs()) { | ||||||||
| if (TransactionRecord::showTransaction()) { | ||||||||
| cachedWallet.append(TransactionRecord::decomposeTransaction(wtx)); | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
| m_loaded = true; | ||||||||
|
There was a problem hiding this comment. 3bccd50 nit, can drop extra brackets diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp
index 0175c88e7..8dc6d4c40 100644
--- a/src/qt/transactiontablemodel.cpp
+++ b/src/qt/transactiontablemodel.cpp
@@ -110,11 +110,9 @@ public:
void refreshWallet(interfaces::Wallet& wallet)
{
assert(!m_loaded);
- {
- for (const auto& wtx : wallet.getWalletTxs()) {
- if (TransactionRecord::showTransaction()) {
- cachedWallet.append(TransactionRecord::decomposeTransaction(wtx));
- }
+ for (const auto& wtx : wallet.getWalletTxs()) {
+ if (TransactionRecord::showTransaction()) {
+ cachedWallet.append(TransactionRecord::decomposeTransaction(wtx));
}
}
m_loaded = true;There was a problem hiding this comment. (similar to the change in the last commit, |
||||||||
| DispatchNotifications(); | ||||||||
| } | ||||||||
|
|
||||||||
| /* Update our model of the wallet incrementally, to synchronize our model of the wallet | ||||||||
|
|
@@ -249,12 +253,12 @@ TransactionTableModel::TransactionTableModel(const PlatformStyle *_platformStyle | |||||||
| fProcessingQueuedTransactions(false), | ||||||||
| platformStyle(_platformStyle) | ||||||||
| { | ||||||||
| subscribeToCoreSignals(); | ||||||||
|
|
||||||||
| columns << QString() << QString() << tr("Date") << tr("Type") << tr("Label") << BitcoinUnits::getAmountColumnTitle(walletModel->getOptionsModel()->getDisplayUnit()); | ||||||||
| priv->refreshWallet(walletModel->wallet()); | ||||||||
|
|
||||||||
| connect(walletModel->getOptionsModel(), &OptionsModel::displayUnitChanged, this, &TransactionTableModel::updateDisplayUnit); | ||||||||
| } | ||||||||
|
|
||||||||
| TransactionTableModel::~TransactionTableModel() | ||||||||
|
|
@@ -719,44 +723,42 @@ void TransactionTablePriv::NotifyTransactionChanged(const uint256 &hash, ChangeT | |||||||
|
|
||||||||
| TransactionNotification notification(hash, status, showTransaction); | ||||||||
|
|
||||||||
| if (!m_loaded || m_loading) | ||||||||
| { | ||||||||
|
Comment on lines
+726
to
727
There was a problem hiding this comment. style nit:
Suggested change
|
||||||||
| vQueueNotifications.push_back(notification); | ||||||||
| return; | ||||||||
| } | ||||||||
| notification.invoke(parent); | ||||||||
| } | ||||||||
|
|
||||||||
| void TransactionTablePriv::DispatchNotifications() | ||||||||
| { | ||||||||
| if (!m_loaded || m_loading) return; | ||||||||
|
|
||||||||
| if (vQueueNotifications.size() > 10) { // prevent balloon spam, show maximum 10 balloons | ||||||||
| bool invoked = QMetaObject::invokeMethod(parent, "setProcessingQueuedTransactions", Qt::QueuedConnection, Q_ARG(bool, true)); | ||||||||
| assert(invoked); | ||||||||
| } | ||||||||
| for (unsigned int i = 0; i < vQueueNotifications.size(); ++i) | ||||||||
| { | ||||||||
|
There was a problem hiding this comment. cafef08 nit, while removing the brackets in this commit could also diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp
index 0175c88e7..734605a47 100644
--- a/src/qt/transactiontablemodel.cpp
+++ b/src/qt/transactiontablemodel.cpp
@@ -739,8 +739,7 @@ void TransactionTablePriv::DispatchNotifications()
bool invoked = QMetaObject::invokeMethod(parent, "setProcessingQueuedTransactions", Qt::QueuedConnection, Q_ARG(bool, true));
assert(invoked);
}
- for (unsigned int i = 0; i < vQueueNotifications.size(); ++i)
- {
+ for (unsigned int i = 0; i < vQueueNotifications.size(); ++i) { |
||||||||
| if (vQueueNotifications.size() - i <= 10) { | ||||||||
| bool invoked = QMetaObject::invokeMethod(parent, "setProcessingQueuedTransactions", Qt::QueuedConnection, Q_ARG(bool, false)); | ||||||||
| assert(invoked); | ||||||||
| } | ||||||||
|
|
||||||||
| vQueueNotifications[i].invoke(parent); | ||||||||
| } | ||||||||
| vQueueNotifications.clear(); | ||||||||
| } | ||||||||
|
|
||||||||
| void TransactionTableModel::subscribeToCoreSignals() | ||||||||
| { | ||||||||
| // Connect signals to wallet | ||||||||
| m_handler_transaction_changed = walletModel->wallet().handleTransactionChanged(std::bind(&TransactionTablePriv::NotifyTransactionChanged, priv, std::placeholders::_1, std::placeholders::_2)); | ||||||||
| m_handler_show_progress = walletModel->wallet().handleShowProgress([this](const std::string&, int progress) { | ||||||||
| priv->m_loading = progress < 100; | ||||||||
| priv->DispatchNotifications(); | ||||||||
| }); | ||||||||
| } | ||||||||
|
|
||||||||
| void TransactionTableModel::unsubscribeFromCoreSignals() | ||||||||
|
|
||||||||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be just my bad English, but it seems should be "... are not being notified", no?
If I'm correct, maybe re-word the comment in positive one?
Or maybe we think about different notifications: (a) from the node vs (b) to a user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I'll rephrase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In commit "qt: Refactor ShowProgress to DispatchNotifications" (c6cbdf1)
re: #121 (comment)
If you're still planning to update this, my attempt at a comment would be: "This will be true when blocks in the chain are being scanned for new transactions, and transaction notification popups should be suppressed until the scan is done. It can be true even after m_loaded is true if a rescan has been triggered." I think I would also call this
m_delay_notificationsinstead ofm_loadingbecause it can be true whenm_loadedis also true, but feel free to stick with whatever name you prefer.