gui: Drop WalletModel dependency to RecentRequestsTableModel #18064

promag · 2020-02-03T21:58:05Z

This PR moves the RecentRequestsTableModel instancing to where it's needed which in turn breaks the circular dependency between WalletModel and RecentRequestsTableModel.

hebasto · 2020-02-03T22:21:29Z

Concept ACK.

DrahtBot · 2020-02-03T23:40:18Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

qt, refactor: Optimize signal-slot connections logic #17966 (qt, refactor: Optimize signal-slot connections logic by hebasto)
Native Descriptor Wallets using DescriptorScriptPubKeyMan #16528 (Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Empact · 2020-02-04T02:06:34Z

src/qt/receivecoinsdialog.cpp

@@ -70,15 +70,17 @@ void ReceiveCoinsDialog::setModel(WalletModel *_model)

    if(_model && _model->getOptionsModel())
    {
-        _model->getRecentRequestsTableModel()->sort(RecentRequestsTableModel::Date, Qt::DescendingOrder);
+        assert(m_recent_requests_table_model == nullptr);
+        m_recent_requests_table_model = new RecentRequestsTableModel(_model);


No delete necessary in this case? I tend to prefer it occur even if not strictly necessary.

Delete on setModel(nullptr)?

No delete necessary in this case?

m_recent_requests_table_model is a QObject with a parent. This ensures that it will be deleted during its parent deletion, no?

Yes. Only thing "wrong" is that when WalletModel is destroyed we will have a dangling
m_recent_requests_table_model. But AFAICT this shouldn't be a concern because that happens when the wallet is being unloaded and everything is being destroyed.

Anyway, I'm thinking we could move away from these setModel(WalletModel*) and pass it on the constructor, but it's going to take a while to get there.

Only thing "wrong" is that when WalletModel is destroyed we will have a dangling
m_recent_requests_table_model.

Sounds not good...

But AFAICT this shouldn't be a concern because that happens when the wallet is being unloaded and everything is being destroyed.

Agree.

Anyway, I'm thinking we could move away from these setModel(WalletModel*) and pass it on the constructor, but it's going to take a while to get there.

Maybe discuss this on IRC meeting?

Maybe discuss this on IRC meeting?

Not sure worth it, a draft PR with the whole change might be better.

This will create a new RecentRequestsTableModel every time setModel is called. I do not think this is expected behavior. setModel should set a model not create one.

Maybe pass it in from the caller?

This is called once, see the assertion above.

There is no use case for switching models, that's why I think we can slowly refactor to get the dependencies on the constructor rather than having these setters, which add unnecessary state management.

promag · 2020-02-04T08:02:36Z

Rebased.

src/qt/test/wallettests.cpp

hebasto

ACK e0cf0e2, tested on Linux Mint 19.3.
No user-faced changes in behavior are observed. Local tests, including test_bitcoin-qt, have passed.

src/qt/test/wallettests.cpp

hebasto

re-ACK bbb33d6, the only change since e0cf0e2 is explicit type instead of auto.

Empact · 2020-02-05T11:07:30Z

src/qt/receivecoinsdialog.h

@@ -44,6 +45,8 @@ class ReceiveCoinsDialog : public QDialog

    void setModel(WalletModel *model);

+    RecentRequestsTableModel* recentRequestsTableModel() const { return m_recent_requests_table_model; }


nit: seems like returning a const pointer would be more correct, given that this is a const method

Perhaps negligible as this is only used in the test, it has no other impact:

diff --git a/src/qt/receivecoinsdialog.h b/src/qt/receivecoinsdialog.h index e400129f7..5dc340aed 100644 --- a/src/qt/receivecoinsdialog.h +++ b/src/qt/receivecoinsdialog.h @@ -45,7 +45,7 @@ public: void setModel(WalletModel *model); - RecentRequestsTableModel* recentRequestsTableModel() const { return m_recent_requests_table_model; } + const RecentRequestsTableModel* recentRequestsTableModel() const { return m_recent_requests_table_model; } public Q_SLOTS: void clear(); diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index c15b1c0d6..276df75f4 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -212,7 +212,7 @@ void TestGUI(interfaces::Node& node) // Check Request Payment button ReceiveCoinsDialog receiveCoinsDialog(platformStyle.get()); receiveCoinsDialog.setModel(&walletModel); - RecentRequestsTableModel* requestTableModel = receiveCoinsDialog.recentRequestsTableModel(); + const RecentRequestsTableModel* requestTableModel = receiveCoinsDialog.recentRequestsTableModel(); assert(requestTableModel); // Label input

nit: seems like returning a const pointer would be more correct, given that this is a const method

More correct why? Those are orthogonal.

Well, not in every case, but in many cases it would be incongruent if an instance was const and its member was not - for example, if I have a const counter object, but I exposed the current count in an externally modifiable way, then that would be incongruent IMO.

In this case, it's not necessary to expose members in a non-const way from a const method, so I would bias toward the more restrictive presentation.

An alternative is a method that returns a bool rather than the member itself, since that is all that is needed to satisfy the test.

jonasschnelli · 2020-02-06T12:37:40Z

Tested ACK bbb33d6

laanwj · 2020-02-10T13:55:06Z

~~code-review ACK bbb33d6~~
(see comment)
#18064 (comment)

promag · 2020-02-16T10:20:42Z

@laanwj not sure if you saw my reply above.

laanwj · 2020-02-25T12:45:30Z

I still dislike setModel actually creating the model, or conceptually, a dialog creating its own model (for that matter). But if everyone else is ok with it's fine…

promag · 2020-02-26T09:22:21Z

@laanwj I'll try to push the whole change where setModel is removed.

promag · 2020-04-13T09:31:52Z

Closing in favor of #18618.

fanquake added the GUI label Feb 3, 2020

Empact reviewed Feb 4, 2020

View reviewed changes

promag force-pushed the 2020-02-recentrequeststablemodel branch from 677a80d to b211a50 Compare February 4, 2020 07:59

promag commented Feb 4, 2020

View reviewed changes

src/qt/test/wallettests.cpp Outdated Show resolved Hide resolved

promag force-pushed the 2020-02-recentrequeststablemodel branch from b211a50 to e0cf0e2 Compare February 4, 2020 09:54

hebasto approved these changes Feb 4, 2020

View reviewed changes

src/qt/test/wallettests.cpp Outdated Show resolved Hide resolved

gui: Drop WalletModel dependency to RecentRequestsTableModel

bbb33d6

promag force-pushed the 2020-02-recentrequeststablemodel branch from e0cf0e2 to bbb33d6 Compare February 4, 2020 13:54

hebasto approved these changes Feb 4, 2020

View reviewed changes

fanquake requested a review from jonasschnelli February 5, 2020 07:40

Empact reviewed Feb 5, 2020

View reviewed changes

This was referenced Feb 11, 2020

gui: Balance/TxStatus polling update based on last block hash. #17993

Merged

qt, refactor: Optimize signal-slot connections logic #17966

Closed

Native Descriptor Wallets using DescriptorScriptPubKeyMan #16528

Merged

This was referenced Feb 21, 2020

External signer support - Wallet Box edition #16546

Merged

UI external signer support (e.g. hardware wallet) #16549

Closed

promag mentioned this pull request Mar 2, 2020

wip: gui: Refactor to drop client and wallet models setters #18239

Closed

promag closed this Apr 13, 2020

promag deleted the 2020-02-recentrequeststablemodel branch April 13, 2020 09:32

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

		@@ -44,6 +45,8 @@ class ReceiveCoinsDialog : public QDialog

		void setModel(WalletModel *model);

		RecentRequestsTableModel* recentRequestsTableModel() const { return m_recent_requests_table_model; }

gui: Drop WalletModel dependency to RecentRequestsTableModel #18064

gui: Drop WalletModel dependency to RecentRequestsTableModel #18064

Uh oh!

Conversation

promag commented Feb 3, 2020

Uh oh!

hebasto commented Feb 3, 2020

Uh oh!

DrahtBot commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Feb 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Feb 4, 2020

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Feb 6, 2020

Uh oh!

laanwj commented Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Feb 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Feb 25, 2020

Uh oh!

promag commented Feb 26, 2020

Uh oh!

promag commented Apr 13, 2020

Uh oh!

Uh oh!

DrahtBot commented Feb 3, 2020 •

edited

Loading

promag Feb 4, 2020 •

edited

Loading

laanwj Feb 10, 2020 •

edited

Loading

laanwj commented Feb 10, 2020 •

edited

Loading

promag commented Feb 16, 2020 •

edited

Loading