[Qt][WIP] allow possibility to add a comment to a WalletTx #5905

jonasschnelli commented at 3:44 PM on March 15, 2015: contributor

Try to sync rpc/qt wallets implementations. Solves #2086.

bildschirmfoto 2015-03-15 um 16 40 49

bildschirmfoto 2015-03-15 um 16 40 59

    {
        "account" : "",
        "address" : "mm2STH7X3rbBVd73XaDrwdJHM69ukj31N8",
        "category" : "send",
        "amount" : -1.00000000,
        "vout" : 1,
        "fee" : -0.00000521,
        "confirmations" : 0,
        "txid" : "151e1e32090b765ebc30ebcc385bd79978081130b331972b94e142f3aeb85e10",
        "walletconflicts" : [
        ],
        "time" : 1426434053,
        "timereceived" : 1426434053,
        "comment" : "This is a test wtx comment."
    }

sipa added the label GUI on Mar 16, 2015

in src/qt/walletmodeltransaction.cpp:None in 7db577d8ad outdated

  64 | @@ -65,3 +65,8 @@ CReserveKey *WalletModelTransaction::getPossibleKeyChange()
  65 |  {
  66 |      return keyChange;
  67 |  }
  68 | +
  69 | +void WalletModelTransaction::setComment(QString comment)

Diapolo commented at 10:11 AM on March 16, 2015:

Can this be a const reference?

laanwj commented at 10:15 AM on March 16, 2015: member

Agree on the idea. Maybe we can make the comment editable too in the details view? As the information is purely local, there is no reason why it shouldn't be possible to add comments in retrospect.

jonasschnelli commented at 10:27 AM on March 16, 2015: contributor

@laanwj Yes. This is a good idea. Let me try to extend the editable comment to the transaction details window. What about adding a possibility to change the comment over RPC (without adding a new rpc call)?

luke-jr commented at 1:39 PM on March 16, 2015: member

How is this any different from the Label? Seems redundant.

jonasschnelli commented at 5:02 PM on March 16, 2015: contributor

@luke-jr comments already exists in RPC world and are per walletTx. Labels are per address as far as I know.

jonasschnelli commented at 1:08 PM on March 18, 2015: contributor

@laanwj making the local comment editable within the TransactionDetailDialog looks after a big diff. It needs changes here and there and may be hard to review. It would also require changes within CWallet. IMO wtx are currently meant to be immutable. I would like to keep the PR as it is and address this "extension" in another PR.

jonasschnelli force-pushed on Mar 18, 2015

luke-jr commented at 1:19 PM on March 18, 2015: member

@jonasschnelli Addresses are per walletTx generally as well (at least in proper use, which should be encouraged). RPC "comments" are basically just the deprecated accounting stuff which didn't have addresses associated.

jonasschnelli commented at 1:25 PM on March 18, 2015: contributor

@luke-jr "comments" are not directly related with "accounts". By now they where not deprecated. sendtoaddress uses them and i think having a possibility to give a transaction a comment makes sense.

luke-jr commented at 1:36 PM on March 18, 2015: member

Hm, interesting. Surprised that isn't using the label stuff. It would make sense to deprecate labels for comments, in that case, except that it's impossible to assign a comment to an incoming request right now. Labels work better in that regard. But having both is just confusing since they serve the same purpose...

laanwj commented at 11:12 AM on March 20, 2015: member

Looks like a lot of people are making the label-comment confusion, even in #2086.

Personally I'd say the distinction is the following:

label is a, usually short, name for the recipient or address
comment is arbitrary free text associated the the transaction. This can be anything from a description to e.g. current exchange rates for accounting/tax purposes

The annoying/user-surprising thing about these comments, though, is that they apply to a network-level transaction, so they would seem to be associated with multiple UI transactions if there are multiple outputs.

luke-jr commented at 2:46 PM on March 20, 2015: member

It also means you can't add comments to incoming transactions until after they're received (and confirm?). Maybe comment should be a text-area in the UI rather than a single line, and in the new wallet associated with an address so it has the same flexibility?

