Undeprecate I18N_NOOP2
ClosedPublic

Authored by dfaure on Oct 23 2019, 11:19 AM.

Details

Summary

and document the pitfalls, where to use I18NC_NOOP, and where to use
I18N_NOOP2.

Test Plan

make test

Diff Detail

Repository
R249 KI18n
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mlaurent created this revision.Oct 23 2019, 11:19 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptOct 23 2019, 11:19 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
mlaurent requested review of this revision.Oct 23 2019, 11:19 AM
mlaurent retitled this revision from I18N_NOOP2 was deprecated but we can't replace by I18NC_NOOP as it expends it as 2 elements (context + text) It will break code as: struct MessageStatusInfo { const char *text; const char *icon; }; static const MessageStatusInfo... to I18N_NOOP2 was deprecated but we can't replace by I18NC_NOOP as it expends it as 2 elements (context + text).Oct 23 2019, 11:21 AM
mlaurent edited the summary of this revision. (Show Details)
mlaurent added reviewers: dfaure, ilic.

Seeing things like https://lxr.kde.org/source/frameworks/ki18n/src/kuitmarkup.cpp#0336 I'm wondering if this isn't rather a bug in I18NC_NOOP?

aacid added a subscriber: aacid.Oct 23 2019, 8:36 PM

Discarding the context is bad practice, if you discard it, it's very easy you will make a mistake when you try to i18n the text again and not pass the correct context.

I'm against bringing any I18NC_NOOP variant that strips the context.

This would also break since I18NC_NOOP_STRIP is unknown to the extraction scripts so nothing would be extracted for translation.

Ideally we would even deprecated I18NC_NOOP too, we now have KLocalizedString that is a string-to-be-localizaed but that hasn't been yet, so that fixes the same problem that _NOOP does, that you're translating too early.

Unfortunately for this case it seems that you also need the untranslated text and KLocalizedString doesn't have a getter for that. I'll propose that API.

ilic added a comment.Oct 24 2019, 11:26 AM

The idea was indeed to deprecate stripping of context, and not only the macro name, for the reason Alber provided.

The apparent counterexample in kuitmarkup.cpp is seen only due to macro-within-macro call and the macro expansion order, which is a situation that does not (or did not at the time of writing) occur anywhere else through KDE projects. But it too can be reformulated easily to the cleaner variant.

However, as I recall, even though KLocalizedString objects do deferred translation, I18N* macros were not deprecated alltogether in order to still allow for well-defined static initializers.

So what is the correct fix for my case ?

Seeing things like https://lxr.kde.org/source/frameworks/ki18n/src/kuitmarkup.cpp#0336 I'm wondering if this isn't rather a bug in I18NC_NOOP?

That's of course a stupid thought from myself, discarding the context will make this likely useless down the road when trying to perform the actual translation, as other have pointed out :)

aacid added a comment.Oct 25 2019, 9:02 PM

So what is the correct fix for my case ?

Port your code to I18NC_NOOP

mlaurent abandoned this revision.Oct 28 2019, 8:19 AM

Looking at deprecated API usage in Okteta, I came over the use of some use of I18N_NOOP2 as well. The use-case there though seems pretty fine to me, porting to I18NC_NOOP will complicate the logic without further need, as the context is the same for all strings in the given array, and always would be:

static constexpr const char* longTypeNames[static_cast<int>(PrimitiveDataType::END) + 1] = {
	    I18N_NOOP2("data type", "bool (1 byte)"),
	    I18N_NOOP2("data type", "signed byte"),
	    I18N_NOOP2("data type", "unsigned byte"),
	    I18N_NOOP2("data type", "char"),
	    I18N_NOOP2("data type", "bool (2 bytes)"),
	    I18N_NOOP2("data type", "signed short"),
	    I18N_NOOP2("data type", "unsigned short"),
	    I18N_NOOP2("data type", "bool (4 bytes)"),
	    I18N_NOOP2("data type", "signed int"),
	    I18N_NOOP2("data type", "unsigned int"),
	    I18N_NOOP2("data type", "bool (8 bytes)"),
	    I18N_NOOP2("data type", "signed long"),
	    I18N_NOOP2("data type", "unsigned long"),
	    I18N_NOOP2("data type", "float"),
	    I18N_NOOP2("data type", "double"),
	    I18N_NOOP2("data type", "bitfield"),
	};

used by some

return i18nc("data type", longTypeNames[static_cast<int>(type)]);

