build: Include qt sources for parsing with extract_strings.py #22764
Conversation
|
GUIX hashes |
|
So this adds missing strings like |
|
Wait, the idea is that inside the Qt sources, |
|
These shouldn't be here at all! |
|
Closing in favor of bitcoin-core/gui#454. |
58765a4 qt: Use only Qt translation primitives in GUI code (W. J. van der Laan) Pull request description: Use `Object::tr`, `QT_TRANSLATE_NOOP`, and `QCoreApplication::translate` as appropriate instead of using `_()` which doesn't get picked up. Replaces bitcoin/bitcoin#22764 Edit: I checked that the strings end up in the appropriate context in `bitcoin_en.ts` after `make translate`: - "Settings file could not be read" "Settings file could not be written" end up in `bitcoin-core` - "(press q to shutdown and continue later)" and "press q to shutdown" end up in `SplashScreen` ACKs for top commit: hebasto: ACK 58765a4 Tree-SHA512: d0cc5901426c47d411d0fe75aac195b9684b51385f74c161da772dbf9f0977f7ad7a0976e17366f49b40718e9b6eb87c3e93306dc845f97adddbc47d00731742
… code 58765a4 qt: Use only Qt translation primitives in GUI code (W. J. van der Laan) Pull request description: Use `Object::tr`, `QT_TRANSLATE_NOOP`, and `QCoreApplication::translate` as appropriate instead of using `_()` which doesn't get picked up. Replaces bitcoin#22764 Edit: I checked that the strings end up in the appropriate context in `bitcoin_en.ts` after `make translate`: - "Settings file could not be read" "Settings file could not be written" end up in `bitcoin-core` - "(press q to shutdown and continue later)" and "press q to shutdown" end up in `SplashScreen` ACKs for top commit: hebasto: ACK 58765a4 Tree-SHA512: d0cc5901426c47d411d0fe75aac195b9684b51385f74c161da772dbf9f0977f7ad7a0976e17366f49b40718e9b6eb87c3e93306dc845f97adddbc47d00731742
|
Reopening this to find out if it makes sense to take it up again.
@laanwj I agree it is good to use We could introduce a new mechanism that wraps |
This file was included in bitcoin#9457, but now it is a part of the BITCOIN_QT_BASE_CPP.
hebasto
force-pushed
the
210821-translation
branch
from
aff62da to
b59b31a
Compare
|
Rebased. |
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK b59b31a. Being able to use _() macro in qt would allow simplifying some code, for example replacing repetitive:
std::string quoted_path = strprintf("%s", fs::quoted(fs::PathToString(settings_path)));
error.original += strprintf("Settings file %s might be corrupt or invalid.", quoted_path);
error.translated += tr("Settings file %1 might be corrupt or invalid.").arg(QString::fromStdString(quoted_path)).toStdString();from bitcoin-core/gui#602 (review) with simpler:
error += strprintf(_("Settings file %s might be corrupt or invalid."), fs::quoted(fs::PathToString(settings_path)));Most strings shown in the GUI should be translated-only, not bilingual, so QObject::tr() is a better choice than _() to provide the translation. But _() is useful for special cases like critical errors where bilingual_str provides a translated string for display and an untranslated string for logs and details.
Conceptually I agree. There are some exceptions where it can be useful. My aversion to this is mostly on a code-hygienic level. |
If it helps, it looks like this is not using bitcoin/src/util/translation.h Lines 63 to 66 in 01e9e2d which is calling Lines 17 to 19 in 01e9e2d So it is a little less efficient than calling Qt directly, but it's not pulling in many more dependencies besides the bilingual_str functions in |
|
@jarolrod you might want to review this? |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
concept ack |
|
Concept ACK |
|
@jarolrod want to followup here? |
|
There hasn't been much activity lately. What is the status here? Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future. |
… code 58765a4 qt: Use only Qt translation primitives in GUI code (W. J. van der Laan) Pull request description: Use `Object::tr`, `QT_TRANSLATE_NOOP`, and `QCoreApplication::translate` as appropriate instead of using `_()` which doesn't get picked up. Replaces bitcoin#22764 Edit: I checked that the strings end up in the appropriate context in `bitcoin_en.ts` after `make translate`: - "Settings file could not be read" "Settings file could not be written" end up in `bitcoin-core` - "(press q to shutdown and continue later)" and "press q to shutdown" end up in `SplashScreen` ACKs for top commit: hebasto: ACK 58765a4 Tree-SHA512: d0cc5901426c47d411d0fe75aac195b9684b51385f74c161da772dbf9f0977f7ad7a0976e17366f49b40718e9b6eb87c3e93306dc845f97adddbc47d00731742
7 participants
On master (4fc15d1) some strings are still untranslated.
This PR fixes this issue.
To verify:
./autogen.sh && ./configure && make -C src translatebefore applying this change./autogen.sh && ./configure && make -C src translateafter applying this changeThe result of
git diff src/qt/bitcoinstrings.cpp: