Description
Jonathan Druart
2021-11-30 15:21:14 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. 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. 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. 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. 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'." (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. (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) 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. (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 (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. (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. 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. 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 :) (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. 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. (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. (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). (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 Do we move forward with this patch or abandon it? It's now or never. Or with bug 20988, maybe one day. I think we should move forward with it. (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. 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 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. 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> 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 ... 5) Changes in intranet-main.tt seems an error ? https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128109&action=diff#a/koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt_sec1 Very useful for repeated tip text : (closed), (uncertain), ... 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 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. 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. 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. 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. 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> ? 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. 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. (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 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. I found some places it will be usful. Only looked at includes by now. (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 (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) (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? 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. Various pages broken due to TT tags broken due to missing % 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> 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> Created attachment 130198 [details] [review] Bug 29602: (follow-up) Fix TT tags broken due to missing % Created attachment 130199 [details] [review] Bug 29602: (follow-up) fix crash: SET KohaSpan → USE KohaSpan Created attachment 130200 [details] [review] Bug 29602: (follow-up) fix duplicate, missing or mismatching <span> 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 If someone proofreads my follow-ups and retests some of the pages then we should be good to go. 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 Sadly these no longer apply and I am not sure about some of the changes. Victor or Jonathan, could you rebase maybe? 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> 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> Created attachment 131423 [details] [review] Bug 29602: (follow-up) Fix TT tags broken due to missing % Created attachment 131424 [details] [review] Bug 29602: (follow-up) fix crash: SET KohaSpan → USE KohaSpan Created attachment 131425 [details] [review] Bug 29602: (follow-up) fix duplicate, missing or mismatching <span> Rebased, and diffed the current and previous version to confirm that the conflict resolutions didn't changed anything. I am out of this one. I'm happy to take care of it 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> 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> Created attachment 131936 [details] [review] Bug 29602: (follow-up) Fix TT tags broken due to missing % Created attachment 131937 [details] [review] Bug 29602: (follow-up) fix crash: SET KohaSpan → USE KohaSpan Created attachment 131938 [details] [review] Bug 29602: (follow-up) fix duplicate, missing or mismatching <span> Rebased 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> 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> Created attachment 133131 [details] [review] Bug 29602: (follow-up) Fix TT tags broken due to missing % Created attachment 133132 [details] [review] Bug 29602: (follow-up) fix crash: SET KohaSpan → USE KohaSpan Created attachment 133133 [details] [review] Bug 29602: (follow-up) fix duplicate, missing or mismatching <span> 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: Rebased 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 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> 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> Created attachment 133372 [details] [review] Bug 29602: (follow-up) Fix TT tags broken due to missing % Created attachment 133373 [details] [review] Bug 29602: (follow-up) fix crash: SET KohaSpan => USE KohaSpan Created attachment 133374 [details] [review] Bug 29602: (follow-up) fix duplicate, missing or mismatching <span> 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 :) 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> 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> 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> 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> 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> 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 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 :) 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>) 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. Pushed to master for 22.05, thanks to everybody involved [U+1F984] 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. Final follow-up fixes one of the failing unit tests on master. Thanks Zeno for another test, more eyes are always welcome. (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 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 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> You also fixed that one, thanks |