Noticed while reviewing #31287 (review) that the [default wallet] part of progress messages remains untranslated while the rest of the string is translated.
Fix this in all places where CWallet::ShowProgress (which has a cancel button) and Chain::showProgress (which doesn’t have a cancel button) are called by making “default wallet” into a translated string.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31296.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | pablomartin4btc, furszy, achow101 |
| Concept ACK | hebasto |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
re: #31296 (comment)
What are the steps to test this with sqlite? If it isn’t needed after the bdb removal, it can probably be skipped?
Not sure if this will always be the case, but it is currently possible to create an sqlite wallet with no name with bitcoin-cli -regtest createwallet "".
Regardless of the details of the bug though, I think the real problem is that the GetDisplayName() method name suggests it should be used to get wallet names for display while the code comment and implementation suggest it’s only meant be used for logging. So I updated the bugfix and added a second refactoring commit to this PR to clean up the interface and prevent this type of problem in the future. That might motivate the PR more beyond the current bugfix.
Updated 87152db6668edbdb863adfb12c45533b37522ca6 -> ebb77ab77df4081f27ce55e8b28b0d35990caac8 (pr/dtran.1 -> pr/dtran.2, compare) adding some refactoring after the bugfix.
pr/dtran.2 -> pr/dtran.3, compare) due to conflict with #31061
Noticed while reviewing #31287
(https://github.com/bitcoin/bitcoin/pull/31287#discussion_r1843809721) that the
[default wallet] part of progress messages remains untranslated while the rest
of the string is translated. Fix this in all places where Wallet::ShowProgress
(which has a cancel button) and chain::showProgress (which doesn't have a
cancel button) are called by making "default wallet" into a translated string.
To minimize scope of this bugfix, this introduces a new wallet DisplayName()
method which behaves differently than the existing GetDisplayName() method. The
existing method will be cleaned up in the following commit.
The GetDisplayName() method name was confusing because it suggested the return
value could be used for display, while documentation and implementation
indicated it only meant to be used for logging. Also the name didn't suggest
that it was formatting the wallet names, which made it harder understand how
messages were formatted in the places it was called. Fix these issues by
splitting up the GetDisplayName() method and replacing it with LogName() /
DisplayName() methods.
This commit is a refactoring that does not change any behavior.
pr/dtran.3 -> pr/dtran.4, compare) due to conflict with #28710
911+ std::string LogName() const override
912 {
913- std::string wallet_name = GetName().length() == 0 ? "default wallet" : GetName();
914- return strprintf("[%s]", wallet_name);
915+ std::string name{GetName()};
916+ return name.empty() ? "default wallet" : name;
nit: perhaps a static const can be used
0 return name.empty() ? DEFAULT_NAME : name;
ACK db225cea56b0531cc42d4b89dc61b02890f432ff
Left a small nit suggestion.
hebasto did this break the soft string freeze? #32275 (comment)
Translations followup in #33193.