laanwj commented at 3:20 PM on March 20, 2015: member

Maybe comment should be a text-area in the UI rather than a single line

+1 I somehow assumed that'd already be the case.

jonasschnelli commented at 1:15 PM on March 26, 2015: contributor

Maybe comment should be a text-area in the UI rather than a single line

This PR will add a multiline text-area for adding comments to outgoing transactions.

[...] and in the new wallet associated with an address so it has the same flexibility?

I think adding comments to a wtx is something different to a address label. Lets say if i do ONE wtx to serval recipients it is nice then to give that ONE wtx a comment and not every receiving address.

But i'm also not completely sure what would be best for handling labels and comments.

in src/qt/walletmodeltransaction.cpp:None in 6b1360da68 outdated

  64 | @@ -65,3 +65,8 @@ CReserveKey *WalletModelTransaction::getPossibleKeyChange()
  65 |  {
  66 |      return keyChange;
  67 |  }
  68 | +
  69 | +void WalletModelTransaction::setComment(const QString comment)

Diapolo commented at 2:46 PM on March 26, 2015:

You could also use a pass-by-reference here ;)? const QString& comment

jonasschnelli force-pushed on Apr 2, 2015

jonasschnelli commented at 8:14 PM on April 2, 2015: contributor

rebased and fixed @Diapolo 's nit

jgarzik commented at 6:10 PM on September 15, 2015: contributor

ut ACK

[Qt] allow possibility to add a comment to a WalletTx 1dcdd428fd

jonasschnelli force-pushed on Sep 15, 2015

jonasschnelli commented at 8:31 PM on September 15, 2015: contributor

Rebased.

It will slightly reduce the space in the already very packed sendcoinsdialog. But i think it's useful. I'm not sure if – long term speaking – the Addressbook has a reason to exists. The word "Adressbook" itself is connected to address reuse. If people using the Addressbook to label and track payments, then we should slowly remove (or rename and rewrite) the Addressbook and switch over to wallet comments.

MarcoFalke commented at 10:23 AM on September 16, 2015: member

Looks good to merge.

Screenshot: screenshot from 2015-09-16 12-10-39

gmaxwell commented at 10:57 PM on November 22, 2015: contributor

utACK. But can we also add an RPC to set these?

jonasschnelli commented at 11:55 AM on November 24, 2015: contributor

@gmaxwell: Thanks for the review. Some test would be required, I agree. I'm also not fully happy with the UI look and feel. Consider the PR as WIP (changed title).

jonasschnelli renamed this:
~~[Qt] allow possibility to add a comment to a WalletTx~~
[Qt][WIP] allow possibility to add a comment to a WalletTx
on Nov 24, 2015

pstratem commented at 9:42 PM on December 8, 2015: contributor

lightly tested ACK 1dcdd428fdee62c1f7fb2e2eff0dc2d63eb0b7f3

sipa commented at 4:19 PM on May 20, 2016: member

Concept ACK

paveljanik commented at 4:46 PM on October 8, 2016: contributor

Concept ACK

RPC first please.

jonasschnelli commented at 7:34 PM on October 18, 2016: contributor

@paveljanik: It's already possible in RPC sendtoaddress \"bitcoinaddress\" amount ( \"comment\" \"comment-to\" subtractfeefromamount )

I don't think the current GUI UX makes sense... Closing for now and maybe reopening after #7729

jonasschnelli closed this on Oct 18, 2016

paveljanik commented at 7:45 PM on October 18, 2016: contributor

@jonasschnelli Hmm, it is not possible to use sendtoaddress RPC command to add comment to already received transaction from someone else.

jonasschnelli commented at 7:48 PM on October 18, 2016: contributor

@paveljanik: this PR only allows to set the comment during GUI tx creation (which is also a point that makes this PR currently not ready).

paveljanik commented at 7:59 PM on October 18, 2016: contributor

Ah, then yes :-)

DrahtBot locked this on Aug 16, 2022