Bug 29602

Summary: We must be nicer with translators
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: TemplatesAssignee: Fridolin Somers <fridolin.somers>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: dcook, fridolin.somers, katrin.fischer, kyle, m.de.rooy, martin.renvoize, tomascohen, victor, ztajoli
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29589
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20988
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30733
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00
Bug Depends on:    
Bug Blocks: 30629    
Attachments: Bug 29602: Surround strings with span tag
Bug 29602: Surround strings with span tag
Bug 29602: Surround strings with span tag
Bug 29602: Surround strings with span tag
Bug 29602: Surround strings with span tag
Bug 29602: (follow-up) Surround strings with span tag
Bug 29602: Surround strings with span tag
Bug 29602: (follow-up) Surround strings with span tag
Bug 29602: (follow-up) Fix TT tags broken due to missing %
Bug 29602: (follow-up) fix crash: SET KohaSpan → USE KohaSpan
Bug 29602: (follow-up) fix duplicate, missing or mismatching <span>
Bug 29602: Surround strings with span tag
Bug 29602: (follow-up) Surround strings with span tag
Bug 29602: (follow-up) Fix TT tags broken due to missing %
Bug 29602: (follow-up) fix crash: SET KohaSpan → USE KohaSpan
Bug 29602: (follow-up) fix duplicate, missing or mismatching <span>
Bug 29602: Surround strings with span tag
Bug 29602: (follow-up) Surround strings with span tag
Bug 29602: (follow-up) Fix TT tags broken due to missing %
Bug 29602: (follow-up) fix crash: SET KohaSpan → USE KohaSpan
Bug 29602: (follow-up) fix duplicate, missing or mismatching <span>
Bug 29602: Surround strings with span tag
Bug 29602: (follow-up) Surround strings with span tag
Bug 29602: (follow-up) Fix TT tags broken due to missing %
Bug 29602: (follow-up) fix crash: SET KohaSpan → USE KohaSpan
Bug 29602: (follow-up) fix duplicate, missing or mismatching <span>
Bug 29602: Surround strings with span tag
Bug 29602: (follow-up) Surround strings with span tag
Bug 29602: (follow-up) Fix TT tags broken due to missing %
Bug 29602: (follow-up) fix crash: SET KohaSpan => USE KohaSpan
Bug 29602: (follow-up) fix duplicate, missing or mismatching <span>
Bug 29602: Surround strings with span tag
Bug 29602: (follow-up) Surround strings with span tag
Bug 29602: (follow-up) Fix TT tags broken due to missing %
Bug 29602: (follow-up) fix crash: SET KohaSpan => USE KohaSpan
Bug 29602: (follow-up) fix duplicate, missing or mismatching <span>
Bug 29602: (follow-up) Fix TemplateToolkit test
Bug 29602: (follow-up) Fix missing filters

Description Jonathan Druart 2021-11-30 15:21:14 UTC
The PO files are really ugly sometimes, the number of %s and the inclusion of TT tags make the translation a nightmare for translators.

We can improve that.
Comment 1 Jonathan Druart 2021-11-30 15:23:02 UTC
Created attachment 128108 [details] [review]
Bug 29602: Surround strings with span tag

To be nicer with translators.

Update the PO files for whichever languages will show how this is
useful.
Comment 2 Jonathan Druart 2021-11-30 15:27:40 UTC
This is a nightmare. For translators (current situation) and for developer (to write the patch).
I think we need the work to be done very quickly to push it very soon this cycle. I can continue the work but I need to get commitment for SO and QA. It must be pushed in the week it has been written, it will be impossible to rebase.
Comment 3 Jonathan Druart 2021-11-30 15:29:14 UTC
An example with "Unknown error":


-#. %1$s:  END 
-#. %2$s:  SWITCH m.error 
-#. %3$s:  CASE 'hold_not_found' 
-#. %4$s:  m.hold_id | html 
-#. %5$s:  CASE 
-#. %6$s:  m.error | html 
-#. %7$s:  END 
-#. %8$s:  CASE 'hold_cancelled' 
-#: koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/batch_hold_cancel.inc:31
-#, c-format
-msgid ""
-"has not been cancelled. %s An error occurred on cancelling. %s %s No hold "
-"with id %s found. %sUnknown error '%s'. %s %s Hold on "
-msgstr ""


-#. %1$s:  FOR e IN log4perl_errors 
-#. %2$s:  SWITCH e 
-#. %3$s:  CASE 'missing_config_entry' 
-#. %4$s:  CASE 'cannot_read_config_file' 
-#. %5$s:  CASE 'logfile_not_writable' 
-#. %6$s:  CASE 'cannot_init_module' 
-#. %7$s:  CASE 
-#. %8$s:  e | html 
-#. %9$s:  END 
-#. %10$s:  END 
-#. %11$s:  END 
-#. %12$s:  IF warnConnectBroker 
-#: koha-tmpl/intranet-tmpl/prog/en/modules/about.tt:281
-#, c-format
-msgid ""
-"%s %s %sThere is no 'log4perl_conf' entry in the config file. %sThe log4perl "
-"config file cannot be opened. %sOne of the logfiles listed in the config "
-"file is not writable. %sThe Koha::Logger module cannot be initiated "
-"correctly (check the log file). %sUnknown error \"%s\" %s %s %s %s "
-msgstr ""
-"%s %s %sIl n'y a pas d'entrée \"log4perl_conf\" dans le fichier de config. "
-"%sImpossible d'ouvrir le fichier de config log4perl. %sUn des fichiers de "
-"log recensé dans le fichier de config n'est pas accessible en écriture. "
-"%sImpossible d'initialiser correctement le module Koha::Logger (vérifier le "
-"fichier de log). %sErreur inconnue \"%s\" %s %s %s %s "



