[Powertop] Internationalization problem with abstract field "type"

[Powertop] Internationalization problem with abstract field "type"

[Igor Zhbanov]

[-- Attachment #1: Type: text/plain, Size: 1583 bytes --]

Hi Chris,

It seems that there is a problem with internationalization with the last commit.
Please look here:
- report.addf(__("Package %i"), _package->get_number());
+ report.addf(__("%s %i"), _package->get_type(), _package->get_number());

I have introduced the macro __() /* Double underscore */ to be a conditional 
variant
of classical _() /* Single underscore */ in the report refactoring commit.

Usual underscore macro calls the gettext() function. And we need some sort 
of conditional
gettext() -- we want to print untranslated strings to CSV-report. (Of 
course, we could simply
set English locale for CSV-reports. But this will look strange.)

So I have introduced the double underscore macro which calls gettext() for 
HTML-reports and
return original strings for CSV.

After your patch the gettext will try to translate the string "%s %i" and 
not the "Package %i",
so we have lost the internationalization here.

We could fix it by internationalizing the "type" field in the class:
   ret->set_type(__("Package"));

But I'm not sure that it is good to have the type depending on current 
locale (especially
if in some future there will be need to compare the value of this field by 
strcmp()).

What you think?

P.S. Why you not sending patches to mailing list for discussion? ;-)

-- 
Best regards,
Igor Zhbanov,
Technical Leader,
phone: +7 (495) 797 25 00 ext 3806
e-mail: i.zhbanov(a)samsung.com

Mobile group, Moscow R&D center, Samsung Electronics
12 Dvintsev street, building 1
127018, Moscow, Russian Federation

Re: [Powertop] Internationalization problem with abstract field "type"

[Ferron, Chris E]

[-- Attachment #1: Type: text/plain, Size: 3265 bytes --]

On Thu, Oct 25, 2012 at 1:55 AM, Igor Zhbanov <i.zhbanov(a)samsung.com> wrote:
> Hi Chris,
>
> It seems that there is a problem with internationalization with the last
> commit.
> Please look here:
> - report.addf(__("Package %i"), _package->get_number());
> + report.addf(__("%s %i"), _package->get_type(), _package->get_number());
>
> I have introduced the macro __() /* Double underscore */ to be a conditional
> variant
> of classical _() /* Single underscore */ in the report refactoring commit.
>
> Usual underscore macro calls the gettext() function. And we need some sort
> of conditional
> gettext() -- we want to print untranslated strings to CSV-report. (Of
> course, we could simply
> set English locale for CSV-reports. But this will look strange.)
>
> So I have introduced the double underscore macro which calls gettext() for
> HTML-reports and
> return original strings for CSV.
>
> After your patch the gettext will try to translate the string "%s %i" and
> not the "Package %i",
> so we have lost the internationalization here.
You are correct, an oversight on my part by including the macro
without consideration.
>
> We could fix it by internationalizing the "type" field in the class:
>   ret->set_type(__("Package"));
Yes you are right this is bad. I think the best solution for this is
to define them as and use  gettext noop. I will add a translation
comment to them so translators have context.
#define gettext_noop(String) String
#define N_(String) gettext_noop(String)
#define PACKAGE_TYPE N_("Package")
#define CORE_TYPE N_("Core")

> But I'm not sure that it is good to have the type depending on current
> locale (especially
> if in some future there will be need to compare the value of this field by
> strcmp()).
My personal opinion is to remove all localization from reports, but
enough people have voiced that they find it valuable to be able to
generate reported in the language of there local.

As such I find there are several issues with internationalization when
it comes to reports. When we did internationalization it was supposed
to be for the UI. We then started thinking and planning to include in
the HTML report the ability to switch from local to engineering
(engineering being English) to solve most of the issues. Also it been
considered that CSV not be localized, as it should be considered
machine language and not meant to be necessarily human readable(which
is a good feature to the reporting work already done). Thus why as for
CSV standard discussions I only asked that "Office" type products be
able to parse them.

>
> What you think?
>
> P.S. Why you not sending patches to mailing list for discussion? ;-)
I do for large changes, but usually do not for quick fixes.

