Bug 25067 - Move PO file manipulation code into gulp tasks
Summary: Move PO file manipulation code into gulp tasks
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Keywords: dependency
Depends on:
Blocks:
 
Reported: 2020-04-06 06:44 UTC by Julian Maurice
Modified: 2020-07-12 18:44 UTC (History)
3 users (show)

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


Attachments
Bug 25067: Move PO file manipulation code into gulp tasks (96.58 KB, patch)
2020-04-06 06:45 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25067: Add missing globs in gulpfile for po:extract (1.29 KB, patch)
2020-04-21 15:12 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25067: Move PO file manipulation code into gulp tasks (96.69 KB, patch)
2020-04-21 18:51 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 25067: Add missing globs in gulpfile for po:extract (1.36 KB, patch)
2020-04-21 18:51 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 25067: Add translate create|update commands that call gulp (1.14 KB, patch)
2020-04-22 07:36 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25067: Add license notice to xgettext* scripts (4.26 KB, patch)
2020-04-23 10:27 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25067: Hide deprecation notice unless --verbose flag is given (1.07 KB, patch)
2020-04-23 10:27 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25067: Move PO file manipulation code into gulp tasks (96.69 KB, patch)
2020-04-23 10:41 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 25067: Add missing globs in gulpfile for po:extract (1.36 KB, patch)
2020-04-23 10:41 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 25067: Add translate create|update commands that call gulp (1.20 KB, patch)
2020-04-23 10:41 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 25067: Add license notice to xgettext* scripts (4.33 KB, patch)
2020-04-23 10:41 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 25067: Hide deprecation notice unless --verbose flag is given (1.13 KB, patch)
2020-04-23 10:41 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 25067: Move PO file manipulation code into gulp tasks (96.50 KB, patch)
2020-07-03 13:16 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25067: Add missing globs in gulpfile for po:extract (1.36 KB, patch)
2020-07-03 13:16 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25067: Add translate create|update commands that call gulp (1.20 KB, patch)
2020-07-03 13:16 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25067: Add license notice to xgettext* scripts (4.33 KB, patch)
2020-07-03 13:16 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25067: Hide deprecation notice unless --verbose flag is given (1.13 KB, patch)
2020-07-03 13:16 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25067: Do not set msgctxt in pref PO files (2.79 KB, patch)
2020-07-12 17:05 UTC, Julian Maurice
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-04-06 06:44:42 UTC

    
Comment 1 Julian Maurice 2020-04-06 06:45:31 UTC
Created attachment 102432 [details] [review]
Bug 25067: Move PO file manipulation code into gulp tasks

misc/translator/translate was doing three different things:
- extract translatable strings
- create or update PO files
- install translated templates

This patch separates responsibilities by moving the string extraction
code into several 'xgettext-like' scripts and adds gulp tasks to
automate string extraction and PO files update

This has several benefits:

- gulp runs tasks in parallel, so it's a lot faster (updating all PO
  files is at least 10 times faster with my 4-cores CPU)

- there is no need for $KOHA_CONF to be defined
  LangInstaller.pm relied on $KOHA_CONF to get the different paths
  needed. I'm not sure why, since string extraction and PO update should
  work on source files, not installed files

- string extraction code can be more easily tested

This patch also brings a couple of fixes and improvements:

- TT string extraction (strings wrapped in [% t(...) %]) was done with
  Template::Parser and PPI, which was extremely slow, and had some
  problems (see bug 24797).
  This is now done with Locale::XGettext::TT2 (new dependency) which is
  a lot faster, and fixes bug 24797

- string extraction for system preferences was putting context inside
  msgid. For instance:

    # Accounting > Policy
    msgid "accounting.pref#FinePaymentAutoPopup# automatically display "
    "a print dialog for a payment receipt when making a payment.."

  Now context is put into msgctxt, and the reference is set, which is
  cleaner

    #: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/accounting.pref
    msgctxt "Accounting > Policy > FinePaymentAutoPopup"
    msgid "automatically display a print dialog for a payment receipt "
    "when making a payment.."

  The downside is that some messages will have to be re-translated,
  especially short messages like 'Do', for which msgmerge has a hard
  time finding the corresponding new msgid.

- Replace deprecated gulp-util by recommended minimist

- Fix header in 3 PO files

