Add translator comments to `TransactionDesc::FormatTxStatus` #583

w0xlt commented at 3:32 PM on April 13, 2022: contributor

This PR adds translator comments to TransactionDesc::FormatTxStatus as suggested in #552 (review) and #552 (comment).

laanwj commented at 6:24 PM on April 13, 2022: member

Concept ACK, but I think this belongs in the GUI repo. It also does more than add translator comments, which should be in the PR description.

hebasto commented at 6:27 PM on April 13, 2022: member

Concept ACK, but I think this belongs in the GUI repo.

It does :)

It also does more than add translator comments, which should be in the PR description.

This PR is based on top of the #552. Should be rebased after merging the latter. Only the last commit belongs to this PR.

hebasto added the label Translations on Apr 13, 2022

laanwj commented at 9:03 PM on April 13, 2022: member

It does :)

Huh. Yes, it is. Sorry.

hebasto commented at 9:39 AM on April 15, 2022: member

@w0xlt

#552 has just been merged. Want to rebase this one?

And please finish every translator comment with a full stop character .

w0xlt force-pushed on Apr 18, 2022

w0xlt commented at 4:54 PM on April 18, 2022: contributor

Rebased. Thanks.

shaavan approved

shaavan commented at 12:57 PM on April 19, 2022: contributor

ACK 767304ee352e1c02bf9b5219555391b07473a5ff

The added translator's comments are clean and concise and clearly represent the meaning of the sentences they intend to explain.

hebasto commented at 6:48 PM on April 21, 2022: member

To avoid unnecessary string split, which makes translation much harder, I'd suggest before applying translation comments to refactor a bit:

