[Qt] Rework of payment request UI (mainly for insecure pr) #3145

Diapolo commented at 2:14 PM on October 24, 2013: none

this shows insecure (unsecured) payment requests in a new yellowish colored UI (based on the secure payment request UI) instead of our normal payment UI
allows us to receive paymentACK messages for insecure payment requests
allows us to handle expirations for insecure payment request
changed walletmodel, so that all types of payment requests don't touch the addressbook

This is an attempt to make payment requests much more usable via our GUI and already addresses some of the problems mentioned in #2936. I'm unsure, if #3095 also belongs here?

This is considered a preview and open for feedback and bug reports!

New insecure pr UI: insecure pr sendcoins UI

Todo:

re-add delete button for payment entries in sendcoinsdialog

laanwj commented at 6:04 PM on October 24, 2013: member

Haven't tested yet, but screenshots look good.

mikehearn commented at 7:02 PM on October 24, 2013: contributor

You might want to write "verified" vs "unverified", it's a bit closer to the truth.

gavinandresen commented at 9:15 PM on October 24, 2013: contributor

Yay! Thanks for tackling this!

Diapolo commented at 12:05 PM on October 25, 2013: none

@mikehearn Agreed, thanks.

Diapolo commented at 8:58 PM on October 27, 2013: none

Updated:

re-worked some UI tooltips to better understand (secure -> verified and insecure -> unverified)
changed code to use SendCoinsRecipient.message for storing the memo

in src/qt/paymentserver.cpp:None in fe168997ba outdated

 450 | @@ -454,9 +451,36 @@ bool PaymentServer::processPaymentRequest(PaymentRequestPlus& request, QList<Sen
 451 |      if (!optionsModel)
 452 |          return false;
 453 |  
 454 | -    QList<std::pair<CScript,qint64> > sendingTos = request.getPayTo();
 455 | -    qint64 totalAmount = 0;
 456 | +    recipients.append(SendCoinsRecipient());
 457 | +
 458 | +    recipients[0].paymentRequest = request;

laanwj commented at 12:14 PM on October 28, 2013:

Can you be sure that the new SendCoinsRecipient is always the first one? (index 0) Nit: It may be better to first build a SendCoinsRecipient and append it to recipients at the end of the function. This also avoids a lot of repeated recipients[0].

Diapolo commented at 12:24 PM on October 28, 2013:

I locally changed all QList<SendCoinsEntry> usages with processPaymentRequest() to a single SendCoinsEntry, as we don't have multiple SendCoinsEntries with payment requests (as that is one of the things which this pull is changing), so yes I can be sure (as long as there are no subtle bugs left then).

Your nit should be fixed by the next push to this branch :).

in src/qt/paymentserver.cpp:None in fef0b715ee outdated

 467 | +        CTxDestination dest;
 468 | +        if (ExtractDestination(sendingTo.first, dest)) {
 469 | +            // Append destination address (for payment requests .address is used ONLY for GUI display)
 470 | +            recipient.address.append(QString::fromStdString(CBitcoinAddress(dest).ToString()));
 471 | +            if (i < sendingTos.size() - 1) // prevent new-line for last entry
 472 | +                recipient.address.append("<br />");

laanwj commented at 12:59 PM on October 28, 2013:

If not generating a new-line for the last entry is the goal, comparing to sendingTos.size() - 1 is not enough, as the next entry may not generate a line of text at all even if it exists (but has a non-address target). I'd do this differently: Build a QStringList, then at the end concatenate them using join("<br />"). This also means that the counter i can go.

Diapolo commented at 1:47 PM on October 28, 2013:

I thought about changing address to a QStringList in general, but this would require an extensive rework, as SendCoinsRecipient is used quite often and makes only sense for payment requests anyway. This here is only used for displaying the addresses in the GUI anyway, no wallet code or such will ever touch this for payment requests currently.

The counter i is really just a quick and ugly hack ^^. And I'm going to change this to a QStringList, which is a much cleaner approach :).

laanwj commented at 3:08 PM on October 28, 2013: member

I've tested a bit, works great.

Next thing we need to deal with is how payment request-generated transactions are shown in the transactions list. We ought to show the name of the merchant and the memo instead of bare addresses/labels.

Diapolo commented at 4:38 PM on October 28, 2013: none

Thanks for testing, I'm with you, we should rework how pr are shown in the tx list. But that should be part of another pull request then.

My roadmap with this is to re-add the delete button for pr's and then re-check the issue for what is left to be done here, before this needs testing by some other devs.

laanwj commented at 4:43 PM on October 28, 2013: member

Eh yes, I certainly am not proposing that you add that to this pull req, it was just a general remark :) (BTW as unverified payments requests don't really have a recipient name/label, just the memo, this makes it challenging to show them in the transaction list in the same way as other transactions)

And indeed a delete button would be useful, currently the only way to get rid of payment requests in the send list is the Clear button.

Diapolo commented at 4:51 PM on October 28, 2013: none

@laanwj A further thing to consider is our baloon pop-up when a transaction is detected. I mainly use 3 addresses I sent coins to when testing a payment request. This is really ugly with the pop-up, as we only show 1 a time, but the 3 are practically created right in a row... any idea here?

laanwj commented at 4:56 PM on October 28, 2013: member

It should really show only one popup per transaction, not per output.

For the transaction list some kind of grouping makes sense too. Right now we always generate one record per output. Grouping on transaction level would be too coarse, but it should ideally show paid payment requests (which can involve multiple consecutive outputs) as one row and not multiple.

[Qt] Rework of payment request UI (mainly for insecure pr)

- this shows insecure (unsecured) payment requests in a new yellowish
  colored UI (based on the secure payment request UI) instead of our
  normal payment UI
- allows us to receive paymentACK messages for insecure payment requests
- allows us to handle expirations for insecure payment request
- changed walletmodel, so that all types of payment requests don't touch
  the addressbook

c6c97e0f4e

payment-request UI: use SendCoinsRecipient.message for memo 983cef4802

make processPaymentRequest() use a single SendCoinsRecipient

- as one this pulls main purpose is to change a payment request to
  be displayed as a single sendcoins entry

952d2cdb56

rework an ugly hack in processPaymentRequest()

- use a QStringList to store valid addresses and format them for GUI and
  debug.log usage via .join()

395d0d5af0

BitcoinPullTester commented at 5:19 PM on October 31, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/395d0d5af017bbf6d432471075608efaf4104a03 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

laanwj commented at 1:46 PM on November 4, 2013: member

@diapolo do you intend to make further changes here or is this ready for merging?

Diapolo commented at 2:10 PM on November 4, 2013: none

@laanwj I currently have no working local build environment (tried to upgrade to a newer MinGW version, which caused troubles ^^). I intended to add back the delete button for payment requests, but this can be done in another pull. If there are no show-stoppers or bugs in here this can be merged. @gavinandresen Can you perhaps check this out and give an ACK?

laanwj referenced this in commit 65d0fc4b73 on Nov 6, 2013

laanwj merged this on Nov 6, 2013

laanwj closed this on Nov 6, 2013

Diapolo deleted the branch on Nov 8, 2013

Bushstar referenced this in commit efd8d2c82b on Apr 8, 2020

DrahtBot locked this on Sep 8, 2021