+#. %1$s:  e | html 
+#: koha-tmpl/intranet-tmpl/prog/en/modules/about.tt:288
+#, fuzzy, c-format
+msgid "Unknown error \"%s\""
+msgstr "Erreur inconnue."
+
+#. %1$s:  m.error | html 
+#: koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/batch_hold_cancel.inc:38
+#, fuzzy, c-format
+msgid "Unknown error '%s'."
+msgstr "Erreur inconnue."

Could even be improved as there is a " vs ', but you got the idea.
Comment 4 Jonathan Druart 2021-11-30 15:31:57 UTC
Created attachment 128109 [details] [review]
Bug 29602: Surround strings with span tag

To be nicer with translators.

Update the PO files for whichever languages will show how this is
useful.
Comment 5 Jonathan Druart 2021-11-30 15:32:27 UTC
With this second patch we get:

+#. For the first occurrence,
+#. %1$s:  e | html 
+#: koha-tmpl/intranet-tmpl/prog/en/modules/about.tt:288
+#: koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/batch_hold_cancel.inc:38
+msgid "Unknown error '%s'."
Comment 6 Owen Leonard 2021-11-30 19:32:28 UTC
What about Bug 26392?
Comment 7 Jonathan Druart 2021-12-01 09:02:54 UTC
(In reply to Owen Leonard from comment #6)
> What about Bug 26392?

It's stuck, its author does not reply, and IIRC there were perf issues (I cannot find the numbers however). Here I am suggesting to not modify the current system, only focus on problematic strings (those with several %s, ie. TT tags).

But you are right, it's certainly stupid to do the work twice. But doing it instead of nothing felt better, yesterday.
Comment 8 Julian Maurice 2021-12-01 09:33:46 UTC
(In reply to Jonathan Druart from comment #7)
> (In reply to Owen Leonard from comment #6)
> > What about Bug 26392?
> 
> It's stuck, its author does not reply, and IIRC there were perf issues (I
> cannot find the numbers however). Here I am suggesting to not modify the
> current system, only focus on problematic strings (those with several %s,
> ie. TT tags).

I think it's stuck because no decision was made to follow this path (and it's probably not a good approach to translate all strings in a template at once). But the system works, and is already used in some places.
About the performance concerns, yes, it affects performances but IMO it's negligible compared to the benefits (plural forms, context, named placeholders instead of %s, and more if we handle all translatable strings with it). See https://gitlab.com/jajm/time-i18n (it's not perfect, so take it with a grain of salt - we should probably test with a real world example)
Comment 9 Katrin Fischer 2021-12-01 10:54:44 UTC
I wonder if a total switch is good at this point in time with the concerns given. 

Pootle doesn't like the named placeholders ({}) for example, it gives an error that is very confusing to the translators as it requires manually removing the warning almost every time a file with those strings is touched. 

For context and plurals: This needs to be introduced by us manually in my opionion, where it's needed and makes the most sense. Plural forms also have the problem as mentioned above right now, causing trouble in translation.

So for the moment I think it makes sense to do these on a 'as needed' basis more than doing a total of templates. For a lot of strings, I am not sure if we would win a lot.

