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.
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
gui: Add dynamic wallets support #13100
Conversation
ce11cae
to
f61aff5
Compare
@ken2812221 does the problem exists if you use the RPC command? |
|
@ken2812221 but it does exists right? What if you launch the application with |
Maybe windows or boost is using big5 encoding. which ends in a right square bracket >>> '錢包'.encode('big5')
b'\xbf\xfa\xa5]'
>>> '錢包'.encode('big5').decode('utf8', 'replace')
'���]' |
@ken2812221 thanks for the review and bug report. Let's move the bug discussion to #13103. |
src/qt/bitcoingui.cpp
Outdated
@@ -701,6 +707,20 @@ void BitcoinGUI::openClicked() | |||
} | |||
} | |||
|
|||
void BitcoinGUI::openWalletClicked() | |||
{ | |||
QString fileName = QFileDialog::getExistingDirectory(this, tr("Open Wallet"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use QDir::toNativeSeparators
to convert /
to \
on Windows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^^
src/interfaces/node.cpp
Outdated
{ | ||
#ifdef ENABLE_WALLET | ||
try { | ||
std::string dummy_warning; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems almost exactly duplicated from loadwallet()
in rpcwallet.cpp. Does it make sense to factor it out to avoid repetition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I would like to move to WalletManager
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jnewbery done in #9888dc2.
src/interfaces/node.cpp
Outdated
wallet->postInitProcess(); | ||
return { MakeWallet(*wallet), std::string() }; | ||
} catch (...) { | ||
return { std::unique_ptr<Wallet>(), "ops" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why you're passing back an error string if it's always the string ops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ops, it's unfinished. Not sure if it should return the pair.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed, changed the return type.
src/qt/bitcoingui.cpp
Outdated
QFileDialog::ShowDirsOnly | ||
| QFileDialog::DontResolveSymlinks); | ||
if (fileName.isEmpty()) return; | ||
const auto& result = m_node.loadWallet(fileName.toStdString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of auto
here makes it difficult to understand the result.second
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No an issue now, return value is not necessary.
Concept ACK |
f61aff5
to
462bf7f
Compare
src/qt/bitcoingui.cpp
Outdated
if (fileName.isEmpty()) return; | ||
|
||
std::vector<std::string> errors; | ||
m_node.loadWallet(QDir::toNativeSeparators(fileName).toStdString(), errors); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a good future enhancement. Not necessary in the initial version IMO.
Please update description to say that this fixes #7675 |
462bf7f
to
0bdb5c6
Compare
Done. Also rebased. |
8c9cf75
to
9170254
Compare
Rebased on master. |
wallet_multiwallet.py fails |
9170254
to
3e88848
Compare
3e88848
to
a27b300
Compare
Rebased. |
src/qt/bitcoingui.cpp
Outdated
void BitcoinGUI::openWalletClicked() | ||
{ | ||
QString fileName = QFileDialog::getExistingDirectory(this, tr("Open Wallet"), | ||
QString(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: start in the data directory for the current network (mainnet/testnet/regtest) instead of the home directory, since that's where the wallet files and folders will usually be (and because it may be difficult to traverse down into .bitcoin
).
src/qt/bitcoingui.cpp
Outdated
{ | ||
QString fileName = QFileDialog::getExistingDirectory(this, tr("Open Wallet"), | ||
QString(), | ||
QFileDialog::ShowDirsOnly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only show directories and not symlinks? Wallets can be .dat
files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right.
Needs rebase |
It'd be nice to get this in to 0.18, is that feasible? Needs a rebase Overall this is looking good, concept ACK with some code review, I'll review more once rebased/the comments above have been addressed and the WIP fixup commit is done |
For a followup, once #11082 is merged, we should add a Noteable changes that may cause a headache during rebase:
|
@@ -330,6 +332,15 @@ void BitcoinGUI::createActions() | |||
openAction = new QAction(platformStyle->TextColorIcon(":/icons/open"), tr("Open &URI..."), this); | |||
openAction->setStatusTip(tr("Open a bitcoin: URI or payment request")); | |||
|
|||
m_new_wallet_action = new QAction(platformStyle->TextColorIcon(":/icons/open"), tr("New &Wallet"), this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to use icons, I think they ought to be different...
Why is there a separate File and Wallet menu in the screenshot above? They should be the same menu... |
@promag: care to rebase/pick this up again? |
A lot of this has been moved to smaller pull requests. E.g. there's now a separate PR for just the open wallet feature #15153. I suppose "unloading" a wallet would be fairly trivial on top of that. For my own work on #14912 I'd love to have a (proof of concept) "create wallet" PR to build on top of. Ideally it would a dialog, with a check box for the read-only feature (and later for the empty wallet feature in #14938). |
Feature freeze is on Feb 15th. I'd really love to have this in v0.18. @promag - any chance of a rebase so we can start testing & reviewing? |
0dd9bde gui: Refactor to use WalletController (João Barbosa) 8fa271f gui: Add WalletController (João Barbosa) cefb399 gui: Use AutoConnection for WalletModel::unload signal (João Barbosa) Pull request description: This PR is a subset of the work done in the context of #13100. This change consists in extracting from the application class the code that manages the wallet models. The role of the `WalletController` instance is to coordinate wallet operations and the window. Tree-SHA512: 6a824054376730eb7d16c643dd2003f5f60778e8ad3af707b82bc12c48438db179ca4446316b28fb17b206f4b9aba8998419aab8c5dd1f7c32467015732b5094
Can this PR be closed? I think all changes are covered by other PRs, and issue #13059 tracks the total progress. |
Thanks for updating that @jnewbery. |
0dd9bde gui: Refactor to use WalletController (João Barbosa) 8fa271f gui: Add WalletController (João Barbosa) cefb399 gui: Use AutoConnection for WalletModel::unload signal (João Barbosa) Pull request description: This PR is a subset of the work done in the context of bitcoin#13100. This change consists in extracting from the application class the code that manages the wallet models. The role of the `WalletController` instance is to coordinate wallet operations and the window. Tree-SHA512: 6a824054376730eb7d16c643dd2003f5f60778e8ad3af707b82bc12c48438db179ca4446316b28fb17b206f4b9aba8998419aab8c5dd1f7c32467015732b5094
0dd9bde gui: Refactor to use WalletController (João Barbosa) 8fa271f gui: Add WalletController (João Barbosa) cefb399 gui: Use AutoConnection for WalletModel::unload signal (João Barbosa) Pull request description: This PR is a subset of the work done in the context of bitcoin#13100. This change consists in extracting from the application class the code that manages the wallet models. The role of the `WalletController` instance is to coordinate wallet operations and the window. Tree-SHA512: 6a824054376730eb7d16c643dd2003f5f60778e8ad3af707b82bc12c48438db179ca4446316b28fb17b206f4b9aba8998419aab8c5dd1f7c32467015732b5094
0dd9bde gui: Refactor to use WalletController (João Barbosa) 8fa271f gui: Add WalletController (João Barbosa) cefb399 gui: Use AutoConnection for WalletModel::unload signal (João Barbosa) Pull request description: This PR is a subset of the work done in the context of bitcoin#13100. This change consists in extracting from the application class the code that manages the wallet models. The role of the `WalletController` instance is to coordinate wallet operations and the window. Tree-SHA512: 6a824054376730eb7d16c643dd2003f5f60778e8ad3af707b82bc12c48438db179ca4446316b28fb17b206f4b9aba8998419aab8c5dd1f7c32467015732b5094
0dd9bde gui: Refactor to use WalletController (João Barbosa) 8fa271f gui: Add WalletController (João Barbosa) cefb399 gui: Use AutoConnection for WalletModel::unload signal (João Barbosa) Pull request description: This PR is a subset of the work done in the context of bitcoin#13100. This change consists in extracting from the application class the code that manages the wallet models. The role of the `WalletController` instance is to coordinate wallet operations and the window. Tree-SHA512: 6a824054376730eb7d16c643dd2003f5f60778e8ad3af707b82bc12c48438db179ca4446316b28fb17b206f4b9aba8998419aab8c5dd1f7c32467015732b5094
0dd9bde gui: Refactor to use WalletController (João Barbosa) 8fa271f gui: Add WalletController (João Barbosa) cefb399 gui: Use AutoConnection for WalletModel::unload signal (João Barbosa) Pull request description: This PR is a subset of the work done in the context of bitcoin#13100. This change consists in extracting from the application class the code that manages the wallet models. The role of the `WalletController` instance is to coordinate wallet operations and the window. Tree-SHA512: 6a824054376730eb7d16c643dd2003f5f60778e8ad3af707b82bc12c48438db179ca4446316b28fb17b206f4b9aba8998419aab8c5dd1f7c32467015732b5094
0dd9bde gui: Refactor to use WalletController (João Barbosa) 8fa271f gui: Add WalletController (João Barbosa) cefb399 gui: Use AutoConnection for WalletModel::unload signal (João Barbosa) Pull request description: This PR is a subset of the work done in the context of bitcoin#13100. This change consists in extracting from the application class the code that manages the wallet models. The role of the `WalletController` instance is to coordinate wallet operations and the window. Tree-SHA512: 6a824054376730eb7d16c643dd2003f5f60778e8ad3af707b82bc12c48438db179ca4446316b28fb17b206f4b9aba8998419aab8c5dd1f7c32467015732b5094
0dd9bde gui: Refactor to use WalletController (João Barbosa) 8fa271f gui: Add WalletController (João Barbosa) cefb399 gui: Use AutoConnection for WalletModel::unload signal (João Barbosa) Pull request description: This PR is a subset of the work done in the context of bitcoin#13100. This change consists in extracting from the application class the code that manages the wallet models. The role of the `WalletController` instance is to coordinate wallet operations and the window. Tree-SHA512: 6a824054376730eb7d16c643dd2003f5f60778e8ad3af707b82bc12c48438db179ca4446316b28fb17b206f4b9aba8998419aab8c5dd1f7c32467015732b5094
This PR adds a menu entry
Open Wallet
to support loading existing wallets in the UI.Fixes #7675.