Internationalization problem with abstract field "type"

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

_______________________________________________
PowerTop mailing list
PowerTop(a)lists.01.org
https://lists.01.org/mailman/listinfo/powertop

Quoted 18 lines · Igor Zhbanov

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

Quoted 7 lines

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.

Quoted 3 lines

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

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 16 lines

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

Igor, Here is what I am thinking ?
---

src/cpu/cpu.cpp +7 -0

index 18cbffc..9767d52 100644
@@ -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);

src/cpu/cpu.cpp +1 -1

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

src/cpu/cpu.cpp +1 -1

-       ret->set_type("Core");
+       ret->set_type(CORE_TYPE);
 
         return ret;
  }

src/cpu/cpu.h +3 -3

index 4480b11..d16303f 100644
@@ -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);

Quoted 63 lines · Chris E

Quoted 18 lines · Igor Zhbanov

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

Quoted 7 lines

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.

Quoted 2 lines

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

Quoted 4 lines

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.

Quoted 3 lines

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.