Skip to content

Ditch wallet model juggling #417

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

Closed
wants to merge 2 commits into from
Closed

Ditch wallet model juggling #417

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f3e7047
qt: Ditch wallet model juggling
promag Sep 2, 2021
e2ffc98
qt: Move wallet preloading out of model constructors
promag Sep 3, 2021
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
17 changes: 11 additions & 6 deletions src/qt/addresstablemodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,13 @@ class AddressTablePriv
{
public:
QList<AddressTableEntry> cachedAddressTable;
AddressTableModel *parent;
AddressTableModel* const parent;
const bool pk_hash_only;

explicit AddressTablePriv(AddressTableModel *_parent):
parent(_parent) {}
explicit AddressTablePriv(AddressTableModel *_parent, bool pk_hash_only):
parent(_parent), pk_hash_only(pk_hash_only) {}

void refreshAddressTable(interfaces::Wallet& wallet, bool pk_hash_only = false)
void refreshAddressTable(interfaces::Wallet& wallet)
{
cachedAddressTable.clear();
{
Expand Down Expand Up @@ -166,15 +167,19 @@ AddressTableModel::AddressTableModel(WalletModel *parent, bool pk_hash_only) :
QAbstractTableModel(parent), walletModel(parent)
{
columns << tr("Label") << tr("Address");
priv = new AddressTablePriv(this);
priv->refreshAddressTable(parent->wallet(), pk_hash_only);
priv = new AddressTablePriv(this, pk_hash_only);
}

AddressTableModel::~AddressTableModel()
{
delete priv;
}

void AddressTableModel::preload()
{
priv->refreshAddressTable(walletModel->wallet());
}

int AddressTableModel::rowCount(const QModelIndex &parent) const
{
if (parent.isValid()) {
Expand Down
2 changes: 2 additions & 0 deletions src/qt/addresstablemodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ class AddressTableModel : public QAbstractTableModel
static const QString Send; /**< Specifies send address */
static const QString Receive; /**< Specifies receive address */

void preload();

/** @name Methods overridden from QAbstractTableModel
@{*/
int rowCount(const QModelIndex &parent) const override;
Expand Down
13 changes: 8 additions & 5 deletions src/qt/recentrequeststablemodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,6 @@
RecentRequestsTableModel::RecentRequestsTableModel(WalletModel *parent) :
QAbstractTableModel(parent), walletModel(parent)
{
// Load entries from wallet
for (const std::string& request : parent->wallet().getAddressReceiveRequests()) {
addNewRequest(request);
}

/* These columns must match the indices in the ColumnIndex enumeration */
columns << tr("Date") << tr("Label") << tr("Message") << getAmountTitle();

Expand All @@ -36,6 +31,14 @@ RecentRequestsTableModel::RecentRequestsTableModel(WalletModel *parent) :

RecentRequestsTableModel::~RecentRequestsTableModel() = default;

void RecentRequestsTableModel::preload()
{
// Load entries from wallet
for (const std::string& request : walletModel->wallet().getAddressReceiveRequests()) {
addNewRequest(request);
}
}

int RecentRequestsTableModel::rowCount(const QModelIndex &parent) const
{
if (parent.isValid()) {
Expand Down
2 changes: 2 additions & 0 deletions src/qt/recentrequeststablemodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ class RecentRequestsTableModel: public QAbstractTableModel
NUMBER_OF_COLUMNS
};

void preload();

/** @name Methods overridden from QAbstractTableModel
@{*/
int rowCount(const QModelIndex &parent) const override;
Expand Down
1 change: 1 addition & 0 deletions src/qt/test/addressbooktests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ void TestAddAddressesToSendBook(interfaces::Node& node)
WalletContext& context = *node.walletLoader().context();
AddWallet(context, wallet);
WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel, platformStyle.get());
walletModel.preload();
RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt);
EditAddressDialog editAddressDialog(EditAddressDialog::NewSendingAddress);
editAddressDialog.setModel(walletModel.getAddressTableModel());
Expand Down
1 change: 1 addition & 0 deletions src/qt/test/wallettests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ void TestGUI(interfaces::Node& node)
WalletContext& context = *node.walletLoader().context();
AddWallet(context, wallet);
WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel, platformStyle.get());
walletModel.preload();
RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt);
sendCoinsDialog.setModel(&walletModel);
transactionView.setModel(&walletModel);
Expand Down
10 changes: 6 additions & 4 deletions src/qt/transactiontablemodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,11 +256,7 @@ 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);
}

Expand All @@ -270,6 +266,12 @@ TransactionTableModel::~TransactionTableModel()
delete priv;
}