The adding of spans is good and bad: By not having the spans we leave strings that appear together in one string. For some things this is nice, but it's also bad because it's a bit hard to maintain at times and a little scary to new translators unaware of the template dynamics.
Comment 10 Jonathan Druart 2021-12-01 11:16:23 UTC
(In reply to Katrin Fischer from comment #9)
> The adding of spans is good and bad: By not having the spans we leave
> strings that appear together in one string. For some things this is nice,
> but it's also bad because it's a bit hard to maintain at times and a little
> scary to new translators unaware of the template dynamics.

Having the spans actually remove strings to translate. The same strings are grouped in a single msgstr and need to be translated only once. Compared to when we have the same string but with different number of %s
Comment 11 Katrin Fischer 2021-12-01 11:22:53 UTC
(In reply to Jonathan Druart from comment #10)
> (In reply to Katrin Fischer from comment #9)
> > The adding of spans is good and bad: By not having the spans we leave
> > strings that appear together in one string. For some things this is nice,
> > but it's also bad because it's a bit hard to maintain at times and a little
> > scary to new translators unaware of the template dynamics.
> 
> Having the spans actually remove strings to translate. The same strings are
> grouped in a single msgstr and need to be translated only once. Compared to
> when we have the same string but with different number of %s

You are right, that is a plus :) I think I am pro span and I didn't check the patch completely yet, but from what I saw they seem to be translatable without the context of the other entries.
Comment 12 Jonathan Druart 2021-12-01 11:41:36 UTC
(In reply to Katrin Fischer from comment #11)
> (In reply to Jonathan Druart from comment #10)
> > (In reply to Katrin Fischer from comment #9)
> > > The adding of spans is good and bad: By not having the spans we leave
> > > strings that appear together in one string. For some things this is nice,
> > > but it's also bad because it's a bit hard to maintain at times and a little
> > > scary to new translators unaware of the template dynamics.
> > 
> > Having the spans actually remove strings to translate. The same strings are
> > grouped in a single msgstr and need to be translated only once. Compared to
> > when we have the same string but with different number of %s
> 
> You are right, that is a plus :) I think I am pro span and I didn't check
> the patch completely yet, but from what I saw they seem to be translatable
> without the context of the other entries.

If you need the context and not the ugly %s then we need the TT plugin.
Comment 13 Katrin Fischer 2021-12-01 11:53:04 UTC
I think your suggestion is ok as is. But I have a feeling we will have to retranslate quite bit - should not stop us from improving tho.
Comment 14 Jonathan Druart 2021-12-01 12:00:55 UTC
Yes, of course, strings will need to be fixed. It should be something for a major version. And that's why I am bringing this topic at the beginning of 21.12 :)
Comment 15 Julian Maurice 2021-12-01 12:58:53 UTC
(In reply to Katrin Fischer from comment #9)
> Pootle doesn't like the named placeholders ({}) for example, it gives an
> error that is very confusing to the translators as it requires manually
> removing the warning almost every time a file with those strings is touched. 

I wasn't aware of this. It's strange, as this syntax is documented in gettext: https://www.gnu.org/software/gettext/manual/html_node/perl_002dformat.html#perl_002dformat
I just tested locally on a fresh install of Pootle, and it seems the warning only appear for strings that have several plural forms AND the {} placeholder is not used in all of them. No warnings for simple (singular) translations or for plural forms that all have the {} placeholder in them. Have you seen the same thing ?

If it's a problem, it can be changed to the usual %1$s, %2$s, ... It's not too late as only a few strings use this.
Comment 16 Victor Grousset/tuxayo 2021-12-02 04:11:43 UTC
Hi, thanks Jonathan for the approach on the SWITCH-es only. It seems to be the biggest pain in our strings.

Would a good solution be to surround same strings as this patch with t() instead of <span> ?

The only worry about progressive use of t() is performance, right?
About the discussion on bug 26392 of t("Koha online catalogue") vs t("opac.homepage.title"), the current approach (1st) doesn't close the door for the 2nd (token based) since the strings would already be wrapped in calls. So it's not closing this door assuming the token based approach is chosen in the end.

So about performance:
Here are results of the benchmarks of bug 15395 on my laptop with a 2012 CPU (i5-3320M @3.3GHz on mono thread loads)

Summary
Average for 10 strings without translation:	0.0001145 seconds
Average for 100 strings without translation:	0.0001114 seconds
Average for 200 strings without translation:	9.82e-05 seconds
Average for 500 strings without translation:	0.0001932 seconds
Average for 10 strings with translation:	0.0014077 seconds
Average for 100 strings with translation:	0.0076227 seconds
Average for 200 strings with translation:	0.0120828 seconds
Average for 500 strings with translation:	0.0306117 seconds

The same PC has a median time of 0.350 sec to generate the untranslated HTML for this page:
/cgi-bin/koha/admin/preferences.pl?tab=circulation
(using browser network devtools)
There are 200 sysprefs shown there. So it's surely one the of the pages with the most translatable strings we have.
(Well, searching "a" in sysprefs yields 740 of them ^^" in more than 0.950 sec)

git grep "circulation.pref#"  misc/translator/po/bg-Cyrl-pref.po | wc -l
=> 633
So it seems we have 633 strings in our circulation sysprefs.
if 500 strings took 0.031 sec in the benchmark
then 633 strings => 0.039 sec
0.039 is 11% of 0.350
So we should expected that using t() on all string on this page should add 11% response time to 0.350 sec.

Would there be another case where the overhead could be the more perceptible than this? (it's not really perceptible)
Is the methodology valid?

If so then we shouldn't expect issues about performance.
Comment 17 Jonathan Druart 2021-12-02 08:56:12 UTC
(In reply to Victor Grousset/tuxayo from comment #16)
> Is the methodology valid?

You need to count how many times you enter t() instead of guessing the number of lines matching a pattern there are in a file.

This discussion should take place on bug 20988.
Comment 18 Jonathan Druart 2021-12-02 08:57:46 UTC
(In reply to Julian Maurice from comment #15)
> I just tested locally on a fresh install of Pootle, and it seems the warning
> only appear for strings that have several plural forms AND the {}
> placeholder is not used in all of them. No warnings for simple (singular)
> translations or for plural forms that all have the {} placeholder in them.
> Have you seen the same thing ?

No, we are seeing it whenever {} is used in the translation (plural form).
Comment 19 Victor Grousset/tuxayo 2021-12-06 01:23:09 UTC
(In reply to Jonathan Druart from comment #17)
> This discussion should take place on bug 20988.

Correct, I moved it and answered in bug 20988 and bug 6392
Comment 20 Jonathan Druart 2021-12-13 15:36:30 UTC
Do we move forward with this patch or abandon it? It's now or never.
Or with bug 20988, maybe one day.
Comment 21 Katrin Fischer 2021-12-13 15:42:12 UTC
I think we should move forward with it.
Comment 22 Fridolin Somers 2021-12-14 02:30:44 UTC
(In reply to Katrin Fischer from comment #21)
> I think we should move forward with it.

+1
I'm sure we already have some code with this technique.

I follow french translation group, sure they will like this.
Comment 23 Fridolin Somers 2021-12-14 22:00:31 UTC
I've done some counting for fr-FR :
Without patch :
 > msggrep -K -e '%s' misc/translator/po/fr-FR-staff-prog.po | grep -c ^msgid
1819
With patch :
 > msggrep -K -e '%s' misc/translator/po/fr-FR-staff-prog.po | grep -c ^msgid
1747
Comment 24 Fridolin Somers 2021-12-14 22:06:00 UTC
Ah some problems :


1)

I get :
Template process failed: file error - parse error - about-team.inc line 3-30: unexpected token (-)
  [% CASE 'manager'               -]<span>Release manager</span>

Should be :
  [% CASE 'manager'               -%]<span>Release manager</span>

2)

I think we must leave koha-tmpl/opac-tmpl/bootstrap/en/modules/text/explodedterms.tt untouched. It is a weird TT usage. 
See :
https://git.koha-community.org/Koha-community/Koha/src/commit/b0767f94d07950e7420deb901b208d131427c535/koha-tmpl/opac-tmpl/bootstrap/en/modules/svc/suggestion.tt#L46

This should maybe use t() calls.
Comment 25 Fridolin Somers 2021-12-14 22:16:31 UTC
3)

A few typo of span closing tag (uppercase S) :

 > git grep '</Span'
koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/batch_hold_cancel.inc:                <span>Hold on [% biblio_link | $KohaSpan %] for [% patron_link | $KohaSpan %] has successfully been cancelled.</Span>
koha-tmpl/intranet-tmpl/prog/en/includes/mana/mana-report-search-result.inc:                                <span>No results found</Span>
koha-tmpl/intranet-tmpl/prog/en/modules/tools/manage-marc-import.tt:                                            <span>Cleaned</Span>
Comment 26 Fridolin Somers 2021-12-14 22:21:25 UTC
4)
We must not impact koha-tmpl/intranet-tmpl/prog/en/includes/csv_headers/catalogue/itemsearch.tt
Its for the CSV export of items search.

Maybe we should also use t() here ...
Comment 28 Fridolin Somers 2021-12-15 02:19:29 UTC
Very useful for repeated tip text : (closed), (uncertain), ...
Comment 29 Fridolin Somers 2021-12-15 02:28:02 UTC
6)

There is a removal in issues_stats.tt, is this on purpose ?
 - [% loopfilte.filter | html %]

https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128109&action=diff#a/koha-tmpl/intranet-tmpl/prog/en/modules/reports/issues_stats.tt_sec1
Comment 30 Fridolin Somers 2021-12-15 02:32:53 UTC
7)

There is a missing openning <span> :
        [% ELSIF ( message_loo.approver ) %]No match for user ([% message_loo.approver | html %]). FILTER REQUIRES BORROWERNUMBER (not name).</span>

https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128109&action=diff#a/koha-tmpl/intranet-tmpl/prog/en/modules/tags/review.tt_sec1

In fact other lines all have :
  <strong>Error :</strong> ...

Maybe this text can be moved outside of IF loop.
Comment 31 Jonathan Druart 2021-12-15 05:29:04 UTC
Created attachment 128534 [details] [review]
Bug 29602: Surround strings with span tag

To be nicer with translators.

Update the PO files for whichever languages will show how this is
useful.
Comment 32 Jonathan Druart 2021-12-15 05:29:37 UTC
All done, thanks Frido!

(In reply to Fridolin Somers from comment #29)
> 6)
> 
> There is a removal in issues_stats.tt, is this on purpose ?
>  - [% loopfilte.filter | html %]
> 
> https://bugs.koha-community.org/bugzilla3/attachment.
> cgi?id=128109&action=diff#a/koha-tmpl/intranet-tmpl/prog/en/modules/reports/
> issues_stats.tt_sec1

Yes, it's moved up to be part of the string to translate.

(In reply to Fridolin Somers from comment #30)
> 7)
> 
> There is a missing openning <span> :
>         [% ELSIF ( message_loo.approver ) %]No match for user ([%
> message_loo.approver | html %]). FILTER REQUIRES BORROWERNUMBER (not
> name).</span>
> 
> https://bugs.koha-community.org/bugzilla3/attachment.
> cgi?id=128109&action=diff#a/koha-tmpl/intranet-tmpl/prog/en/modules/tags/
> review.tt_sec1
> 
> In fact other lines all have :
>   <strong>Error :</strong> ...
> 
> Maybe this text can be moved outside of IF loop.

I fixed the missing span but prefer to not modify the logic here.
Comment 33 Jonathan Druart 2021-12-15 05:34:06 UTC
Created attachment 128535 [details] [review]
Bug 29602: Surround strings with span tag

To be nicer with translators.

Update the PO files for whichever languages will show how this is
useful.
Comment 34 Fridolin Somers 2021-12-15 06:46:14 UTC
I confirm 1 to 7 problems are solved.

Sorry there are still problems (I've looked at all changes now) :

8) Missing % :
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128535&action=diff#a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-frequencies.tt_sec1

9) Missing closing tag </span>
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128535&action=diff#a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/additional-contents.tt_sec1

10) viewlog.tt :
Why not changing BLOCK translate_log_interface ?

11) opac-authorities.inc in BLOCK authtypelabel :
Don't we have <span> inside <span> ?
Comment 35 Fridolin Somers 2021-12-15 06:58:25 UTC
11)
We should not impact koha-tmpl/intranet-tmpl/prog/en/includes/biblio-title-head.inc, it is called in <title> and must be text only.
Comment 36 Jonathan Druart 2021-12-15 07:45:43 UTC
Created attachment 128542 [details] [review]
Bug 29602: Surround strings with span tag

To be nicer with translators.

Update the PO files for whichever languages will show how this is
useful.
Comment 37 Jonathan Druart 2021-12-15 07:45:59 UTC
(In reply to Fridolin Somers from comment #34)
> I confirm 1 to 7 problems are solved.
> 
> Sorry there are still problems (I've looked at all changes now) :
> 
> 8) Missing % :
> https://bugs.koha-community.org/bugzilla3/attachment.
> cgi?id=128535&action=diff#a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/
> subscription-frequencies.tt_sec1

Fixed

> 9) Missing closing tag </span>
> https://bugs.koha-community.org/bugzilla3/attachment.
> cgi?id=128535&action=diff#a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/
> additional-contents.tt_sec1

Fixed

> 10) viewlog.tt :
> Why not changing BLOCK translate_log_interface ?