Test plan:
1. Run `yarn install` and install Locale::XGettext::TT2
2. Run `gulp po:update`
3. Verify the contents of updated PO files
4. Run `cd misc/translator && ./translate install <lang>`
5. Verify that all (templates, sysprefs, xslt, installer files) is
   correctly translated
6. Run `gulp po:create --lang <lang>` and verify that it created all PO
   files for that language
7. Run `prove t/misc/translator`
Comment 2 Katrin Fischer 2020-04-06 06:56:00 UTC
I wonder about the pref file - the # comment kept all strings for one pref together and we have translated "Do" and "Don't" quite differently for some of them, according to context as it works not really well to put the Verb in front of the sentence in German like that. So it appearing more than once was a feature for us. Can we keep that?
Comment 3 Julian Maurice 2020-04-06 07:05:13 UTC
(In reply to Katrin Fischer from comment #2)
> I wonder about the pref file - the # comment kept all strings for one pref
> together and we have translated "Do" and "Don't" quite differently for some
> of them, according to context as it works not really well to put the Verb in
> front of the sentence in German like that. So it appearing more than once
> was a feature for us. Can we keep that?

That feature is kept. msgids will be identical ("Do") but msgctxt won't ("Section > Subsection > Syspref"), allowing translators to give different translations depending on the context.
Comment 4 Katrin Fischer 2020-04-06 20:11:13 UTC
Thx Julian.
Comment 5 Bernardo Gonzalez Kriegel 2020-04-21 14:36:54 UTC
Hi Julian, nice work.

I did a comparison of the msgids on each PO file before and after patch.
pref file needed some touching, I removed the text that now is excluded using
's/\".*#\s/\"/;s/\".*\.pref\s/\"/;'

Comparing only msgid, then doing a diff and counting different lines before (old) and after (new) I got (filename old new)

xx-YY-installer-MARC21.po 0 0
xx-YY-installer.po 0 0
xx-YY-marc-MARC21.po 0 2  *
xx-YY-marc-NORMARC.po 0 0
xx-YY-marc-UNIMARC.po 0 0
xx-YY-messages-js.po 0 0
xx-YY-messages.po 0 2 *
xx-YY-opac-bootstrap.po 4 0 +
xx-YY-pref.po 19 17 #
xx-YY-staff-prog.po 49 0 +

There are a few differences :)
Lines with (*) are new strings, great
Lines with (#), preference file, most differences expected here, most can be ignored
Lines with (+), missing strings after patch

On opac
< msgid "000 "
< msgid "LCCN:"
< msgid "MARC Card View"
< msgid "MARC View"

examples on staff
< msgid "Gone no address flag"
< msgid "Illustrations"
< msgid "Item number"
< msgid "Item number (internal)"
< msgid "Koha itemtype"
< msgid "Koha normalized classification for sorting"
< msgid "Lost card flag"
< msgid "MARC Card View"
< msgid "MARC View"
Comment 6 Julian Maurice 2020-04-21 14:56:29 UTC
Thanks Bernardo for this detailed analysis. It looks like I forgot some files in the gulp globs. In particular columns.def and the XSL files that are not related to a specific marc flavour.
I will try to fix that ASAP.
Comment 7 Julian Maurice 2020-04-21 15:12:31 UTC
Created attachment 103383 [details] [review]
Bug 25067: Add missing globs in gulpfile for po:extract
Comment 8 Julian Maurice 2020-04-21 15:14:59 UTC
With the last patch I get the same count of msgid than on master for opac-bootstrap and staff-prog
Comment 9 Bernardo Gonzalez Kriegel 2020-04-21 16:30:17 UTC
(In reply to Julian Maurice from comment #8)
> With the last patch I get the same count of msgid than on master for
> opac-bootstrap and staff-prog

Yes, they have the same content :)

Last question: some may be used to the old syntax, doing ./translate create|update on misc/translator/, they need to use it anyway to do the install

What do you think of executing 'gulp' from translate script?
and perhaps using "--silent" to suppress messages unless verbose mode is requested?
Comment 10 Bernardo Gonzalez Kriegel 2020-04-21 18:51:20 UTC
Created attachment 103403 [details] [review]
Bug 25067: Move PO file manipulation code into gulp tasks

misc/translator/translate was doing three different things:
- extract translatable strings
- create or update PO files
- install translated templates

This patch separates responsibilities by moving the string extraction
code into several 'xgettext-like' scripts and adds gulp tasks to
automate string extraction and PO files update

This has several benefits:

- gulp runs tasks in parallel, so it's a lot faster (updating all PO
  files is at least 10 times faster with my 4-cores CPU)

