[Qt] Optimize SendToSelf rendering with a single non-change output #11471

jonasschnelli commented at 9:20 PM on October 9, 2017: contributor

Partially fixes #11464

This is a simple improvement to render singe non-change output self-to-self transactions with the corresponding output-address/label. Multi non-change output self-to-self transaction do keep the (n.a.) label (we could show all the addresses coma separated).

Screen: <img width="917" alt="bildschirmfoto 2017-10-09 um 14 18 13" src="https://user-images.githubusercontent.com/178464/31358984-c627acfa-acfc-11e7-990d-8c176c1cb201.png">

jonasschnelli added the label GUI on Oct 9, 2017

in src/qt/transactionrecord.cpp:104 in c6cb006587 outdated

  99 | +            {
 100 | +                if (wallet->IsChange(txout))
 101 | +                {
 102 | +                    continue;
 103 | +                }
 104 | +                if (wtx.tx->vout.size() == 2)

promag commented at 10:57 PM on October 9, 2017:

This if can be moved up to guard the for loop (avoid the loop if transaction hasn't 2 outputs).

in src/qt/transactionrecord.cpp:100 in c6cb006587 outdated

  93 | @@ -94,10 +94,25 @@ QList<TransactionRecord> TransactionRecord::decomposeTransaction(const CWallet *
  94 |  
  95 |          if (fAllFromMe && fAllToMe)
  96 |          {
  97 | +            std::string address = "";
  98 | +            for (const CTxOut& txout : wtx.tx->vout)
  99 | +            {
 100 | +                if (wallet->IsChange(txout))

promag commented at 10:59 PM on October 9, 2017:

One line? if (wallet->IsChange(txout)) continue;

in src/qt/transactionrecord.cpp:98 in c6cb006587 outdated

  93 | @@ -94,10 +94,25 @@ QList<TransactionRecord> TransactionRecord::decomposeTransaction(const CWallet *
  94 |  
  95 |          if (fAllFromMe && fAllToMe)
  96 |          {
  97 | +            std::string address = "";
  98 | +            for (const CTxOut& txout : wtx.tx->vout)

promag commented at 10:59 PM on October 9, 2017:

{ in this line.

jonasschnelli commented at 8:40 PM on October 10, 2017:

I always try to keep the surrounding code-folding style... and here, we have \n{ everywhere.

in src/qt/transactionrecord.cpp:105 in c6cb006587 outdated

 102 | +                    continue;
 103 | +                }
 104 | +                if (wtx.tx->vout.size() == 2)
 105 | +                {
 106 | +                    CTxDestination destination;
 107 | +                    if (ExtractDestination(txout.scriptPubKey, destination))

promag commented at 10:59 PM on October 9, 2017:

{ in this line.

in src/qt/transactiontablemodel.cpp:426 in c6cb006587 outdated

 422 | @@ -423,6 +423,7 @@ QString TransactionTableModel::formatTxToAddress(const TransactionRecord *wtx, b
 423 |      case TransactionRecord::SendToOther:
 424 |          return QString::fromStdString(wtx->address) + watchAddress;
 425 |      case TransactionRecord::SendToSelf:
 426 | +        if (wtx->address != "") { return lookupAddress(wtx->address, tooltip) + watchAddress; }

promag commented at 11:12 PM on October 9, 2017:

if (!wtx->address.empty()) return lookupAddress(wtx->address, tooltip) + watchAddress;

promag commented at 11:14 PM on October 9, 2017: member

Concept ACK.

we could show all the addresses coma separated

That sounds sensible.

laanwj commented at 7:43 AM on October 10, 2017: member

Optimizing rendering sounds good, do you have FPS numbers? :)

Sorry. Concept ACK.

jonasschnelli force-pushed on Oct 10, 2017

jonasschnelli commented at 8:58 PM on October 10, 2017: contributor

Fixed @promag points.

Optimizing rendering sounds good, do you have FPS numbers? :)

Heh. Yes. Rending is maybe not the best word for decomposing a transactions.

Sjors commented at 1:02 PM on November 9, 2017: member

Concept ACK.

I can't get it to work. Before & After: <img width="833" alt="schermafbeelding 2017-11-09 om 12 56 28" src="https://user-images.githubusercontent.com/10217/32604306-775a7332-c54d-11e7-8ba7-f1551907c5a1.png">

I made a transaction from one non-segwit wallet address to another non-segwit wallet address, without change.

Maybe I'm doing something wrong?

in src/qt/transactiontablemodel.cpp:427 in a0102314df outdated

 422 | @@ -423,6 +423,7 @@ QString TransactionTableModel::formatTxToAddress(const TransactionRecord *wtx, b
 423 |      case TransactionRecord::SendToOther:
 424 |          return QString::fromStdString(wtx->address) + watchAddress;
 425 |      case TransactionRecord::SendToSelf:
 426 | +        if (!wtx->address.empty()) { return lookupAddress(wtx->address, tooltip) + watchAddress; }

luke-jr commented at 2:50 PM on November 10, 2017:

Please document the intended fallthru.

luke-jr approved

luke-jr commented at 2:50 PM on November 10, 2017: member

I think we should remove "Payment to yourself" (and just show a send+receive pair), but this seems like a strict improvement.

luke-jr referenced this in commit 2e3d29ab29 on Nov 11, 2017

luke-jr referenced this in commit d0856b07f8 on Nov 11, 2017

laanwj commented at 10:05 AM on November 28, 2017: member

utACK, agree w/ luke-jr to please add /* Falls through. */. This will be useful when we enable -Wimplicit-fallthrough warnings, but also for general readability.

jb55 commented at 6:37 PM on December 29, 2017: member

Lightly Tested ACK a0102314df7ac9c025ef71c4ba0dee64285687d5

dec29-103140

luke-jr referenced this in commit 158abc1a1b on Mar 11, 2018

[Qt] Optimize SendToSelf rendering with a single non-change output c23bd2892b

jonasschnelli force-pushed on Mar 19, 2018

luke-jr referenced this in commit 8e6b803f85 on Mar 19, 2018

in src/qt/transactionrecord.cpp:102 in c23bd2892b

  93 | @@ -94,10 +94,23 @@ QList<TransactionRecord> TransactionRecord::decomposeTransaction(const CWallet *
  94 |  
  95 |          if (fAllFromMe && fAllToMe)
  96 |          {
  97 | +            std::string address;
  98 | +            if (wtx.tx->vout.size() == 2)
  99 | +            {
 100 | +                for (const CTxOut& txout : wtx.tx->vout)
 101 | +                {
 102 | +                    if (wallet->IsChange(txout)) continue;

jonasschnelli commented at 7:50 PM on April 10, 2018:

Not sure if its worth do add a call to IsChange for every transaction decomposing with the new interface structure...

MarcoFalke added the label Needs rebase on Jun 6, 2018

DrahtBot commented at 11:18 PM on November 8, 2018: member

There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened.

DrahtBot added the label Up for grabs on Nov 8, 2018

DrahtBot closed this on Nov 8, 2018

fanquake referenced this in commit a2ae766a61 on Oct 9, 2019

MarcoFalke removed the label Up for grabs on Oct 9, 2019

sidhujag referenced this in commit ceec215816 on Oct 9, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

xdustinface referenced this in commit 3ca59d8bbb on Nov 20, 2020

MarcoFalke locked this on Dec 16, 2021