Skip to content
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

Jump to bottom

bugfix: Call setWalletActionsEnabled(true) only for the first wallet #43

Merged
merged 1 commit into from
Oct 24, 2020
Merged

bugfix: Call setWalletActionsEnabled(true) only for the first wallet #43

merged 1 commit into from
Oct 24, 2020

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Aug 3, 2020

On master (a787428) there is a bug:

  • open an encrypted wallet; please note that the "Encrypt Wallet..." menu item is disabled that is expected:
    Screenshot from 2020-08-03 12-38-37
  • then open any other wallet; note that the "Encrypt Wallet..." menu item gets enabled that is wrong:
    Screenshot from 2020-08-03 12-42-36

This PR fixes this bug.

@hebasto
Copy link
Member Author

hebasto commented Aug 3, 2020

The bug initially was reported by @Saibato in #42:

@hebasto hebasto mentioned this pull request Aug 3, 2020
Closed
@hebasto
Copy link
Member Author

hebasto commented Aug 3, 2020

friendly ping @achow101 @meshcollider @promag @Sjors

@maflcko maflcko added the Bug Something isn't working label Aug 3, 2020
achow101
achow101 reviewed Aug 3, 2020
View reviewed changes
Copy link
Member

@achow101 achow101 left a comment

Choose a reason for hiding this comment

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

It's not clear to me why this fixes the problem. Wouldn't a better fix be to enable/disable that menu item depending on the wallet being selected?