void TransactionTableModel::preload()
{
subscribeToCoreSignals();
priv->refreshWallet(walletModel->wallet());
}

/** Updates the column title to "Amount (DisplayUnit)" and emits headerDataChanged() signal for table headers to react. */
void TransactionTableModel::updateAmountColumnTitle()
{
Expand Down
2 changes: 2 additions & 0 deletions src/qt/transactiontablemodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ class TransactionTableModel : public QAbstractTableModel
RawDecorationRole,
};

void preload();

int rowCount(const QModelIndex &parent) const override;
int columnCount(const QModelIndex &parent) const override;
QVariant data(const QModelIndex &index, int role) const override;
Expand Down
2 changes: 2 additions & 0 deletions src/qt/walletcontroller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wal
wallet_model = new WalletModel(std::move(wallet), m_client_model, m_platform_style, this);
}, GUIUtil::blockingGUIThreadConnection());

wallet_model->preload();
Copy link
Contributor Author

@promag promag Feb 23, 2022

Choose a reason for hiding this comment

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

In a follow-up, some preload will be replaced by lazy loading.

Copy link
Contributor

@ryanofsky ryanofsky Nov 15, 2022

Choose a reason for hiding this comment

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

In commit "qt: Move wallet preloading out of model constructors" (e2ffc98)

I think it would be good to switch the order of the two commits so the other commit doesn't cause a performance regression, and so wallet preloading doesn't move from the outside thread to the GUI thread than back to the outside thread again within the PR.

Additionally might suggest renaming preload to loadData to give a clearer idea what this method is actually doing and avoid the "pre" prefix, which I'm not sure actually means anything, since it runs separately after the constructor so might more logically be called "postload".

Also would add a rationale to the commit description like:

The allows WalletModel constructor to be called on the GUI thread (so queued signals sent to the wallet model object will execute on the GUI thread), while allowing wallet data to be loaded off the GUI thread, so the GUI will not freeze while the data is loading.


m_wallets.push_back(wallet_model);

// WalletModel::startPollBalance needs to be called in a thread managed by
Expand Down
10 changes: 8 additions & 2 deletions src/qt/walletmodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,21 @@ WalletModel::WalletModel(std::unique_ptr<interfaces::Wallet> wallet, ClientModel
addressTableModel = new AddressTableModel(this);
transactionTableModel = new TransactionTableModel(platformStyle, this);
recentRequestsTableModel = new RecentRequestsTableModel(this);

subscribeToCoreSignals();
}

WalletModel::~WalletModel()
{
unsubscribeFromCoreSignals();
}

void WalletModel::preload()
{
addressTableModel->preload();
transactionTableModel->preload();
recentRequestsTableModel->preload();
subscribeToCoreSignals();
}

void WalletModel::startPollBalance()
{
// This timer will be fired repeatedly to update the balance
Expand Down
2 changes: 2 additions & 0 deletions src/qt/walletmodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ class WalletModel : public QObject
Unlocked // wallet->IsCrypted() && !wallet->IsLocked()
};

void preload();

OptionsModel* getOptionsModel() const;
AddressTableModel* getAddressTableModel() const;
TransactionTableModel* getTransactionTableModel() const;
Expand Down