Created attachment 359641 [details] [review]
Migrate from Intltool to Gettext

The code has also been uploaded in the meson's wip branch: https://git.gnome.org/browse/gnome-control-center/log/?h=wip/inigomartinez/meson

Created attachment 359644 [details] [review]
sharing: add XML declaration to the .policy file

Created attachment 359645 [details] [review]
sound: specify encoding in the XML declaration of gnome-sounds-default.xml

(In reply to Iñigo Martínez from comment #0)
> Although all strings are translated, `gettext` surprisingly uses an
> alternative character representation (&#xXXXX;) for files
> `gnome-sounds-default.xml` and the
> `org.gnome.controlcenter.remote-login-helper.policy`.
> 

Could you check if these patches help with that?

I've just checked it and it's solved now. They are using the same character representation used by the other translated files.

Thank you!

Created attachment 367033 [details] [review]
build: Migrate from Intltool to Gettext

Here goes an update to the `Migrate from intltool to gettext` patch after the recent merge of the meson build port.

Created attachment 367035 [details] [review]
build: Move timezone languages to LINGUAS file

The timezone panel uses translations for the existing timezone names. This was using a set of predefined languages inside the meson's build file.

This set of languages has been moved to the LINGUAS file, (which can also help damned-lies).

Review of attachment 359644 [details] [review]:

Is there a bug filed against gettext to default to UTF-8? If so, please reference it in the commit message with "See http..."

Review of attachment 359645 [details] [review]:

Same as previous patch.

Review of attachment 367033 [details] [review]:

Looks fine otherwise.

::: panels/bluetooth/gnome-bluetooth-panel.desktop.in.in
@@ +14,1 @@
 X-GNOME-Bugzilla-Component=properties

I don't think it needs to be removed, and if it did, it should be in a separate patch.

::: panels/color/gnome-color-panel.desktop.in.in
@@ +14,1 @@
 X-GNOME-Bugzilla-Component=color

And here as well, and probably in a number of other places.

::: panels/keyboard/50-accessibility.xml.in
@@ +1,2 @@
 <?xml version="1.0" encoding="UTF-8"?>
+<KeyListEntries group="system" name="Universal Access" schema="org.gnome.settings-daemon.plugins.media-keys">

Is a rule, its or loc necessary for those types of files?

Review of attachment 367035 [details] [review]:

Sure, pending the previous patch getting merged.

(In reply to Bastien Nocera from comment #10)
> ::: panels/bluetooth/gnome-bluetooth-panel.desktop.in.in
> @@ +14,1 @@
>  X-GNOME-Bugzilla-Component=properties
> 
> I don't think it needs to be removed, and if it did, it should be in a
> separate patch.
> 
> ::: panels/color/gnome-color-panel.desktop.in.in
> @@ +14,1 @@
>  X-GNOME-Bugzilla-Component=color
> 
> And here as well, and probably in a number of other places.

There must be some confusion here. The `X-GNOME-Bugzilla-Component` entries are still in place in the desktop files here. I have also checked the patch and it doesn't touch them.

> ::: panels/keyboard/50-accessibility.xml.in
> @@ +1,2 @@
>  <?xml version="1.0" encoding="UTF-8"?>
> +<KeyListEntries group="system" name="Universal Access"
> schema="org.gnome.settings-daemon.plugins.media-keys">
> 
> Is a rule, its or loc necessary for those types of files?

I don't what should be the proper behaviour here. The build commands from autotools (just before the meson port merge) used `INTLTOOL_XML_NOMERGE_RULE` rule, that applies the following command:

LC_ALL=C /usr/bin/intltool-merge -x -u --no-translations

The command doesn't translate those files. I don't know the reason behing this. However, there are some keyworkds in the file `description` and `name` that are supposed to be translated, even there are translated strings in the language `.po` files.

I wonder if the reason for not translating them was that the `its` and `loc` files was present.

If you think that this should also be translated I can create proper rules to be  applied.

[0] https://git.gnome.org/browse/gnome-control-center/tree/panels/keyboard/Makefile.am?id=ca3779467e075c2b693adf4c6034e5b5e4b9bb6a#n48

Sorry I made some mistakes.

> I don't _know_ what should be the proper behaviour here.

> I don't know the reason behind this.

> I wonder if the reason for not translating them was that the `its` and `loc` files were not present.

(In reply to Iñigo Martínez from comment #12)
> (In reply to Bastien Nocera from comment #10)
> > ::: panels/bluetooth/gnome-bluetooth-panel.desktop.in.in
> > @@ +14,1 @@
> >  X-GNOME-Bugzilla-Component=properties
> > 
> > I don't think it needs to be removed, and if it did, it should be in a
> > separate patch.
> > 
> > ::: panels/color/gnome-color-panel.desktop.in.in
> > @@ +14,1 @@
> >  X-GNOME-Bugzilla-Component=color
> > 
> > And here as well, and probably in a number of other places.
> 
> There must be some confusion here. The `X-GNOME-Bugzilla-Component` entries
> are still in place in the desktop files here. I have also checked the patch
> and it doesn't touch them.

I might have been confused by the review UI then, feel free to ignore.

> > ::: panels/keyboard/50-accessibility.xml.in
> > @@ +1,2 @@
> >  <?xml version="1.0" encoding="UTF-8"?>
> > +<KeyListEntries group="system" name="Universal Access"
> > schema="org.gnome.settings-daemon.plugins.media-keys">
> > 
> > Is a rule, its or loc necessary for those types of files?
> 
> I don't what should be the proper behaviour here. The build commands from
> autotools (just before the meson port merge) used
> `INTLTOOL_XML_NOMERGE_RULE` rule, that applies the following command:
> 
> LC_ALL=C /usr/bin/intltool-merge -x -u --no-translations
> 
> The command doesn't translate those files. I don't know the reason behing
> this. However, there are some keyworkds in the file `description` and `name`
> that are supposed to be translated, even there are translated strings in the
> language `.po` files.
> 
> I wonder if the reason for not translating them was that the `its` and `loc`
> files was present.
> 
> If you think that this should also be translated I can create proper rules
> to be  applied.

IIRC, the files themselves aren't translated, but the strings are marked for translation, and the translations added to the .mo/.gmo files, and translated in the code.

I'm guessing that wouldn't be the case anymore.

> [0]
> https://git.gnome.org/browse/gnome-control-center/tree/panels/keyboard/
> Makefile.am?id=ca3779467e075c2b693adf4c6034e5b5e4b9bb6a#n48

We have the .its/.loc files for these already, see https://bugzilla.gnome.org/show_bug.cgi?id=769066 and https://git.gnome.org/browse/gnome-control-center/commit/?id=f7fad92c3ef00134e59f4cca5d38387e6784a0fc

Comment on attachment 359644 [details] [review]
sharing: add XML declaration to the .policy file

There is now: https://savannah.gnu.org/bugs/?52932

Pushed in https://git.gnome.org/browse/gnome-control-center/commit/?id=3807e457555e7a894e0751a7987e61f71ed5c38a

Comment on attachment 359645 [details] [review]
sound: specify encoding in the XML declaration of gnome-sounds-default.xml

https://git.gnome.org/browse/gnome-control-center/commit/?id=9db20522d3402679ad17fe867b881a770d900c0c

(In reply to Piotr Drąg from comment #16)
> Comment on attachment 359644 [details] [review] [review]
> sharing: add XML declaration to the .policy file
> 
> There is now: https://savannah.gnu.org/bugs/?52932
> 

A gettext developer is asking questions I’m not qualified to answer.

Created attachment 367103 [details] [review]
build: Migrate from Intltool to Gettext

(In reply to Piotr Drąg from comment #15)
> We have the .its/.loc files for these already, see
> https://bugzilla.gnome.org/show_bug.cgi?id=769066 and
> https://git.gnome.org/browse/gnome-control-center/commit/
> ?id=f7fad92c3ef00134e59f4cca5d38387e6784a0fc

Thanks! Those files were copied to the gettext directory under installation directory but I wasn't doing anything else with them.

I have moved them to the `${srcdir}/gettext/its` directory along `sounds.[its|loc]` files, which makes sense to me. The whole directory is also copied to the gettext directory under installation directory, so now `sounds.[its|loc]` files are also copied (which I think should have been done before but it's fixed now).

Bastien, could you please take a quick view at this? (it has some minor change only).

(In reply to Piotr Drąg from comment #18)
> 
> A gettext developer is asking questions I’m not qualified to answer.

Although I'm not an expert, I tried to find a proper answer for this and I have found the following[0]:

> 5.1.4 Input file interpretation
>
> ‘--from-code=name’
>
>     Specifies the encoding of the input files. This option is needed only if some untranslated message strings or their corresponding comments contain non-ASCII characters. Note that Tcl and Glade input files are always assumed to be in UTF-8, regardless of this option.
>
> By default the input files are assumed to be in ASCII. 

Taking this in consideration, the XML files are expected to be using ASCII unless the encoding is especified, and that was our issue when translating the files. I suppose that `intltool` should be assuming that the file was encoded using `UTF-8`.

[0] https://www.gnu.org/software/gettext/manual/html_node/xgettext-Invocation.html

(In reply to Piotr Drąg from comment #18)
> (In reply to Piotr Drąg from comment #16)
> > Comment on attachment 359644 [details] [review] [review] [review]
> > sharing: add XML declaration to the .policy file
> > 
> > There is now: https://savannah.gnu.org/bugs/?52932
> > 
> 
> A gettext developer is asking questions I’m not qualified to answer.

Following the thread (I have also answered this there) and one of its links, it says the following:

> In the absence of information provided by an external transport protocol (e.g. HTTP or MIME), it is a fatal error for an entity including an encoding declaration to be presented to the XML processor in an encoding other than that named in the declaration, or for an entity which begins with neither a Byte Order Mark nor an encoding declaration to use an encoding other than UTF-8. Note that since ASCII is a subset of UTF-8, ordinary ASCII entities do not strictly need an encoding declaration.

As I said in that thread, the last sentence is the critical one for me. The fact that ASCII doesn't need the encoding declaration makes it the default in those cases. In our case, the encoding must be UTF-8, so actually the modifications in attachment 359644 [details] [review] and on attachment 359645 [details] [review] are necessary.

[0] https://www.w3.org/TR/xml/#charencoding

(In reply to Iñigo Martínez from comment #20)
> (In reply to Piotr Drąg from comment #18)
> > (In reply to Piotr Drąg from comment #16)
> > > Comment on attachment 359644 [details] [review] [review] [review] [review]
> > > sharing: add XML declaration to the .policy file
> > > 
> > > There is now: https://savannah.gnu.org/bugs/?52932
> > > 
> > 
> > A gettext developer is asking questions I’m not qualified to answer.
> 
> Following the thread (I have also answered this there) and one of its links,
> it says the following:
> 
> > In the absence of information provided by an external transport protocol (e.g. HTTP or MIME), it is a fatal error for an entity including an encoding declaration to be presented to the XML processor in an encoding other than that named in the declaration, or for an entity which begins with neither a Byte Order Mark nor an encoding declaration to use an encoding other than UTF-8. Note that since ASCII is a subset of UTF-8, ordinary ASCII entities do not strictly need an encoding declaration.
> 
> As I said in that thread, the last sentence is the critical one for me. The
> fact that ASCII doesn't need the encoding declaration makes it the default
> in those cases.

No, it just says that ASCII is a subset of UTF-8. But UTF-8 is the default. I commented on the savannah bug.

Re-read the first sentence, the encoding is either, in order:
- provided by an external transport protocol (like the encoding header in HTTP)
- named in the declaration (as you did in the patch)
- provided through the presence of a BOM in the file
- or is implicitly UTF-8

And that last phrase "ASCII is a subset of UTF-8, so you don't need to declare that the source is ASCII because ASCII is already contained within the default UTF-8 encoding".

> In our case, the encoding must be UTF-8, so actually the
> modifications in attachment 359644 [details] [review] [review] and on attachment
> 359645 [details] [review] are necessary.

They wouldn't be if gettext followed the XML spec :)

Attachment 367035 [details] pushed as 811ba92 - build: Move timezone languages to LINGUAS file
Attachment 367103 [details] pushed as 0dd386f - build: Migrate from Intltool to Gettext

It seems that gnome-keybindings.its can’t handle https://git.gnome.org/browse/gnome-control-center/tree/panels/keyboard/01-launchers.xml.in#n16. It’s the only one with msgctxt.

(In reply to Piotr Drąg from comment #23)
> It seems that gnome-keybindings.its can’t handle
> https://git.gnome.org/browse/gnome-control-center/tree/panels/keyboard/01-
> launchers.xml.in#n16. It’s the only one with msgctxt.

Ha, my fault. I tested the Keyboard and Date & Time panels in a translated language and didn't catch that problem.

Would this fix it? Note that I didn't even compile test it.

diff --git a/gettext/its/gnome-keybindings.its b/gettext/its/gnome-keybindings.its
index 4d4b7c6ee..0d52827b4 100644
--- a/gettext/its/gnome-keybindings.its
+++ b/gettext/its/gnome-keybindings.its
@@ -4,6 +4,7 @@
   <its:translateRule selector="/KeyListEntries/@name" translate="yes"/>
   <its:translateRule selector="//KeyListEntry" translate="no"/>
   <its:translateRule selector="//KeyListEntry/@description" translate="yes"/>
+  <its:Context selector="//KeyListEntry/@msgctxt" translate="no"/>
 
   <!-- Extracted strings are consumed by the library and are never
        merged back; we don't want to escape special characters.  -->

Info from:
https://www.gnu.org/software/gettext/manual/html_node/Preparing-ITS-Rules.html

I tried “meson .. && ninja gnome-control-center-2.0-pot” and I get this, with and without the patched gnome-keybindings.its:

#: panels/keyboard/01-launchers.xml.in:16
#: panels/search/gnome-search-panel.desktop.in.in:3 shell/window.ui:157
msgid "Search"
msgstr ""

I might be doing something wrong, though.

Created attachment 367322 [details] [review]
build: Fix translation context missing from keyboard entries

Extract the message context from 01-launchers.xml.in so it's available
for translation.

The only other Meson and gettext project in GNOME I could find is libgweather: https://gitlab.gnome.org/GNOME/libgweather/commit/a5d17340753eff516ea1a1e3304643c7b5a2c3fe (although it doesn’t have a .loc file, so something might be wrong there.)

(I meant “The only other Meson and gettext project in GNOME *with msgctxt in an .its file*”, of course.)

Created attachment 367331 [details] [review]
build: Fix translation context missing from keyboard entries

I have updated Bastien's patch which missed a couple of things. It should also extract now the `msgctxt`.

I am not qualified to give a final word on it. It *looks* correct though. How do I test this patch? Piotr, Bastien, do you have any other concerns?

From within the jhbuild build directory I get:
$ ninja -v gnome-control-center-2.0-pot
[0/1] /usr/bin/python3 /usr/bin/meson --internal commandrunner /home/hadess/Projects/jhbuild/gnome-control-center /home/hadess/.cache/jhbuild/build/gnome-control-center po /usr/bin/python3 /usr/bin/meson /usr/bin/python3 /usr/bin/meson --internal gettext pot --pkgname=gnome-control-center-2.0 --datadirs=/home/hadess/Projects/jhbuild/gnome-control-center/gettext --extra-args=--from-code=UTF-8@@--keyword=g_dngettext:2,3@@--flag=g_string_append_printf:2:c-format@@--keyword=g_dpgettext2:2c,3@@--flag=g_string_printf:2:c-format@@--add-comments@@--flag=g_strdup_printf:1:c-format@@--flag=g_set_error:4:c-format@@--keyword=g_dcgettext:2@@--flag=g_error_new:3:c-format@@--flag=NC_:2:pass-c-format@@--keyword=C_:1c,2@@--flag=g_dngettext:2:pass-c-format@@--flag=N_:1:pass-c-format@@--flag=C_:2:pass-c-format@@--keyword=NC_:1c,2@@--keyword=N_@@--keyword=_
xgettext: warning: file '	../panels/background/background.ui \' extension 'ui \' is unknown; will try C
xgettext: error while opening "	../panels/background/background.ui \" for reading: No such file or directory
FAILED: meson-gnome-control-center-2.0-pot 
/usr/bin/python3 /usr/bin/meson --internal commandrunner /home/hadess/Projects/jhbuild/gnome-control-center /home/hadess/.cache/jhbuild/build/gnome-control-center po /usr/bin/python3 /usr/bin/meson /usr/bin/python3 /usr/bin/meson --internal gettext pot --pkgname=gnome-control-center-2.0 --datadirs=/home/hadess/Projects/jhbuild/gnome-control-center/gettext --extra-args=--from-code=UTF-8@@--keyword=g_dngettext:2,3@@--flag=g_string_append_printf:2:c-format@@--keyword=g_dpgettext2:2c,3@@--flag=g_string_printf:2:c-format@@--add-comments@@--flag=g_strdup_printf:1:c-format@@--flag=g_set_error:4:c-format@@--keyword=g_dcgettext:2@@--flag=g_error_new:3:c-format@@--flag=NC_:2:pass-c-format@@--keyword=C_:1c,2@@--flag=g_dngettext:2:pass-c-format@@--flag=N_:1:pass-c-format@@--flag=C_:2:pass-c-format@@--keyword=NC_:1c,2@@--keyword=N_@@--keyword=_
ninja: build stopped: subcommand failed.

(In reply to Bastien Nocera from comment #33)
> From within the jhbuild build directory I get:
> $ ninja -v gnome-control-center-2.0-pot
> [0/1] /usr/bin/python3 /usr/bin/meson --internal commandrunner
> /home/hadess/Projects/jhbuild/gnome-control-center
> /home/hadess/.cache/jhbuild/build/gnome-control-center po /usr/bin/python3
> /usr/bin/meson /usr/bin/python3 /usr/bin/meson --internal gettext pot
> --pkgname=gnome-control-center-2.0
> --datadirs=/home/hadess/Projects/jhbuild/gnome-control-center/gettext
> --extra-args=--from-code=UTF-8@@--keyword=g_dngettext:2,3@@--
> flag=g_string_append_printf:2:c-format@@--keyword=g_dpgettext2:2c,3@@--
> flag=g_string_printf:2:c-format@@--add-comments@@--flag=g_strdup_printf:1:c-
> format@@--flag=g_set_error:4:c-format@@--keyword=g_dcgettext:2@@--
> flag=g_error_new:3:c-format@@--flag=NC_:2:pass-c-format@@--keyword=C_:1c,2@@-
> -flag=g_dngettext:2:pass-c-format@@--flag=N_:1:pass-c-format@@--flag=C_:2:
> pass-c-format@@--keyword=NC_:1c,2@@--keyword=N_@@--keyword=_
> xgettext: warning: file '	../panels/background/background.ui \' extension
> 'ui \' is unknown; will try C
> xgettext: error while opening "	../panels/background/background.ui \" for
> reading: No such file or directory

That sounds a bit odd because:

1. The `*.ui` files should be covered by the gtkbuilder.loc[0] and gtkbuilder.its[1] its rules.
2. The background.ui file should be present[2].

I have also tried it in a jhbduild environment with the following output

$ ninja -v -C _build/ gnome-control-center-2.0-pot 
ninja: Entering directory `_build/'
[0/1] /usr/bin/python3 /usr/bin/meson --internal commandrunner /gnome/gnome-control-center /gnome/gnome-control-center/_build po /usr/bin/python3 /usr/bin/meson /usr/bin/python3 /usr/bin/meson --internal gettext pot --pkgname=gnome-control-center-2.0 --datadirs=/gnome/gnome-control-center/gettext --extra-args=--keyword=g_dcgettext:2@@--keyword=g_dngettext:2,3@@--flag=g_set_error:4:c-format@@--flag=g_string_printf:2:c-format@@--keyword=g_dpgettext2:2c,3@@--add-comments@@--flag=NC_:2:pass-c-format@@--keyword=N_@@--from-code=UTF-8@@--keyword=_@@--keyword=C_:1c,2@@--flag=N_:1:pass-c-format@@--flag=C_:2:pass-c-format@@--flag=g_strdup_printf:1:c-format@@--flag=g_error_new:3:c-format@@--keyword=NC_:1c,2@@--flag=g_dngettext:2:pass-c-format@@--flag=g_string_append_printf:2:c-format

Could it be possible that something is not properly configured in your jhbuild environment? Just guessing.

[0] https://git.gnome.org/browse/gtk+/tree/gtk/gtkbuilder.loc
[1] https://git.gnome.org/browse/gtk+/tree/gtk/gtkbuilder.its
[2] https://git.gnome.org/browse/gnome-control-center/tree/panels/background/background.ui

> $ ninja -v -C _build/ gnome-control-center-2.0-pot 
> ninja: Entering directory `_build/'

No, your jhbuild is using a different build directory than the default. Mine is completely out of tree:
jhbuild/defaults.jhbuildrc:buildroot = os.path.join(xdg_cache_home, 'jhbuild', 'build')

I guess the current code only works when the build directory is a sub-directory of the source directory.

It's still working here:

$ ninja -v gnome-control-center-2.0-pot 
[0/1] /usr/bin/python3 /usr/bin/meson --internal commandrunner /home/imartinez/jhbuild/checkout/gnome-control-center /home/imartinez/.cache/jhbuild/build/gnome-control-center po /usr/bin/python3 /usr/bin/meson /usr/bin/python3 /usr/bin/meson --internal gettext pot --pkgname=gnome-control-center-2.0 --datadirs=/home/imartinez/jhbuild/checkout/gnome-control-center/gettext --extra-args=--flag=g_dngettext:2:pass-c-format@@--keyword=_@@--flag=g_string_printf:2:c-format@@--flag=g_string_append_printf:2:c-format@@--from-code=UTF-8@@--flag=C_:2:pass-c-format@@--keyword=NC_:1c,2@@--keyword=N_@@--add-comments@@--keyword=g_dcgettext:2@@--keyword=g_dngettext:2,3@@--flag=NC_:2:pass-c-format@@--flag=g_error_new:3:c-format@@--flag=N_:1:pass-c-format@@--keyword=C_:1c,2@@--flag=g_set_error:4:c-format@@--keyword=g_dpgettext2:2c,3@@--flag=g_strdup_printf:1:c-format

I've also tried using a build directory in the tmp directory, which is also out of tree:

$ ninja -v gnome-control-center-2.0-pot
[0/1] /usr/bin/python3 /usr/bin/meson --internal commandrunner /home/imartinez/gnome/gnome-control-center /tmp/gcc-build po /usr/bin/python3 /usr/bin/meson /usr/bin/python3 /usr/bin/meson --internal gettext pot --pkgname=gnome-control-center-2.0 --datadirs=/home/imartinez/gnome/gnome-control-center/gettext --extra-args=--from-code=UTF-8@@--add-comments@@--flag=g_set_error:4:c-format@@--keyword=N_@@--flag=NC_:2:pass-c-format@@--keyword=C_:1c,2@@--keyword=_@@--flag=g_dngettext:2:pass-c-format@@--flag=g_string_append_printf:2:c-format@@--keyword=g_dngettext:2,3@@--keyword=g_dcgettext:2@@--flag=C_:2:pass-c-format@@--flag=g_strdup_printf:1:c-format@@--keyword=NC_:1c,2@@--flag=g_error_new:3:c-format@@--flag=g_string_printf:2:c-format@@--flag=N_:1:pass-c-format@@--keyword=g_dpgettext2:2c,3

The command runs successfully after nuking my build directory, and starting again, but it's not extracting the msgctxt for the Search key entry.

Could it be that I already have an its/loc pair for keybindings installed through the g-c-c package?

$ rpm -qf /usr/share/gettext/its/gnome-keybindings.loc
control-center-3.26.2-2.fc27.x86_64

(In reply to Bastien Nocera from comment #37)
> Could it be that I already have an its/loc pair for keybindings installed
> through the g-c-c package?

Yes, that's the problem. We were yesterday talking about this issue on IRC.

I have filled a bug against gettext: https://savannah.gnu.org/bugs/?52966

In which case, and if it works for you, looks fine from my end.

Thanks Bastien.

Piotr, in the meantime, while distributions distribute these files, do you think that a workaround might be necessary to help translators getting and translating these strings?

Not really, because GNOME translators are expected to download .pot/.po files from damned-lies, and not clone and build them themselves. Damned-lies uses .its files from git, so it should be all fine (after bug #792829 is fixed).

Comment on attachment 367331 [details] [review]
build: Fix translation context missing from keyboard entries

Pushed as 535d68425 - build: Fix translation context missing from keyboard entries