Add Wallet Restore in the GUI #471

pull w0xlt wants to merge 2 commits into bitcoin-core:master from w0xlt:restore_wallet_gui changing 5 files +100 −1
  1. w0xlt commented at 6:22 am on November 14, 2021: contributor

    This PR adds a menu item to restore a wallet from a backup file in the GUI. Currently this option exists only in RPC interface.

    Motivation: It makes easier for non-technical users to restore backups.

    Master PR
  2. in src/wallet/wallet.cpp:369 in 985c64ebae outdated 
    366+    auto wallet_file = wallet_path / "wallet.dat";
367+
368+    fs::copy_file(backup_file, wallet_file, fs::copy_option::fail_if_exists);
369+
370+
371+    return LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings);
    shaavan commented at 7:48 am on November 14, 2021:

    nit:

    0    auto wallet_file = wallet_path / "wallet.dat";
1    fs::copy_file(backup_file, wallet_file, fs::copy_option::fail_if_exists);
2    
3    return LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings);
    w0xlt commented at 8:26 am on November 14, 2021:
    @shaavan I applied your suggestions. Thanks for them.
  3. shaavan commented at 7:51 am on November 14, 2021: contributor

    Concept ACK

    I am going to do a full tested review in some time. It would help to add the screenshots of the difference between the master branch and the PR branch in the description.

    In the meantime, here’s one nit I caught.

  4. w0xlt force-pushed on Nov 14, 2021
  5. in src/interfaces/wallet.h:323 in f202ddbb02 outdated 
    317@@ -318,6 +318,8 @@ class WalletClient : public ChainClient
318    //! Load existing wallet.
319    virtual std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;
320 
321+   virtual std::unique_ptr<Wallet> restoreWallet(const std::string& backup_file, const std::string& wallet_name, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;
    shaavan commented at 10:40 am on November 14, 2021: 
    0   //! Restore backup wallet
1   virtual std::unique_ptr<Wallet> restoreWallet(const std::string& backup_file, const std::string& wallet_name, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;
  6. in src/qt/walletcontroller.cpp:398 in f202ddbb02 outdated 
    384+    QTimer::singleShot(0, worker(), [this, backup_file, wallet_name] {
385+       std::unique_ptr<interfaces::Wallet> wallet = node().walletClient().restoreWallet(backup_file, wallet_name, m_error_message, m_warning_message);
386+
387+        if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));
388+
389+        QTimer::singleShot(0, this, &RestoreWalletActivity::finish);
    shaavan commented at 11:00 am on November 14, 2021:

    Standardise spacing:

    0        std::unique_ptr<interfaces::Wallet> wallet = node().walletClient().restoreWallet(backup_file, wallet_name, m_error_message, m_warning_message);
1
2        if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));
3
4        QTimer::singleShot(0, this, &RestoreWalletActivity::finish);

    Suggestion: How about using the auto keyword here? That would make the code simpler and more readable. @hebasto, what do you think about this suggestion?

    0       auto wallet = node().walletClient().restoreWallet(backup_file, wallet_name, m_error_message, m_warning_message);
1
2        if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));
3
4        QTimer::singleShot(0, this, &RestoreWalletActivity::finish);
  7. shaavan commented at 11:30 am on November 14, 2021: contributor

    Tested Successfully on Ubuntu 20.04

    I was able to successfully load the wallet from the wallet.dat file using the menu option introduced in this PR.

    Master PR
    Screenshot from 2021-11-14 16-11-50 Screenshot from 2021-11-14 16-07-42

    Tested that error message is working perfectly using the following patch:

     0--- a/src/wallet/wallet.cpp
 1+++ b/src/wallet/wallet.cpp
 2@@ -357,7 +357,7 @@ std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const std::string
 3 {
 4     const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::u8path(wallet_name));
 5
 6-    if (fs::exists(wallet_path) || !TryCreateDirectories(wallet_path)) {
 7+    if (!fs::exists(wallet_path) || !TryCreateDirectories(wallet_path)) {
 8         error = Untranslated(strprintf("Failed to create database path '%s'. Database already exists.", fs::PathToString(wallet_path)));
 9         status = DatabaseStatus::FAILED_ALREADY_EXISTS;
10         return nullptr;

    Screenshot of error message: Screenshot from 2021-11-14 16-40-23

    Tested warning message using the following patch

    0--- a/src/wallet/wallet.cpp
1+++ b/src/wallet/wallet.cpp
2@@ -85,7 +85,7 @@ static void UpdateWalletSetting(interfaces::Chain& chain,
3                                 std::vector<bilingual_str>& warnings)
4 {
5     if (!load_on_startup) return;
6-    if (load_on_startup.value() && !AddWalletSetting(chain, wallet_name)) {
7+    if (load_on_startup.value() && AddWalletSetting(chain, wallet_name)) {
8         warnings.emplace_back(Untranslated("Wallet load on startup setting could not be updated, so wallet may not be loaded next node startup."));
9     } else if (!load_on_startup.value() && !RemoveWalletSetting(chain, wallet_name)) {

    Screenshot of warning message:

    Screenshot from 2021-11-14 16-52-31

    There are some minor corrections and suggestions I found that might help improve this PR.

  8. in src/qt/bitcoingui.cpp:356 in f202ddbb02 outdated 
    360@@ -360,6 +361,10 @@ void BitcoinGUI::createActions()
361     m_create_wallet_action->setEnabled(false);
362     m_create_wallet_action->setStatusTip(tr("Create a new wallet"));
363 
364+    m_restore_wallet_action = new QAction(tr("Restore Wallet…"), this);
365+    m_restore_wallet_action->setEnabled(false);
366+    m_restore_wallet_action->setStatusTip(tr("Restore a wallet from a backup file"));
    jarolrod commented at 7:06 am on November 21, 2021:

    The strings being introduced need a translator comment. I have written documentation on how to construct these comments here: qml/translator-comments.md

    The difference here would be that instead of following the qml style, you’d be looking at how to mark a comment through the qt widgets style.

    For a multi-line comment, use /*: ... */. Let me know if you need help creating the translator comments.

    w0xlt commented at 6:39 am on November 22, 2021:
    Done in a047c1a
  9. jarolrod commented at 7:06 am on November 21, 2021: member
    concept ack
  10. hebasto added the label Feature on Nov 21, 2021
  11. hebasto added the label Wallet on Nov 21, 2021
  12. hebasto renamed this:
    gui: Add Wallet Restore in the GUI
    Add Wallet Restore in the GUI
    on Nov 21, 2021
  13. hebasto commented at 8:59 pm on November 21, 2021: member
    Concept ACK. @w0xlt And warm welcome as a new contributor!
  14. hebasto requested review from promag on Nov 21, 2021
  15. w0xlt force-pushed on Nov 22, 2021
  16. w0xlt force-pushed on Nov 22, 2021
  17. in src/qt/bitcoingui.cpp:364 in a047c1a335 outdated 
    360@@ -360,6 +361,12 @@ void BitcoinGUI::createActions()
361     m_create_wallet_action->setEnabled(false);
362     m_create_wallet_action->setStatusTip(tr("Create a new wallet"));
363 
364+    //: A menu item for Restore Wallet
    shaavan commented at 12:35 pm on November 22, 2021: 
    0    //: Name of the menu item that restores wallet from a backup file.
    w0xlt commented at 6:33 pm on November 22, 2021:
    Done in b5ad1bd
  18. in src/qt/bitcoingui.cpp:453 in a047c1a335 outdated 
    448+                tr("Wallet Data File (*.dat)"), nullptr);
449+            if (backupFile.isEmpty()) return;
450+
451+            bool walletNameOk;
452+            //: Title of the Restore Wallet input dialog (where the wallet name is entered)
453+            //: Restore Wallet input dialog label
    shaavan commented at 12:36 pm on November 22, 2021:
    The second line is a little confusing and feels redundant. I would suggest removing the second line.
    w0xlt commented at 6:33 pm on November 22, 2021:
    Done in b5ad1bd
  19. shaavan commented at 12:37 pm on November 22, 2021: contributor

    Changes since my last review:

    • Addressed following suggestions: 1, 2
    • Added translator comment for strings displayed on the user’s side.

    The translator comment looks good and helps significantly clarify the meaning and purpose of each string. I have some suggestions for the translator comment, though, that you might consider, to improve upon these comments.

  20. w0xlt force-pushed on Nov 22, 2021
  21. shaavan commented at 10:00 am on November 23, 2021: contributor

    Changes since my last review:

    • Translator comments are updated according to this suggestion.

    I think this PR is functionally correct now. However, there are some formatting corrections I would like to suggest. (p.s. Sorry, I forgot to check formatting in my earlier reviews of this PR.)

    To automatically correct formatting, you can use the following line of code:

    0git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v

    The differences I observed after running the above statement:

     0diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
 1index f21c5048f..363b618d8 100644
 2--- a/src/qt/bitcoingui.cpp
 3+++ b/src/qt/bitcoingui.cpp
 4@@ -442,10 +442,10 @@ void BitcoinGUI::createActions()
 5         });
 6         connect(m_restore_wallet_action, &QAction::triggered, [this] {
 7             QString backupFile = GUIUtil::getOpenFileName(this,
 8-                //: The title for Restore Wallet File Windows
 9-                tr("Load Wallet Backup"), QString(),
10-                //: The file extension for Restore Wallet File Windows
11-                tr("Wallet Data File (*.dat)"), nullptr);
12+                                                          //: The title for Restore Wallet File Windows
13+                                                          tr("Load Wallet Backup"), QString(),
14+                                                          //: The file extension for Restore Wallet File Windows
15+                                                          tr("Wallet Data File (*.dat)"), nullptr);
16             if (backupFile.isEmpty()) return;
17 
18             bool walletNameOk;
19diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp
20index 38095ee74..81822d5e5 100644
21--- a/src/qt/walletcontroller.cpp
22+++ b/src/qt/walletcontroller.cpp
23@@ -381,8 +381,7 @@ void RestoreWalletActivity::restore(const std::string& backup_file, const std::s
24         tr("Restore Wallet"),
25         /*: Descriptive text of the restore wallets progress window which indicates to
26             the user that wallets are currently being restored.*/
27-        tr("Restoring Wallet <b>%1</b>…").arg(name.toHtmlEscaped())
28-    );
29+        tr("Restoring Wallet <b>%1</b>…").arg(name.toHtmlEscaped()));
30 
31     QTimer::singleShot(0, worker(), [this, backup_file, wallet_name] {
32         std::unique_ptr<interfaces::Wallet> wallet = node().walletClient().restoreWallet(backup_file, wallet_name, m_error_message, m_warning_message);
33@@ -410,4 +409,3 @@ void RestoreWalletActivity::finish()
34 
35     Q_EMIT finished();
36 }
  22. w0xlt force-pushed on Nov 24, 2021
  23. shaavan approved
  24. shaavan commented at 11:29 am on November 26, 2021: contributor

    ACK e0b6d197f01e5f6ef1593f5dbf56bbdc4f90effb

    Changes since my last review:

    • Formatting has been fixed.

    Great work, @w0xlt!

  25. in src/wallet/interfaces.cpp:555 in e0b6d197f0 outdated 
    548@@ -549,6 +549,14 @@ class WalletClientImpl : public WalletClient
549         options.require_existing = true;
550         return MakeWallet(m_context, LoadWallet(m_context, name, true /* load_on_start */, options, status, error, warnings));
551     }
552+    std::unique_ptr<Wallet> restoreWallet(const std::string& backup_file, const std::string& wallet_name, bilingual_str& error, std::vector<bilingual_str>& warnings) override
    jarolrod commented at 6:54 am on November 28, 2021:
    This PR can’t make changes outside of the qt directory. You can make these changes in the core repo and then base this PR on that.
    w0xlt commented at 3:31 pm on December 17, 2021:
    Done in https://github.com/bitcoin/bitcoin/pull/23721
  26. luke-jr changes_requested
  27. luke-jr commented at 11:41 pm on November 30, 2021: member

    This duplicates new RPC functionality. Please first abstract the RPC code so GUI can share it instead of duplicating it.

    Also, I think it would be better to handle error conditions nicer before exposing this to the GUI. For example, we should detect if the wallet is already present and error if so. If an error occurs, we should delete the copy-destination file we made.

  28. DrahtBot added the label Needs rebase on Dec 8, 2021
  29. w0xlt force-pushed on Dec 8, 2021
  30. w0xlt force-pushed on Dec 8, 2021
  31. DrahtBot removed the label Needs rebase on Dec 8, 2021
  32. w0xlt cross-referenced this on Dec 8, 2021 from issue ci, error: bitcoind should have exited within 2400s with expected error Error opening block database. by w0xlt
  33. w0xlt cross-referenced this on Dec 9, 2021 from issue wallet: Move restorewallet() logic to the wallet section by w0xlt
  34. w0xlt commented at 2:59 am on December 9, 2021: contributor

    d021bf9 modifies the restoredwallet() RPC to reuse the wallet code and also deletes the wallet folder if the reload fails, as suggested in #471#pullrequestreview-819668069 .

    A separate PR (https://github.com/bitcoin/bitcoin/pull/23721) has been opened in the Bitcoin repository to change files that are not GUI related, as suggested in #471 (review)

  35. Rspigler commented at 11:44 pm on December 14, 2021: contributor
    Will test soon. Concept ACK
  36. maflcko referenced this in commit a30642926a on Dec 16, 2021
  37. DrahtBot added the label Needs rebase on Dec 16, 2021
  38. sidhujag referenced this in commit 4f9fa8c482 on Dec 16, 2021
  39. w0xlt force-pushed on Dec 16, 2021
  40. DrahtBot removed the label Needs rebase on Dec 16, 2021
  41. w0xlt cross-referenced this on Dec 16, 2021 from issue ci, error: p2p_timeouts.py failed by w0xlt
  42. w0xlt commented at 11:56 pm on December 16, 2021: contributor
    This PR was updated to include the qt directory changes only after https://github.com/bitcoin/bitcoin/pull/23721 has been merged.
  43. Rspigler commented at 1:26 am on December 21, 2021: contributor
    tACK e82a65423f03d52d30478e065383489e9969621c
  44. shaavan approved
  45. shaavan commented at 10:31 am on December 21, 2021: contributor

    ACK e82a65423f03d52d30478e065383489e9969621c

    • Tested that the added button works correctly and as intended on the updated PR.
    • Checked that there are no formatting issues with the latest commit.
    • Also made sure that the PR does not modify the code outside the qt directory.
  46. achow101 cross-referenced this on Jan 10, 2022 from issue Block unsafe std::string fs::path conversion copy_file calls by hebasto
  47. hebasto commented at 1:43 pm on January 11, 2022: member
    Please rebase due to the silent merge conflict with the recently merged https://github.com/bitcoin/bitcoin/pull/24026.
  48. hebasto added the label Needs rebase on Jan 11, 2022
  49. DrahtBot removed the label Needs rebase on Jan 11, 2022
  50. w0xlt force-pushed on Jan 16, 2022
  51. w0xlt commented at 11:09 am on January 16, 2022: contributor
    Rebased.
  52. shaavan approved
  53. shaavan commented at 11:42 am on January 20, 2022: contributor

    reACK 8dd1f21cc014112d0449f7fb98248e2912e8b802

    Changes since my last review:

    • backupFile -> backup_file
    • walletClient() -> walletLoader()
    • 1st type of argument of restore function changed from std::string -> fs::path. The following function’s code remained unchanged.

    I successfully compiled the updated PR, created a backup wallet, and restored it in GUI. This goes to show the PR is working perfectly.

    p.s.: To check the diff between the older version of this PR and the current one, I used the following command:

    0git range-diff 8c0bd871fcf6c5ff5851ccb18a7bc7554a0484b0..e82a65423f03d52d30478e065383489e9969621c 807169e10b4a18324356ed6ee4d69587b96a7c70..8dd1f21cc014112d0449f7fb98248e2912e8b802

    Diff:

     01:  e82a65423 ! 1:  8dd1f21cc gui: Add Wallet Restore in the GUI
 1    @@ src/qt/bitcoingui.cpp: void BitcoinGUI::createActions()
 2                  }
 3              });
 4     +        connect(m_restore_wallet_action, &QAction::triggered, [this] {
 5    -+            QString backupFile = GUIUtil::getOpenFileName(this,
 6    ++            QString backup_file = GUIUtil::getOpenFileName(this,
 7     +                                                          //: The title for Restore Wallet File Windows
 8     +                                                          tr("Load Wallet Backup"), QString(),
 9     +                                                          //: The file extension for Restore Wallet File Windows
10     +                                                          tr("Wallet Data File (*.dat)"), nullptr);
11    -+            if (backupFile.isEmpty()) return;
12    ++            if (backup_file.isEmpty()) return;
13     +
14     +            bool walletNameOk;
15     +            //: Title of the Restore Wallet input dialog (where the wallet name is entered)
16    @@ src/qt/bitcoingui.cpp: void BitcoinGUI::createActions()
17     +
18     +            auto activity = new RestoreWalletActivity(m_wallet_controller, this);
19     +            connect(activity, &RestoreWalletActivity::restored, this, &BitcoinGUI::setCurrentWallet);
20    -+            activity->restore(backupFile.toStdString(), walletName.toStdString());
21    ++
22    ++            auto backup_file_path = fs::PathFromString(backup_file.toStdString());
23    ++            activity->restore(backup_file_path, walletName.toStdString());
24     +        });
25              connect(m_close_wallet_action, &QAction::triggered, [this] {
26                  m_wallet_controller->closeWallet(walletFrame->currentWalletModel(), this);
27    @@ src/qt/walletcontroller.cpp: void LoadWalletsActivity::load()
28     +{
29     +}
30     +
31    -+void RestoreWalletActivity::restore(const std::string& backup_file, const std::string& wallet_name)
32    ++void RestoreWalletActivity::restore(const fs::path& backup_file, const std::string& wallet_name)
33     +{
34     +    QString name = QString::fromStdString(wallet_name);
35     +
36    @@ src/qt/walletcontroller.cpp: void LoadWalletsActivity::load()
37     +        tr("Restoring Wallet <b>%1</b>…").arg(name.toHtmlEscaped()));
38     +
39     +    QTimer::singleShot(0, worker(), [this, backup_file, wallet_name] {
40    -+        std::unique_ptr<interfaces::Wallet> wallet = node().walletClient().restoreWallet(backup_file, wallet_name, m_error_message, m_warning_message);
41    ++        std::unique_ptr<interfaces::Wallet> wallet = node().walletLoader().restoreWallet(backup_file, wallet_name, m_error_message, m_warning_message);
42     +
43     +        if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));
44     +
45    @@ src/qt/walletcontroller.h: public:
46     +public:
47     +    RestoreWalletActivity(WalletController* wallet_controller, QWidget* parent_widget);
48     +
49    -+    void restore(const std::string& backup_file, const std::string& wallet_name);
50    ++    void restore(const fs::path& backup_file, const std::string& wallet_name);
51     +
52     +Q_SIGNALS:
53     +    void restored(WalletModel* wallet_model);
  54. hebasto added this to the milestone 23.0 on Jan 26, 2022
  55. hebasto removed this from the milestone 23.0 on Feb 9, 2022
  56. DrahtBot added the label Needs rebase on Mar 22, 2022
  57. hebasto commented at 8:14 am on April 3, 2022: member

    @w0xlt

    It needs to be rebased one more time.

  58. hebasto added the label Waiting for author on Apr 3, 2022
  59. w0xlt commented at 5:00 am on April 12, 2022: contributor
    @hebasto Thanks. Will force-push soon.
  60. w0xlt force-pushed on Apr 12, 2022
  61. DrahtBot removed the label Needs rebase on Apr 12, 2022
  62. w0xlt commented at 5:58 pm on April 12, 2022: contributor
    Rebased. CI error is unrelated.
  63. hebasto removed the label Waiting for author on Apr 12, 2022
  64. hebasto commented at 9:21 pm on April 12, 2022: member

    Rebased. CI error is unrelated.

    CI job has been restarted.

  65. hebasto commented at 9:57 pm on April 12, 2022: member
    Approach ACK e681634bfbfd9df4753d364f6240a1740c9dcde7. @promag Mind having a look into this PR?
  66. in src/qt/walletcontroller.h:8 in e681634bfb outdated 
    4@@ -5,6 +5,7 @@
5 #ifndef BITCOIN_QT_WALLETCONTROLLER_H
6 #define BITCOIN_QT_WALLETCONTROLLER_H
7 
8+#include <fs.h>
    furszy commented at 7:06 pm on May 30, 2022:

    To not add the dependency on the header, could replace it for a:

    0namespace fs { class path; }

    (The cpp already has access to the fs file)

    w0xlt commented at 6:32 pm on June 2, 2022:
    Done. Thanks.
  67. furszy changes_requested
  68. furszy commented at 2:09 pm on May 31, 2022: member

    Tested and code reviewed e681634b

    Found the following issue:

    The restored signal is being processed prior to the full execution of addWallet (walletAdded signal slot), which causes a mismatch where the GUI shows the frame of the recently loaded wallet without updating the wallet selector (the top right combo box) nor the window title.

    It seems to be happening because QT lowers the addWallet execution priority while is creating the new WalletView object.

    It can be fixed by making the restored signal connection a Qt::QueuedConnection type. In other words, patch:

     0diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
 1--- a/src/qt/bitcoingui.cpp	(revision 34dff82864965126e35876c02c8158e06d652013)
 2+++ b/src/qt/bitcoingui.cpp	(date 1654004973424)
 3@@ -433,7 +433,7 @@
 4             if (!walletNameOk || walletName.isEmpty()) return;
 5 
 6             auto activity = new RestoreWalletActivity(m_wallet_controller, this);
 7-            connect(activity, &RestoreWalletActivity::restored, this, &BitcoinGUI::setCurrentWallet);
 8+            connect(activity, &RestoreWalletActivity::restored, this, &BitcoinGUI::setCurrentWallet, Qt::QueuedConnection);
 9 
10             auto backup_file_path = fs::PathFromString(backup_file.toStdString());
11             activity->restore(backup_file_path, walletName.toStdString());
  69. w0xlt force-pushed on Jun 2, 2022
  70. w0xlt commented at 6:36 pm on June 2, 2022: contributor
    @furszy Thanks for the sugestion. After applying it, the GUI now switches to the wallet as soon as it finishes restoring it.
  71. w0xlt commented at 9:35 pm on June 2, 2022: contributor
    CI error seems unrelated.
  72. furszy approved
  73. furszy commented at 12:44 pm on June 3, 2022: member
    Code review + tested ACK c99f58a0
  74. DrahtBot commented at 9:48 pm on June 12, 2022: contributor

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

    Conflicts

    No conflicts as of last run.

  75. in src/qt/bitcoingui.cpp:427 in c99f58a07e outdated 
    418@@ -412,6 +419,25 @@ void BitcoinGUI::createActions()
419                 action->setEnabled(false);
420             }
421         });
422+        connect(m_restore_wallet_action, &QAction::triggered, [this] {
423+            QString backup_file = GUIUtil::getOpenFileName(this,
424+                                                          //: The title for Restore Wallet File Windows
425+                                                          tr("Load Wallet Backup"), QString(),
426+                                                          //: The file extension for Restore Wallet File Windows
427+                                                          tr("Wallet Data File (*.dat)"), nullptr);
    hebasto commented at 5:28 pm on June 26, 2022:

    This line and this one https://github.com/bitcoin-core/gui/blob/c99f58a07ecc85291a140b12c71fd2a9870a005e/src/qt/walletview.cpp#L216 must be consistent. Although, have no preference which one is better.

    And please separate untranslatable file extension from the translatable string.

    w0xlt commented at 1:38 pm on June 27, 2022:
    Done in https://github.com/bitcoin-core/gui/pull/471/commits/e7a3f698b5f3f7dde8632c4911abd4e5bc1f06cb. The file extension is now separated from the file format name.
  76. in src/qt/bitcoingui.cpp:430 in c99f58a07e outdated 
    425+                                                          tr("Load Wallet Backup"), QString(),
426+                                                          //: The file extension for Restore Wallet File Windows
427+                                                          tr("Wallet Data File (*.dat)"), nullptr);
428+            if (backup_file.isEmpty()) return;
429+
430+            bool walletNameOk;
    hebasto commented at 5:31 pm on June 26, 2022:

    Please follow our naming convention:

    0            bool wallet_name_ok;
  77. in src/qt/bitcoingui.cpp:432 in c99f58a07e outdated 
    427+                                                          tr("Wallet Data File (*.dat)"), nullptr);