Done

> 11) opac-authorities.inc in BLOCK authtypelabel :
> Don't we have <span> inside <span> ?

Is that a problem? Nested spans are valid in HTML5.

(In reply to Fridolin Somers from comment #35)
> 11)
> We should not impact
> koha-tmpl/intranet-tmpl/prog/en/includes/biblio-title-head.inc, it is called
> in <title> and must be text only.

Removed
Comment 38 Fridolin Somers 2021-12-15 07:49:14 UTC
Created attachment 128543 [details] [review]
Bug 29602: (follow-up) Surround strings with span tag

To be nicer with translators.

Update the PO files for whichever languages will show how this is useful.
Comment 39 Fridolin Somers 2021-12-15 07:49:56 UTC
I found some places it will be usful.
Only looked at includes by now.
Comment 40 Fridolin Somers 2021-12-15 07:51:23 UTC
(In reply to Jonathan Druart from comment #37)
> 
> > 11) opac-authorities.inc in BLOCK authtypelabel :
> > Don't we have <span> inside <span> ?
> 
> Is that a problem? Nested spans are valid in HTML5.
> 
Ah OK cool

Thanks for the fixes :D
Comment 41 Victor Grousset/tuxayo 2021-12-22 23:43:20 UTC
(In reply to Jonathan Druart from comment #20)
> Do we move forward with this patch or abandon it? It's now or never.
> Or with bug 20988, maybe one day.

+1 to move forward. Even without t()

Thanks for fixes.

Would the replacing of the <span> be easy latter? Or does that set them in stone because it would be too hard to tell the <span> that have a role in the layout or CSS vs these ones? (that influence our translation system to break down the strings)
Comment 42 Jonathan Druart 2022-01-03 11:41:23 UTC
(In reply to Victor Grousset/tuxayo from comment #41)
> (In reply to Jonathan Druart from comment #20)
> > Do we move forward with this patch or abandon it? It's now or never.
> > Or with bug 20988, maybe one day.
> 
> +1 to move forward. Even without t()
> 
> Thanks for fixes.
> 
> Would the replacing of the <span> be easy latter? Or does that set them in
> stone because it would be too hard to tell the <span> that have a role in
> the layout or CSS vs these ones? (that influence our translation system to
> break down the strings)

We can still keep the spans and use t(). Why would that be a problem?
Comment 43 Victor Grousset/tuxayo 2022-01-05 19:33:41 UTC
It would add some clutter in the templates. Would adding a class to the <span> that tells it's semantic be worth it?

In case it's not, no big deal the patch can be tested as it.
Comment 44 Victor Grousset/tuxayo 2022-02-05 23:56:43 UTC
Various pages broken due to TT tags broken due to missing %
Comment 45 Victor Grousset/tuxayo 2022-02-05 23:57:00 UTC
Created attachment 130196 [details] [review]
Bug 29602: Surround strings with span tag

To be nicer with translators.

Update the PO files for whichever languages will show how this is
useful.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 46 Victor Grousset/tuxayo 2022-02-05 23:57:05 UTC
Created attachment 130197 [details] [review]
Bug 29602: (follow-up) Surround strings with span tag

To be nicer with translators.

Update the PO files for whichever languages will show how this is useful.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 47 Victor Grousset/tuxayo 2022-02-05 23:57:10 UTC
Created attachment 130198 [details] [review]
Bug 29602: (follow-up) Fix TT tags broken due to missing %
Comment 48 Victor Grousset/tuxayo 2022-02-05 23:57:15 UTC
Created attachment 130199 [details] [review]
Bug 29602: (follow-up) fix crash: SET KohaSpan → USE KohaSpan
Comment 49 Victor Grousset/tuxayo 2022-02-05 23:57:20 UTC
Created attachment 130200 [details] [review]
Bug 29602: (follow-up) fix duplicate, missing or mismatching <span>
Comment 50 Victor Grousset/tuxayo 2022-02-06 00:04:32 UTC
Patches rebased.

And followups added that fix various issues.
After rereading carefully the diff (`git diff --color-words=.` was very useful!) and using some greps,
rg --glob '{*.tt,*.inc}' '\-]'
rg --glob '{*.tt,*.inc}' ' ]<span'
There shouldn't be any crash.

Pages that had the issues or more complex changes (so not all) were tested for loading without error:
- staff Patrons → Manual invoice 
- OPAC → own list of invoices/charges
- circulation → check in
- staff → catalogue → record details
- OPAC → catalogue → record details
- CSV profiles
- Patron card creator → Batches
- Tools → Label creator → Batches
- Tools → Label creator → Layouts → Edit a label layout
- reports → Dictionary
- reports → circulation
- basket details
- Administration Record → matching rules
- batch authority record modification → details of job
- batch biblio record modification → details of job
- batch hold cancel → details of job
- Batch patron modification
- staff → Lists → Your lists → filter by "sorted by"
- OPAC → auth search results
Comment 51 Victor Grousset/tuxayo 2022-02-06 00:07:16 UTC
If someone proofreads my follow-ups and retests some of the pages then we should be good to go.
Comment 52 Victor Grousset/tuxayo 2022-02-06 22:11:10 UTC
Updating the .po files on master and then updating with the patches allows to see that it cleans up a lot of stuff <3

It seems the net change is +400 strings but the ~200 removed are generally terrible and long and the ~600 are generally simple. The most weird stuff I've found in a few minutes of skimming are like:

msgid "\", \"dt_count\": \"%s\", \"dt_is_shared\": \"%s"

It looks like the extraction script that takes things that it shouldn't but at least these don't have work to do besides copy and paste.

Counting:
git diff | grep "\-msgid" | wc -l
218
git diff | grep "\+msgid" | wc -l
647
Comment 53 Katrin Fischer 2022-02-26 22:04:33 UTC
Sadly these no longer apply and I am not sure about some of the changes. Victor or Jonathan, could you rebase maybe?
Comment 54 Victor Grousset/tuxayo 2022-03-06 23:19:19 UTC
Created attachment 131421 [details] [review]
Bug 29602: Surround strings with span tag

To be nicer with translators.

Update the PO files for whichever languages will show how this is
useful.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 55 Victor Grousset/tuxayo 2022-03-06 23:19:24 UTC
Created attachment 131422 [details] [review]
Bug 29602: (follow-up) Surround strings with span tag

To be nicer with translators.

Update the PO files for whichever languages will show how this is useful.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 56 Victor Grousset/tuxayo 2022-03-06 23:19:30 UTC
Created attachment 131423 [details] [review]
Bug 29602: (follow-up) Fix TT tags broken due to missing %
Comment 57 Victor Grousset/tuxayo 2022-03-06 23:19:35 UTC
Created attachment 131424 [details] [review]
Bug 29602: (follow-up) fix crash: SET KohaSpan → USE KohaSpan
Comment 58 Victor Grousset/tuxayo 2022-03-06 23:19:40 UTC
Created attachment 131425 [details] [review]
Bug 29602: (follow-up) fix duplicate, missing or mismatching <span>
Comment 59 Victor Grousset/tuxayo 2022-03-06 23:31:21 UTC
Rebased, and diffed the current and previous version to confirm that the conflict resolutions didn't changed anything.
Comment 60 Jonathan Druart 2022-03-07 08:32:48 UTC
I am out of this one.
Comment 61 Fridolin Somers 2022-03-07 19:50:32 UTC
I'm happy to take care of it
Comment 62 Victor Grousset/tuxayo 2022-03-19 16:08:36 UTC
Created attachment 131934 [details] [review]
Bug 29602: Surround strings with span tag

To be nicer with translators.

Update the PO files for whichever languages will show how this is
useful.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 63 Victor Grousset/tuxayo 2022-03-19 16:08:41 UTC
Created attachment 131935 [details] [review]
Bug 29602: (follow-up) Surround strings with span tag

To be nicer with translators.

Update the PO files for whichever languages will show how this is useful.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 64 Victor Grousset/tuxayo 2022-03-19 16:08:46 UTC
Created attachment 131936 [details] [review]
Bug 29602: (follow-up) Fix TT tags broken due to missing %
Comment 65 Victor Grousset/tuxayo 2022-03-19 16:08:51 UTC
Created attachment 131937 [details] [review]
Bug 29602: (follow-up) fix crash: SET KohaSpan → USE KohaSpan
Comment 66 Victor Grousset/tuxayo 2022-03-19 16:08:55 UTC
Created attachment 131938 [details] [review]
Bug 29602: (follow-up) fix duplicate, missing or mismatching <span>
Comment 67 Victor Grousset/tuxayo 2022-03-19 16:28:52 UTC
Rebased
Comment 68 Victor Grousset/tuxayo 2022-04-10 02:45:12 UTC
Created attachment 133129 [details] [review]
Bug 29602: Surround strings with span tag

To be nicer with translators.

Update the PO files for whichever languages will show how this is
useful.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 69 Victor Grousset/tuxayo 2022-04-10 02:45:18 UTC
Created attachment 133130 [details] [review]
Bug 29602: (follow-up) Surround strings with span tag

To be nicer with translators.

Update the PO files for whichever languages will show how this is useful.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 70 Victor Grousset/tuxayo 2022-04-10 02:45:24 UTC
Created attachment 133131 [details] [review]
Bug 29602: (follow-up) Fix TT tags broken due to missing %
Comment 71 Victor Grousset/tuxayo 2022-04-10 02:45:29 UTC
Created attachment 133132 [details] [review]
Bug 29602: (follow-up) fix crash: SET KohaSpan → USE KohaSpan
Comment 72 Victor Grousset/tuxayo 2022-04-10 02:45:34 UTC
Created attachment 133133 [details] [review]
Bug 29602: (follow-up) fix duplicate, missing or mismatching <span>
Comment 73 Victor Grousset/tuxayo 2022-04-10 02:47:22 UTC
Comment on attachment 131937 [details] [review]
Bug 29602: (follow-up) fix crash: SET KohaSpan → USE KohaSpan

Oops, git bz doesn't support "→" and fails to mark the old patch as obsolete.

/usr/local/bin/git-bz:1993: UnicodeWarning: Unicode equal comparison failed to convert both arguments to Unicode - interpreting them as being unequal
  if attachment.description == commit.subject:
Comment 74 Victor Grousset/tuxayo 2022-04-10 02:47:49 UTC
Rebased
Comment 75 Zeno Tajoli 2022-04-16 11:08:19 UTC
Hi Victor, I'm sorry that there are still problem on check this patch.
1)There are problems on try it on sandboxes, see https://sandbox.bywatersolutions.com/provision_log/nicetrad
2)I try to check it 'manualy' but it needs rebase.
Git log:
koha@deb10:~/kohaclone$ git checkout -b qa_bug_29602
Switched to a new branch 'qa_bug_29602'
koha@deb10:~/kohaclone$ git am -3 -i -u /home/koha/b29602_1
Commit Body is:
--------------------------
Bug 29602: Surround strings with span tag

To be nicer with translators.

Update the PO files for whichever languages will show how this is
useful.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
--------------------------
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: y
Applying: Bug 29602: Surround strings with span tag
Using index info to reconstruct a base tree...
M       koha-tmpl/intranet-tmpl/prog/en/modules/about.tt
M       koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt
M       koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt
M       koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqbudgetperiods.tt
M       koha-tmpl/intranet-tmpl/prog/en/modules/admin/marctagstructure.tt
M       koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences.tt
M       koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbooks.tt
M       koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbooks.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/marctagstructure.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqbudgetperiods.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/about.tt
error: Failed to merge in the changes.
Patch failed at 0001 Bug 29602: Surround strings with span tag
Comment 76 Victor Grousset/tuxayo 2022-04-17 19:00:23 UTC
Created attachment 133370 [details] [review]
Bug 29602: Surround strings with span tag

To be nicer with translators.

Update the PO files for whichever languages will show how this is
useful.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 77 Victor Grousset/tuxayo 2022-04-17 19:00:30 UTC
Created attachment 133371 [details] [review]
Bug 29602: (follow-up) Surround strings with span tag

To be nicer with translators.

Update the PO files for whichever languages will show how this is useful.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 78 Victor Grousset/tuxayo 2022-04-17 19:00:36 UTC
Created attachment 133372 [details] [review]
Bug 29602: (follow-up) Fix TT tags broken due to missing %
Comment 79 Victor Grousset/tuxayo 2022-04-17 19:00:42 UTC
Created attachment 133373 [details] [review]
Bug 29602: (follow-up) fix crash: SET KohaSpan => USE KohaSpan
Comment 80 Victor Grousset/tuxayo 2022-04-17 19:00:47 UTC
Created attachment 133374 [details] [review]
Bug 29602: (follow-up) fix duplicate, missing or mismatching <span>
Comment 81 Victor Grousset/tuxayo 2022-04-17 19:04:06 UTC
Hi Zeno, this patch needs frequent rebases to fix conflicts because it touches stuff in a lot of places. Here is another rebase, thanks for testing :)
Comment 82 Martin Renvoize 2022-04-19 07:20:07 UTC
Created attachment 133381 [details] [review]
Bug 29602: Surround strings with span tag

