Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply translator comments to reset options confirmation dialog #627

Merged
merged 1 commit into from
Jul 12, 2022

Conversation

jarolrod
Copy link
Member

This is a followup to #617. Because the strings were being concatenated, we can not apply translator comments to all of the revelant strings. This can be tested by applying the following diff to current master and running make translate; then check the resulting diff:

diff --git a/src/qt/optionsdialog.cpp b/src/qt/optionsdialog.cpp
index 462b923d6..3cf165004 100644
--- a/src/qt/optionsdialog.cpp
+++ b/src/qt/optionsdialog.cpp
@@ -286,9 +286,17 @@ void OptionsDialog::on_resetButton_clicked()
 {
     if (model) {
         // confirmation dialog
+        //: Window title text of pop-up window shown when the user has chosen to reset options.
         QMessageBox::StandardButton btnRetVal = QMessageBox::question(this, tr("Confirm options reset"),
+            /*: Text explaining that the settings the user changed will not come
+                into effect until the client is restarted. */
             tr("Client restart required to activate changes.") + "<br><br>" +
+            /*: Text explaining to the user that the client's current settings
+                will be backed up at a specific location. %1 is a stand-in
+                argument for the backup location's path. */
             tr("Current settings will be backed up at \"%1\".").arg(m_client_model->dataDir()) + "<br><br>" +
+            /*: Text asking the user to confirm if they would like to proceed
+                with a client shutdown. */
             tr("Client will be shut down. Do you want to proceed?"),
             QMessageBox::Yes | QMessageBox::Cancel, QMessageBox::Cancel);

To apply the above translator comments, what we want to do instead is have a variable in which the translatable strings are appended to using the QString append function.

When you run make translate with this PR, you will see the translator comments properly applied, as shown below:

diff --git a/src/qt/locale/bitcoin_en.ts b/src/qt/locale/bitcoin_en.ts
index 35d820187..9e5158b3e 100644
--- a/src/qt/locale/bitcoin_en.ts
+++ b/src/qt/locale/bitcoin_en.ts
@@ -1942,28 +1942,37 @@ Signing is only possible with addresses of the type &apos;legacy&apos;.</source>
         <translation>default</translation>
     </message>
     <message>
-        <location line="+81"/>
+        <location line="+86"/>
         <source>none</source>
         <translation type="unfinished"></translation>
     </message>
     <message>
-        <location line="+97"/>
+        <location line="+107"/>
         <source>Confirm options reset</source>
+        <extracomment>Window title text of pop-up window shown when the user has chosen to reset options.</extracomment>
         <translation>Confirm options reset</translation>
     </message>
     <message>
-        <location line="+1"/>
-        <location line="+70"/>
+        <location line="-9"/>
+        <location line="+79"/>
         <source>Client restart required to activate changes.</source>
+        <extracomment>Text explaining that the settings changed will not come into effect until the client is restarted.</extracomment>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <location line="-75"/>
+        <source>Current settings will be backed up at &quot;%1&quot;.</source>
+        <extracomment>Text explaining to the user that the client&apos;s current settings will be backed up at a specific location. %1 is a stand-in argument for the backup location&apos;s path.</extracomment>
         <translation type="unfinished"></translation>
     </message>
     <message>
-        <location line="-70"/>
+        <location line="+3"/>
         <source>Client will be shut down. Do you want to proceed?</source>
+        <extracomment>Text asking the user to confirm if they would like to proceed with a client shutdown.</extracomment>
         <translation type="unfinished"></translation>
     </message>
     <message>
-        <location line="+18"/>
+        <location line="+20"/>
         <source>Configuration options</source>
         <extracomment>Window title text of pop-up box that allows opening up of configuration file.</extracomment>
         <translation type="unfinished"></translation>

No difference in rendering between master and PR

master PR
Screen Shot 2022-06-29 at 11 39 17 PM Screen Shot 2022-06-29 at 11 39 51 PM

Follow-up to bitcoin-core#617. This applies translator strings to the
reset options confirmation dialog and also refactors the way we pass the
strings to the dialog in order to allow the comments to be applied.
Because the strings were being concatenated, we can not apply translator
comments to all of the relevant strings. What we want to do instead is
have a variable in which the translatable strings are appended to using
the QString append function. This satisfies the Qt translator engine and
the comments are then properly applied within the `extracomment` field
in the translation file.
@jarolrod jarolrod changed the title qt: apply translator comments to reset options confirmation dialog Apply translator comments to reset options confirmation dialog Jun 30, 2022
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK d5c141f

  • I was able to verify that applying the patch given in the description leads to failing of make translate.

Master_Patch_Translate

  • The make translate command ran successfully for this PR.
  • I was able to verify that the displayed message remains unchanged.

Screenshot:

Master PR
Master Screenshot from 2022-07-01 23-49-30

Notes:
Instructions on how to using make translate are given in this documentation

Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested ACK d5c141f, no functional changes.

Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK d5c141f

@hebasto hebasto merged commit 27a4dd0 into bitcoin-core:master Jul 12, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 12, 2022
…s confirmation dialog

d5c141f qt: apply translator comments to reset options confirmation dialog (Jarol Rodriguez)

Pull request description:

  This is a followup to bitcoin#617. Because the strings were being concatenated, we can not apply translator comments to all of the revelant strings. This can be tested by applying the following diff to current master and running `make translate`; then check the resulting diff:

  ```diff
  diff --git a/src/qt/optionsdialog.cpp b/src/qt/optionsdialog.cpp
  index 462b923d6..3cf165004 100644
  --- a/src/qt/optionsdialog.cpp
  +++ b/src/qt/optionsdialog.cpp
  @@ -286,9 +286,17 @@ void OptionsDialog::on_resetButton_clicked()
   {
       if (model) {
           // confirmation dialog
  +        //: Window title text of pop-up window shown when the user has chosen to reset options.
           QMessageBox::StandardButton btnRetVal = QMessageBox::question(this, tr("Confirm options reset"),
  +            /*: Text explaining that the settings the user changed will not come
  +                into effect until the client is restarted. */
               tr("Client restart required to activate changes.") + "<br><br>" +
  +            /*: Text explaining to the user that the client's current settings
  +                will be backed up at a specific location. %1 is a stand-in
  +                argument for the backup location's path. */
               tr("Current settings will be backed up at \"%1\".").arg(m_client_model->dataDir()) + "<br><br>" +
  +            /*: Text asking the user to confirm if they would like to proceed
  +                with a client shutdown. */
               tr("Client will be shut down. Do you want to proceed?"),
               QMessageBox::Yes | QMessageBox::Cancel, QMessageBox::Cancel);
  ```

  To apply the above translator comments, what we want to do instead is have a variable in which the translatable strings are appended to using the [QString append function](https://doc.qt.io/qt-5/qstring.html#append).

  When you run `make translate` with this PR, you will see the translator comments properly applied, as shown below:
  ``` diff
  diff --git a/src/qt/locale/bitcoin_en.ts b/src/qt/locale/bitcoin_en.ts
  index 35d820187..9e5158b3e 100644
  --- a/src/qt/locale/bitcoin_en.ts
  +++ b/src/qt/locale/bitcoin_en.ts
  @@ -1942,28 +1942,37 @@ Signing is only possible with addresses of the type &apos;legacy&apos;.</source>
           <translation>default</translation>
       </message>
       <message>
  -        <location line="+81"/>
  +        <location line="+86"/>
           <source>none</source>
           <translation type="unfinished"></translation>
       </message>
       <message>
  -        <location line="+97"/>
  +        <location line="+107"/>
           <source>Confirm options reset</source>
  +        <extracomment>Window title text of pop-up window shown when the user has chosen to reset options.</extracomment>
           <translation>Confirm options reset</translation>
       </message>
       <message>
  -        <location line="+1"/>
  -        <location line="+70"/>
  +        <location line="-9"/>
  +        <location line="+79"/>
           <source>Client restart required to activate changes.</source>
  +        <extracomment>Text explaining that the settings changed will not come into effect until the client is restarted.</extracomment>
  +        <translation type="unfinished"></translation>
  +    </message>
  +    <message>
  +        <location line="-75"/>
  +        <source>Current settings will be backed up at &quot;%1&quot;.</source>
  +        <extracomment>Text explaining to the user that the client&apos;s current settings will be backed up at a specific location. %1 is a stand-in argument for the backup location&apos;s path.</extracomment>
           <translation type="unfinished"></translation>
       </message>
       <message>
  -        <location line="-70"/>
  +        <location line="+3"/>
           <source>Client will be shut down. Do you want to proceed?</source>
  +        <extracomment>Text asking the user to confirm if they would like to proceed with a client shutdown.</extracomment>
           <translation type="unfinished"></translation>
       </message>
       <message>
  -        <location line="+18"/>
  +        <location line="+20"/>
           <source>Configuration options</source>
           <extracomment>Window title text of pop-up box that allows opening up of configuration file.</extracomment>
           <translation type="unfinished"></translation>
  ```

  No difference in rendering between master and PR

  | master | PR |
  | ------- | --- |
  <img width="532" alt="Screen Shot 2022-06-29 at 11 39 17 PM" src="https://user-images.githubusercontent.com/23396902/176588495-9d3761b6-9d96-489a-bbe5-a8907f7d5f99.png"> | <img width="532" alt="Screen Shot 2022-06-29 at 11 39 51 PM" src="https://user-images.githubusercontent.com/23396902/176588513-92e29564-b74a-46f5-a5dd-469c4ee953f7.png"> |

ACKs for top commit:
  shaavan:
    ACK d5c141f
  furszy:
    Tested ACK d5c141f, no functional changes.
  w0xlt:
    tACK bitcoin-core/gui@d5c141f

Tree-SHA512: 6175a096c6f99edb3041cc2429e1ea0670a10cd2cab0364f664a56c7dee1aa8631d52c0a36edb5d571f6ef934e947d45017e446cea7dddae044085c39c8835ef
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Jul 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants