qt: remove unused ‘Decrypt’ wallet #42

1. 
   sehyunc commented at 11:03 pm on July 31, 2020: contributor

   Fix #1 remove ‘Decrypt’ case from enum in AskPassPhraseDialog remove ‘Decrypt’ case from switch statements show only Encrypt option in dialog would appreciate feedback, specifically on src/qt/walletview.cpp:265
2. remove unused decrypt case b3e566a6f4
3. change decrypt case in walletview dialog f9d41dacff
4. remove decrypt case from switch statements fb5b460e78
5. 
   hebasto commented at 11:11 am on August 1, 2020: member

   Concept ACK.
6. 
   Saibato commented at 11:16 am on August 1, 2020: contributor

   Concept tACK https://github.com/bitcoin-core/gui/pull/42/commits/f9d41dacff0d9d3bd2985966a19ee8d6a99c0b28

   minor nit how about this?

    0
    1index 26aa38320..2ba74082f 100644
    2--- a/src/qt/walletview.cpp
    3+++ b/src/qt/walletview.cpp
    4@@ -262,9 +262,11 @@ void WalletView::encryptWallet(bool status)
    5 {
    6     if(!walletModel)
    7         return;
    8-    AskPassphraseDialog dlg(status ? AskPassphraseDialog::Encrypt : AskPassphraseDialog::Encrypt, this);
    9-    dlg.setModel(walletModel);
   10-    dlg.exec();
   11+    if (walletModel->getEncryptionStatus() == WalletModel::Unencrypted) {
   12+        AskPassphraseDialog dlg(AskPassphraseDialog::Encrypt, this);
   13+        dlg.setModel(walletModel);
   14+        dlg.exec();
   15+    }
   16 
   17     updateEncryptionStatus();
   18 }
   

7. 
   hebasto commented at 11:58 am on August 1, 2020: member

   As a general rule each commit must compile and pass tests. Therefore, please squash all commits into a single one.

   Mind refactoring enum Mode into enum class Mode? (this suggestion could be leaved for an followup though)

   Since the encryptWalletAction is enabled only after encryptWalletAction->setChecked(false) (see: BitcoinGUI::setEncryptionStatus()), may I suggest to explicitly ignore the bool parameter (or, better, assert(!status)) in WalletFrame::encryptWallet(), and make WalletView::encryptWallet() parameterless?

8. 
   hebasto commented at 12:02 pm on August 1, 2020: member

   @Saibato

   Concept tACK f9d41da

   minor nit how about this?

    0index 26aa38320..2ba74082f 100644
    1--- a/src/qt/walletview.cpp
    2+++ b/src/qt/walletview.cpp
    3@@ -262,9 +262,11 @@ void WalletView::encryptWallet(bool status)
    4 {
    5     if(!walletModel)
    6         return;
    7-    AskPassphraseDialog dlg(status ? AskPassphraseDialog::Encrypt : AskPassphraseDialog::Encrypt, this);
    8-    dlg.setModel(walletModel);
    9-    dlg.exec();
   10+    if (walletModel->getEncryptionStatus() == WalletModel::Unencrypted) {
   11+        AskPassphraseDialog dlg(AskPassphraseDialog::Encrypt, this);
   12+        dlg.setModel(walletModel);
   13+        dlg.exec();
   14+    }
   15 
   16     updateEncryptionStatus();
   17 }
   

   Condition walletModel->getEncryptionStatus() == WalletModel::Unencrypted is always true here, right? Why do we need to add it?

9. 
   Saibato commented at 5:54 pm on August 1, 2020: contributor

   @hebasto tyi , not what I observed, when testing, maybe a minor bug?

   always true here, right? Why do we need to add it?

   The flag is an int and give us the info we need here.

   The bool does sometimes not correctly reflect all three states. And then you could encrypt and encrypted again, what makes not that much sense. Could be a minor bug that sets the status wrong before call?

   Maybe @sehyunc like to find out as an exercise?

   Did not dig deeper. tyi

10. 
    hebasto commented at 11:55 am on August 2, 2020: member

    @Saibato

    @hebasto tyi , not what I observed, when testing, maybe a minor bug?

    Mind providing steps to reproduce it?

    The bool does sometimes not correctly reflect all three states.

    The bool is just a value of encryptWalletAction->checked(), see:

    • https://doc.qt.io/qt-5/qaction.html#checked-prop
    • https://doc.qt.io/qt-5/qaction.html#triggered
11. 
    Saibato commented at 4:47 pm on August 2, 2020: contributor

    Mind providing steps to reproduce it? @hebasto sure if iirc, compile i.e. with the PR or just

    0  `AskPassphraseDialog dlg(AskPassphraseDialog::Encrypt, this) `
    

    then create multiple wallets load all encrypt all, close all and then load them randomly and then try click encrypt. You probably will find by that even the next bug not mentioned yet, hoops you asked, I like that ping pong push things forward. And as they say, its a marathon not a sprint, One runs the other backup and then baton or name/avatar change before the bugs hit in.

12. 
    hebasto commented at 11:17 pm on August 2, 2020: member

    @Saibato

    Testing fb5b460e7882eec85373bca0f41d66170392fe1c on Linux Mint 20 (Qt 5.12.8)…

    then create multiple wallets load all encrypt all

    … done…

    close all

    … done…

    and then load them randomly

    … done…

    and then try click encrypt.

    I cannot click “Encrypt Wallet” menu item because it is disabled. As expected :)

    

13. 
    Saibato commented at 6:20 am on August 3, 2020: contributor

    @hebasto And i could, so could it be since that now has happened more than often that your configuration and testing setup, could not confirm things,

    So you as somewhat main gui dev should maybe switch to common used platforms ubuntu/lubuntu/debian/mac with there default qt dev libs or could be we have here a hardware dependent racing condition?

    Or maybe I have the most exotic combination and the AI that simulates me is not efficient or fast enough to guess how it must simulate the desktop my virtual eye’s will see?

    

14. 
    hebasto commented at 9:47 am on August 3, 2020: member

    @Saibato The bug you described fixed in #43. Mind testing it?
15. 
    achow101 commented at 4:48 pm on August 3, 2020: member

    Concept ACK

    As noted by others, the commit order should be reversed or all the commits squashed so that each commit individually compiles.

16. 
    MarcoFalke commented at 5:43 pm on August 3, 2020: contributor

    Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
17. 
    hebasto commented at 7:37 am on September 9, 2020: member

    @sehyunc Are you going to address the latest comment?
18. MarcoFalke added the label up for grabs on Sep 9, 2020
19. MarcoFalke added the label Waiting for author on Sep 9, 2020
20. 
    sehyunc commented at 5:15 pm on September 11, 2020: contributor

    @sehyunc Are you going to address the latest comment?

    Yes, apologies, have been caught up in school work. Will try my best to push a new commit.

21. 
    hebasto commented at 5:13 pm on October 23, 2020: member

    Fellow reviewers! Please review #109.
22. MarcoFalke referenced this in commit 54532f46c4 on Nov 18, 2020
23. MarcoFalke closed this on Nov 18, 2020

    ---

24. laanwj referenced this in commit 90c0f267bd on Dec 9, 2020
25. 
    jarolrod commented at 7:27 pm on March 4, 2021: member

    @MarcoFalke @hebasto can the waiting for author and up for grab labels be removed from here as to not confuse anyone who may start on this by accident.
26. hebasto removed the label Waiting for author on Mar 4, 2021
27. hebasto removed the label up for grabs on Mar 4, 2021
28. bitcoin-core locked this on Aug 16, 2022

Contributors
sehyunc hebasto Saibato achow101 MarcoFalke jarolrod