To be nicer with translators.

Update the PO files for whichever languages will show how this is
useful.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 83 Martin Renvoize 2022-04-19 07:20:12 UTC
Created attachment 133382 [details] [review]
Bug 29602: (follow-up) Surround strings with span tag

To be nicer with translators.

Update the PO files for whichever languages will show how this is useful.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 84 Martin Renvoize 2022-04-19 07:20:17 UTC
Created attachment 133383 [details] [review]
Bug 29602: (follow-up) Fix TT tags broken due to missing %

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 85 Martin Renvoize 2022-04-19 07:20:23 UTC
Created attachment 133384 [details] [review]
Bug 29602: (follow-up) fix crash: SET KohaSpan => USE KohaSpan

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 86 Martin Renvoize 2022-04-19 07:20:28 UTC
Created attachment 133385 [details] [review]
Bug 29602: (follow-up) fix duplicate, missing or mismatching <span>

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 87 Martin Renvoize 2022-04-19 07:21:27 UTC
All looking good here from my perspective. I'm not an active translator, but this appears to improve things from my untrained eye.

Signing off
Comment 88 Victor Grousset/tuxayo 2022-04-19 18:06:09 UTC
After having reread a lot the code and tested a lot of the pages, this look pretty safe and a very welcome change.
Passing QA :)
Comment 89 Victor Grousset/tuxayo 2022-04-19 18:15:49 UTC
The QA script complained about some existing missing filters. It's not from the patch, but are they worth a follow up here or should it be a separate ticket? This one isn't meant to be backported anyway.

 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/batch_authority_record_modification.inc
   FAIL	  filters
		missing_filter at line 32 (                <span>Authority record [% authority_link | $KohaSpan %] has not been modified. An error occurred on modifying it</span>)
		missing_filter at line 35 (                <span>Authority record [% authority_link | $KohaSpan %] has successfully been modified.</span>)

