Rendering an amp characters in the wallet name for QMenu #828

knst · 2024-07-13T12:32:11Z

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:

DrahtBot · 2024-07-13T12:32:14Z

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

Reviews

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#824 (Migrate legacy wallets that are not loaded by achow101)

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.

hebasto · 2024-07-14T10:59:39Z

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.

knst · 2024-07-14T12:52:10Z

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

hebasto · 2024-07-14T13:20:52Z

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.

hebasto

Tested 948520b on Ubuntu 24.04 with Qt 5.15.3. It works as expected.

Going to test other platforms.

hebasto · 2024-07-14T13:29:01Z

src/qt/bitcoingui.cpp

-                // Menu items remove single &. Single & are shown when && is in
-                // the string, but only the first occurrence. So replace only
-                // the first & with &&.


I think that the comment should be adjusted rather than being deleted entirely.

Maybe:

// Menu items remove single &. Single & are shown when && is in // the string. So replace & with &&.

?

I agree with @hebasto. Small nit:

// Menu items remove single &. Single & is shown when && are in // the string. So replace & with &&.

hebasto · 2024-07-14T13:32:15Z

cc @achow101 @jarolrod

knst · 2024-07-14T16:25:49Z

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

hebasto

ACK 948520b, 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).

hebasto · 2024-07-15T08:39:21Z

src/qt/bitcoingui.cpp

-                // Menu items remove single &. Single & are shown when && is in
-                // the string, but only the first occurrence. So replace only
-                // the first & with &&.


Maybe:

// Menu items remove single &. Single & are shown when && is in // the string. So replace & with &&.

?

hebasto

re-ACK 7581817

hebasto · 2024-07-15T13:11:24Z

src/qt/bitcoingui.cpp

-                // the string, but only the first occurrence. So replace only
-                // the first & with &&.
-                name.replace(name.indexOf(QChar('&')), 1, QString("&&"));
+                // The single ampersand in the menu item's text sets a shortcut for this menu.


Maybe

Suggested change

// The single ampersand in the menu item's text sets a shortcut for this menu.

// A single ampersand in the menu item's text sets a shortcut for this item.

?

pablomartin4btc

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:

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.

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.

hebasto

re-ACK 8233ee4.

CI failures are unrelated.

pablomartin4btc

tACK 8233ee4

It corrects the bug in master.

before
after

pablomartin4btc · 2024-07-17T22:30:23Z

src/qt/bitcoingui.cpp

-                // Menu items remove single &. Single & are shown when && is in
-                // the string, but only the first occurrence. So replace only
-                // the first & with &&.


I agree with @hebasto. Small nit:

// Menu items remove single &. Single & is shown when && are in // the string. So replace & with &&.

pablomartin4btc · 2024-07-17T22:35:53Z

src/qt/bitcoingui.cpp

-                // the string, but only the first occurrence. So replace only
-                // the first & with &&.
-                name.replace(name.indexOf(QChar('&')), 1, QString("&&"));
+                // 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:

Suggested change

// An single ampersand in the menu item's text sets a shortcut for this item.

// A single ampersand in the menu item's text sets a shortcut for this item.

BrandonOdiwuor

ACK 8233ee4. Tested on Ubuntu 22.04 using Qt version 5.15.3

