refactor: Pass WalletModel object to the WalletView constructor

hebasto commented at 3:28 pm on August 7, 2021: member

An instance of the WalletView class without the walletModel data member being set is invalid. So, it is better to set it in the constructor.

Establishing one more WalletView class’s invariant in constructor:

allows to drop all of checks of thewalletModel in member functions
makes reasoning about the code that uses instances of the WalletView class easier

Possible follow ups could extend this approach to other classes, e.g., OverviewPage, TransactionView, ReceiveCoinsDialog, SendCoinsDialog, AddressBookPage.

hebasto added the label Refactoring on Aug 7, 2021

hebasto added the label Wallet on Aug 7, 2021

Talkless commented at 2:26 pm on August 10, 2021: none

Concept ACK

in src/qt/walletview.cpp:128 in 210b67beb4 outdated

150-        // Handle changes in encryption status
151-        connect(_walletModel, &WalletModel::encryptionStatusChanged, this, &WalletView::encryptionStatusChanged);
152-        updateEncryptionStatus();
153-
154-        // update HD status
155-        Q_EMIT hdEnabledStatusChanged();

Talkless commented at 2:49 pm on August 10, 2021:

I don’t see this signal being emmited in new code. It’s because initialization in the constructor, as there’s no “change” per se?

hebasto commented at 1:40 pm on August 26, 2021:

Thanks! Rebased on top of #403.

in src/qt/walletview.h:45 in 210b67beb4 outdated

47-    /** Set the wallet model.
48-        The wallet model represents a bitcoin wallet, and offers access to the list of transactions, address book and sending
49-        functionality.
50-    */
51-    void setWalletModel(WalletModel *walletModel);
52+    WalletModel* getWalletModel() const { return walletModel; }

Talkless commented at 2:53 pm on August 10, 2021:

Maybe worth adding noexcept and [[nodiscard]] too? Or at least just noexcept, as it’s not “heavyweight” operation to ask for “nodsicardness”, but still…

Talkless commented at 3:01 pm on August 10, 2021:

Also forgot to add, WalletModel *walletModel; pointer itself now can be const.

hebasto commented at 3:21 pm on August 10, 2021:

Also forgot to add, WalletModel *walletModel; pointer itself now can be const.

0In file included from qt/moc_walletview.cpp:9:
1./qt/walletview.h:45:50: error: cannot initialize return object of type 'WalletModel *' with an lvalue of type 'const WalletModel *const'
2    WalletModel* getWalletModel() const { return walletModel; }
3                                                 ^~~~~~~~~~~
41 error generated

Talkless commented at 5:24 pm on August 10, 2021:

Not const WalletModel * but WalletModel *const walletModel. Pointer itself const, not the object pointed to.

hebasto commented at 1:40 pm on August 26, 2021:

Thanks! Updated.

Talkless approved

Talkless commented at 5:23 pm on August 10, 2021: none

Wrong tab, sorry, deletec ack.

hebasto commented at 5:33 pm on August 11, 2021: member

@Talkless

I don’t see this signal being emmited in new code. It’s because initialization in the constructor, as there’s no “change” per se?

#403 could make things simpler.

So labeling this PR as a draft for now, and begging to review #403 first.

hebasto marked this as a draft on Aug 11, 2021

hebasto referenced this in commit 774a4f517c on Aug 26, 2021

qt, refactor: Pass WalletModel object to WalletView constructor

An instance of the WalletView class without walletModel data member
being set is invalid. So, it is better to set it in the constructor.

404373bc6a

qt, refactor: Drop redundant checks of walletModel

The walletModel member is set in the WalletView constructor now.

ca0e680bdc

qt, refactor: Declare getWalletModel with const and noexcept qualifiers 92ddc02a16

hebasto marked this as ready for review on Aug 26, 2021

hebasto force-pushed on Aug 26, 2021

hebasto commented at 1:39 pm on August 26, 2021: member

Updated 210b67beb4f395a3a7a606cba3cd1ec8c31cd0eb -> 92ddc02a16a74e10f24190929f05e2dcf2b55871 (pr398.01 -> pr398.02):

rebased on top of #403, effectively addressed @Talkless’s comment
addressed another @Talkless’s comment

