Refactor `TransactionDesc::FormatTxStatus` and ` TransactionStatus` #552

pull w0xlt wants to merge 5 commits into bitcoin-core:master from w0xlt:pr24067-follow-ups changing 3 files +19 −30
  1. w0xlt commented at 7:08 PM on February 18, 2022: contributor

    This PR implements the changes suggested in #538 (comment) .

    . remove redundant scope, rename nDepth -> depth, remove unused parameters and add translator comments in TransactionDesc::FormatTxStatus() . Use member initializers and remove unused field qint64 TransactionStatus::open_for in TransactionStatus.

    Closes https://github.com/bitcoin-core/gui/issues/538

  2. in src/qt/transactiondesc.cpp:1 in cbf23e493c outdated 
       0 | @@ -1,4 +1,4 @@
   1 | -// Copyright (c) 2011-2021 The Bitcoin Core developers
   2 | +// Copyright (c) 2011-2022 The Bitcoin Core developers
    jonatack commented at 12:01 AM on February 19, 2022:

    38316d4 Generally there's no need to touch the copyright headers; they are updated annually with a script.

    w0xlt commented at 2:10 AM on February 19, 2022:

    Thanks. Done.

  3. in src/qt/transactionrecord.h:40 in cbf23e493c outdated 
      34 | @@ -40,22 +35,19 @@ class TransactionStatus
  35 |      };
  36 |  
  37 |      /// Transaction counts towards available balance
  38 | -    bool countsForBalance;
  39 | +    bool countsForBalance{false};
  40 |      /// Sorting key based on status
  41 | -    std::string sortKey;
  42 | +    std::string sortKey{};
    jonatack commented at 12:03 AM on February 19, 2022:

    cbf23e49 This change should be unneeded; std::string is default-initialized to an empty string.

    w0xlt commented at 2:10 AM on February 19, 2022:

    Thanks. Done.

  4. jonatack commented at 12:10 AM on February 19, 2022: contributor

    Code review ACK cbf23e493c7b448734119818217cc41865524278 modulo a couple comments

  5. w0xlt force-pushed on Feb 19, 2022
  6. in src/qt/transactionrecord.h:24 in b0ffe4e47c outdated 
      25 | -public:
  26 | -    TransactionStatus() : countsForBalance(false), sortKey(""),
  27 | -                          matures_in(0), status(Unconfirmed), depth(0), open_for(0)
  28 | -    { }
  29 | +struct TransactionStatus {
  30 |
    jonatack commented at 4:49 PM on February 20, 2022:

    b0ffe4e47c0500bcd54f82e6b88a0941ca nit if you retouch, can drop the extra newline

    hebasto commented at 8:19 PM on February 20, 2022:

    Applying clang-format-diff.py after each commit is a good habit :)

    w0xlt commented at 7:38 PM on February 21, 2022:

    Done. Thanks.

  7. jonatack commented at 4:51 PM on February 20, 2022: contributor

    Code review ACK b0ffe4e47c0500bcd54f82e6b88a0941cae9485e

  8. hebasto commented at 8:17 PM on February 20, 2022: member

    Approach ACK 0b234f96eaf85bcbe106e12750eefc57e009170d.

    In 0b234f96eaf85bcbe106e12750eefc57e009170d "qt: remove redundant scope":

    • it would be nice to rename s/nDepth/depth/
    • parameters const interfaces::WalletTx& wtx and int numBlocks are not used, and could be dropped (maybe in a separated commit)
    • if you feel comfortable doing that, translator comments are always welcome :)

    In b0ffe4e47c0500bcd54f82e6b88a0941cae9485e "qt: refactor TransactionStatus":

    • removing of open_for member could be split into a separated commit
    • bool needsUpdate is still uninitialized
    • maybe put into commit message something like this " qt, refactor: Use member initializers in TransactionStatus"?

    Also could you please, in addition to the link to #538, add actual descriptions of this PR changes to OP, because it goes into a merge commit message.

  9. w0xlt force-pushed on Feb 21, 2022
  10. w0xlt force-pushed on Feb 21, 2022
  11. w0xlt commented at 7:40 PM on February 21, 2022: contributor

    @hebasto I have addressed all of your suggestions. Thanks.

  12. in src/qt/transactionrecord.h:50 in b279331569 outdated 
      49 |         @{*/
  50 | -    Status status;
  51 | -    qint64 depth;
  52 | +    Status status{Unconfirmed};
  53 | +    qint64 depth{0};
  54 |      qint64 open_for; /**< Timestamp if status==OpenUntilDate, otherwise number
    promag commented at 10:08 PM on February 22, 2022:

    b279331569e42cab721949d6669a452d6ba1558a

    nit, initialize open_for or move this commit after 064bbdf243?

    w0xlt commented at 4:59 PM on February 24, 2022:

    Done. Thanks.

  13. promag approved
  14. promag commented at 10:11 PM on February 22, 2022: contributor

    Code review ACK b279331569e42cab721949d6669a452d6ba1558a

  15. in src/qt/transactiondesc.cpp:42 in cf9aa72e5a outdated 
      35 | @@ -36,13 +36,17 @@ QString TransactionDesc::FormatTxStatus(const interfaces::WalletTxStatus& status
  36 |  {
  37 |      int depth = status.depth_in_main_chain;
  38 |      if (depth < 0) {
  39 | +        //: This status represents an unconfirmed transaction that conflicts with another
  40 |          return tr("conflicted with a transaction with %1 confirmations").arg(-depth);
  41 |      } else if (depth == 0) {
  42 |          const QString abandoned{status.is_abandoned ? QLatin1String(", ") + tr("abandoned") : QString()};
  43 | +        //: This status represents an unconfirmed transaction that is in the memory pool or possibly abandoned
  44 |          return tr("0/unconfirmed, %1").arg(inMempool ? tr("in memory pool") : tr("not in memory pool")) + abandoned;
    jarolrod commented at 6:59 AM on February 23, 2022:

    A translator comments applied to a line with multiple independent translatable strings will only apply to the first one. You can check what translator comment is applying to what string by making the translations, see more here: translation_process.md

    You could break this into multiple lines in order to give translator comments to the strings in memory pool and not in memory pool. Seems useful to provide context for this strings.

    w0xlt commented at 5:34 PM on February 24, 2022:

    I think I addressed your suggestion in 62ab92b.

    w0xlt commented at 3:35 PM on April 13, 2022:

    Moved this commit to PR #583.

  16. jarolrod commented at 6:59 AM on February 23, 2022: member

    concept ACK

  17. qt, refactor: remove redundant scope in `TransactionDesc::FormatTxStatus()` b1bc1431db
  18. scripted-diff: rename nDepth -> depth
    -BEGIN VERIFY SCRIPT-
s() { sed -i -e 's/nDepth/depth/g' $(git grep -l 'nDepth' $1); }
s src/qt/transactiondesc.cpp
-END VERIFY SCRIPT-
    045f8d0310
  19. qt, refactor: remove unused parameters in `TransactionDesc::FormatTxStatus()` ad6adedb46
  20. w0xlt force-pushed on Feb 24, 2022
  21. w0xlt force-pushed on Feb 24, 2022
  22. Gemk83 approved
  23. Gemk83 approved
  24. hebasto commented at 10:31 PM on April 12, 2022: member

    Almost ACK 2c5dbf2fc4556ff83009eca2178f860efcb73f8e except for 62ab92b5e992d9c5bbe251632acbc7a5df517510 "qt, refactor: add translator comments in TransactionDesc::FormatTxStatus()".

    If we are adding translator comments, we should also strive to prevent splitting of strings into less meaningful parts.

    Suggesting to separate that commit into a dedicated PR, as all of other commits are good to be merged.

    @w0xlt Please name this PR in a bit meaningful way because the PR title goes into the project commit history.

  25. hebasto added the label Refactoring on Apr 13, 2022
  26. w0xlt renamed this:
    PR24067 follow-ups
    Format `TransactionDesc::FormatTxStatus` and ` TransactionStatus`
    on Apr 13, 2022
  27. qt, refactor: remove unused field `qint64 TransactionStatus::open_for` 66d58ad7a9
  28. qt, refactor: Use member initializers in TransactionStatus 343f83d088
  29. w0xlt force-pushed on Apr 13, 2022
  30. w0xlt commented at 3:34 PM on April 13, 2022: contributor

    @hebasto the mentioned commit was moved to PR #583.

  31. hebasto renamed this:
    Format `TransactionDesc::FormatTxStatus` and ` TransactionStatus`
    Refactor `TransactionDesc::FormatTxStatus` and ` TransactionStatus`
    on Apr 13, 2022
  32. hebasto approved
  33. hebasto commented at 3:40 PM on April 13, 2022: member

    ACK 343f83d0886ae39c9dacb29762ce712711b2bad2, I have reviewed the code and it looks OK, I agree it can be merged.

  34. hebasto commented at 4:44 PM on April 14, 2022: member

    @jonatack @jarolrod @promag

    Mind having another (final?) look? @MarcoFalke This PR addresses your #538 (comment). Does it look ok?

  35. jarolrod approved
  36. jarolrod commented at 5:52 PM on April 14, 2022: member

    Code Review ACK https://github.com/bitcoin-core/gui/commit/343f83d0886ae39c9dacb29762ce712711b2bad2

    Thanks for working on this refactoring. I have reviewed the code and agree that it can be merged.

  37. hebasto merged this on Apr 15, 2022
  38. hebasto closed this on Apr 15, 2022
  39. w0xlt deleted the branch on Apr 16, 2022
  40. sidhujag referenced this in commit 5c6c60dd11 on Apr 18, 2022
  41. hebasto referenced this in commit b11ab25afb on Jun 2, 2022
  42. sidhujag referenced this in commit 243c842ae9 on Jun 3, 2022
  43. bitcoin-core locked this on Apr 16, 2023
Contributors
w0xltjonatackhebastopromagjarolrodGemk83
Labels
Refactoring
Linked (view graph)
#538 PR24067 follow-ups#583 Add translator comments to `TransactionDesc::FormatTxStatus`
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-20 11:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me