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

Connection Type Translator Comments #345

Merged

Conversation

jarolrod
Copy link
Member

This PR introduces Qt translator comments for Connection Type strings in guiutil.cpp as well as rpcconsole.cpp.

This is an alternate implementation of the idea presented in the last three commits of #289. It is especially inspired by commit 842f4e8.

Per Qt Dev Notes, it is better to not break up strings when not necessary. This way we preserve the full context for translators.

@jarolrod jarolrod changed the title qt: connection type translator comments Connection Type Translator Comments May 27, 2021
@jarolrod jarolrod force-pushed the connection-translator-comments branch from 0819d50 to f5c2377 Compare May 27, 2021 00:58
@jarolrod
Copy link
Member Author

updated from 0819d50 -> f5c2377 (pr345.01 -> pr345.02, diff)

Changes:

  • Add translator comments for some missed connection type strings.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Approach ACK f5c2377.

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
@hebasto hebasto requested a review from jonatack May 29, 2021 11:49
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.

Thanks @jarolrod for reminding me to look at this.

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
src/qt/guiutil.cpp Outdated Show resolved Hide resolved
src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
src/qt/guiutil.cpp Outdated Show resolved Hide resolved
src/qt/guiutil.cpp Outdated Show resolved Hide resolved
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.

Concept ACK

I like the changes that are done here. They would very much help a translator to translate the strings with proper context correctly.
However, I want to make some suggestions here:

  • In the Guiutil.cpp file in lines 675 to 677: It would help a translator if the meaning of inbound and outbound was briefly explained there.
  • In the rpconsole.cpp file at line number 494: It should be mentioned that the Outbound peer connection is temporary, as mentioned in line 491

@jarolrod jarolrod force-pushed the connection-translator-comments branch from f5c2377 to 87b8129 Compare August 10, 2021 17:12
@jarolrod
Copy link
Member Author

Updated from f5c2377 -> 87b8129 (pr345.02 -> pr345.03)

Thanks @hebasto @jonatack @ShaMan239 for the reviews. I have implemented all of the review suggestions.

@jarolrod jarolrod force-pushed the connection-translator-comments branch from 87b8129 to 07adcbe Compare August 10, 2021 17:19
@jarolrod
Copy link
Member Author

updated from 87b8129 -> 07adcbe (pr345.03 -> pr345.04)

Changes: small cleanup with capitalized Inbound Connection

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.

Code review ACK, comments looks good.

@hebasto
Copy link
Member

hebasto commented Aug 11, 2021

@Talkless

Code review ACK, comments looks good.

Please insert a top commit hash :)

src/qt/guiutil.cpp Outdated Show resolved Hide resolved
src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
@jonatack
Copy link
Member

(Maybe update the translations in merged #317 per the writing style here, if relevant.)

@jarolrod jarolrod force-pushed the connection-translator-comments branch from 07adcbe to 8c13340 Compare September 5, 2021 20:55
@jarolrod
Copy link
Member Author

jarolrod commented Sep 5, 2021

updated from 07adcbe -> 8c13340 (pr345.04 - > pr345.05)

Changes: address @hebasto and @jonatack comments, thanks for the review!

@jonatack

(Maybe update the translations in merged #317 per the writing style here, if relevant.)

I will leave for follow-ups that improve translator comments throughout the GUI

src/qt/guiutil.cpp Outdated Show resolved Hide resolved
@jarolrod jarolrod force-pushed the connection-translator-comments branch from 8c13340 to 371e2b9 Compare September 21, 2021 05:31
@jarolrod
Copy link
Member Author

Updated from 8c13340 -> 371e2b9 (pr345.05 -> pr345.06, diff)

Addressed @shaavan comments and removed a redundant word; thanks for the review!

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 371e2b9

The translator's comments are looking great! Kudos to @jarolrod!

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.

ACK 371e2b9

modulo some comments below, thanks for improving this!

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
src/qt/guiutil.cpp Outdated Show resolved Hide resolved
src/qt/guiutil.cpp Outdated Show resolved Hide resolved
src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
Introduce Qt translator comments for connection types.
@jarolrod jarolrod force-pushed the connection-translator-comments branch from 371e2b9 to 4832737 Compare September 22, 2021 03:34
@jarolrod
Copy link
Member Author

jarolrod commented Sep 22, 2021

updated from 371e2b9 -> 4832737 (pr345.06 -> pr345.07, diff)

Changes: addressed all of @jonatack comments, thanks for the review!

@jonatack
Copy link
Member

Code review re-ACK 4832737 per git diff 371e2b9 4832737, changes are translator comment edits since my review yesterday (thank you for updating)

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 4832737

@hebasto hebasto merged commit ccc4b91 into bitcoin-core:master Sep 29, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 29, 2021
4832737 qt: connection type translator comments (Jarol Rodriguez)

Pull request description:

  This PR introduces Qt translator comments for `Connection Type` strings in `guiutil.cpp` as well as `rpcconsole.cpp`.

  This is an alternate implementation of the idea presented in the last three commits of #289. It is especially inspired by commit 842f4e834dfe5fd2786a5092f78ea28da1b36e4f.

  Per [Qt Dev Notes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Developer-Notes-for-Qt-Code), it is better to not break up strings when not necessary. This way we preserve the full context for translators.

ACKs for top commit:
  jonatack:
    Code review re-ACK 4832737 per `git diff 371e2b9 4832737`, changes are translator comment edits since my review yesterday (thank you for updating)
  hebasto:
    ACK 4832737

Tree-SHA512: 67e1741e10a2e30cde6d50d3293eec89f0b7641b34463865dc6909d2926cdcf33a7d8c1dc8055d2f85906ad2002cdaa594d37b184d16e2f06614b6c5ad00c982
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Sep 29, 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.

5 participants