Last Comment Bug 760804 - PDF viewer should use a single unicode character instead of "..." and should have localization comments
: PDF viewer should use a single unicode character instead of "..." and should ...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: PDF Viewer (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 16
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
Mentors:
Depends on:
Blocks: 748924
  Show dependency treegraph
 
Reported: 2012-06-02 06:38 PDT by Francesco Lodolo [:flod]
Modified: 2012-06-05 06:20 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
patch (3.61 KB, patch)
2012-06-04 15:23 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
updated patch (3.80 KB, patch)
2012-06-04 16:07 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
updated again (3.76 KB, patch)
2012-06-04 16:18 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
l10n: feedback+
Details | Diff | Splinter Review

Description Francesco Lodolo [:flod] 2012-06-02 06:38:40 PDT
http://hg.mozilla.org/mozilla-central/file/65f748c63f4c/browser/locales/en-US/pdfviewer/viewer.properties

loading=Loading... {{percent}}%

All Mozilla's products use "…" instead of "...". Also, that file need at least an initial localization comment explaining what should be left untranslated (variables).

text_annotation_type=[{{type}} Annotation]
This string needs a l10n comment explaining what "type" could be.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-04 15:23:36 PDT
Created attachment 629965 [details] [diff] [review]
patch
Comment 2 Axel Hecht [pto-Aug-30][:Pike] 2012-06-04 15:35:13 PDT
Comment on attachment 629965 [details] [diff] [review]
patch

Review of attachment 629965 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/en-US/pdfviewer/viewer.properties
@@ +11,3 @@
>  page_label=Page:
>  page_of=of {{pageCount}}
> +

I don't expect this to work in general. I'd expect a combi of double plurals here, similar to what we do in download manager. Notably, there could be text that depends on the current page number preceding the current page number.

I know this is a comment fix, but it makes the code problem more obvious.

@@ +24,5 @@
>  download_label=Download
>  bookmark.title=Current view (copy or open in new window)
>  bookmark_label=Current View
>  
> +# Side panel toolbar button tooltips

As a non-native speaker, I can't parse this string.

@@ +37,1 @@
>  thumbs_label=Thumbnails

What's the point of the re-ordering here? Why's thumbs_label separate from thumbs.title?

(yeah, _ and ., sob).

@@ +49,1 @@
>  thumb_page_canvas=Thumbnail of Page {{page}}

I wonder if these should use plural forms. flod?

@@ +83,5 @@
>  loading_error_indicator=Error
>  loading_error=An error occurred while loading the PDF.
>  
>  # Misc labels and messages
> +# LOCALIZATION NOTE (text_annotation_type): "{{[percent}}" will be replaced with a percentage

Can't match this comment to the string.
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-04 16:03:42 PDT
(In reply to Axel Hecht [:Pike] from comment #2)
> I know this is a comment fix, but it makes the code problem more obvious.

Yes, indeed. Too late to change this now, though... We can follow up on trunk?

> > +# Side panel toolbar button tooltips
> 
> As a non-native speaker, I can't parse this string.

Would "Tooltips for side panel toolbar buttons" be clearer?

> What's the point of the re-ordering here? Why's thumbs_label separate from
> thumbs.title?

I thought it'd make it clearer how the strings differed. Really these should use .label/.tooltip, but changing the string names is too much of a PITA at this point.

> > +# LOCALIZATION NOTE (text_annotation_type): "{{[percent}}" will be replaced with a percentage
> 
> Can't match this comment to the string.

Fixed this per discussion on IRC:
-# LOCALIZATION NOTE (text_annotation_type): "{{[percent}}" will be replaced with a percentage
+# LOCALIZATION NOTE (text_annotation_type): This is used as a tooltip.
+# "{{[type}}" will be replaced with an annotation type from a list defined in
+# the PDF spec (32000-1:2008 Table 169 – Annotation types).
+# Some common types are e.g.: "Check", "Text", "Comment", "Note"
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-04 16:07:06 PDT
Created attachment 629985 [details] [diff] [review]
updated patch
Comment 5 Yury Delendik (:yury) 2012-06-04 16:17:48 PDT
> I thought it'd make it clearer how the strings differed. Really these should use .label/.tooltip, but changing the string names is too much of a PITA at this point

Currently PDF.js is using webL10n for localization of the HTML code. The dot sign is reserved for separation the element localization id from the HTML attribute name. So it will not be possible to change "xxx.yyy" to "xxx_yyy" and vice verse, without adding the complexity to the HTML code localization.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-04 16:18:28 PDT
Created attachment 629987 [details] [diff] [review]
updated again

Note that the only functional change here is the change to the ellipsis.
Comment 7 Axel Hecht [pto-Aug-30][:Pike] 2012-06-04 16:26:27 PDT
Comment on attachment 629987 [details] [diff] [review]
updated again

Review of attachment 629987 [details] [diff] [review]:
-----------------------------------------------------------------

The tooltip vs title story seems to be awkward, but not part of this patch, f=me to get some comments in for our l10n community.
Comment 9 Francesco Lodolo [:flod] 2012-06-04 21:49:40 PDT
> @@ +49,1 @@
> >  thumb_page_canvas=Thumbnail of Page {{page}}
> 
> I wonder if these should use plural forms. flod?

I don't think so, I expect {{page}} to be a page number, and thumbnail to remain singular.

Unfortunately all these features land way too late on trunk, so we'll never get the chance to fix them before the switch to aurora (another example is GLCI, I just found 2 hard-coded strings...).

Another doubt:
page_scale_width=Page Width
page_scale_fit=Page Fit

I expect the first to be "Fit to Width" and the second "Fit to Page", at least if I got the real meaning of the first right.
Comment 10 Geoff Lankow (:darktrojan) 2012-06-05 06:20:02 PDT
https://hg.mozilla.org/mozilla-central/rev/eee5cd8316af

Note You need to log in before you can comment on or make changes to this bug.