src/qt/bitcoingui.cpp
if (m_wallet_selector->count() == 2) {
if (m_wallet_selector->count() == 0) {
setWalletActionsEnabled(true);
} else if (m_wallet_selector->count() == 1) {
Copy link
Member

Choose a reason for hiding this comment

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

Why does this change from 2 to 1?

Copy link
Member Author

@hebasto hebasto Aug 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Because m_wallet_selector->addItem() is called after this code now.

Copy link
Contributor

@promag promag Aug 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Why not just add the count == 1 case in the old code?

Edit: if it's due to some other signal/slot then I'd say to refactor to remove that dependency.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not just add the count == 1 case in the old code?

It would change wallet selector behavior, and it won't fix a bug due to the unconditional setWalletActionsEnabled(true) call.

Edit: if it's due to some other signal/slot then I'd say to refactor to remove that dependency.

Not sure what do you mean. FWIW, moving this logic into setCurrentWalletBySelectorIndex() or setCurrentWallet() won't work as these methods do not know if a wallet has been added or removed.

@hebasto
Copy link
Member Author

hebasto commented Aug 3, 2020 •
edited
Loading

@achow101

It's not clear to me why this fixes the problem. Wouldn't a better fix be to enable/disable that menu item depending on the wallet being selected?

This is exactly how it works with this PR :)
When a new wallet added in m_wallet_selector->addItem(), a signal calls BitcoinGUI::setCurrentWalletBySelectorIndex() slot that in turn calls BitcoinGUI::setCurrentWallet().

Things are broken in master due to the unconditional call setWalletActionsEnabled(true).

@achow101
Copy link
Member

achow101 commented Aug 3, 2020

When a new wallet added in m_wallet_selector->addItem(), a signal calls BitcoinGUI::setCurrentWalletBySelectorIndex() slot that in turn calls BitcoinGUI::setCurrentWallet().

Things are broken in master due to the unconditional call setWalletActionsEnabled(true).

BitcoinGUI::setCurrentWallet() should be occurring after setWalletActionsEnabled(true), so in theory, it should be fine to do it unconditionally. So why doesn't it work?

@hebasto
Copy link
Member Author

hebasto commented Aug 3, 2020 •
edited
Loading

@achow101
I was a bit wrong/incomplete describing adding a new wallet ((

When no wallets are open, and a new first wallet is added, the currentIndexChanged(0) signal is emitted with parameter index=0. This signal calls setCurrentWalletBySelectorIndex(0) slot. Result: wallet menu items are set correctly.

When any wallet is open already, and a non-first wallet is added, the currentIndexChanged() signal is not emitted. And setCurrentWalletBySelectorIndex() is not called. Result: wallet menu items are ignored.

In the second case the setWalletActionsEnabled(true) call enables all wallet menu items unconditionally. That is wrong.

@hebasto hebasto changed the title Call setWalletActionsEnabled(true) only for the first wallet bugfix: Call setWalletActionsEnabled(true) only for the first wallet Oct 16, 2020
@jonasschnelli jonasschnelli added this to the 0.21.0 milestone Oct 22, 2020
@jonasschnelli
Copy link
Contributor

Tested ACK 20c9e03 - I could reproduce the issue on master and have verify that this PR fixes it.

@achow101
Copy link
Member

ACK 20c9e03

@maflcko maflcko merged commit d67883d into bitcoin-core:master Oct 24, 2020
@hebasto hebasto deleted the 200803-encrypt branch October 24, 2020 08:55
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 25, 2020
Merge bitcoin-core/gui#43: bugfix: Call setWalletActionsEnabled(true)…
c4ad8e8 
… only for the first wallet

20c9e03 gui: Call setWalletActionsEnabled(true) only for the first wallet (Hennadii Stepanov)

Pull request description:

  On master (a787428) there is a bug:
  - open an encrypted wallet; please note that the "Encrypt Wallet..." menu item is disabled that is expected:
  ![Screenshot from 2020-08-03 12-38-37](https://user-images.githubusercontent.com/32963518/89169084-70060c80-d586-11ea-86b9-05ef38d08f41.png)
  - then open any other wallet; note that the "Encrypt Wallet..." menu item gets enabled that is wrong:
  ![Screenshot from 2020-08-03 12-42-36](https://user-images.githubusercontent.com/32963518/89169385-d68b2a80-d586-11ea-9813-a533a847e098.png)

  This PR fixes this bug.

ACKs for top commit:
  jonasschnelli:
    Tested ACK 20c9e03 - I could reproduce the issue on master and have verify that this PR fixes it.
  achow101:
    ACK 20c9e03

Tree-SHA512: 2c9ab94bde8c4f413b0a95c05bf3a1a29f5910e0f99d6639a11dd77758c78af25b060b3fecd78117066ef15b113feb79870bc1347cc04289da915c00623e5787
apoelstra added a commit to apoelstra/elements that referenced this pull request Dec 3, 2020
@apoelstra
Merge d67883d into merged_master (Bitcoin PR bitcoin-core/gui#43)
6723d3a
laanwj pushed a commit that referenced this pull request Dec 9, 2020
Squashed 'src/crc32c/' changes from 224988680f..b5ef9be675
90c0f26 
b5ef9be675 Merge #1: Merge changes from upstream
9e7f512430 Merge remote-tracking branch 'origin/master' into bitcoin-fork
1f85030246 Add support for ARM64 darwin (#43)
3bb959c982 Remove unnecessary reinterpret_cast (#42)
2e97ab26b1 Fix (unused) ReadUint64LE for BE machines (#41)
47b40d2209 Bump dependencies. (#40)
ba74185625 Move CI to Visual Studio 2019.
efa301a7e5 Allow different C/C++ standards when this is used as a subproject.
cc6d71465e CMake: Use configure_package_config_file()

git-subtree-dir: src/crc32c
git-subtree-split: b5ef9be6755a2e61e2988bb238f13d1c0ee1fa0a
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Mar 23, 2021
@apoelstra
Merge d67883d into merged_master (Bitcoin PR bitcoin-core/gui#43)
c257b0d
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Dec 4, 2021
@hebasto @PiRK
gui: Call setWalletActionsEnabled(true) only for the first wallet
96b2719 
Summary:
> On master there is a bug:
>  - open an encrypted wallet; please note that the "Encrypt Wallet..." menu item is disabled that is expected
>  - then open any other wallet; note that the "Encrypt Wallet..." menu item gets enabled that is wrong
> This PR fixes this bug.

This is a backport of [[bitcoin-core/gui#43 | core-gui#43]]

Test Plan:
`ninja all check-all`

Failed to reproduce the bug after this change.

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10604
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@promag promag promag left review comments

@achow101 achow101 achow101 left review comments

Assignees
No one assigned
Labels
Bug Something isn't working
Projects
None yet
Milestone
0.21.0
Development

Successfully merging this pull request may close these issues.
5 participants
@hebasto @achow101 @jonasschnelli @promag @maflcko