… name for QMenu 8233ee4 gui: correct replacement of amp character in the wallet name for QMenu (Konstantin Akimov) Pull request description: 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](https://github.com/bitcoin/bitcoin/pull/30446/files#diff-2ecf8cbf369cf3d2f3d2b1cf5cfe4c1a647d63e11e2885d2fd0ac11fb5f7a804L402-L404) 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: ![Screenshot_20240713_122454](https://github.com/user-attachments/assets/e36d6e4c-d872-4b4c-b55e-bcfde9881281) ![Screenshot_20240713_131304](https://github.com/user-attachments/assets/9484687d-0aea-4061-a461-5d187762a4b4) ACKs for top commit: hebasto: re-ACK 8233ee4. pablomartin4btc: tACK 8233ee4 BrandonOdiwuor: ACK 8233ee4. Tested on Ubuntu 22.04 using Qt version 5.15.3 Tree-SHA512: 918c2c05555d203a8b203794c138651d4a1691a05a858631d5a4664b78e150402d1ae4a02ee5181f63a5b22a09badca0a4ea14a626f45f8cbe557226c308b8c5

…, #362, #828, bitcoin#21912, bitcoin#21942, bitcoin#21988 c7d3161 Merge bitcoin-core/gui#362: Add keyboard shortcuts to context menus (Hennadii Stepanov) 25f87b9 Merge bitcoin-core/gui#121: Early subscribe core signals in transaction table model (Hennadii Stepanov) ed56e28 Merge bitcoin-core/gui#335: test: Use QSignalSpy instead of QEventLoop (Hennadii Stepanov) c52b756 Merge bitcoin-core/gui#281: set shortcuts for console's resize buttons (W. J. van der Laan) b442a59 Merge bitcoin#21988: doc: note that brew installed qt is not supported (W. J. van der Laan) 0e2e315 Merge bitcoin#21942: docs: improve make with parallel jobs description. (MarcoFalke) c2735a8 Merge bitcoin#21912: doc: Remove mention of priority estimation (W. J. van der Laan) 1d56d20 Merge bitcoin-core/gui#257: refactor: Use template function qOverload in signal-slot connections (Hennadii Stepanov) b5fb559 Merge bitcoin-core/gui#18: Add peertablesortproxy module (Hennadii Stepanov) 1cdd9fb refactor: use new QAction style for governance list and masternode list (Konstantin Akimov) 4f89c98 Merge bitcoin-core/gui#263: Revamp context menus (Hennadii Stepanov) c36bb8e fix: use && in governance urls instead & (Konstantin Akimov) 1e585b1 Merge bitcoin-core/gui#828: Rendering an amp characters in the wallet name for QMenu (Hennadii Stepanov) Pull request description: ## Issue being fixed or feature implemented Just regular backports from bitcoin v22, mostly Qt related ## What was done? See commits for a list of backports. This PR also fixes a rendering an url on Governance tab if it has any '&' inside. see screenshots. Original url: `https://example.com/?test=nothing&to=see&&lol` - yes, double '&&' just for test even if url is silly. Failed behaviour: ![image](https://github.com/user-attachments/assets/ac45c192-7d0e-4cd2-97f8-060af8f3911b) Correctly rendered: ![image](https://github.com/user-attachments/assets/5e345197-776a-4bb8-9476-cab4aba3429e) ## How Has This Been Tested? Run unit/functional tests ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK c7d3161 UdjinM6: utACK c7d3161 Tree-SHA512: 67e7e8e0ec1a768d1f13baa48c123e4a415d3f32177a427d8117339a5eacf70864ebf46e9f1165bb8a3bf9c231f7929d33ac6aa19742e06a4e19d2f86dda6dc3

knst mentioned this pull request Jul 13, 2024

fix: rendering an amp characters in the wallet name for QMenu bitcoin/bitcoin#30446

Closed

This was referenced Jul 13, 2024

Migrate legacy wallets that are not loaded #824

Merged

Remove the legacy wallet and BDB dependency bitcoin/bitcoin#28710

Draft

hebasto reviewed Jul 14, 2024

View reviewed changes

hebasto approved these changes Jul 15, 2024

View reviewed changes

knst force-pushed the fix-qt-wallet-underscore branch from 948520b to 7581817 Compare July 15, 2024 12:19

knst requested a review from hebasto July 15, 2024 12:21

hebasto approved these changes Jul 15, 2024

View reviewed changes

pablomartin4btc reviewed Jul 15, 2024

View reviewed changes

knst changed the title ~~fix: rendering an amp characters in the wallet name for QMenu~~ gui: rendering an amp characters in the wallet name for QMenu Jul 15, 2024

knst force-pushed the fix-qt-wallet-underscore branch from 7581817 to 8233ee4 Compare July 15, 2024 15:45

DrahtBot added the CI failed label Jul 15, 2024

hebasto approved these changes Jul 15, 2024

View reviewed changes

DrahtBot requested a review from pablomartin4btc July 15, 2024 18:27

hebasto changed the title ~~gui: rendering an amp characters in the wallet name for QMenu~~ Rendering an amp characters in the wallet name for QMenu Jul 15, 2024

1alexbc1 approved these changes Jul 16, 2024

View reviewed changes

pablomartin4btc approved these changes Jul 17, 2024

View reviewed changes

hebasto added this to the 28.0 milestone Jul 18, 2024

BrandonOdiwuor approved these changes Jul 25, 2024

View reviewed changes

hebasto merged commit c9b7a79 into bitcoin-core:master Jul 29, 2024
15 of 16 checks passed

knst deleted the fix-qt-wallet-underscore branch July 29, 2024 09:37

knst mentioned this pull request Aug 20, 2024

backport: bitcoin-core#gui18, #121, #257, #263, #281, #335, #362, #828, bitcoin#21912, #21942, #21988 dashpay/dash#6168

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rendering an amp characters in the wallet name for QMenu #828

Rendering an amp characters in the wallet name for QMenu #828

knst commented Jul 13, 2024 •

edited

Loading

DrahtBot commented Jul 13, 2024 •

edited

Loading

hebasto commented Jul 14, 2024

knst commented Jul 14, 2024 •

edited

Loading

hebasto commented Jul 14, 2024

hebasto left a comment

hebasto Jul 14, 2024

hebasto Jul 15, 2024

pablomartin4btc Jul 17, 2024

hebasto commented Jul 14, 2024

knst commented Jul 14, 2024

hebasto left a comment •

edited

Loading

hebasto Jul 15, 2024

hebasto left a comment

hebasto Jul 15, 2024 •

edited

Loading

pablomartin4btc left a comment

hebasto left a comment

pablomartin4btc left a comment

pablomartin4btc Jul 17, 2024

pablomartin4btc Jul 17, 2024

BrandonOdiwuor left a comment

	// The single ampersand in the menu item's text sets a shortcut for this menu.
	// A single ampersand in the menu item's text sets a shortcut for this item.

	// An single ampersand in the menu item's text sets a shortcut for this item.
	// A single ampersand in the menu item's text sets a shortcut for this item.

Rendering an amp characters in the wallet name for QMenu #828

Rendering an amp characters in the wallet name for QMenu #828

Conversation

knst commented Jul 13, 2024 • edited Loading

DrahtBot commented Jul 13, 2024 • edited Loading

Reviews

Conflicts

hebasto commented Jul 14, 2024

knst commented Jul 14, 2024 • edited Loading

hebasto commented Jul 14, 2024

hebasto left a comment

Choose a reason for hiding this comment

hebasto Jul 14, 2024

Choose a reason for hiding this comment

hebasto Jul 15, 2024

Choose a reason for hiding this comment

pablomartin4btc Jul 17, 2024

Choose a reason for hiding this comment

hebasto commented Jul 14, 2024

knst commented Jul 14, 2024

hebasto left a comment • edited Loading

Choose a reason for hiding this comment

hebasto Jul 15, 2024

Choose a reason for hiding this comment

hebasto left a comment

Choose a reason for hiding this comment

hebasto Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

pablomartin4btc left a comment

Choose a reason for hiding this comment

hebasto left a comment

Choose a reason for hiding this comment

pablomartin4btc left a comment

Choose a reason for hiding this comment

pablomartin4btc Jul 17, 2024

Choose a reason for hiding this comment

pablomartin4btc Jul 17, 2024

Choose a reason for hiding this comment

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

knst commented Jul 13, 2024 •

edited

Loading

DrahtBot commented Jul 13, 2024 •

edited

Loading

knst commented Jul 14, 2024 •

edited

Loading

hebasto left a comment •

edited

Loading

hebasto Jul 15, 2024 •

edited

Loading