gui: Drop WalletModel dependency to RecentRequestsTableModel #18064

promag commented at 9:58 PM on February 3, 2020: member

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

fanquake added the label GUI on Feb 3, 2020

hebasto commented at 10:21 PM on February 3, 2020: member

Concept ACK.

DrahtBot commented at 11:40 PM on February 3, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17966 (qt, refactor: Optimize signal-slot connections logic by hebasto)
#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.

in src/qt/receivecoinsdialog.cpp:74 in 677a80d7ed outdated

  69 | @@ -70,15 +70,17 @@ void ReceiveCoinsDialog::setModel(WalletModel *_model)
  70 |  
  71 |      if(_model && _model->getOptionsModel())
  72 |      {
  73 | -        _model->getRecentRequestsTableModel()->sort(RecentRequestsTableModel::Date, Qt::DescendingOrder);
  74 | +        assert(m_recent_requests_table_model == nullptr);
  75 | +        m_recent_requests_table_model = new RecentRequestsTableModel(_model);

Empact commented at 2:06 AM on February 4, 2020:

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

promag commented at 8:04 AM on February 4, 2020:

Delete on setModel(nullptr)?

hebasto commented at 8:56 AM on February 4, 2020:

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?

promag commented at 9:19 AM on February 4, 2020:

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.

hebasto commented at 10:01 AM on February 4, 2020:

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?

promag commented at 10:04 AM on February 4, 2020:

Maybe discuss this on IRC meeting?

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

laanwj commented at 1:57 PM on February 10, 2020:

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?

promag commented at 2:30 PM on February 10, 2020:

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 force-pushed on Feb 4, 2020

promag commented at 8:02 AM on February 4, 2020: member

Rebased.

in src/qt/test/wallettests.cpp:215 in b211a5000f outdated

 211 | @@ -212,7 +212,7 @@ void TestGUI(interfaces::Node& node)
 212 |      // Check Request Payment button
 213 |      ReceiveCoinsDialog receiveCoinsDialog(platformStyle.get());
 214 |      receiveCoinsDialog.setModel(&walletModel);
 215 | -    RecentRequestsTableModel* requestTableModel = walletModel.getRecentRequestsTableModel();
 216 | +    auto requestTableModel = MakeUnique<RecentRequestsTableModel>(&walletModel);

promag commented at 9:48 AM on February 4, 2020:

Obviously this is wrong, neeed to get receiveCoinsDialog.m_recent_requests_table_model.. will fix.

promag force-pushed on Feb 4, 2020

in src/qt/test/wallettests.cpp:215 in e0cf0e2b0e outdated

 211 | @@ -212,7 +212,8 @@ void TestGUI(interfaces::Node& node)
 212 |      // Check Request Payment button
 213 |      ReceiveCoinsDialog receiveCoinsDialog(platformStyle.get());
 214 |      receiveCoinsDialog.setModel(&walletModel);
 215 | -    RecentRequestsTableModel* requestTableModel = walletModel.getRecentRequestsTableModel();
 216 | +    auto requestTableModel = receiveCoinsDialog.recentRequestsTableModel();

hebasto commented at 11:28 AM on February 4, 2020:

nanonit: diff could be smaller if RecentRequestsTableModel* remains. Also using auto for simple types seems overkill ;)

promag commented at 1:54 PM on February 4, 2020:

Done, previous version was unique_ptr.

hebasto approved

hebasto commented at 11:31 AM on February 4, 2020: member

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

gui: Drop WalletModel dependency to RecentRequestsTableModel bbb33d6431

promag force-pushed on Feb 4, 2020

hebasto approved

hebasto commented at 2:12 PM on February 4, 2020: member

re-ACK bbb33d6431d13a0ae244ab2beca506126a6430e7, the only change since e0cf0e2b0e680b7143f4019dfe7d93bc14adb070 is explicit type instead of auto.

fanquake requested review from jonasschnelli on Feb 5, 2020

in src/qt/receivecoinsdialog.h:48 in bbb33d6431

  44 | @@ -44,6 +45,8 @@ class ReceiveCoinsDialog : public QDialog
  45 |  
  46 |      void setModel(WalletModel *model);
  47 |  
  48 | +    RecentRequestsTableModel* recentRequestsTableModel() const { return m_recent_requests_table_model; }

Empact commented at 11:07 AM on February 5, 2020:

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

promag commented at 12:04 PM on February 5, 2020:

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

More correct why? Those are orthogonal.

Empact commented at 12:10 PM on February 6, 2020:

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.

Empact commented at 5:51 PM on February 6, 2020:

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 commented at 12:37 PM on February 6, 2020: contributor

Tested ACK bbb33d6431d13a0ae244ab2beca506126a6430e7

laanwj commented at 1:55 PM on February 10, 2020: member

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

promag commented at 10:20 AM on February 16, 2020: member

@laanwj not sure if you saw my reply above.

laanwj commented at 12:45 PM on February 25, 2020: member

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 commented at 9:22 AM on February 26, 2020: member

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

promag commented at 9:31 AM on April 13, 2020: member

Closing in favor of #18618.

promag closed this on Apr 13, 2020

promag deleted the branch on Apr 13, 2020

DrahtBot locked this on Feb 15, 2022