---

 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/batch_biblio_record_modification.inc
   FAIL	  filters
		missing_filter at line 43 (                <span>Bibliographic record [% biblio_link %] has not been modified. An error occurred on modifying it.</span>)
		missing_filter at line 46 (                <span>Bibliographic record [% biblio_link %] has successfully been modified.</span>)

---

 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/batch_hold_cancel.inc
   FAIL	  filters
		missing_filter at line 43 (                <span>Hold on [% biblio_link | $KohaSpan %] for [% patron_link | $KohaSpan %] has successfully been cancelled.</span>)
		missing_filter at line 43 (                <span>Hold on [% biblio_link | $KohaSpan %] for [% patron_link | $KohaSpan %] has successfully been cancelled.</span>)

---

 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/batch_item_record_deletion.inc
   FAIL	  filters
		missing_filter at line 44 (                    <span>Item with barcode [% item_link | $KohaSpan%] cannot be deleted:</span>)
Comment 90 Zeno Tajoli 2022-04-19 20:32:12 UTC
Hi Victor Grousset, I test the patch on sanbox and on my VM.
I confirm that it works and there are not problems on templates. For me OK on sign-off.
I see that  the patch is on 'Passed QA' so I don't re-upload patches with my sign-off.
Comment 91 Fridolin Somers 2022-04-20 07:23:52 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 92 Martin Renvoize 2022-04-20 17:45:27 UTC
Created attachment 133495 [details] [review]
Bug 29602: (follow-up) Fix TemplateToolkit test

