D24884: I18N_NOOP2 was deprecated but we can't replace by I18NC_NOOP as it expends it as 2 elements (context + text)

[Laurent Montel]

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)".
mlaurent edited the summary of this revision.
mlaurent added reviewers: dfaure, ilic.

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D24884

To: mlaurent, dfaure, ilic
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24884: I18N_NOOP2 was deprecated but we can't replace by I18NC_NOOP as it expends it as 2 elements (context + text)

[Volker Krause]

vkrause added a comment.


  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?

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D24884

To: mlaurent, dfaure, ilic
Cc: vkrause, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24884: I18N_NOOP2 was deprecated but we can't replace by I18NC_NOOP as it expends it as 2 elements (context + text)

[Albert Astals Cid]

aacid added a comment.


  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.

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D24884

To: mlaurent, dfaure, ilic
Cc: aacid, vkrause, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D24884: I18N_NOOP2 was deprecated but we can't replace by I18NC_NOOP as it expends it as 2 elements (context + text)

[Albert Astals Cid]

aacid added a comment.


  https://phabricator.kde.org/D24898

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D24884

To: mlaurent, dfaure, ilic
Cc: aacid, vkrause, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D24884: I18N_NOOP2 was deprecated but we can't replace by I18NC_NOOP as it expends it as 2 elements (context + text)

[Chusslove Illich]

ilic added a comment.


  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.

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D24884

To: mlaurent, dfaure, ilic
Cc: aacid, vkrause, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D24884: I18N_NOOP2 was deprecated but we can't replace by I18NC_NOOP as it expends it as 2 elements (context + text)

[Laurent Montel]

mlaurent added a comment.


  So what is the correct fix for my case ?

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D24884

To: mlaurent, dfaure, ilic
Cc: aacid, vkrause, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D24884: I18N_NOOP2 was deprecated but we can't replace by I18NC_NOOP as it expends it as 2 elements (context + text)

[Volker Krause]

vkrause added a comment.


  In D24884#552701 , @vkrause wrote:
  
  > 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 :)

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D24884

To: mlaurent, dfaure, ilic
Cc: aacid, vkrause, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D24884: I18N_NOOP2 was deprecated but we can't replace by I18NC_NOOP as it expends it as 2 elements (context + text)

[Albert Astals Cid]

aacid added a comment.


  In D24884#553786 , @mlaurent wrote:
  
  > So what is the correct fix for my case ?
  
  
  Port your code to I18NC_NOOP

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D24884

To: mlaurent, dfaure, ilic
Cc: aacid, vkrause, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D24884: I18N_NOOP2 was deprecated but we can't replace by I18NC_NOOP as it expends it as 2 elements (context + text)

[Laurent Montel]

mlaurent added a comment.


  Ok thanks.

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D24884

To: mlaurent, dfaure, ilic
Cc: aacid, vkrause, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D24884: I18N_NOOP2 was deprecated but we can't replace by I18NC_NOOP as it expends it as 2 elements (context + text)

[Laurent Montel]

mlaurent abandoned this revision.

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D24884

To: mlaurent, dfaure, ilic
Cc: aacid, vkrause, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D24884: I18N_NOOP2 was deprecated but we can't replace by I18NC_NOOP as it expends it as 2 elements (context + text)

[Friedrich W. H. Kossebau]

kossebau added a comment.


  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(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(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.

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D24884

To: mlaurent, dfaure, ilic
Cc: kossebau, aacid, vkrause, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24884: I18N_NOOP2 was deprecated but we can't replace by I18NC_NOOP as it expends it as 2 elements (context + text)

[Albert Astals Cid]

aacid added a comment.


  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 ...

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D24884

To: mlaurent, dfaure, ilic
Cc: kossebau, aacid, vkrause, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24884: I18N_NOOP2 was deprecated but we can't replace by I18NC_NOOP as it expends it as 2 elements (context + text)

[Friedrich W. H. Kossebau]

kossebau added a comment.


  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.

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D24884

To: mlaurent, dfaure, ilic
Cc: kossebau, aacid, vkrause, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24884: I18N_NOOP2 was deprecated but we can't replace by I18NC_NOOP as it expends it as 2 elements (context + text)

[David Faure]

dfaure added a comment.


  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.

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D24884

To: mlaurent, dfaure, ilic
Cc: kossebau, aacid, vkrause, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24884: I18N_NOOP2 was deprecated but we can't replace by I18NC_NOOP as it expends it as 2 elements (context + text)

[Albert Astals Cid]

aacid added a comment.


  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

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D24884

To: mlaurent, dfaure, ilic
Cc: kossebau, aacid, vkrause, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24884: I18N_NOOP2 was deprecated but we can't replace by I18NC_NOOP as it expends it as 2 elements (context + text)

[David Faure]

dfaure added a comment.


  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).

INLINE COMMENTS

> klocalizedstring.h:69
>   *
> - * \deprecated Since 5.0, use \c I18NC_NOOP.
> + * \deprecated Since 5.0, use \c I18NC_NOOP_TRIP.
>   */

typo: *S*TRIP

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D24884

To: mlaurent, dfaure, ilic
Cc: kossebau, aacid, vkrause, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24884: 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;

[Laurent Montel]

mlaurent created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
mlaurent requested review of this revision.

REVISION SUMMARY
  ...StatusValues[] = {
  
{ I18N_NOOP2("message status", "Important"), "emblem-important"},
  
  ...
  }
  
  it will return 3 elements and not 2.
  
  I think that idea was to remove this macro as name was not good.
  So I added a new macro which strip context
  
  Allow to replace I18N_NOOP2 macro

TEST PLAN
  make test

REPOSITORY
  R249 KI18n

BRANCH
  de_deprecated_i18n_noop2

REVISION DETAIL
  https://phabricator.kde.org/D24884

AFFECTED FILES
  src/klocalizedstring.h

To: mlaurent
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns