512623 - Implement the basic infrastructure for localization of all custom and standard field values

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement)

Description

[Max Kanat-Alexander]

There's a very simple way to allow localization of custom field values. Instead of having a get_status and a get_resolution, we just have a display_value function, where you pass the field name and the value, and it returns you a localized display value. I actually need this functionality for another bug that I'm working on (bug 512606), so I'm just going to implement it.

Comment 1

[Vitaly Fedrushkov]

Bug 426222 has been already boiled down to that

Comment 2

[Max Kanat-Alexander]

Attachment: v1

Here it is! The only important code changes are in global/field-descs.html.tmpl, filterexceptions, and bug/field.html.tmpl. The other changes I made automatically, like this:

find template -type f -exec perl -pe 's/get_resolution\(/display_value("resolution", /g' -i {} \;

find template -type f -exec perl -pe 's/get_status\(/display_value("bug_status", /g' -i {} \;

Comment 3

[Vitaly Fedrushkov]

(In reply to comment #2)

As we expect display_value() to expand many fields and do no harm otherwise, IMHO we should stop checking field names whereever possible:

https://bugzilla.mozilla.org/attachment.cgi?id=396657&action=diff#template/en/default/bug/activity/table.html.tmpl_sec1

| [% ELSIF change.fieldname == 'bug_status' %]
|   [% display_value("bug_status", change.removed) FILTER html %]
| [% ELSIF change.fieldname == 'resolution' %]
|   [% display_value("resolution", change.removed) FILTER html %]

this should probably read

 [% ELSE %]
   [% display_value(change.fieldname, change.removed) FILTER html %]

same for 'change.added' next chunk

https://bugzilla.mozilla.org/attachment.cgi?id=396657&action=diff#template/en/default/list/list.csv.tmpl_sec1

| [% ELSIF column == 'bug_status' %]
|   [% bug.$column = display_value("bug_status", bug.$column) %]
| [% ELSIF column == 'resolution' %]
|   [%- bug.$column = display_value("resolution", bug.$column) %]
| [% END %]

again can avoid ELSIFs:

 [% ELSE %]
   [% bug.$column = display_value(column, bug.$column) %]

https://bugzilla.mozilla.org/attachment.cgi?id=396657&action=diff#template/en/default/bug/create/create.html.tmpl_sec2

 [% ELSE %]
   [% display_value(sel.name, x) FILTER html %]

https://bugzilla.mozilla.org/attachment.cgi?id=396657&action=diff#template/en/default/list/table.html.tmpl_sec1

 [% ELSE %]
   [%- display_value(column, bug.$column) FILTER html %]

https://bugzilla.mozilla.org/attachment.cgi?id=396657&action=diff#template/en/default/reports/report-bar.png.tmpl_sec1

  [% FOR i IN [ 0 .. data.0.0.max ] %]
    [% data.0.0.$i = display_value(col_field, data.0.0.$i) %]
  [% END %]

same for next few chunks

https://bugzilla.mozilla.org/attachment.cgi?id=396657&action=diff#template/en/default/search/form.html.tmpl_sec1

| [% ELSIF sel.name == "resolution" %]
|   [%# Again, resolution has that odd empty value. Replace it with '---' %]
|   [% display_value("resolution", value.name) OR '---' FILTER html %]
| [% ELSE %]
|   [% value.name FILTER html %]
| [% END %]

this hack may probably be suppressed if we provide '---' as value for empty resolution in value_descs

Comment 4

[Max Kanat-Alexander]

I just want to focus on the simple basic parts for now--so there are some places where custom fields are displayed without using field.html.tmpl, but I actually don't need to fix those yet for what I'm doing, so I'd like to handle those in a separate bug and just keep it simple in this bug for now.

Comment 5

[Max Kanat-Alexander]

(In reply to comment #3)
> As we expect display_value() to expand many fields and do no harm otherwise,
> IMHO we should stop checking field names whereever possible:

  We'll do it in a separate bug. I think it's too much to review if all the get_status/get_resolution changes aren't just straight automated changes, for now.

> this hack may probably be suppressed if we provide '---' as value for empty
> resolution in value_descs

  Good point. Something we can probably also save for another bug.

Comment 6

[Vitaly Fedrushkov]

(In reply to comment #2)

> other changes I made automatically, like this:

> find template -type f -exec perl -pe
> 's/get_resolution\(/display_value("resolution", /g' -i {} \;

I guess these changes may be left to bug 426222 as well.  Instead we should keep old MACROs as 

  [% MACRO get_resolution(res) GET display_value("resolution", $res) %]
  [% MACRO get_status(stat) GET display_value("bug_status", $stat) %]

This is also necessary to maintain backwards compatibility for customized templates.

IMHO display_value() is great thing by itself, and migrating default templates to new syntax is orthogonal to bug 512606 goals.

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> This is also necessary to maintain backwards compatibility for customized
> templates.

  Mmm, this is trunk, so I don't need or want to maintain backwards compatibility with old customizations.

> IMHO display_value() is great thing by itself, and migrating default templates
> to new syntax is orthogonal to bug 512606 goals.

  Mmm, that could be true. We'll see if LpSolit says that this patch is too long to review.

Comment 8

[Frédéric Buclin]

(In reply to comment #7)
> We'll see if LpSolit says that this patch is too long to review.

Nothing is too long to review for me. :-D

Comment 9

[Frédéric Buclin]

Comment on attachment 396657 [details] [diff] [review]
v1

>=== modified file 'template/en/default/filterexceptions.pl'

>+'global/field-descs.none.tmpl' => [
>+    'value_descs.${field_name}.${value_name}',
>+    'value_name',
>+],

These strings are not safe, and should not be treated as exceptions. You should explicitly put FILTER none in field-descs.none.tmpl with a comment explaining why we don't filter them there.



>=== modified file 'template/en/default/global/field-descs.none.tmpl'

>+[% value_descs = {
>+  # You can localize field values by adding them to this hash, like this:
>+  #
>+  # "bug_status" => { "UNCONFIRMED" => "UNCO",
>+  #                   "NEW"         => "NEWISH", },
>+  #
>+  # "bug_status" is the field name from the fielddefs table in the database.
>+  # The values on the left are that field's values as they appear in the
>+  # database, and the value on the right is the value as it will appear
>+  # in the user interface. For this example, the Status value "NEW"
>+  # would appear in the UI as "NEWISH", and "UNCONFIRMED" would appear as
>+  # "UNCO".
>+  #
>+  # That is only an example. You can do this with any value of any field,
>+  # though currently it is only guaranteed to work everywhere for the
>+  # bug_status and resolution fields. (But it will still work in many
>+  # places for other fields, too--just maybe not everywhere in Bugzilla.)
>+} %]

I think the comment is a bit too verbose. Anyway, put this comment out of the value_descs hash. I had to read the macro to understand where I had to put localized strings, and it took me a while to realize that the value_descs hash already exists (I thought this whole block was a comment, not a comment within a hash). So I think something like that would be clearer to localizers (which probably don't speak Perl):

[%# A very long comment explaining that localized strings must go into their respective sections below, and that lines starting with # are comments only, and that you can add additional fields besides the already present bug_status and resolution fields. %]

[% value_descs = {
  bug_status = {
    # "UNCONFIRMED" => "UNCO",
    # "NEW"         => "NEWISH",
  },
  resolution = {
    # "FIXED" => "NO LONGER AN ISSUE",
    # "MOVED" => "BYE-BYE",
  },
} %]

I think this indentation is clearer, so that localizers don't have to worry about brackets.


>+[% MACRO display_value(field_name, value_name) BLOCK %][% FILTER trim %]
>+  [% IF value_descs.${field_name}.${value_name}.defined %]
>+    [% value_descs.${field_name}.${value_name} %]
>+  [% ELSE %]
>+    [% value_name %]
>+  [% END %]
>+[% END %][% END %]

Here, explicitly write FILTER none, and a comment before the MACRO explaining why we don't filter them here (else we won't have to wait for too long before something goes wrong).



>=== modified file 'template/en/default/pages/release-notes.html.tmpl'

>-  <li>You should now be using <code>get_status('NEW')</code> instead of
>+  <li>You should now be using <code>display_value("bug_status", 'NEW')</code> instead of

These are the release notes for Bugzilla 3.2, which really still uses get_status(), not display_value(). So you have to revert this change.


Vitaly made a very good point about now useless [% ELSIF status... %] [% ELSIF resolution... %]. If you don't want to fix this in this bug, please file a separate bug to keep track of it.

Comment 10

[Vitaly Fedrushkov]

(In reply to comment #7)
> (In reply to comment #6)
> > This is also necessary to maintain backwards compatibility for customized
> > templates.

>   Mmm, this is trunk, so I don't need or want to maintain backwards
> compatibility with old customizations.

Then I'd like to see this bug marked for release notes.

Comment 11

[Vitaly Fedrushkov]

(In reply to comment #9)
> Vitaly made a very good point about now useless [% ELSIF status... %] [% ELSIF
> resolution... %]. If you don't want to fix this in this bug, please file a
> separate bug to keep track of it.

Bug 519142 submitted

Comment 12

[Max Kanat-Alexander]

Attachment: v2 (Important Code)

I adopted all your suggestions and changes! :-) To make things easier to review, I'm splitting this into "important code" and the other bit that actually translates get_* to display_value.

Comment 13

[Max Kanat-Alexander]

Attachment: v2 (get_* changes)

And here's the conversion of get_status/get_resolution into display_value. (And I fixed release-notes.html.tmpl, like you asked.)

Comment 14

[Frédéric Buclin]

Comment on attachment 403709 [details] [diff] [review]
v2 (Important Code)

r=LpSolit

Comment 15

[Max Kanat-Alexander]

And to make life easier on people reading our commit history, I'm also committing the two patches separately:

Important changes:

Checking in template/en/default/bug/field.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v  <--  field.html.tmpl
new revision: 1.32; previous revision: 1.31
done
Checking in template/en/default/global/field-descs.none.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/field-descs.none.tmpl,v  <--  field-descs.none.tmpl
new revision: 1.37; previous revision: 1.36
done

get_* changes:

Checking in template/en/default/admin/products/edit-common.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/edit-common.html.tmpl,v  <--  edit-common.html.tmpl
new revision: 1.11; previous revision: 1.10
done
Checking in template/en/default/admin/workflow/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/workflow/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.7; previous revision: 1.6
done
Checking in template/en/default/attachment/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.43; previous revision: 1.42
done
Checking in template/en/default/bug/dependency-tree.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/dependency-tree.html.tmpl,v  <--  dependency-tree.html.tmpl
new revision: 1.32; previous revision: 1.31
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.164; previous revision: 1.163
done
Checking in template/en/default/bug/format_comment.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/format_comment.txt.tmpl,v  <--  format_comment.txt.tmpl
new revision: 1.2; previous revision: 1.1
done
Checking in template/en/default/bug/knob.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/knob.html.tmpl,v  <--  knob.html.tmpl
new revision: 1.43; previous revision: 1.42
done
Checking in template/en/default/bug/show-multiple.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show-multiple.html.tmpl,v  <--  show-multiple.html.tmpl
new revision: 1.45; previous revision: 1.44
done
Checking in template/en/default/bug/summarize-time.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/summarize-time.html.tmpl,v  <--  summarize-time.html.tmpl
new revision: 1.16; previous revision: 1.15
done
Checking in template/en/default/bug/activity/table.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/activity/table.html.tmpl,v  <--  table.html.tmpl
new revision: 1.18; previous revision: 1.17
done
Checking in template/en/default/bug/create/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.97; previous revision: 1.96
done
Checking in template/en/default/email/whine.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/email/whine.txt.tmpl,v  <--  whine.txt.tmpl
new revision: 1.8; previous revision: 1.7
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.117; previous revision: 1.116
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v  <--  messages.html.tmpl
new revision: 1.90; previous revision: 1.89
done
Checking in template/en/default/list/edit-multiple.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/edit-multiple.html.tmpl,v  <--  edit-multiple.html.tmpl
new revision: 1.57; previous revision: 1.56
done
Checking in template/en/default/list/list.csv.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list.csv.tmpl,v  <--  list.csv.tmpl
new revision: 1.9; previous revision: 1.8
done
Checking in template/en/default/list/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.68; previous revision: 1.67
done
Checking in template/en/default/list/table.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/table.html.tmpl,v  <--  table.html.tmpl
new revision: 1.49; previous revision: 1.48
done
Checking in template/en/default/pages/fields.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/pages/fields.html.tmpl,v  <--  fields.html.tmpl
new revision: 1.18; previous revision: 1.17
done
Checking in template/en/default/reports/report-bar.png.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/report-bar.png.tmpl,v  <--  report-bar.png.tmpl
new revision: 1.9; previous revision: 1.8
done
Checking in template/en/default/reports/report-line.png.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/report-line.png.tmpl,v  <--  report-line.png.tmpl
new revision: 1.10; previous revision: 1.9
done
Checking in template/en/default/reports/report-pie.png.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/report-pie.png.tmpl,v  <--  report-pie.png.tmpl
new revision: 1.8; previous revision: 1.7
done
Checking in template/en/default/reports/report-table.csv.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/report-table.csv.tmpl,v  <--  report-table.csv.tmpl
new revision: 1.13; previous revision: 1.12
done
Checking in template/en/default/reports/report-table.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/report-table.html.tmpl,v  <--  report-table.html.tmpl
new revision: 1.18; previous revision: 1.17
done
Checking in template/en/default/search/form.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/search/form.html.tmpl,v  <--  form.html.tmpl
new revision: 1.59; previous revision: 1.58
done
Checking in template/en/default/whine/mail.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/whine/mail.html.tmpl,v  <--  mail.html.tmpl
new revision: 1.8; previous revision: 1.7
done
Checking in template/en/default/whine/mail.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/whine/mail.txt.tmpl,v  <--  mail.txt.tmpl
new revision: 1.8; previous revision: 1.7
done

Comment 18

[Max Kanat-Alexander]

Added to the release notes in bug 547466.