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

Early subscribe core signals in transaction table model #121

Merged
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 14 additions & 11 deletions src/qt/transactiontablemodel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -96,25 +96,28 @@ class TransactionTablePriv
*/
QList<TransactionRecord> cachedWallet;

bool fQueueNotifications = false;
/** True when model finishes loading all wallet transactions on start */
bool m_loaded = false;
/** True when transactions are being notified, for instance when scanning */
Copy link
Member

@hebasto hebasto Apr 29, 2021

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I'll rephrase.

Copy link
Contributor

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)

Right, I'll rephrase.

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_notifications instead of m_loading because it can be true when m_loaded is also true, but feel free to stick with whatever name you prefer.

bool m_loading = false;
std::vector< TransactionNotification > vQueueNotifications;

void NotifyTransactionChanged(const uint256 &hash, ChangeType status);
void ShowProgress(const std::string &title, int nProgress);
void DispatchNotifications();
Copy link
Member

Choose a reason for hiding this comment

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

A doxygen comment?


/* Query entire wallet anew from core.
*/
void refreshWallet(interfaces::Wallet& wallet)
{
qDebug() << "TransactionTablePriv::refreshWallet";
promag marked this conversation as resolved.
Show resolved Hide resolved
cachedWallet.clear();
assert(!m_loaded);
{
for (const auto& wtx : wallet.getWalletTxs()) {
if (TransactionRecord::showTransaction()) {
cachedWallet.append(TransactionRecord::decomposeTransaction(wtx));
}
}
}
m_loaded = true;
Copy link
Member

Choose a reason for hiding this comment

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

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;

Copy link
Member

Choose a reason for hiding this comment

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

(similar to the change in the last commit, git show cafef080a2e --ignore-all-space)

}

/* Update our model of the wallet incrementally, to synchronize our model of the wallet
Expand Down Expand Up @@ -719,22 +722,19 @@ void TransactionTablePriv::NotifyTransactionChanged(const uint256 &hash, ChangeT

TransactionNotification notification(hash, status, showTransaction);

if (fQueueNotifications)
if (m_loading)
{
vQueueNotifications.push_back(notification);
return;
}
notification.invoke(parent);
}

void TransactionTablePriv::ShowProgress(const std::string &title, int nProgress)
void TransactionTablePriv::DispatchNotifications()
{
if (nProgress == 0)
fQueueNotifications = true;
if (m_loading) return;

if (nProgress == 100)
{
Copy link
Member

Choose a reason for hiding this comment

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

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

fQueueNotifications = false;
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);
Expand All @@ -756,7 +756,10 @@ 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(std::bind(&TransactionTablePriv::ShowProgress, 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()
Expand Down