refactor: Pass WalletModel object to the WalletView constructor #398
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Concept ACK |
Talkless
reviewed
Aug 10, 2021
hebasto
added a commit
that referenced
this pull request
Aug 26, 2021
…tatuses simpler b8aa84b qt, refactor: Replace `if` check with `assert` (Hennadii Stepanov) fcdc8b0 qt, refactor: Drop redundant signalling in WalletView::setWalletModel (Hennadii Stepanov) 37dcf16 qt, refactor: Emit WalletView::encryptionStatusChanged signal directly (Hennadii Stepanov) 7d0d4c0 qt: Add WalletFrame::currentWalletSet signal (Hennadii Stepanov) Pull request description: This PR makes signal-slot paths to reach `setHDStatus` and `setEncryptionStatus` functions shorter and easier to reason about them. Required to simplify #398 (see #398 (comment)). --- **Note for reviewers.** Please verify that "Encrypt Wallet..." menu item, and the following icons  and updated properly in each and every possible scenario. ACKs for top commit: jarolrod: tACK b8aa84b Talkless: Code review ACK b8aa84b. Did build on Debian Sid with Qt 5.15.2 but no actual testing performed. ryanofsky: Code review ACK b8aa84b. Only change since last review was rebase Tree-SHA512: 275737cdba02baff71049df41bc24089e916f96326dd2dea26ec607c7949cb3aae368eeabbe3ad5a0a27651503a1d65536873726de854c5f6af259bcc29727e7
An instance of the WalletView class without walletModel data member being set is invalid. So, it is better to set it in the constructor.
The walletModel member is set in the WalletView constructor now.
|
Code review ACK 92ddc02 |
promag
reviewed
Sep 2, 2021
No need to pass an instance of the WalletModel class to this method. Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>
shaavan
reviewed
Sep 4, 2021
There was a problem hiding this comment.
Code review ACK d319c4d
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
walletmodelhas been moved under the constructor of theWalletViewclass. - The redundant function of creating a
walletmodelhas been removed. walletview.hfile has been appropriately modified to account for all the changes made in thewalletview.cppfile.
- The process of creating
- qt, refactor: Drop redundant checks of walletModel
- As the name suggests, all the redundant checks of
walletModelin thewalletview.cppfile have been removed, as thewalletmodelis now constructed during the construction of thewalletviewobject itself.
- As the name suggests, all the redundant checks of
- 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
walletmodelis generated with the constructor itself).
- 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
- qt, refactor: Replace WalletFrame::addWallet with WalletFrame::addView
- Modified the
addWalletfunction such that it now doesn’t need to takewalletmodelas an argument. And also removed the need to check for the existence of awalletmodelinside this function.
- Modified the
If some of my observation is erroneous or lacking thorough observation, please do inform me.
promag
reviewed
Sep 5, 2021
jarolrod
reviewed
Sep 6, 2021
There was a problem hiding this comment.
ACK d319c4d
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!
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Sep 7, 2021
domob1812
added a commit
to domob1812/namecoin-core
that referenced
this pull request
Sep 13, 2021
Updated Namecoin UI for bitcoin-core/gui#398.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
An instance of the
WalletViewclass without thewalletModeldata member being set is invalid. So, it is better to set it in the constructor.Establishing one more
WalletViewclass's invariant in constructor:walletModelin member functionsWalletViewclass easierPossible follow ups could extend this approach to other classes, e.g.,
OverviewPage,TransactionView,ReceiveCoinsDialog,SendCoinsDialog,AddressBookPage.