In the current implementation Qt uses ‘&’ as a signal to underscore letter and use it as a hot-key, which is not expected for case of wallet name.
The comment in the code regarding the use of an “&” on a menu item is misleading. If a wallet name has an “&” in it, it is not supposed to be interpreted as a hot-key, but it should be shown as it is without replacing it to an underscore.
See screenshots before & after:
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hebasto, pablomartin4btc, BrandonOdiwuor |
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.
In the current implementation Qt uses ‘&’ as a signal to underscore letter and use it as a hot-key, which is not expected for case of wallet name.
Does this hold for all supported Qt versions?
The comment in the code is misleading, if you replace one & to double &&, the next & will be a “single one”. All of them in the string should be replaced, not only the first occurrence.
I’m not certain about that, but the mentioned comment might have been correct for some earlier version of Qt.
I’m not certain about that, but the mentioned comment might have been correct for some earlier version of Qt.
Doc for qt 6 says:
The ampersand in the menu item’s text sets Alt+F as a shortcut for this menu. (You can use “&&” to get a real ampersand in the menu bar.)
Doc for qt 5 says the same:
The ampersand in the menu item’s text sets Alt+F as a shortcut for this menu. (You can use “&&” to get a real ampersand in the menu bar.)
And even doc for qt 4 says the same:
The ampersand in the menu item’s text sets Alt+F as a shortcut for this menu. (You can use “&&” to get a real ampersand in the menu bar.)
I believe that in original PR introduced a “partial fix” which works for the first ampersand no one just tested a wallet with several ampersands.
I tested this fix for 2 versions: Qt 5.15.14 and Qt 5.15.11, I don’t have an older one to test, but I am pretty sure that if there’s no bug in the qt itself: it should works starting from 4.8 or even earlier and up to now
I believe that in original PR introduced a “partial fix” which works for the first ampersand no one just tested a wallet with several ampersands.
It might be a platform-specific issue though. See https://bugreports.qt.io/browse/QTBUG-63361.
398@@ -399,10 +399,7 @@ void BitcoinGUI::createActions()
399 for (const std::pair<const std::string, bool>& i : m_wallet_controller->listWalletDir()) {
400 const std::string& path = i.first;
401 QString name = path.empty() ? QString("["+tr("default wallet")+"]") : QString::fromStdString(path);
402- // Menu items remove single &. Single & are shown when && is in
403- // the string, but only the first occurrence. So replace only
404- // the first & with &&.
Maybe:
0 // Menu items remove single &. Single & are shown when && is in
1 // the string. So replace & with &&.
?
I agree with @hebasto. Small nit:
0 // Menu items remove single &. Single & is shown when && are in
1 // the string. So replace & with &&.
Tested 948520b72d7fe8e7a3aa0d774159807058188627 on Ubuntu 24.04 with Qt 5.15.3. It works as expected.
Going to test other platforms.
It might be a platform-specific issue though. See https://bugreports.qt.io/browse/QTBUG-63361.
could someone test on Mac? I will make probably platform’s depend implementation of char replacement
ACK 948520b72d7fe8e7a3aa0d774159807058188627, modulo https://github.com/bitcoin-core/gui/pull/828/files#r1677136023.
Tested on Ubuntu 24.04, Windows 11 Pro and macOS 12.7.5 Monterey (x86).
398@@ -399,10 +399,9 @@ void BitcoinGUI::createActions()
399 for (const std::pair<const std::string, bool>& i : m_wallet_controller->listWalletDir()) {
400 const std::string& path = i.first;
401 QString name = path.empty() ? QString("["+tr("default wallet")+"]") : QString::fromStdString(path);
402- // Menu items remove single &. Single & are shown when && is in
403- // the string, but only the first occurrence. So replace only
404- // the first & with &&.
405- name.replace(name.indexOf(QChar('&')), 1, QString("&&"));
406+ // The single ampersand in the menu item's text sets a shortcut for this menu.
Maybe
0 // A single ampersand in the menu item's text sets a shortcut for this item.
?
Concept ACK - I’ll test it soon.
There’s a typo in the commit body (“missleading”), since you are there, perhaps a better prefix for its title could be “gui:” and a nit on the text could be:
0The comment in the code regarding the use of an "&"
1on a menu item is misleading. If a wallet name has an "&" in it,
2it is not supposed to be interpreted as a hot-key, but it should be
3shown as it is without replacing it to an underscore.
The comment in the code regarding the use of an "&"
on a menu item is misleading. If a wallet name has an "&" in it,
it is not supposed to be interpreted as a hot-key, but it should be
shown as it is without replacing it to an underscore.
re-ACK 8233ee41ab9648cd0c3bd78bc2a8d692a54d9ea0.
CI failures are unrelated.
398@@ -399,10 +399,9 @@ void BitcoinGUI::createActions()
399 for (const std::pair<const std::string, bool>& i : m_wallet_controller->listWalletDir()) {
400 const std::string& path = i.first;
401 QString name = path.empty() ? QString("["+tr("default wallet")+"]") : QString::fromStdString(path);
402- // Menu items remove single &. Single & are shown when && is in
403- // the string, but only the first occurrence. So replace only
404- // the first & with &&.
405- name.replace(name.indexOf(QChar('&')), 1, QString("&&"));
406+ // An single ampersand in the menu item's text sets a shortcut for this item.
In case the above suggestion is not taken, typo here:
0 // A single ampersand in the menu item's text sets a shortcut for this item.
tACK 8233ee41ab9648cd0c3bd78bc2a8d692a54d9ea0
before
after