- there is no need for $KOHA_CONF to be defined
  LangInstaller.pm relied on $KOHA_CONF to get the different paths
  needed. I'm not sure why, since string extraction and PO update should
  work on source files, not installed files

- string extraction code can be more easily tested

This patch also brings a couple of fixes and improvements:

- TT string extraction (strings wrapped in [% t(...) %]) was done with
  Template::Parser and PPI, which was extremely slow, and had some
  problems (see bug 24797).
  This is now done with Locale::XGettext::TT2 (new dependency) which is
  a lot faster, and fixes bug 24797

- string extraction for system preferences was putting context inside
  msgid. For instance:

    # Accounting > Policy
    msgid "accounting.pref#FinePaymentAutoPopup# automatically display "
    "a print dialog for a payment receipt when making a payment.."

  Now context is put into msgctxt, and the reference is set, which is
  cleaner

    #: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/accounting.pref
    msgctxt "Accounting > Policy > FinePaymentAutoPopup"
    msgid "automatically display a print dialog for a payment receipt "
    "when making a payment.."

  The downside is that some messages will have to be re-translated,
  especially short messages like 'Do', for which msgmerge has a hard
  time finding the corresponding new msgid.

- Replace deprecated gulp-util by recommended minimist

- Fix header in 3 PO files

Test plan:
1. Run `yarn install` and install Locale::XGettext::TT2
2. Run `gulp po:update`
3. Verify the contents of updated PO files
4. Run `cd misc/translator && ./translate install <lang>`
5. Verify that all (templates, sysprefs, xslt, installer files) is
   correctly translated
6. Run `gulp po:create --lang <lang>` and verify that it created all PO
   files for that language
7. Run `prove t/misc/translator`

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Need to install yarn & gulp, no errors
Comment 11 Bernardo Gonzalez Kriegel 2020-04-21 18:51:25 UTC
Created attachment 103404 [details] [review]
Bug 25067: Add missing globs in gulpfile for po:extract

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 12 Julian Maurice 2020-04-22 06:01:26 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #9)
> (In reply to Julian Maurice from comment #8)
> > With the last patch I get the same count of msgid than on master for
> > opac-bootstrap and staff-prog
> 
> Yes, they have the same content :)
> 
> Last question: some may be used to the old syntax, doing ./translate
> create|update on misc/translator/, they need to use it anyway to do the
> install
> 
> What do you think of executing 'gulp' from translate script?
> and perhaps using "--silent" to suppress messages unless verbose mode is
> requested?

I think it is not needed, but it can't hurt to have it. Maybe QA has an opinion on that ?
Comment 13 Katrin Fischer 2020-04-22 06:36:53 UTC
Hi Julian and Bernardo, happy to see the thorough testing of this change. 

If we could make koha-translate and the translate script behave the same with this change than before, I'd be in favor of it.
Comment 14 Julian Maurice 2020-04-22 07:11:51 UTC
It seems that koha-translate only uses the 'install' mode of misc/translator/translate, so its behavior should not change.
Comment 15 Julian Maurice 2020-04-22 07:36:08 UTC
Created attachment 103440 [details] [review]
Bug 25067: Add translate create|update commands that call gulp
Comment 16 Bernardo Gonzalez Kriegel 2020-04-22 10:47:49 UTC
(In reply to Julian Maurice from comment #15)
> Created attachment 103440 [details] [review] [review]
> Bug 25067: Add translate create|update commands that call gulp

Sorry to bother you with this, but can we left the deprecation notice for verbose mode? 
I would like the 'same' behavior as before
(It's my liking of course, and you can ignore it. I'll sign anyway)

Finally, files gulpfile.js, xgettext-installer, xgettext-pref and xgettext-tt2 need a copyright notice.
Comment 17 Julian Maurice 2020-04-22 11:06:04 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #16)
> Sorry to bother you with this, but can we left the deprecation notice for
> verbose mode? 
> I would like the 'same' behavior as before
> (It's my liking of course, and you can ignore it. I'll sign anyway)

In my opinion, people should be warned even if they don't want to, and they can use gulp directly if they don't want to see this warning, right ? :)
But in the end, you are probably the only one directly concerned by this change, so if you think it's better this way, I'll make those changes ASAP
Comment 18 Julian Maurice 2020-04-23 10:27:19 UTC
Created attachment 103557 [details] [review]
Bug 25067: Add license notice to xgettext* scripts
Comment 19 Julian Maurice 2020-04-23 10:27:22 UTC
Created attachment 103558 [details] [review]
Bug 25067: Hide deprecation notice unless --verbose flag is given
Comment 20 Bernardo Gonzalez Kriegel 2020-04-23 10:41:45 UTC
Created attachment 103562 [details] [review]
Bug 25067: Move PO file manipulation code into gulp tasks