This patch fixes the t/db_dependent/Letters/TemplateToolkit.t by adding
the nested `<span>` into the expected output string to reflect the
change added to the primary include.
Comment 93 Martin Renvoize 2022-04-20 17:46:26 UTC
Final follow-up fixes one of the failing unit tests on master.
Comment 94 Victor Grousset/tuxayo 2022-04-20 20:11:57 UTC
Thanks Zeno for another test, more eyes are always welcome.
Comment 95 Victor Grousset/tuxayo 2022-04-20 20:40:46 UTC
(In reply to Victor Grousset/tuxayo from comment #89)
> The QA script complained about some existing missing filters. It's not from
> the patch

Actually they are new from the patch. And find-missing-filters.t is failing because of that. It's when changing the way of displaying the item link using KohaSpan.

from:

Item with barcode <a href="/cgi-bin/koha/catalogue/moredetail.pl?biblionumber=[% m.biblionumber | uri %]&itemnumber=[% m.itemnumber | uri %]">[% m.barcode | html %]</a> cannot be deleted:

to:

[% item_link = BLOCK %]<a href="/cgi-bin/koha/catalogue/moredetail.pl?biblionumber=[% m.biblionumber | uri %]&itemnumber=[% m.itemnumber | uri %]">[% m.barcode | html %]</a>[% END %]

<span>Item with barcode [% item_link | $KohaSpan%] cannot be deleted:</span>

--------

Actually it's a false positive because when assigning item_link, variables already go though a filter. Anyway to make the check happy which filter to use? raw?
Comment 96 Fridolin Somers 2022-04-21 06:50:56 UTC
Comment on attachment 133495 [details] [review]
Bug 29602: (follow-up) Fix TemplateToolkit test

Thanks for the followup.

I had already fixed.
I prefer to edit the include to avoid adding surrounding span :

https://git.koha-community.org/Koha-community/Koha/commit/f55b16145c96167afc63784447cdb8420d49dbae
Comment 97 Martin Renvoize 2022-04-21 06:55:54 UTC
Created attachment 133521 [details] [review]
Bug 29602: (follow-up) Fix missing filters

$KohaSpan is not seen as a filter by xt/find-missing-filters.t and thus
we need to filter the block variables before passing them to $KohaSpan

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 98 Martin Renvoize 2022-04-21 07:15:23 UTC
You also fixed that one, thanks