#118 tdeio: minor improvement of translatability of a message

Merged
SlavekB merged 1 commits from other/tdeio-message-fixup into master 1 month ago
Fat-Zer commented 1 month ago
Collaborator
  • added plural forms

Signed-off-by: Alexander Golubev fatzer2@gmail.com

* added plural forms Signed-off-by: Alexander Golubev <fatzer2@gmail.com>
Fat-Zer added 1 commit 1 month ago
3971e3a493 tdeio: minor improvement of translatability of a message
MicheleC reviewed 1 month ago
tdeio/tdeio/global.cpp
.arg( KProtocolManager::connectTimeout() )
.arg( KProtocolManager::responseTimeout() )
.arg( KProtocolManager::proxyConnectTimeout() );
.arg(i18n( "<li>Timeout for establishing a connection: %1 second</li>",
MicheleC commented 1 month ago

It would be better to keep <li> and </li> out of the strings that require translation.

It would be better to keep ```<li>``` and ```</li>``` out of the strings that require translation.
Fat-Zer commented 1 month ago

done

done
Fat-Zer marked this conversation as resolved
MicheleC reviewed 1 month ago
tdeio/tdeio/global.cpp
solutions << sTryagain << sServeradmin;
break;
}
break;
MicheleC commented 1 month ago

This break should be before the brace, so we abide to the format style that we want to introduce at some point.

This ```break``` should be before the brace, so we abide to the format style that we want to introduce at some point.
Fat-Zer commented 1 month ago

done as well

done as well
Fat-Zer marked this conversation as resolved
Fat-Zer force-pushed other/tdeio-message-fixup from 3971e3a493 to 40fb307819 1 month ago
Fat-Zer force-pushed other/tdeio-message-fixup from 40fb307819 to 565cc9b030 1 month ago
SlavekB reviewed 1 month ago
SlavekB left a comment

Great, now it looks better than the previous variant using .append.
Just a supplementary question below.

tdeio/tdeio/global.cpp
.arg( KProtocolManager::connectTimeout() )
.arg( KProtocolManager::responseTimeout() )
.arg( KProtocolManager::proxyConnectTimeout() );
.arg(TQString::fromLatin1("<ul><li>%1</li><li>%2</li><li>%3</li></ul>")
SlavekB commented 1 month ago

Are you sure you want to use fromLatin1? Is this applied only to the format string before %1, %2, and %3 are populated? Because otherwise I would be worried, fromLatin1 will successfully kill any utf8 characters that do not fit in Latin1.

Are you sure you want to use `fromLatin1`? Is this applied only to the format string before `%1`, `%2`, and `%3` are populated? Because otherwise I would be worried, `fromLatin1` will successfully kill any utf8 characters that do not fit in Latin1.
MicheleC commented 1 month ago

Yeah, same doubt here.... perhaps using fromLocal8Bit() would be more appropriate.

Yeah, same doubt here.... perhaps using ```fromLocal8Bit()``` would be more appropriate.
SlavekB commented 1 month ago

Here are the strings from the translations, which are utf8, not the local 8bit. So there I would expect fromUtf8().

Here are the strings from the translations, which are utf8, not the local 8bit. So there I would expect `fromUtf8()`.
Fat-Zer commented 1 month ago

Yeap, I'm sure =) and of course it's applied before: first a TQString (with internal unicode representation) is created from a C-string and only then arg() is applied... it can't be vice versa...

And yes, I know that Qt3 strings were wierd and they kept an implicit 8-bit version alongside with unicode one... But all high-level calls like arg() work only to the unicode. TQString::fromLatin1() is unsafe only if its argument itself may contain a non-latin1 chars... after that it's an ordinary TQString...

Yeap, I'm sure =) and of course it's applied before: first a `TQString` (with internal unicode representation) is created from a C-string and only then `arg()` is applied... it can't be vice versa... And yes, I know that Qt3 strings were wierd and they kept an implicit 8-bit version alongside with unicode one... But all high-level calls like `arg()` work only to the unicode. `TQString::fromLatin1()` is unsafe only if its argument itself may contain a non-latin1 chars... after that it's an ordinary `TQString`...
MicheleC commented 1 month ago

Here are the strings from the translations, which are utf8, not the local 8bit. So there I would expect fromUtf8().

👍 thanks for the correction

> Here are the strings from the translations, which are utf8, not the local 8bit. So there I would expect fromUtf8(). :+1: thanks for the correction
MicheleC commented 1 month ago

Yeap, I'm sure =) ...

Perhaps I am missing something, but I struggle to understand how fromLatin1() would work correctly on (for example) a string in Chinese from the relative .po file.

> Yeap, I'm sure =) ... Perhaps I am missing something, but I struggle to understand how fromLatin1() would work correctly on (for example) a string in Chinese from the relative .po file.
SlavekB commented 1 month ago

As confirmed by Fat-Zer, fromLatin1 is used only for the format string itself during its loading into TQString. Then filling in the values from arg() is already done in the unicode string, so it will be without damaging the strings.

As confirmed by Fat-Zer, `fromLatin1` is used only for the format string itself during its loading into `TQString`. Then filling in the values from `arg()` is already done in the unicode string, so it will be without damaging the strings.
Fat-Zer commented 1 month ago

yep... so I consider this one settled...

yep... so I consider this one settled...
MicheleC commented 1 month ago

As confirmed by Fat-Zer, fromLatin1 is used only for the format string itself during its loading into TQString

Oh yeah, totally missed that 😧 😴

> As confirmed by Fat-Zer, fromLatin1 is used only for the format string itself during its loading into TQString Oh yeah, totally missed that 😧 😴
Fat-Zer marked this conversation as resolved
Fat-Zer force-pushed other/tdeio-message-fixup from 565cc9b030 to f001458957 1 month ago
Fat-Zer force-pushed other/tdeio-message-fixup from f001458957 to 748beb6125 1 month ago
Fat-Zer commented 1 month ago
Poster
Collaborator

Forced-pushed to replace all the argument counters inside the translatable strings with %1. And also fixed the copy-paste mistake where respTimeout was passed instead of the prConnTimeout.

Forced-pushed to replace all the argument counters inside the translatable strings with `%1`. And also fixed the copy-paste mistake where `respTimeout` was passed instead of the `prConnTimeout`.
MicheleC commented 1 month ago
Poster
Owner

I have a question. Why not use something simpler like this? I see no point in creating a string inside an arg(), it just adds additional work.

description = (i18n("Although ... Preferences.") + TQString("<ul><li>%1</li><li>%2</li><li>%3</li></ul>"))
    .arg(i18n("Timeout...)).arg(connTimeout))
    .arg(i18n("Timeout...)).arg(respTimeout))
    .arg(i18n("Timeout...)).arg(prConnTimeout)

?

I have a question. Why not use something simpler like this? I see no point in creating a string inside an arg(), it just adds additional work. ``` description = (i18n("Although ... Preferences.") + TQString("<ul><li>%1</li><li>%2</li><li>%3</li></ul>"))    .arg(i18n("Timeout...)).arg(connTimeout))    .arg(i18n("Timeout...)).arg(respTimeout))    .arg(i18n("Timeout...)).arg(prConnTimeout) ``` ?
Fat-Zer force-pushed other/tdeio-message-fixup from 748beb6125 to ea75a54abc 1 month ago
Fat-Zer commented 1 month ago
Poster
Collaborator

@MicheleC, whatever... let it be your way...

As for why: no special reason...
I was a bit concerned the gettext/weblate won't recognize the string as modified and will suggest to translate it from scratch which is annoying...
also I was a bit concerned a translator might want to move the list inside the message...
also I've tried to keep changes minimal...

The amount of additional work in terms of performance is minuscule anyway...

@MicheleC, whatever... let it be your way... As for why: no special reason... I was a bit concerned the gettext/weblate won't recognize the string as modified and will suggest to translate it from scratch which is annoying... also I was a bit concerned a translator might want to move the list inside the message... also I've tried to keep changes minimal... The amount of additional work in terms of performance is minuscule anyway...
MicheleC commented 1 month ago
Poster
Owner

I had clearly made wrong comments about the use of fromLatin1(), too quick looking at the PR instead of taking the time to think about it 😧

Other than that, trying to keep the code clean and more readable is always good practice, hence my last post above 😄

I had clearly made wrong comments about the use of fromLatin1(), too quick looking at the PR instead of taking the time to think about it 😧 Other than that, trying to keep the code clean and more readable is always good practice, hence my last post above :smile:
SlavekB reviewed 1 month ago
SlavekB left a comment

It looks good, but I have a supplementary question…

tdeio/tdeio/global.cpp
description = i18n("Although contact was made with the server, a "
"response was not received within the amount of time allocated for "
"the request as follows:")
.append(TQString::fromLatin1("<ul><li>"))
SlavekB commented 1 month ago

Is there any reason to use TQString::fromLatin1(...)? I'm interested because a simple use of .append("<ul><li>") seems to be sufficient.

Is there any reason to use `TQString::fromLatin1(...)`? I'm interested because a simple use of `.append("<ul><li>")` seems to be sufficient.
MicheleC commented 1 month ago

yes, exactly the same thing I was wondering too and that I tried to explain in my previous comments 👍

yes, exactly the same thing I was wondering too and that I tried to explain in my previous comments :+1:
Fat-Zer commented 1 month ago

It's considered a bad practice to call [T]QString's methods accepting const char *...
In Qt3/4 this effectively calls QString::fromAscii() which is kinda messy by itself: it doesn't actually converts from ASCII, but from an encoding defined by QTextCodec::codecForCStrings(). By default it's still latin-1, but in general it might be not... all in all it should be avoided unless it's specifically intended...

It looks like this convention actually followed through that particular file (not absolutely consistently though).

A lot of projects consider good practice and actually do define QT_NO_CAST_ASCII to force developers to use the explicit static methods like tr()/i18n() and fromLatin1/local8Bit/Utf8 to construct QStrings.

It's considered a bad practice to call [T]QString's methods accepting `const char *`... In Qt3/4 this effectively calls `QString::fromAscii()` which is kinda messy by itself: it doesn't actually converts from ASCII, but from an encoding defined by `QTextCodec::codecForCStrings()`. By default it's still latin-1, but in general it might be not... all in all it should be avoided unless it's specifically intended... It looks like this convention actually followed through that particular file (not absolutely consistently though). A lot of projects consider good practice and actually do define `QT_NO_CAST_ASCII` to force developers to use the explicit static methods like `tr()`/`i18n()` and `fromLatin1`/`local8Bit`/`Utf8` to construct `QString`s.
SlavekB commented 1 month ago

That makes sense, thank you.

That makes sense, thank you.
SlavekB marked this conversation as resolved
SlavekB merged commit d3d85b6550 into master 1 month ago
SlavekB deleted branch other/tdeio-message-fixup 1 month ago
SlavekB added this to the R14.0.10 release milestone 1 month ago
The pull request has been merged as d3d85b6550.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.