428+            if (backup_file.isEmpty()) return;
429+
430+            bool walletNameOk;
431+            //: Title of the Restore Wallet input dialog (where the wallet name is entered)
432+            QString walletName = QInputDialog::getText(this, tr("Restore Name"), tr("Wallet Name:"), QLineEdit::Normal, "", &walletNameOk);
    hebasto commented at 5:32 pm on June 26, 2022:

    Please follow our naming convention:

    0            QString wallet_name = QInputDialog::getText(this, tr("Restore Name"), tr("Wallet Name:"), QLineEdit::Normal, "", &walletNameOk);
  78. in src/qt/walletcontroller.cpp:418 in c99f58a07e outdated 
    413+    }
414+
415+    if (m_wallet_model) Q_EMIT restored(m_wallet_model);
416+
417+    Q_EMIT finished();
418+}
    hebasto commented at 5:33 pm on June 26, 2022:
    Add a newline?
  79. hebasto commented at 5:34 pm on June 26, 2022: member

    Approach ACK c99f58a07ecc85291a140b12c71fd2a9870a005e.

    Suggesting to apply clang-format-diff.py to your commit(s).

    Is it better to combine actions in a menu group:

     0--- a/src/qt/bitcoingui.cpp
 1+++ b/src/qt/bitcoingui.cpp
 2@@ -473,12 +473,13 @@ void BitcoinGUI::createMenuBar()
 3     {
 4         file->addAction(m_create_wallet_action);
 5         file->addAction(m_open_wallet_action);
 6-        file->addAction(m_restore_wallet_action);
 7         file->addAction(m_close_wallet_action);
 8         file->addAction(m_close_all_wallets_action);
 9         file->addSeparator();
10-        file->addAction(openAction);
11         file->addAction(backupWalletAction);
12+        file->addAction(m_restore_wallet_action);
13+        file->addSeparator();
14+        file->addAction(openAction);
15         file->addAction(signMessageAction);
16         file->addAction(verifyMessageAction);
17         file->addAction(m_load_psbt_action);

    ?

  80. gui: Add Wallet Restore in the GUI 
    Co-authored-by: Shashwat Vangani <85434418+shaavan@users.noreply.github.com>
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
    e7a3f698b5
  81. w0xlt force-pushed on Jun 27, 2022
  82. w0xlt commented at 1:41 pm on June 27, 2022: contributor
    @hebasto Thanks for the review. Your suggestions were addressed in https://github.com/bitcoin-core/gui/commit/e7a3f698b5f3f7dde8632c4911abd4e5bc1f06cb.
  83. jarolrod commented at 2:17 am on June 28, 2022: member
    Concept ACK, could use a release note
  84. shaavan commented at 12:36 pm on June 29, 2022: contributor

    Changes since my last review:

    • renamed walletName → wallet_name
    • renamed walletNameOk → wallet_name_ok
    • The connection between restored and set current wallet has been changed to Qt::QueuedConnection, which is explained in this comment #471#pullrequestreview-989631968). On a side note, a very nice catch of bug indeed, @furszy!
    • The backup wallet and restore wallet options are grouped in a menu group.
    • Declared class path.h at the beginning of walletcontroller.h to facilitate using fs::path.

    I successfully compiled this PR on Ubuntu 22.04 with Qt version 5.15.3. I was able to test that the backup wallet and restore wallet are visually grouped in the GUI.

    Screenshot:

    Screenshot from 2022-06-29 17-37-20

    I agree with @jarolrod. Adding release notes would be a good idea!

  85. doc: Add a release note about the "restore wallet" menu item bc13ec888c
  86. w0xlt commented at 1:21 pm on July 1, 2022: contributor
    Added a release note in a new commit (https://github.com/bitcoin-core/gui/pull/471/commits/bc13ec888cdc2791f79eeb6eb76b9134d670043e) to not invalidate the ACKs for the previous one.
  87. furszy approved
  88. furszy commented at 1:29 pm on July 1, 2022: member
    utACK bc13ec8
  89. shaavan approved
  90. shaavan commented at 10:57 am on July 5, 2022: contributor

    ACK bc13ec888cdc2791f79eeb6eb76b9134d670043e

    In conjunction with my previous review, I find the changes in this PR to be complete and ready for merge.

    Great work, @w0xlt!

  91. hebasto added this to the milestone 24.0 on Jul 10, 2022
  92. in src/qt/bitcoingui.cpp:434 in bc13ec888c 
    429+            QString backup_file = GUIUtil::getOpenFileName(this, title_windows, QString(), name_data_file + QLatin1String(" (*.dat)"), nullptr);
430+            if (backup_file.isEmpty()) return;
431+
432+            bool wallet_name_ok;
433+            //: Title of the Restore Wallet input dialog (where the wallet name is entered)
434+            QString wallet_name = QInputDialog::getText(this, tr("Restore Name"), tr("Wallet Name:"), QLineEdit::Normal, "", &wallet_name_ok);
    hebasto commented at 7:36 pm on July 10, 2022:
    A translator comment followed by a line with more than one tr() could not work as expected.
  93. hebasto approved
  94. hebasto commented at 7:36 pm on July 10, 2022: member
    ACK bc13ec888cdc2791f79eeb6eb76b9134d670043e
  95. hebasto merged this on Jul 10, 2022
  96. hebasto closed this on Jul 10, 2022
  97. sidhujag referenced this in commit ae76e37dcf on Jul 11, 2022
  98. w0xlt deleted the branch on Jul 11, 2022
  99. w0xlt cross-referenced this on Jul 11, 2022 from issue Fix translator comment for Restore Wallet `QInputDialog` by w0xlt
  100. hebasto referenced this in commit 194f6dc43c on Jul 23, 2022
  101. sidhujag referenced this in commit 760faf7f67 on Jul 24, 2022
  102. w0xlt cross-referenced this on Sep 9, 2022 from issue Switch to the selected wallet after loading by w0xlt
  103. bitcoin-core locked this on Nov 16, 2023


w0xlt shaavan jarolrod hebasto luke-jr Rspigler furszy DrahtBot


promag

Labels
Feature Wallet

Milestone
24.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 17:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me