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

Do not translate file extensions #220

Merged
merged 1 commit into from
Mar 16, 2021
Merged

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Feb 23, 2021

File extensions are untranslatable by their nature.

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

ACK 6af0a9a

Nice catch, this will help prevent any unintended translations

Copy link
Member

@jonatack jonatack 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 6af0a9a

Two questions: (a) does this change require translators to update every language? (b) Testing this patch, I don't see the file extensions in the dialogs. I then built master and I don't see the extensions there either 🤔 (checked in default and in French)

src/qt/locale/bitcoin_fr.ts:1574:        <source>Partially Signed Transaction (Binary) (*.psbt)</source>
src/qt/locale/bitcoin_fr.ts:1575:        <translation>Transaction signée partiellement (fichier binaire) (*.psbt)</translation>
src/qt/locale/bitcoin_fr.ts:2557:        <source>Partially Signed Transaction (Binary) (*.psbt)</source>
src/qt/locale/bitcoin_fr.ts:2558:        <translation>Transaction signée partiellement (fichier binaire) (*.psbt)</translation>
src/qt/locale/bitcoin_fr.ts:3496:        <source>Partially Signed Transaction (*.psbt)</source>
src/qt/locale/bitcoin_fr.ts:3497:        <translation>Transaction signée partiellement (*.psbt)</translation>

@hebasto
Copy link
Member Author

hebasto commented Feb 23, 2021

(a) does this change require translators to update every language?

Yes. I'd say that each translator updates her language :)

(b) Testing this patch, I don't see the file extensions in the dialogs. I then built master and I don't see the extensions there either thinking (checked in default and in French)

See GUIUtil::getSaveFileName implementation.

@hebasto
Copy link
Member Author

hebasto commented Feb 24, 2021

Updated 6af0a9a -> 5fe0ff3 (pr220.01 -> pr220.02, diff):

@hebasto
Copy link
Member Author

hebasto commented Feb 24, 2021 via email

@hebasto hebasto changed the title qt: Do not translate file extensions Do not translate file extensions Feb 25, 2021
@hebasto
Copy link
Member Author

hebasto commented Feb 25, 2021

Updated 5fe0ff3 -> d4ed848 (pr220.02 -> pr220.03, diff):

@jonatack
Copy link
Member

jonatack commented Feb 25, 2021

re-ACK d4ed848

(IDK if the "Wallet Data" ts string should be annotated as well or if it's self-evident.)

@Talkless
Copy link

Talkless commented Mar 4, 2021

Concept ACK. I agree to @luke-jr that one annotation is missing.

@hebasto
Copy link
Member Author

hebasto commented Mar 6, 2021

Updated d4ed848 -> 88df300 (pr220.03 -> pr220.04, diff).

Now each of changed translation has own disambiguation string.

Copy link

@Talkless Talkless left a comment

Choose a reason for hiding this comment

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

tACK 88df300, tested on Debian Sid with Qt 5.15.2. Tested all filters except for .psbt.

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

re-ACK 88df300

when to use and how to place disambiguation strings is another thing to add to the Qt dev notes

@hebasto
Copy link
Member Author

hebasto commented Mar 14, 2021

@luke-jr @laanwj

Some things appear unclear to me.

  1. Does the non-default disambiguation argument, which is passed to the tr function, is visible to translators on transifex?

  2. What are other options to make designated notes to translators on transifex?
    Do "Translator Comments" work in such a manner?


In the meantime, converting this pr to a draft.

@hebasto hebasto marked this pull request as draft March 14, 2021 20:03
@laanwj
Copy link
Member

laanwj commented Mar 16, 2021

Concept and code review ACK 88df300

Does the non-default disambiguation argument, which is passed to the tr function, is visible to translators on transifex?
What are other options to make designated notes to translators on transifex?
Do "Translator Comments" work in such a manner?

I think those are great questions. I do not know the answer though. Maybe @wtogami knows who can weigh in—from what i remember he knows some people at Transifex.

@hebasto
Copy link
Member Author

hebasto commented Mar 16, 2021

Even if disambiguation arguments won't actually help translators, they won't hurt.

As already many ACKs are collected, reverted back from "Draft" to "Ready for review".

@hebasto hebasto marked this pull request as ready for review March 16, 2021 11:31
@laanwj
Copy link
Member

laanwj commented Mar 16, 2021

At least the comment makes it into the xml, that's one part of it being picked up

     <message>
         <location line="+1"/>
-        <source>Comma separated file (*.csv)</source>
+        <source>Comma separated file</source>
+        <comment>Name of CSV file format</comment>
         <translation type="unfinished"></translation>
     </message>

@laanwj laanwj merged commit 7723479 into bitcoin-core:master Mar 16, 2021
@hebasto hebasto deleted the 210223-fileext branch March 16, 2021 13:22
hebasto added a commit that referenced this pull request May 27, 2021
8b77133 qt: Replace disambiguation strings with translator comments (Hennadii Stepanov)

Pull request description:

  Since bitcoin/bitcoin#21694 is merged, translator comments is the right way to pass context to translators.

  This PR fixes changes were made:
  - in #220 before bitcoin/bitcoin#21694
  - in bitcoin/bitcoin#21694 on testing purpose
  - in #125

  Closes #288.

ACKs for top commit:
  jarolrod:
    ACK 8b77133

Tree-SHA512: 466ade35f4969a41fbf3196780b1ae9fa810bab5d2f09077f8631604636cc63b24a901c719f6b5797366d2aa307993d0aa419ce35200c8d0a741a3d81cad3e6b
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
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.

6 participants