misc/translator/translate was doing three different things:
- extract translatable strings
- create or update PO files
- install translated templates

This patch separates responsibilities by moving the string extraction
code into several 'xgettext-like' scripts and adds gulp tasks to
automate string extraction and PO files update

This has several benefits:

- gulp runs tasks in parallel, so it's a lot faster (updating all PO
  files is at least 10 times faster with my 4-cores CPU)

- there is no need for $KOHA_CONF to be defined
  LangInstaller.pm relied on $KOHA_CONF to get the different paths
  needed. I'm not sure why, since string extraction and PO update should
  work on source files, not installed files

- string extraction code can be more easily tested

This patch also brings a couple of fixes and improvements:

- TT string extraction (strings wrapped in [% t(...) %]) was done with
  Template::Parser and PPI, which was extremely slow, and had some
  problems (see bug 24797).
  This is now done with Locale::XGettext::TT2 (new dependency) which is
  a lot faster, and fixes bug 24797

- string extraction for system preferences was putting context inside
  msgid. For instance:

    # Accounting > Policy
    msgid "accounting.pref#FinePaymentAutoPopup# automatically display "
    "a print dialog for a payment receipt when making a payment.."

  Now context is put into msgctxt, and the reference is set, which is
  cleaner

    #: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/accounting.pref
    msgctxt "Accounting > Policy > FinePaymentAutoPopup"
    msgid "automatically display a print dialog for a payment receipt "
    "when making a payment.."

  The downside is that some messages will have to be re-translated,
  especially short messages like 'Do', for which msgmerge has a hard
  time finding the corresponding new msgid.

- Replace deprecated gulp-util by recommended minimist

- Fix header in 3 PO files

Test plan:
1. Run `yarn install` and install Locale::XGettext::TT2
2. Run `gulp po:update`
3. Verify the contents of updated PO files
4. Run `cd misc/translator && ./translate install <lang>`
5. Verify that all (templates, sysprefs, xslt, installer files) is
   correctly translated
6. Run `gulp po:create --lang <lang>` and verify that it created all PO
   files for that language
7. Run `prove t/misc/translator`

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Need to install yarn & gulp, no errors
Comment 21 Bernardo Gonzalez Kriegel 2020-04-23 10:41:48 UTC
Created attachment 103563 [details] [review]
Bug 25067: Add missing globs in gulpfile for po:extract

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 22 Bernardo Gonzalez Kriegel 2020-04-23 10:41:52 UTC
Created attachment 103564 [details] [review]
Bug 25067: Add translate create|update commands that call gulp

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 23 Bernardo Gonzalez Kriegel 2020-04-23 10:41:56 UTC
Created attachment 103565 [details] [review]
Bug 25067: Add license notice to xgettext* scripts

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 24 Bernardo Gonzalez Kriegel 2020-04-23 10:41:59 UTC
Created attachment 103566 [details] [review]
Bug 25067: Hide deprecation notice unless --verbose flag is given

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 25 Bernardo Gonzalez Kriegel 2020-04-23 10:42:45 UTC
All tested and signed!
Comment 26 Julian Maurice 2020-07-03 13:16:12 UTC
Created attachment 106503 [details] [review]
Bug 25067: Move PO file manipulation code into gulp tasks

misc/translator/translate was doing three different things:
- extract translatable strings
- create or update PO files
- install translated templates

This patch separates responsibilities by moving the string extraction
code into several 'xgettext-like' scripts and adds gulp tasks to
automate string extraction and PO files update

This has several benefits:

- gulp runs tasks in parallel, so it's a lot faster (updating all PO
  files is at least 10 times faster with my 4-cores CPU)

- there is no need for $KOHA_CONF to be defined
  LangInstaller.pm relied on $KOHA_CONF to get the different paths
  needed. I'm not sure why, since string extraction and PO update should
  work on source files, not installed files

- string extraction code can be more easily tested

This patch also brings a couple of fixes and improvements:

- TT string extraction (strings wrapped in [% t(...) %]) was done with
  Template::Parser and PPI, which was extremely slow, and had some
  problems (see bug 24797).
  This is now done with Locale::XGettext::TT2 (new dependency) which is
  a lot faster, and fixes bug 24797

- string extraction for system preferences was putting context inside
  msgid. For instance:

    # Accounting > Policy
    msgid "accounting.pref#FinePaymentAutoPopup# automatically display "
    "a print dialog for a payment receipt when making a payment.."

  Now context is put into msgctxt, and the reference is set, which is
  cleaner

    #: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/accounting.pref
    msgctxt "Accounting > Policy > FinePaymentAutoPopup"
    msgid "automatically display a print dialog for a payment receipt "
    "when making a payment.."

  The downside is that some messages will have to be re-translated,
  especially short messages like 'Do', for which msgmerge has a hard
  time finding the corresponding new msgid.

- Replace deprecated gulp-util by recommended minimist

- Fix header in 3 PO files

Test plan:
1. Run `yarn install` and install Locale::XGettext::TT2
2. Run `gulp po:update`
3. Verify the contents of updated PO files
4. Run `cd misc/translator && ./translate install <lang>`
5. Verify that all (templates, sysprefs, xslt, installer files) is
   correctly translated
6. Run `gulp po:create --lang <lang>` and verify that it created all PO
   files for that language
7. Run `prove t/misc/translator`

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Need to install yarn & gulp, no errors
Comment 27 Julian Maurice 2020-07-03 13:16:16 UTC
Created attachment 106504 [details] [review]
Bug 25067: Add missing globs in gulpfile for po:extract

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 28 Julian Maurice 2020-07-03 13:16:19 UTC
Created attachment 106505 [details] [review]
Bug 25067: Add translate create|update commands that call gulp

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 29 Julian Maurice 2020-07-03 13:16:24 UTC
Created attachment 106506 [details] [review]
Bug 25067: Add license notice to xgettext* scripts

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 30 Julian Maurice 2020-07-03 13:16:28 UTC
Created attachment 106507 [details] [review]
Bug 25067: Hide deprecation notice unless --verbose flag is given

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 31 Julian Maurice 2020-07-03 13:18:26 UTC
All patches rebased on master
Comment 32 Alex Arnaud 2020-07-10 14:23:21 UTC
I agree with the fact that the translate script should behave the same way than before. But i think it's not possible.

I think of people that will upgrade with the gulp feature and surprisingly note that the script no longer works for create or update if they didn't install the gulp-cli / yarn command.

My feeling is that we should have the command ./translate create|update telling the user: "Creating or updating po process has changed, please use it the way <here>" 

Bernardo, Katrin any thoughts ?
Comment 33 Alex Arnaud 2020-07-10 14:27:10 UTC
Also, i have concerns about the .pot generated. They are no longer cleaned now.
Do we need these .pot files actually ?
Comment 34 Katrin Fischer 2020-07-12 12:00:49 UTC
I don't know about the .pot files, but was looking at the test plan again (thx for the write-up!):

- Can 'gulp po:update' be run for a selected language? It takes a long while to do this for all languages, so you might want to do it only for the ones you need/test with.

- In the test plan only installing a language is used with the old syntax, creating and updating are different. I think mixing would not be good. Why is it not possible to move all to the old syntax? and should we them move installing as well with the deprecation note you suggest?

You write:
>- there is no need for $KOHA_CONF to be defined
>  LangInstaller.pm relied on $KOHA_CONF to get the different paths
>  needed. I'm not sure why, since string extraction and PO update should
>  work on source files, not installed files

But aren't the paths to the templates we need to parse different depending on installation type? And also the po files will be stored in a different place?