Talkless commented at 3:44 pm on August 26, 2021: none

Code review ACK 92ddc02a16a74e10f24190929f05e2dcf2b55871

hebasto requested review from jarolrod on Aug 26, 2021

hebasto requested review from promag on Aug 26, 2021

qt, refactor: Replace WalletFrame::addWallet with WalletFrame::addView

No need to pass an instance of the WalletModel class to this method.

Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>

d319c4dae9

in src/qt/bitcoingui.cpp:680 in 92ddc02a16 outdated

675@@ -676,7 +676,7 @@ void BitcoinGUI::addWallet(WalletModel* walletModel)
676 {
677     if (!walletFrame) return;
678 
679-    WalletView* wallet_view = new WalletView(platformStyle, walletFrame);
680+    WalletView* wallet_view = new WalletView(walletModel, platformStyle, walletFrame);
681     if (!walletFrame->addWallet(walletModel, wallet_view)) return;

promag commented at 9:04 pm on September 2, 2021:

Could a commit to simplify like

0-bool WalletFrame::addWallet(WalletModel* walletModel, WalletView* walletView)
1+bool WalletFrame::addView(WalletView* walletView)

hebasto commented at 9:30 pm on September 2, 2021:

Thanks! Updated.

hebasto commented at 9:30 pm on September 2, 2021: member

Updated 92ddc02a16a74e10f24190929f05e2dcf2b55871 -> d319c4dae9ed7d59d71b926e677707fce4194d0c (pr398.02 -> pr398.03, diff):

addressed @promag’s comment

shaavan commented at 2:12 pm on September 4, 2021: contributor

Code review ACK d319c4dae9ed7d59d71b926e677707fce4194d0c

These changes seem to simplify the code a great bit. Thanks for this amazing work!

I have compiled some notes regarding the changes made in each of the commits in this PR. However, @hebasto has very clearly mentioned the details in the commit message of each commit. I still think my notes might be of some help to fellow reviewers.

NOTES:

qt, refactor: Pass WalletModel object to WalletView constructor
- The process of creating walletmodel has been moved under the constructor of the WalletView class.
- The redundant function of creating a walletmodel has been removed.
- walletview.h file has been appropriately modified to account for all the changes made in the walletview.cpp file.
qt, refactor: Drop redundant checks of walletModel
- As the name suggests, all the redundant checks of walletModel in the walletview.cpp file have been removed, as the walletmodel is now constructed during the construction of the walletview object itself.
qt, refactor: Declare getWalletModel with const and noexcept qualifiers
- This is done to ensure that the function is not changing a member variable’s value. And the given function will not throw an exception (since the walletmodel is generated with the constructor itself).
qt, refactor: Replace WalletFrame::addWallet with WalletFrame::addView
- Modified the addWallet function such that it now doesn’t need to take walletmodel as an argument. And also removed the need to check for the existence of a walletmodel inside this function.

If some of my observation is erroneous or lacking thorough observation, please do inform me.

promag commented at 10:25 am on September 5, 2021: contributor

Code review ACK d319c4dae9ed7d59d71b926e677707fce4194d0c.

hebasto commented at 4:37 am on September 6, 2021: member

@Talkless @jarolrod

Do you mind having another look into this PR?

jarolrod commented at 8:51 pm on September 6, 2021: member

ACK d319c4dae9ed7d59d71b926e677707fce4194d0c

I did light testing with different wallets and switching between them. I also ran the qt tests locally. Performed identical testing steps on macOS and on Ubuntu.

This is a nice simplification of the WalletModel/WalletView logic. It ties up nicely at the end with the last commit where we now only need to pass the WalletView object; not both a WalletModel and WalletView object. @ShaMan239 your notes on the PR are excellent and I looked at them before beginning my review. Thanks for posting them!

hebasto merged this on Sep 7, 2021

hebasto closed this on Sep 7, 2021

hebasto deleted the branch on Sep 7, 2021

sidhujag referenced this in commit 5d8af9ceda on Sep 7, 2021

domob1812 referenced this in commit a3a5d6ef3e on Sep 13, 2021

bitcoin-core locked this on Sep 7, 2022

refactor: Pass WalletModel object to the WalletView constructor #398