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

achow101 commented at 4:32 AM on September 8, 2019: member

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

fanquake added the label GUI on Sep 8, 2019

in src/qt/bitcoingui.cpp:378 in ba9f82a3ec outdated

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

jonatack commented at 9:55 AM on September 8, 2019:

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

promag commented at 9:50 PM on September 8, 2019:

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

fanquake commented at 8:54 AM on September 9, 2019:

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

jonatack commented at 9:08 AM on September 9, 2019:

Appended the change to https://github.com/bitcoin/bitcoin/pull/16822/commits/cad3ab5db835e1d0c44c6a5fc0e4b2d1661dcd5c while rebasing.

jonatack commented at 10:05 AM on September 8, 2019: member

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

darosior commented at 11:39 AM on September 8, 2019: member

ACK ba9f82a3ec81b8cd8fd729e43b87e93debbd7fba

kristapsk commented at 3:26 PM on September 8, 2019: contributor

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 commented at 3:59 PM on September 8, 2019: member

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 approved

kristapsk commented at 5:02 PM on September 8, 2019: contributor

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

jonatack commented at 7:10 PM on September 8, 2019: member

Thanks @achow101.

jonatack commented at 7:32 PM on September 8, 2019: member

ACK ba9f82a3ec81b8cd8fd729e43b87e93debbd7fba, 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 commented at 7:44 PM on September 8, 2019: contributor

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

jonatack commented at 7:47 PM on September 8, 2019: member

@kristapsk good catch, reproduced your issue.

HTML escape the wallet name in more dialogs and notifications 1770a972d4

HTML escape address labels in more dialogs and notifications 2c530ea2ad

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

achow101 force-pushed on Sep 8, 2019

achow101 commented at 8:42 PM on September 8, 2019: member

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 commented at 9:13 PM on September 8, 2019: member

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 commented at 10:16 PM on September 8, 2019: contributor

Tested close wallet dialog, is ok now.

in src/qt/walletcontroller.cpp:78 in ad52f054f6

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