We need to keep the option for everyone to update/install their own adapted po files. I know a lot of people make local changes. Even we do to a certain extend to have our own XSLT files translated.

Still a bit worried about this one:

>  Now context is put into msgctxt, and the reference is set, which is
>  cleaner

>    #: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/accounting.pref
>    msgctxt "Accounting > Policy > FinePaymentAutoPopup"
>    msgid "automatically display a print dialog for a payment receipt "
>    "when making a payment.."

I think in translation practice using Pootle this could be quite hard on tranlators. :(

Right now you can search in the prefs file for a system preference and it will turn up all strings related to it. They also appear in alphabetic order, so when you start translating, you have the 'context' because the strings appear together with the prefixed note. This was intentional and a 'feature'. 

It doesn't seem that we can search by 'context' in Pootle, so seeing all strings for a pref together will no longer be possible.

To explain: Translating prefs is quite hard because you have to make sure things work grammatically with sentence parts torn apart. This depends on language I am sure, but for German it's not an easy task and we can't translate "Do" and "Don't" just like that. Often we need to move parts of the rest of the sentence into the options for things to make more sense.

Also:

>  The downside is that some messages will have to be re-translated,
>  especially short messages like 'Do', for which msgmerge has a hard
>  time finding the corresponding new msgid.

This is quite a burden for translators, escpecially since you have to look up every single one of them to see what the rest of the sentence looks like. Can't we figure them out? We do have the pref and the current translation.
Comment 35 Katrin Fischer 2020-07-12 12:07:51 UTC
> - In the test plan only installing a language is used with the old syntax,
> creating and updating are different. I think mixing would not be good. Why
> is it not possible to move all to the old syntax? and should we them move
> installing as well with the deprecation note you suggest?

Realizing that last bit made no sense:

And should we not move installing to the new syntax as well with a deprecation note like you suggest? 

Overall I think we need to keep things as easy as possible since not only experienced Koha users will be using these commands (update, install, create).
Comment 36 Julian Maurice 2020-07-12 16:29:10 UTC
(In reply to Katrin Fischer from comment #34)
> I don't know about the .pot files, but was looking at the test plan again
> (thx for the write-up!):
> 
> - Can 'gulp po:update' be run for a selected language? It takes a long while
> to do this for all languages, so you might want to do it only for the ones
> you need/test with.
Yep, just use --lang option:

  gulp po:update --lang de-DE

> You write:
> >- there is no need for $KOHA_CONF to be defined
> >  LangInstaller.pm relied on $KOHA_CONF to get the different paths
> >  needed. I'm not sure why, since string extraction and PO update should
> >  work on source files, not installed files
> 
> But aren't the paths to the templates we need to parse different depending
> on installation type? And also the po files will be stored in a different
> place?
I believe create and update operations should work exclusively on source files. I don't see why we would want to work on the copy installed under /usr/share/koha for instance.
Note that 'install' operation still work the same, translated templates are installed in the right place.

> 
> We need to keep the option for everyone to update/install their own adapted
> po files. I know a lot of people make local changes. Even we do to a certain
> extend to have our own XSLT files translated.
We do too, but local translations are handled in separate PO files.

Do you alter the original PO files ? How do you handle Koha upgrades ?

> 
> Still a bit worried about this one:
> 
> >  Now context is put into msgctxt, and the reference is set, which is
> >  cleaner
> 
> >    #: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/accounting.pref
> >    msgctxt "Accounting > Policy > FinePaymentAutoPopup"
> >    msgid "automatically display a print dialog for a payment receipt "
> >    "when making a payment.."
> 
> I think in translation practice using Pootle this could be quite hard on
> tranlators. :(
> 
> Right now you can search in the prefs file for a system preference and it
> will turn up all strings related to it. They also appear in alphabetic
> order, so when you start translating, you have the 'context' because the
> strings appear together with the prefixed note. This was intentional and a
> 'feature'. 
> 
> It doesn't seem that we can search by 'context' in Pootle, so seeing all
> strings for a pref together will no longer be possible.
It seems that Pootle can search by comment. Would that help if the syspref name and category appear in a comment ?

> 
> To explain: Translating prefs is quite hard because you have to make sure
> things work grammatically with sentence parts torn apart. This depends on
> language I am sure, but for German it's not an easy task and we can't
> translate "Do" and "Don't" just like that. Often we need to move parts of
> the rest of the sentence into the options for things to make more sense.
This will still be possible

> 
> Also:
> 
> >  The downside is that some messages will have to be re-translated,
> >  especially short messages like 'Do', for which msgmerge has a hard
> >  time finding the corresponding new msgid.
> 
> This is quite a burden for translators, escpecially since you have to look
> up every single one of them to see what the rest of the sentence looks like.
> Can't we figure them out? We do have the pref and the current translation.
Probably.
We can also easily revert this change from this patchset and address the issue in a separate bug. This bug brings already too many different things, so if it helps I will gladly remove that. I just thought it would be a nice "clean up" to do at the same time.

(In reply to Katrin Fischer from comment #35)
> > - In the test plan only installing a language is used with the old syntax,
> > creating and updating are different. I think mixing would not be good. Why
> > is it not possible to move all to the old syntax? and should we them move
> > installing as well with the deprecation note you suggest?
> 
> Realizing that last bit made no sense:
> 
> And should we not move installing to the new syntax as well with a
> deprecation note like you suggest? 
> 
> Overall I think we need to keep things as easy as possible since not only
> experienced Koha users will be using these commands (update, install,
> create).
In my opinion, install is really different from create/update so it makes sense for it to be in a different script/called in a different way.
Only install should be called by end users (people who manage Koha instances)
create/update is more for Koha translation maintainer (aka Bernardo)
Comment 37 Julian Maurice 2020-07-12 17:05:40 UTC
Created attachment 106807 [details] [review]
Bug 25067: Do not set msgctxt in pref PO files

By having msgctxt set, msgmerge will "lose" some translations, forcing
translators to re-translate them.
Also, Pootle does not offer the ability to search by context, making it
harder to translate all strings related to a single preference.
Comment 38 Julian Maurice 2020-07-12 17:08:22 UTC
Last patch tested with fr-FR PO files : msgmerge correctly keep all translations (even short ones like "Do"). The only difference is that long lines are now wrapped.
Comment 39 Katrin Fischer 2020-07-12 17:38:27 UTC
Hm, I think I don't understand what you are referring to as source files. Will this work for tarball installations, packaging installations and git based installations alike? I believe we don't always have 2 copies of the files? 
I am worried this could break things for some of those?

Adding the info in a comment could help a bit, I tihnk, but it could get quite a lot, maybe just the names of the prefs?

I think it's a wrong assumption that update and create will only be used by our translation manager or developers. I believe libraries have for example changed terms in po files and similar for the normal GUI. 

But I'd be interested in your workflow for separate po for other use cases. :)
Comment 40 Katrin Fischer 2020-07-12 17:38:45 UTC
(In reply to Julian Maurice from comment #38)
> Last patch tested with fr-FR PO files : msgmerge correctly keep all
> translations (even short ones like "Do"). The only difference is that long
> lines are now wrapped.

That's some good news. :)
Comment 41 Julian Maurice 2020-07-12 18:44:58 UTC
(In reply to Katrin Fischer from comment #39)
> Hm, I think I don't understand what you are referring to as source files.
> Will this work for tarball installations, packaging installations and git
> based installations alike? I believe we don't always have 2 copies of the
> files? 
> I am worried this could break things for some of those?
By source files I mean the files that are committed in the git repo. All files that you have directly after a git clone.
IMO create and update should not be run on "installations", but directly on files inside the git repository, independently of any installation.
'create' and 'update' are essentially developer tools to manage PO files within the git repository.

> I think it's a wrong assumption that update and create will only be used by
> our translation manager or developers. I believe libraries have for example
> changed terms in po files and similar for the normal GUI.
I think that we should not support nor encourage that. Even if it's "only" translations, it's still a fork of Koha. Everyone is free to do that, of course, but then they are on their own to maintain their changes.
If translations are wrong, they should be fixed on translate.koha-community.org

> But I'd be interested in your workflow for separate po for other use cases.
> :)
Basically, for each client-specific development that adds new strings, we create a PO file in misc/translator/po/fr-FR-<category>-<ticket-id>.po containing all the new strings and their translation. When we need to update fr-FR templates we call a script that "concatenates" (msgcat) all fr-FR-<category>-*.po and runs misc/translator/translate install