--- a/src/qt/transactiondesc.cpp
+++ b/src/qt/transactiondesc.cpp
@@ -38,8 +38,16 @@ QString TransactionDesc::FormatTxStatus(const interfaces::WalletTxStatus& status
     if (depth < 0) {
         return tr("conflicted with a transaction with %1 confirmations").arg(-depth);
     } else if (depth == 0) {
-        const QString abandoned{status.is_abandoned ? QLatin1String(", ") + tr("abandoned") : QString()};
-        return tr("0/unconfirmed, %1").arg(inMempool ? tr("in memory pool") : tr("not in memory pool")) + abandoned;
+        QString s;
+        if (inMempool) {
+            s = tr("0/unconfirmed, in memory pool");
+        } else {
+            s = tr("0/unconfirmed, not in memory pool");
+        }
+        if (status.is_abandoned) {
+            s += QLatin1String(", ") + tr("abandoned");
+        }
+        return s;
     } else if (depth < 6) {
         return tr("%1/unconfirmed").arg(depth);
     } else {

w0xlt force-pushed on Apr 26, 2022

w0xlt commented at 2:23 AM on April 26, 2022: contributor

@hebasto thanks for suggestion. Done.

in src/qt/transactiondesc.cpp:59 in b0ecfd9c52 outdated

  56 | +        return s;
  57 |      } else if (depth < 6) {
  58 | +        //: This status represents a transaction confirmed in at least one block, but less than 6 blocks.
  59 |          return tr("%1/unconfirmed").arg(depth);
  60 |      } else {
  61 | +        //: This status represents a transaction confirmed in 6 or more blocks.

katesalazar commented at 6:41 PM on April 26, 2022:

6 is a magic number, but you are not to blame.

katesalazar commented at 6:42 PM on April 26, 2022: contributor

Concept ACK

in src/qt/transactiondesc.cpp:39 in b0ecfd9c52 outdated

  35 | @@ -36,13 +36,27 @@ 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.

jarolrod commented at 6:30 PM on May 30, 2022:

Suggesting to add more context on the location of this string and what it represents:

diff --git a/src/qt/transactiondesc.cpp b/src/qt/transactiondesc.cpp
index c921b5fb1..0a52d2e3e 100644
--- a/src/qt/transactiondesc.cpp
+++ b/src/qt/transactiondesc.cpp
@@ -36,27 +36,41 @@ QString TransactionDesc::FormatTxStatus(const interfaces::WalletTxStatus& status
 {
     int depth = status.depth_in_main_chain;
     if (depth < 0) {
-        //: This status represents an unconfirmed transaction that conflicts with another.
+        /*: Text explaining the current status of a transaction, shown in the
+            status field of the details window for this transaction. This status
+            represents an unconfirmed transaction that conflicts with another
+            confirmed transaction. */
         return tr("conflicted with a transaction with %1 confirmations").arg(-depth);
     } else if (depth == 0) {
         QString s;
         if (inMempool) {
-            //: This status represents an unconfirmed transaction that is in the memory pool.
+            /*: Text explaining the current status of a transaction, shown in the
+                status field of the details window for this transaction. This status
+                represents an unconfirmed transaction that is in the memory pool. */
             s = tr("0/unconfirmed, in memory pool");
         } else {
-            //: This status represents an unconfirmed transaction that is not in the memory pool.
+            /*: Text explaining the current status of a transaction, shown in the
+                status field of the details window for this transaction. This status
+                represents an unconfirmed transaction that is not in the memory pool. */
             s = tr("0/unconfirmed, not in memory pool");
         }
         if (status.is_abandoned) {
-            //: This status represents an abandoned transaction.
+            /*: Text explaining the current status of a transaction, shown in the
+                status field of the details window for this transaction. This
+                status represents an abandoned transaction. */
             s += QLatin1String(", ") + tr("abandoned");
         }
         return s;
     } else if (depth < 6) {
-        //: This status represents a transaction confirmed in at least one block, but less than 6 blocks.
+        /*: Text explaining the current status of a transaction, shown in the
+            status field of the details window for this transaction. This
+            status represents a transaction confirmed in at least one block,
+            but less than 6 blocks. */
         return tr("%1/unconfirmed").arg(depth);
     } else {
-        //: This status represents a transaction confirmed in 6 or more blocks.
+        /*: Text explaining the current status of a transaction, shown in the
+            status field of the details window for this transaction. This status
+            represents a transaction confirmed in 6 or more blocks. */
         return tr("%1 confirmations").arg(depth);
     }
 }

jarolrod commented at 6:31 PM on May 30, 2022:

I have written about how translator comments should be written here: translator-comments.md

Extra information on what it represents to the user and where it is helps the translator think of a correct translation but also allows them to locate the string themselves in the GUI, which would also aid with translations.

I also have a branch with these suggested changes here: https://github.com/jarolrod/gui/tree/tc-fixup

w0xlt commented at 6:01 PM on June 1, 2022:

Thanks. Great article. I updated the code with your suggestions.

jarolrod commented at 6:32 PM on May 30, 2022: member

Concept ACK

w0xlt force-pushed on Jun 1, 2022

in src/qt/transactiondesc.cpp:41 in dde7e9ae23 outdated

  35 | @@ -36,13 +36,41 @@ QString TransactionDesc::FormatTxStatus(const interfaces::WalletTxStatus& status
  36 |  {
  37 |      int depth = status.depth_in_main_chain;
  38 |      if (depth < 0) {
  39 | +        /*: Text explaining the current status of a transaction, shown in the
  40 | +            status field of the details window for this transaction. This status
  41 | +            represents an unconfirmed transaction that conflicts with another

jonatack commented at 9:31 PM on June 1, 2022:

"another" here is a little bit contradictory as it suggests that the other transaction is also unconfirmed; propose s/another/a/

w0xlt commented at 4:57 PM on June 2, 2022:

Done. Thanks.

in src/qt/transactiondesc.cpp:45 in dde7e9ae23 outdated

  42 | +            confirmed transaction. */
  43 |          return tr("conflicted with a transaction with %1 confirmations").arg(-depth);
  44 |      } else if (depth == 0) {
  45 | -        const QString abandoned{status.is_abandoned ? QLatin1String(", ") + tr("abandoned") : QString()};
  46 | -        return tr("0/unconfirmed, %1").arg(inMempool ? tr("in memory pool") : tr("not in memory pool")) + abandoned;
  47 | +        QString s;

jonatack commented at 9:41 PM on June 1, 2022:

<details><summary><strike>Maybe simplify/avoid the temporary.</strike> Edit: never mind.</summary><p>

@@ -42,25 +42,23 @@ QString TransactionDesc::FormatTxStatus(const interfaces::WalletTxStatus& status
             confirmed transaction. */
         return tr("conflicted with a transaction with %1 confirmations").arg(-depth);
     } else if (depth == 0) {
-        QString s;
         if (inMempool) {
             /*: Text explaining the current status of a transaction, shown in the
                 status field of the details window for this transaction. This status
                 represents an unconfirmed transaction that is in the memory pool. */
-            s = tr("0/unconfirmed, in memory pool");
+            return tr("0/unconfirmed, in memory pool");
         } else {
             /*: Text explaining the current status of a transaction, shown in the
                 status field of the details window for this transaction. This status
                 represents an unconfirmed transaction that is not in the memory pool. */
-            s = tr("0/unconfirmed, not in memory pool");
+            return tr("0/unconfirmed, not in memory pool");
         }
         if (status.is_abandoned) {
             /*: Text explaining the current status of a transaction, shown in the
                 status field of the details window for this transaction. This
                 status represents an abandoned transaction. */
-            s += QLatin1String(", ") + tr("abandoned");
+            return QLatin1String(", ") + tr("abandoned");
         }
-        return s;
     } else if (depth < 6) {

</p></details>

hebasto commented at 9:53 PM on June 1, 2022:

What about s += QLatin1String(", ") + tr("abandoned"); ?

hebasto commented at 9:30 AM on June 2, 2022: member

@w0xlt This looks ready to be merged with or without addressing #583 (review)

What is your opinion?

qt, refactor: add translator comments in `TransactionDesc::FormatTxStatus()` 8cfb5627d5

w0xlt force-pushed on Jun 2, 2022

w0xlt commented at 4:57 PM on June 2, 2022: contributor

@hebasto I addressed #583 (review) . Thanks.

hebasto approved

hebasto commented at 5:30 PM on June 2, 2022: member

ACK 8cfb5627d51ecaa1d1e92ec21e2ac56a380c77e6

hebasto merged this on Jun 2, 2022

hebasto closed this on Jun 2, 2022

jonatack commented at 5:39 PM on June 2, 2022: contributor

ACK, thanks for updating.

w0xlt deleted the branch on Jun 3, 2022

sidhujag referenced this in commit 243c842ae9 on Jun 3, 2022

bitcoin-core locked this on Jun 3, 2023