Add Wallet Restore in the GUI #471

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 | --- | --- <img width="307" alt="master" src="https://user-images.githubusercontent.com/94266259/141673349-0bf8a237-ecec-42e4-a0d7-1d5863940036.png"> | <img width="307" alt="pr" src="https://user-images.githubusercontent.com/94266259/141673350-972dea23-ae56-4283-a365-819da62b7067.png"> |

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:

    auto wallet_file = wallet_path / "wallet.dat";
    fs::copy_file(backup_file, wallet_file, fs::copy_option::fail_if_exists);
    
    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.

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.

w0xlt force-pushed on Nov 14, 2021

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:

   //! Restore backup wallet
   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;

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:

        std::unique_ptr<interfaces::Wallet> wallet = node().walletClient().restoreWallet(backup_file, wallet_name, m_error_message, m_warning_message);

        if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));

        QTimer::singleShot(0, this, &RestoreWalletActivity::finish);

<p>

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?

       auto wallet = node().walletClient().restoreWallet(backup_file, wallet_name, m_error_message, m_warning_message);

        if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));

        QTimer::singleShot(0, this, &RestoreWalletActivity::finish);

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

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

--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -357,7 +357,7 @@ std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const std::string
 {
     const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::u8path(wallet_name));

-    if (fs::exists(wallet_path) || !TryCreateDirectories(wallet_path)) {
+    if (!fs::exists(wallet_path) || !TryCreateDirectories(wallet_path)) {
         error = Untranslated(strprintf("Failed to create database path '%s'. Database already exists.", fs::PathToString(wallet_path)));
         status = DatabaseStatus::FAILED_ALREADY_EXISTS;
         return nullptr;

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

Tested warning message using the following patch

--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -85,7 +85,7 @@ static void UpdateWalletSetting(interfaces::Chain& chain,
                                 std::vector<bilingual_str>& warnings)
 {
     if (!load_on_startup) return;
-    if (load_on_startup.value() && !AddWalletSetting(chain, wallet_name)) {
+    if (load_on_startup.value() && AddWalletSetting(chain, wallet_name)) {
         warnings.emplace_back(Untranslated("Wallet load on startup setting could not be updated, so wallet may not be loaded next node startup."));
     } 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.

<p>

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

jarolrod commented at 7:06 AM on November 21, 2021: member

concept ack

hebasto added the label Feature on Nov 21, 2021

hebasto added the label Wallet on Nov 21, 2021

hebasto renamed this:
~~gui: Add Wallet Restore in the GUI~~
Add Wallet Restore in the GUI
on Nov 21, 2021

hebasto commented at 8:59 PM on November 21, 2021: member

Concept ACK. @w0xlt And warm welcome as a new contributor!

hebasto requested review from promag on Nov 21, 2021

w0xlt force-pushed on Nov 22, 2021

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:

    //: 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

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

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.

w0xlt force-pushed on Nov 22, 2021

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:

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

The differences I observed after running the above statement:

diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
index f21c5048f..363b618d8 100644
--- a/src/qt/bitcoingui.cpp
+++ b/src/qt/bitcoingui.cpp
@@ -442,10 +442,10 @@ void BitcoinGUI::createActions()
         });
         connect(m_restore_wallet_action, &QAction::triggered, [this] {
             QString backupFile = GUIUtil::getOpenFileName(this,
-                //: The title for Restore Wallet File Windows
-                tr("Load Wallet Backup"), QString(),
-                //: The file extension for Restore Wallet File Windows
-                tr("Wallet Data File (*.dat)"), nullptr);
+                                                          //: The title for Restore Wallet File Windows
+                                                          tr("Load Wallet Backup"), QString(),
+                                                          //: The file extension for Restore Wallet File Windows
+                                                          tr("Wallet Data File (*.dat)"), nullptr);
             if (backupFile.isEmpty()) return;
 
             bool walletNameOk;
diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp
index 38095ee74..81822d5e5 100644
--- a/src/qt/walletcontroller.cpp
+++ b/src/qt/walletcontroller.cpp
@@ -381,8 +381,7 @@ void RestoreWalletActivity::restore(const std::string& backup_file, const std::s
         tr("Restore Wallet"),
         /*: Descriptive text of the restore wallets progress window which indicates to
             the user that wallets are currently being restored.*/
-        tr("Restoring Wallet <b>%1</b>…").arg(name.toHtmlEscaped())
-    );
+        tr("Restoring Wallet <b>%1</b>…").arg(name.toHtmlEscaped()));
 
     QTimer::singleShot(0, worker(), [this, backup_file, wallet_name] {
         std::unique_ptr<interfaces::Wallet> wallet = node().walletClient().restoreWallet(backup_file, wallet_name, m_error_message, m_warning_message);
@@ -410,4 +409,3 @@ void RestoreWalletActivity::finish()
 
     Q_EMIT finished();
 }

w0xlt force-pushed on Nov 24, 2021

shaavan approved

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!

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

luke-jr changes_requested

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.

DrahtBot added the label Needs rebase on Dec 8, 2021

w0xlt force-pushed on Dec 8, 2021

DrahtBot removed the label Needs rebase on Dec 8, 2021

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

w0xlt cross-referenced this on Dec 9, 2021 from issue wallet: Move restorewallet() logic to the wallet section by w0xlt

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)

Rspigler commented at 11:44 PM on December 14, 2021: contributor

Will test soon. Concept ACK

maflcko referenced this in commit a30642926a on Dec 16, 2021

DrahtBot added the label Needs rebase on Dec 16, 2021

sidhujag referenced this in commit 4f9fa8c482 on Dec 16, 2021

w0xlt force-pushed on Dec 16, 2021

DrahtBot removed the label Needs rebase on Dec 16, 2021

w0xlt cross-referenced this on Dec 16, 2021 from issue ci, error: p2p_timeouts.py failed by w0xlt

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.

Rspigler commented at 1:26 AM on December 21, 2021: contributor

tACK e82a65423f03d52d30478e065383489e9969621c

shaavan approved

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.

achow101 cross-referenced this on Jan 10, 2022 from issue Block unsafe std::string fs::path conversion copy_file calls by hebasto

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.

hebasto added the label Needs rebase on Jan 11, 2022

DrahtBot removed the label Needs rebase on Jan 11, 2022

w0xlt force-pushed on Jan 16, 2022

w0xlt commented at 11:09 AM on January 16, 2022: contributor

Rebased.

shaavan approved

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:

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

Diff:

1:  e82a65423 ! 1:  8dd1f21cc gui: Add Wallet Restore in the GUI
    @@ src/qt/bitcoingui.cpp: void BitcoinGUI::createActions()
                  }
              });
     +        connect(m_restore_wallet_action, &QAction::triggered, [this] {
    -+            QString backupFile = GUIUtil::getOpenFileName(this,
    ++            QString backup_file = GUIUtil::getOpenFileName(this,
     +                                                          //: The title for Restore Wallet File Windows
     +                                                          tr("Load Wallet Backup"), QString(),
     +                                                          //: The file extension for Restore Wallet File Windows
     +                                                          tr("Wallet Data File (*.dat)"), nullptr);
    -+            if (backupFile.isEmpty()) return;
    ++            if (backup_file.isEmpty()) return;
     +
     +            bool walletNameOk;
     +            //: Title of the Restore Wallet input dialog (where the wallet name is entered)
    @@ src/qt/bitcoingui.cpp: void BitcoinGUI::createActions()
     +
     +            auto activity = new RestoreWalletActivity(m_wallet_controller, this);
     +            connect(activity, &RestoreWalletActivity::restored, this, &BitcoinGUI::setCurrentWallet);
    -+            activity->restore(backupFile.toStdString(), walletName.toStdString());
    ++
    ++            auto backup_file_path = fs::PathFromString(backup_file.toStdString());
    ++            activity->restore(backup_file_path, walletName.toStdString());
     +        });
              connect(m_close_wallet_action, &QAction::triggered, [this] {
                  m_wallet_controller->closeWallet(walletFrame->currentWalletModel(), this);
    @@ src/qt/walletcontroller.cpp: void LoadWalletsActivity::load()
     +{
     +}
     +
    -+void RestoreWalletActivity::restore(const std::string& backup_file, const std::string& wallet_name)
    ++void RestoreWalletActivity::restore(const fs::path& backup_file, const std::string& wallet_name)
     +{
     +    QString name = QString::fromStdString(wallet_name);
     +
    @@ src/qt/walletcontroller.cpp: void LoadWalletsActivity::load()
     +        tr("Restoring Wallet <b>%1</b>…").arg(name.toHtmlEscaped()));
     +
     +    QTimer::singleShot(0, worker(), [this, backup_file, wallet_name] {
    -+        std::unique_ptr<interfaces::Wallet> wallet = node().walletClient().restoreWallet(backup_file, wallet_name, m_error_message, m_warning_message);
    ++        std::unique_ptr<interfaces::Wallet> wallet = node().walletLoader().restoreWallet(backup_file, wallet_name, m_error_message, m_warning_message);
     +
     +        if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));
     +
    @@ src/qt/walletcontroller.h: public:
     +public:
     +    RestoreWalletActivity(WalletController* wallet_controller, QWidget* parent_widget);
     +
    -+    void restore(const std::string& backup_file, const std::string& wallet_name);
    ++    void restore(const fs::path& backup_file, const std::string& wallet_name);
     +
     +Q_SIGNALS:
     +    void restored(WalletModel* wallet_model);

hebasto added this to the milestone 23.0 on Jan 26, 2022

hebasto removed this from the milestone 23.0 on Feb 9, 2022

DrahtBot added the label Needs rebase on Mar 22, 2022

hebasto commented at 8:14 AM on April 3, 2022: member

@w0xlt

It needs to be rebased one more time.

hebasto added the label Waiting for author on Apr 3, 2022

w0xlt commented at 5:00 AM on April 12, 2022: contributor

@hebasto Thanks. Will force-push soon.

w0xlt force-pushed on Apr 12, 2022

DrahtBot removed the label Needs rebase on Apr 12, 2022

w0xlt commented at 5:58 PM on April 12, 2022: contributor

Rebased. CI error is unrelated.

hebasto removed the label Waiting for author on Apr 12, 2022

hebasto commented at 9:21 PM on April 12, 2022: member

Rebased. CI error is unrelated.

CI job has been restarted.

hebasto commented at 9:57 PM on April 12, 2022: member

Approach ACK e681634bfbfd9df4753d364f6240a1740c9dcde7. @promag Mind having a look into this PR?

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:

namespace fs { class path; }

(The cpp already has access to the fs file)

w0xlt commented at 6:32 PM on June 2, 2022:

Done. Thanks.

furszy changes_requested

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:

diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
--- a/src/qt/bitcoingui.cpp	(revision 34dff82864965126e35876c02c8158e06d652013)
+++ b/src/qt/bitcoingui.cpp	(date 1654004973424)
@@ -433,7 +433,7 @@
             if (!walletNameOk || walletName.isEmpty()) return;
 
             auto activity = new RestoreWalletActivity(m_wallet_controller, this);
-            connect(activity, &RestoreWalletActivity::restored, this, &BitcoinGUI::setCurrentWallet);
+            connect(activity, &RestoreWalletActivity::restored, this, &BitcoinGUI::setCurrentWallet, Qt::QueuedConnection);
 
             auto backup_file_path = fs::PathFromString(backup_file.toStdString());
             activity->restore(backup_file_path, walletName.toStdString());

w0xlt force-pushed on Jun 2, 2022

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.

w0xlt commented at 9:35 PM on June 2, 2022: contributor

CI error seems unrelated.

furszy approved

furszy commented at 12:44 PM on June 3, 2022: member

Code review + tested ACK c99f58a0

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.

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.

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:

            bool wallet_name_ok;

w0xlt commented at 1:39 PM on June 27, 2022:

Done in https://github.com/bitcoin-core/gui/commit/e7a3f698b5f3f7dde8632c4911abd4e5bc1f06cb.

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:

            QString wallet_name = QInputDialog::getText(this, tr("Restore Name"), tr("Wallet Name:"), QLineEdit::Normal, "", &walletNameOk);

w0xlt commented at 1:39 PM on June 27, 2022:

Done in https://github.com/bitcoin-core/gui/commit/e7a3f698b5f3f7dde8632c4911abd4e5bc1f06cb.

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?

w0xlt commented at 1:39 PM on June 27, 2022:

Done in https://github.com/bitcoin-core/gui/commit/e7a3f698b5f3f7dde8632c4911abd4e5bc1f06cb.

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:

--- a/src/qt/bitcoingui.cpp
+++ b/src/qt/bitcoingui.cpp
@@ -473,12 +473,13 @@ void BitcoinGUI::createMenuBar()
     {
         file->addAction(m_create_wallet_action);
         file->addAction(m_open_wallet_action);
-        file->addAction(m_restore_wallet_action);
         file->addAction(m_close_wallet_action);
         file->addAction(m_close_all_wallets_action);
         file->addSeparator();
-        file->addAction(openAction);
         file->addAction(backupWalletAction);
+        file->addAction(m_restore_wallet_action);
+        file->addSeparator();
+        file->addAction(openAction);
         file->addAction(signMessageAction);
         file->addAction(verifyMessageAction);
         file->addAction(m_load_psbt_action);

?

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

w0xlt force-pushed on Jun 27, 2022

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.

jarolrod commented at 2:17 AM on June 28, 2022: member

Concept ACK, could use a release note

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!

doc: Add a release note about the "restore wallet" menu item bc13ec888c

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.

furszy approved

furszy commented at 1:29 PM on July 1, 2022: member

utACK bc13ec8

shaavan approved

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!

hebasto added this to the milestone 24.0 on Jul 10, 2022

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.

hebasto approved

hebasto commented at 7:36 PM on July 10, 2022: member

ACK bc13ec888cdc2791f79eeb6eb76b9134d670043e

hebasto merged this on Jul 10, 2022

hebasto closed this on Jul 10, 2022

sidhujag referenced this in commit ae76e37dcf on Jul 11, 2022

w0xlt deleted the branch on Jul 11, 2022

w0xlt cross-referenced this on Jul 11, 2022 from issue Fix translator comment for Restore Wallet `QInputDialog` by w0xlt

hebasto referenced this in commit 194f6dc43c on Jul 23, 2022

sidhujag referenced this in commit 760faf7f67 on Jul 24, 2022

w0xlt cross-referenced this on Sep 9, 2022 from issue Switch to the selected wallet after loading by w0xlt

bitcoin-core locked this on Nov 16, 2023