Bug 26392 - Wrap all translatable strings in opac-main.tt
Summary: Wrap all translatable strings in opac-main.tt
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 20988
  Show dependency treegraph
 
Reported: 2020-09-04 14:56 UTC by Julian Maurice
Modified: 2020-09-13 12:12 UTC (History)
1 user (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 26392: Wrap all translatable strings in opac-main.tt (20.81 KB, patch)
2020-09-04 14:57 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 26392: Wrap all translatable strings in opac-main.tt (21.27 KB, patch)
2020-09-11 13:31 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 26392: Fix opac news display issue when there is no author (1.87 KB, patch)
2020-09-11 13:31 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 26392: Wrap all translatable strings in opac-main.tt (21.35 KB, patch)
2020-09-11 14:23 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 26392: Fix opac news display issue when there is no author (1.92 KB, patch)
2020-09-11 14:23 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2020-09-04 14:56:45 UTC

    
Comment 1 Julian Maurice 2020-09-04 14:57:26 UTC
Created attachment 109666 [details] [review]
Bug 26392: Wrap all translatable strings in opac-main.tt
Comment 2 Julian Maurice 2020-09-04 15:04:07 UTC
Test plan:
1. Apply patch
2. cd misc/translator && ./translate update fr-FR
3. Open and translate misc/translator/po/fr-FR-messages.po (don't forget plural forms)
4. cd misc/translator && ./translate install fr-FR
5. Go to http://OPAC/cgi-bin/koha/opac-main.pl and try to make all translated strings to show up (log in, log out, create news, do some checkouts, holds, ...)
Comment 3 Owen Leonard 2020-09-11 13:19:20 UTC
This looks good overall but I see one issue:

If I have the NewsAuthorDisplay set to "none" it looks like the template is still trying to show an author. I see something like this on each news item:

Published on 10.03.2019 by

I think the "author" variable is getting evaluated as true even if the [% IF %] block inside it evaluates as false. I think the "author" BLOCK belongs inside the "[% IF ( (newsdisp == 'opac'..."
Comment 4 Julian Maurice 2020-09-11 13:31:22 UTC Comment hidden (obsolete)
Comment 5 Julian Maurice 2020-09-11 13:31:27 UTC Comment hidden (obsolete)
Comment 6 Julian Maurice 2020-09-11 13:33:14 UTC
(In reply to Owen Leonard from comment #3)
> This looks good overall but I see one issue:
> 
> If I have the NewsAuthorDisplay set to "none" it looks like the template is
> still trying to show an author. I see something like this on each news item:
> 
> Published on 10.03.2019 by
> 
> I think the "author" variable is getting evaluated as true even if the [% IF
> %] block inside it evaluates as false. I think the "author" BLOCK belongs
> inside the "[% IF ( (newsdisp == 'opac'..."

You're 100% right :) Thanks
Fixed in the last patch.
Comment 7 Owen Leonard 2020-09-11 14:23:22 UTC
Created attachment 109947 [details] [review]
Bug 26392: Wrap all translatable strings in opac-main.tt

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 8 Owen Leonard 2020-09-11 14:23:28 UTC
Created attachment 109948 [details] [review]
Bug 26392: Fix opac news display issue when there is no author

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 9 Katrin Fischer 2020-09-12 22:10:16 UTC
This appears to work well, but it doesn't really make the templates easier to read...

- <li><a href="/cgi-bin/koha/opac-user.pl#opac-user-checkouts"><span class="user_checkouts_count count_label">[% checkouts | html %]</span> checkout(s)</a></li>

Turns into:

+                                        [% count = BLOCK %]<span class="user_checkouts_count count_label">[% checkouts | html %]</span>[% END %]
+                                        <li><a href="/cgi-bin/koha/opac-user.pl#opac-user-checkouts">[% tn('{count} checkout', '{count} checkouts', checkouts) | html | $Expand count = count %]</a></li>

Can you explain a bit how this works? What is the BLOCK and Expand needed for?

We add tons more of | html filters... could this impact performance? I remember when adding the filters in the first place it was an issue. It also adds to the text. I wonder if there would be another way to go about these.

Could it make sense to only use the new syntax when we need singular/pluaral or other more advanced methods?
Comment 10 Julian Maurice 2020-09-13 12:12:38 UTC
(In reply to Katrin Fischer from comment #9)
> This appears to work well, but it doesn't really make the templates easier
> to read...
> 
> - <li><a href="/cgi-bin/koha/opac-user.pl#opac-user-checkouts"><span
> class="user_checkouts_count count_label">[% checkouts | html %]</span>
> checkout(s)</a></li>
> 
> Turns into:
> 
> +                                        [% count = BLOCK %]<span
> class="user_checkouts_count count_label">[% checkouts | html %]</span>[% END
> %]
> +                                        <li><a
> href="/cgi-bin/koha/opac-user.pl#opac-user-checkouts">[% tn('{count}
> checkout', '{count} checkouts', checkouts) | html | $Expand count = count
> %]</a></li>
> 
> Can you explain a bit how this works? What is the BLOCK and Expand needed
> for?
Sure.

[% var = BLOCK %]...[% END %] puts the content of the block inside variable var, so here the 'count' variable will be equal to something like this:

    <span class="user_checkouts_count count_label">42</span>

The expand plugin replace text enclosed with braces by another string passed in parameter

    [% t("Hello {name}") | $Expand name = "World !" %]

Will result in "Hello World !".
It is the same as

    [% tx("Hello {name}", name = "World !") %]

But having this as a separate plugin allows to add other filters in between.

    [% t("Hello {name}") | html | $Expand name = "<strong>World</strong> !" %]

Which is needed if we want to prevent translations to add unwanted html (like a <script> element for instance) and if expanded variables contain html that we do not want to filter, like in the example in the patch.

> We add tons more of | html filters... could this impact performance? I
> remember when adding the filters in the first place it was an issue. It also
> adds to the text. I wonder if there would be another way to go about these.
If I remember correctly, the issue was not about HTML filtering in itself, but the module we used was doing a lot of expensive string concatenation, which was the real performance issue.

> Could it make sense to only use the new syntax when we need singular/pluaral
> or other more advanced methods?
It could.
I agree with you when you say the template is less easy to read, but there is a long term benefit if we manage to translate all the strings that way : it will be no longer needed to copy the templates using `./translate install`