Porting to I18NC_NOOP would mean having to make the table an array of struct {context, text}, and extracting both properties from the table. More runtime cost and complicated logic to read. Which I find cumbersome. I would prefer to have a I18NC_NOOP_STRIP available.
Let's have a warning in the API dox, to tell people they need to use this carefully instead. But making them having to write more complicated logic like here it not developer friendly.

aacid added a comment.Oct 28 2019, 6:27 PM

That's the problem with deprecation warnings, they force you to write better code.

You don't like the fact that you have to do it, but at least please acknowledge that having "data type" in one place and then "data type" again in a totally different place just works "by change"/"by magic".

Also if you don't like how I18NC_NOOP(context, text) behaves you can always define your own I18NC_NOOP(context, text) before this include and make it strip the context. it's bad, but ...

As said, I do not think this would be "better" code. And it's violating a bit the goal of zero cost abstractions, given this adds runtime need which could be avoided compared to the old code.
When using the I18N_NOOP, one has to know what you do and when you later on pass data pointers instead of literals to i18n calls. Thinking people who do that are in general challenged to ensure the proper context call is still used might be challenged in other places before IMHO.

And no, I do not want to do hacks in my own code, that proposal will be ignored. Let's have KI18n provide a nice API for developers, and not get in their way in some halfhearted way to be foolproof.

I agree with Friederich. In kio/filewidgets/kfileplacesmodel.cpp (for which you accepted the change in D24970) I'm passing the context to the method because I18NC_NOOP forces me to, but then I'm *discarding* this context because:

  1. I have nowhere to store it
  2. I don't need to store it, it's the same for every item --- just like in Friederich's example.

Why store duplicated and redundant data?

So if I had a STRIP version that would all be much simpler.

One has to be careful anyway when using I18N_NOOP, to actually call i18n on those strings, and to never call i18n on a variable whose contents wasn't marked with I18N_NOOP. The API can't be foolproof against that. So I don't see why it's trying to be foolproof against the context being provided externally, at the expense of more complicated code for no purpose.

You don't want to accept there's a potential problem with typos and that either using KLocalizedString or forcing you to store the context is the solution?

Fine then just undeprecate Noop2, but adding strip that does the same is stupid

I don't exactly mind if we resurrect NOOP2 or rename it to STRIP, but STRIP kind of makes sense because the name explicitly says something is being stripped (and the documentation should warn loudly about the risks of doing that). It's a better name than "2" where the "2" means nothing else than "oops I can't overload macros".

I didn't realize one solution was to store a KLocalizedString, but that wouldn't work for KFilePlacesItem anyway, it uses KBookmark as storage (which takes a QString and can't be changed, it makes sense for it to be a QString, for actual user bookmarks).

I agree that the risk here is typos. The documentation should warn about that. But it's a logical risk when having a number of I18N markers (all of which have to repeat the context anyway), and then *one* call to i18nc for all of them (where repeating the context is IMHO an acceptable tradeoff for the complexity of storing the context in each string, when the underlying storage doesn't allow it).

src/klocalizedstring.h
64

typo: *S*TRIP

dfaure updated this revision to Diff 70212.Sat, Nov 23, 1:29 PM
dfaure retitled this revision from I18N_NOOP2 was deprecated but we can't replace by I18NC_NOOP as it expends it as 2 elements (context + text) to Provide I18NC_NOOP_STRIP as a new name for the deprecated I18N_NOOP2.
dfaure edited the summary of this revision. (Show Details)
dfaure added a reviewer: aacid.

Improve docu, update version, change commit log

dfaure commandeered this revision.Sat, Nov 23, 1:29 PM
dfaure edited reviewers, added: mlaurent; removed: dfaure.
aacid added a comment.Sat, Nov 23, 5:27 PM

As mentioned i don't see the point.

Doint this won't work, since needs an associated change in scripty, just undeprecated NOOP2 if you'll agree that this is what you want

OK, to avoid having to change scripty, I'll stick to the 2 name.

dfaure updated this revision to Diff 70234.Sat, Nov 23, 5:48 PM
dfaure retitled this revision from Provide I18NC_NOOP_STRIP as a new name for the deprecated I18N_NOOP2 to Undeprecate I18N_NOOP2.

keep old macro name

mlaurent accepted this revision.Thu, Nov 28, 5:51 AM
This revision is now accepted and ready to land.Thu, Nov 28, 5:51 AM
This revision was automatically updated to reflect the committed changes.