Do additional character escaping for wallet names and address labels #16826

achow101 · 2019-09-08T04:32:09Z

Fixes some places where wallet names and address labels which contain valid html or other interpreted characters are displayed incorrectly.

In the send coins dialog, if the wallet name or the address label contains valid html, then the html would be shown rather than the literal string for the wallet name or label. This PR fixes that so the true name or label is shown.

The Open Wallet menu would incorrectly show wallet names with ampersands (&). For some reason, Qt removes the first ampersand in a string. So by replacing the first ampersand with 2 ampersands, the correct number of ampersands will be shown.

Fixes the HTML escaping issues in #16820

jonatack

Tested ACK ba9f82a for the ampersand issue. I don't know how to test the send coins dialog issue. The code looks fine.

jonatack · 2019-09-08T09:55:32Z

src/qt/bitcoingui.cpp

@@ -375,6 +375,8 @@ void BitcoinGUI::createActions()
            for (const std::pair<const std::string, bool>& i : m_wallet_controller->listWalletDir()) {
                const std::string& path = i.first;
                QString name = path.empty() ? QString("["+tr("default wallet")+"]") : QString::fromStdString(path);
+                // Menu items remove single &. Single & are shown when && is in the string, but only the first occurrence. So replace only the first & with &&


nit: long comment; perhaps split it over 2 lines if you have to retouch.

Reference https://doc.qt.io/qt-5/qshortcut.html#mnemonic.

@jonatack Given you'll need to rebase #16822, you can fix the comment there if you want. I don't mind either way.

Appended the change to cad3ab5 while rebasing.

darosior · 2019-09-08T11:39:27Z

ACK ba9f82a

kristapsk · 2019-09-08T15:26:42Z

For some reason, Qt removes the first ampersand in a string.

First ampersand is removed because it is used to specify hot key. Replacing it with "&&" is recommended method. https://doc.qt.io/qt-5/qmenubar.html#details

achow101 · 2019-09-08T15:59:03Z

I don't know how to test the send coins dialog issue.

Make a wallet named like A. It should appear as A everywhere. But in the send coins dialog, on master, it will appear as A (bold A). The send coins dialog is the confirmation dialog that appears after you click Send on the send page.

When you are on the send page, set a label for the recipient in the same way that the wallet name was set. Then check the send coins dialog.

kristapsk

ACK ba9f82a (tested both cases with and without these changes)

jonatack · 2019-09-08T19:10:31Z

Thanks @achow101.

jonatack · 2019-09-08T19:32:41Z

ACK ba9f82a, I have reviewed the code and manually tested these changes, and agree they can be merged.

Additional test to my previous manual testing, with send label supplied as A:

On master:

Are you sure you want to send?
Please, review your transaction.

0.00100000 BTC from wallet 'PR16826' to 'A'  (A in bold)

This PR:

Are you sure you want to send?
Please, review your transaction.

0.00100000 BTC from wallet 'PR16826' to '<b>A</b>'

kristapsk · 2019-09-08T19:44:23Z

There is similar issue with close wallet dialog, maybe could be fixed with this PR too.

jonatack · 2019-09-08T19:47:10Z

@kristapsk good catch, reproduced your issue.

achow101 · 2019-09-08T20:42:19Z

Fixed the escaping in the close dialog. Also noticed that notifications needed HTML escaping too, so I've added escapes for the wallet name and address label in transaction notifications.

DrahtBot · 2019-09-08T21:13:05Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16822 (gui: Create wallet menu option follow-ups by jonatack)

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.

kristapsk · 2019-09-08T22:16:03Z

Tested close wallet dialog, is ok now.

jonatack · 2019-09-09T06:52:03Z

src/qt/walletcontroller.cpp

@@ -75,7 +75,7 @@ void WalletController::closeWallet(WalletModel* wallet_model, QWidget* parent)
 {
    QMessageBox box(parent);
    box.setWindowTitle(tr("Close wallet"));
-    box.setText(tr("Are you sure you wish to close wallet <i>%1</i>?").arg(wallet_model->getDisplayName()));
+    box.setText(tr("Are you sure you wish to close wallet <i>%1</i>?").arg(GUIUtil::HtmlEscape(wallet_model->getDisplayName())));


Re DrahtBot: this PR and #16822 both touch this one line. Trivial rebase for the whichever is not merged first.

laanwj · 2019-09-09T07:18:26Z

Untested ACK, thanks for adding proper escaping, ad52f05

fanquake

ACK ad52f05

Ampersand escaping. master:

#16826:

HTML escaping. master:

#16826

…ddress labels ad52f05 Escape ampersands (&) in wallet names in Open Wallet menu (Andrew Chow) 2c530ea HTML escape address labels in more dialogs and notifications (Andrew Chow) 1770a97 HTML escape the wallet name in more dialogs and notifications (Andrew Chow) Pull request description: Fixes some places where wallet names and address labels which contain valid html or other interpreted characters are displayed incorrectly. In the send coins dialog, if the wallet name or the address label contains valid html, then the html would be shown rather than the literal string for the wallet name or label. This PR fixes that so the true name or label is shown. The Open Wallet menu would incorrectly show wallet names with ampersands (`&`). For some reason, Qt removes the first ampersand in a string. So by replacing the first ampersand with 2 ampersands, the correct number of ampersands will be shown. Fixes the HTML escaping issues in #16820 ACKs for top commit: laanwj: Untested ACK, thanks for adding proper escaping, ad52f05 fanquake: ACK ad52f05 Tree-SHA512: 264bef28a8061c7f43cc30c3e04b361c614ea78b9915e8763c44553c8967131b066db500977fa6130de1f8874b9bba59e630486c58e1e3c5c165555105a6c254

cad3ab5 gui: fix autofocus in CreateWalletActivity::askPassphrase() (Jon Atack) 539d940 gui: fix passphrase labels/tooltip in createwalletdialog/askpassphrasedialog (Jon Atack) 43aa9b0 gui: rename encrypt(), blank(), and askPasshprase() (Jon Atack) Pull request description: Closes bitcoin#16820. The wallet [name escaping issue](bitcoin#15450 (review)) in that issue predates bitcoin#15450 and is fixed by bitcoin#16826. - [x] rename encrypt() to encryptWallet(), and blank() to makeBlankWallet() // EDIT: updated to isEncryptWalletChecked() isDisablePrivateKeysChecked() isMakeBlankWalletChecked() - [x] fix naming of askPasshprase() to askPassphrase() - [x] fix passphrase labels and tooltip in createwalletdialog.ui and askpassphrasedialog.ui - [x] fix grammar of labels in askpassphrase dialog and WalletController::closeWallet - [x] fix autofocus in CreateWalletActivity::askPassphrase() Squashed down to three commits. Reviewers, to test manually: build, launch the gui wallet, and look at labels/tooltips/focus with the create wallet, encrypt wallet, change password, and close wallet commands. ACKs for top commit: jb55: Approach ACK cad3ab5 instagibbs: code review and tACK cad3ab5 fanquake: ACK cad3ab5 Tree-SHA512: b441fbf8f8cd370dd692bac24f0d3c1b32fc7d947b6c3a2c9ba7cf0bc175a72b3460440f2f10f7632c0e8e0f8e65fe15615a30c46e2c7763bf258c504b457dd6

Github-Pull: bitcoin#16826 Rebased-From: 1770a97

Github-Pull: bitcoin#16826 Rebased-From: 2c530ea

Github-Pull: bitcoin#16826 Rebased-From: ad52f05

Github-Pull: bitcoin#16826 Rebased-From: 1770a97

Github-Pull: bitcoin#16826 Rebased-From: 2c530ea

Github-Pull: bitcoin#16826 Rebased-From: ad52f05

Summary: Qt interprets HTML and uses the first ampersand in a string to specify hot key. Escape HTML and ampersands in wallet names to display them unambigously. Backport of Core [[bitcoin/bitcoin#16826 | PR16826]] Test Plan: `ninja && ninja check` Run `bitcoin-qt` and create a new wallet called "Hel&lo World, and make sure it is display as a raw string everywhere (send transaction dialog, Open Wallet menu) Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7931

…s and address labels

fanquake added the GUI label Sep 8, 2019

jonatack mentioned this pull request Sep 8, 2019

gui: Create wallet menu option follow-ups #16822

Merged

5 tasks

jonatack reviewed Sep 8, 2019

View reviewed changes

kristapsk approved these changes Sep 8, 2019

View reviewed changes

achow101 added 3 commits September 8, 2019 16:40

HTML escape the wallet name in more dialogs and notifications

1770a97

HTML escape address labels in more dialogs and notifications

2c530ea

Escape ampersands (&) in wallet names in Open Wallet menu

ad52f05

achow101 force-pushed the escape-wallet-name branch from ba9f82a to ad52f05 Compare September 8, 2019 20:41

jonatack reviewed Sep 9, 2019

View reviewed changes

fanquake approved these changes Sep 9, 2019

View reviewed changes

fanquake merged commit ad52f05 into bitcoin:master Sep 9, 2019

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Sep 21, 2019

HTML escape the wallet name in more dialogs and notifications

29c75f2

Github-Pull: bitcoin#16826 Rebased-From: 1770a97

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Sep 21, 2019

HTML escape address labels in more dialogs and notifications

ce48c26

Github-Pull: bitcoin#16826 Rebased-From: 2c530ea

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Sep 21, 2019

Escape ampersands (&) in wallet names in Open Wallet menu

c978662

Github-Pull: bitcoin#16826 Rebased-From: ad52f05

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Sep 23, 2019

HTML escape the wallet name in more dialogs and notifications

982c09a

Github-Pull: bitcoin#16826 Rebased-From: 1770a97

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Sep 23, 2019

HTML escape address labels in more dialogs and notifications

ca3e3c4

Github-Pull: bitcoin#16826 Rebased-From: 2c530ea

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Sep 23, 2019

Escape ampersands (&) in wallet names in Open Wallet menu

ddb313b

Github-Pull: bitcoin#16826 Rebased-From: ad52f05

kwvg added a commit to kwvg/dash that referenced this pull request Nov 6, 2021

merge bitcoin#16826: Do additional character escaping for wallet name…

e8a5335

…s and address labels

kwvg added a commit to kwvg/dash that referenced this pull request Dec 5, 2021

merge bitcoin#16826: Do additional character escaping for wallet name…

b18211f

…s and address labels

kwvg added a commit to kwvg/dash that referenced this pull request Dec 5, 2021

merge bitcoin#16826: Do additional character escaping for wallet name…

d7281c8

…s and address labels

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Do additional character escaping for wallet names and address labels #16826

Do additional character escaping for wallet names and address labels #16826

Uh oh!

Conversation

achow101 commented Sep 8, 2019

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Sep 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Sep 8, 2019

Choose a reason for hiding this comment

Uh oh!

fanquake Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

jonatack Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

darosior commented Sep 8, 2019

Uh oh!

kristapsk commented Sep 8, 2019

Uh oh!

achow101 commented Sep 8, 2019

Uh oh!

kristapsk left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Sep 8, 2019

Uh oh!

jonatack commented Sep 8, 2019

Uh oh!

kristapsk commented Sep 8, 2019

Uh oh!

jonatack commented Sep 8, 2019

Uh oh!

achow101 commented Sep 8, 2019

Uh oh!

DrahtBot commented Sep 8, 2019

Conflicts

Uh oh!

kristapsk commented Sep 8, 2019

Uh oh!

jonatack Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj commented Sep 9, 2019

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack left a comment •

edited

Loading

jonatack Sep 8, 2019 •

edited

Loading