Thank you for pointing this out, I will fix this.
>
> --
> Best regards,
> Igor Zhbanov,
> Technical Leader,
> phone: +7 (495) 797 25 00 ext 3806
> e-mail: i.zhbanov(a)samsung.com
>
> Mobile group, Moscow R&D center, Samsung Electronics
> 12 Dvintsev street, building 1
> 127018, Moscow, Russian Federation
>
> _______________________________________________
> PowerTop mailing list
> PowerTop(a)lists.01.org
> https://lists.01.org/mailman/listinfo/powertop

Re: [Powertop] Internationalization problem with abstract field "type"

[Ferron, Chris E]

[-- Attachment #1: Type: text/plain, Size: 5691 bytes --]

Igor, Here is what I am thinking ?
---
diff --git a/src/cpu/cpu.cpp b/src/cpu/cpu.cpp
index 18cbffc..9767d52 100644
--- a/src/cpu/cpu.cpp
+++ b/src/cpu/cpu.cpp
@@ -39,6 +39,13 @@
 #include "../report/report.h"
 #include "../report/report-maker.h"

+#define gettext_noop(String) String
+#define N_(String) gettext_noop(String)
+/*TRANSLATORS: This is "package" as in a  processor Package */
+#define PACKAGE_TYPE N_("Package")
+/*TRANSLATORS: This is "Core" as in a  processor Core  */
+#define CORE_TYPE N_("Core")
+
 static class abstract_cpu system_level;

 vector<class abstract_cpu *> all_cpus;
@@ -87,7 +94,7 @@ static class abstract_cpu * new_package(int package,
int cpu, char * vendor, int
                ret = new class cpu_package;

        ret->set_number(package, cpu);
-       ret->set_type("Package");
+       ret->set_type(PACKAGE_TYPE);
        ret->childcount = 0;

        sprintf(packagename, _("cpu package %i"), cpu);
@@ -123,7 +130,7 @@ static class abstract_cpu * new_core(int core, int
cpu, char * vendor, int famil
                ret = new class cpu_core;
        ret->set_number(core, cpu);
        ret->childcount = 0;
-       ret->set_type("Core");
+       ret->set_type(CORE_TYPE);

        return ret;
 }
diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
index 4480b11..d16303f 100644
--- a/src/cpu/cpu.h
+++ b/src/cpu/cpu.h
@@ -88,7 +88,7 @@ public:
        uint64_t        total_stamp;
        int     number;
        int     childcount;
-       const char*    name;
+       const char*    type;
        bool    idle, old_idle;
        uint64_t        current_frequency;
        uint64_t        effective_frequency;
@@ -103,9 +103,9 @@ public:


        void    set_number(int _number, int cpu) {this->number =
_number; this->first_cpu = cpu;};
-       void    set_type(const char* _name) {this->name = _name;};
+       void    set_type(const char* _type) {this->type = _type;};
        int     get_number(void) { return number; };
-       const char* get_type(void) { return name; };
+       const char* get_type(void) { return type; };

        virtual void    measurement_start(void);
        virtual void    measurement_end(void);


On Thu, Oct 25, 2012 at 10:39 AM, Ferron, Chris E
<chris.e.ferron(a)intel.com> wrote:
> On Thu, Oct 25, 2012 at 1:55 AM, Igor Zhbanov <i.zhbanov(a)samsung.com> wrote:
>> Hi Chris,
>>
>> It seems that there is a problem with internationalization with the last
>> commit.
>> Please look here:
>> - report.addf(__("Package %i"), _package->get_number());
>> + report.addf(__("%s %i"), _package->get_type(), _package->get_number());
>>
>> I have introduced the macro __() /* Double underscore */ to be a conditional
>> variant
>> of classical _() /* Single underscore */ in the report refactoring commit.
>>
>> Usual underscore macro calls the gettext() function. And we need some sort
>> of conditional
>> gettext() -- we want to print untranslated strings to CSV-report. (Of
>> course, we could simply
>> set English locale for CSV-reports. But this will look strange.)
>>
>> So I have introduced the double underscore macro which calls gettext() for
>> HTML-reports and
>> return original strings for CSV.
>>
>> After your patch the gettext will try to translate the string "%s %i" and
>> not the "Package %i",
>> so we have lost the internationalization here.
> You are correct, an oversight on my part by including the macro
> without consideration.
>>
>> We could fix it by internationalizing the "type" field in the class:
>>   ret->set_type(__("Package"));
> Yes you are right this is bad. I think the best solution for this is
> to define them as and use  gettext noop. I will add a translation
> comment to them so translators have context.
> #define gettext_noop(String) String
> #define N_(String) gettext_noop(String)
> #define PACKAGE_TYPE N_("Package")
> #define CORE_TYPE N_("Core")
>
>> But I'm not sure that it is good to have the type depending on current
>> locale (especially
>> if in some future there will be need to compare the value of this field by
>> strcmp()).
> My personal opinion is to remove all localization from reports, but
> enough people have voiced that they find it valuable to be able to
> generate reported in the language of there local.
>
> As such I find there are several issues with internationalization when
> it comes to reports. When we did internationalization it was supposed
> to be for the UI. We then started thinking and planning to include in
> the HTML report the ability to switch from local to engineering
> (engineering being English) to solve most of the issues. Also it been
> considered that CSV not be localized, as it should be considered
> machine language and not meant to be necessarily human readable(which
> is a good feature to the reporting work already done). Thus why as for
> CSV standard discussions I only asked that "Office" type products be
> able to parse them.
>
>>
>> What you think?
>>
>> P.S. Why you not sending patches to mailing list for discussion? ;-)
> I do for large changes, but usually do not for quick fixes.
>
> Thank you for pointing this out, I will fix this.
>>
>> --
>> Best regards,
>> Igor Zhbanov,
>> Technical Leader,
>> phone: +7 (495) 797 25 00 ext 3806
>> e-mail: i.zhbanov(a)samsung.com
>>
>> Mobile group, Moscow R&D center, Samsung Electronics
>> 12 Dvintsev street, building 1
>> 127018, Moscow, Russian Federation
>>
>> _______________________________________________
>> PowerTop mailing list
>> PowerTop(a)lists.01.org
>> https://lists.01.org/mailman/listinfo/powertop

Re: [Powertop] Internationalization problem with abstract field "type"

[Igor Zhbanov]

[-- Attachment #1: Type: text/plain, Size: 6390 bytes --]

Hi Chris,

In the version before:
	report.addf(__("Package %i"), _package->get_number());
I have used the conditional gettext() macro which generates localized table column names
for HTML-reports and generate original column names for CSV-reports:
	/* Conditional gettext. We need original strings for CSV. */
	#define __(STRING) \
	        ((report.get_type() == REPORT_CSV) ? (STRING) : gettext(STRING))

And in your version we have lost the translation for HTML-reports.
I don't know whether we need to translate words "CPU", "Core" and "Package" to different
languages in column names in HTML report.

It should be your decision. ;-)

Ferron, Chris E wrote:
> Igor, Here is what I am thinking ?
> ---
> diff --git a/src/cpu/cpu.cpp b/src/cpu/cpu.cpp
> index 18cbffc..9767d52 100644
> --- a/src/cpu/cpu.cpp
> +++ b/src/cpu/cpu.cpp
> @@ -39,6 +39,13 @@
>   #include "../report/report.h"
>   #include "../report/report-maker.h"
>
> +#define gettext_noop(String) String
> +#define N_(String) gettext_noop(String)
> +/*TRANSLATORS: This is "package" as in a  processor Package */
> +#define PACKAGE_TYPE N_("Package")
> +/*TRANSLATORS: This is "Core" as in a  processor Core  */
> +#define CORE_TYPE N_("Core")
> +
>   static class abstract_cpu system_level;
>
>   vector<class abstract_cpu *> all_cpus;
> @@ -87,7 +94,7 @@ static class abstract_cpu * new_package(int package,
> int cpu, char * vendor, int
>                  ret = new class cpu_package;
>
>          ret->set_number(package, cpu);
> -       ret->set_type("Package");
> +       ret->set_type(PACKAGE_TYPE);
>          ret->childcount = 0;
>
>          sprintf(packagename, _("cpu package %i"), cpu);
> @@ -123,7 +130,7 @@ static class abstract_cpu * new_core(int core, int
> cpu, char * vendor, int famil
>                  ret = new class cpu_core;
>          ret->set_number(core, cpu);
>          ret->childcount = 0;
> -       ret->set_type("Core");
> +       ret->set_type(CORE_TYPE);
>
>          return ret;
>   }
> diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
> index 4480b11..d16303f 100644
> --- a/src/cpu/cpu.h
> +++ b/src/cpu/cpu.h
> @@ -88,7 +88,7 @@ public:
>          uint64_t        total_stamp;
>          int     number;
>          int     childcount;
> -       const char*    name;
> +       const char*    type;
>          bool    idle, old_idle;
>          uint64_t        current_frequency;
>          uint64_t        effective_frequency;
> @@ -103,9 +103,9 @@ public:
>
>
>          void    set_number(int _number, int cpu) {this->number =
> _number; this->first_cpu = cpu;};
> -       void    set_type(const char* _name) {this->name = _name;};
> +       void    set_type(const char* _type) {this->type = _type;};
>          int     get_number(void) { return number; };
> -       const char* get_type(void) { return name; };
> +       const char* get_type(void) { return type; };
>
>          virtual void    measurement_start(void);
>          virtual void    measurement_end(void);
>
>
> On Thu, Oct 25, 2012 at 10:39 AM, Ferron, Chris E
> <chris.e.ferron(a)intel.com> wrote:
>> On Thu, Oct 25, 2012 at 1:55 AM, Igor Zhbanov <i.zhbanov(a)samsung.com> wrote:
>>> Hi Chris,
>>>
>>> It seems that there is a problem with internationalization with the last
>>> commit.
>>> Please look here:
>>> - report.addf(__("Package %i"), _package->get_number());
>>> + report.addf(__("%s %i"), _package->get_type(), _package->get_number());
>>>
>>> I have introduced the macro __() /* Double underscore */ to be a conditional
>>> variant
>>> of classical _() /* Single underscore */ in the report refactoring commit.
>>>
>>> Usual underscore macro calls the gettext() function. And we need some sort
>>> of conditional
>>> gettext() -- we want to print untranslated strings to CSV-report. (Of
>>> course, we could simply
>>> set English locale for CSV-reports. But this will look strange.)
>>>
>>> So I have introduced the double underscore macro which calls gettext() for
>>> HTML-reports and
>>> return original strings for CSV.
>>>
>>> After your patch the gettext will try to translate the string "%s %i" and
>>> not the "Package %i",
>>> so we have lost the internationalization here.
>> You are correct, an oversight on my part by including the macro
>> without consideration.
>>> We could fix it by internationalizing the "type" field in the class:
>>>    ret->set_type(__("Package"));
>> Yes you are right this is bad. I think the best solution for this is
>> to define them as and use  gettext noop. I will add a translation
>> comment to them so translators have context.
>> #define gettext_noop(String) String
>> #define N_(String) gettext_noop(String)
>> #define PACKAGE_TYPE N_("Package")
>> #define CORE_TYPE N_("Core")
>>
>>> But I'm not sure that it is good to have the type depending on current
>>> locale (especially
>>> if in some future there will be need to compare the value of this field by
>>> strcmp()).
>> My personal opinion is to remove all localization from reports, but
>> enough people have voiced that they find it valuable to be able to
>> generate reported in the language of there local.
>>
>> As such I find there are several issues with internationalization when
>> it comes to reports. When we did internationalization it was supposed
>> to be for the UI. We then started thinking and planning to include in
>> the HTML report the ability to switch from local to engineering
>> (engineering being English) to solve most of the issues. Also it been
>> considered that CSV not be localized, as it should be considered
>> machine language and not meant to be necessarily human readable(which
>> is a good feature to the reporting work already done). Thus why as for
>> CSV standard discussions I only asked that "Office" type products be
>> able to parse them.
>>
>>> What you think?
>>>
>>> P.S. Why you not sending patches to mailing list for discussion? ;-)
>> I do for large changes, but usually do not for quick fixes.
>>
>> Thank you for pointing this out, I will fix this.

-- 
Best regards,
Igor Zhbanov,
Technical Leader,
phone: +7 (495) 797 25 00 ext 3806
e-mail: i.zhbanov(a)samsung.com

Mobile group, Moscow R&D center, Samsung Electronics
12 Dvintsev street, building 1
127018, Moscow, Russian Federation