Created attachment 342402 [details] [review]
[Patch] Replace intltool with xgettext

So I took a jab at this, with the attached patch as the result.

Some remarks:
* The minimal supported version of xgettext is 0.19.8. The CMake-script now exits with a fatal error, but I don't know if that is a good choice (if there is an alternative).
* Before merging this, we should probably check this really well (all seems to be ok on first sight, but l10n is not my strong point).

Review of attachment 342402 [details] [review]:

I've just has a quick look at this, but haven't tried it out yet. There's a few minor nits below but I'm generally happy with the approach.

I think to review it properly I'd need to try it out and understand the migration process a bit better myself, since I'm definitely not a translation expert.

::: CMakeLists.txt
@@ +145,3 @@
+    else ()
+        message(STATUS "xgettext found, version ${XGETTEXT_VERSION}")
+    endif ()

Is it worth incorporating this version check and error message into the xgettext include/macro/whatever, so it works kind of similarly to how pkg_check_modules works? That would de-clutter this change a bit.

::: cmake/Gettext.cmake
@@ +72,3 @@
+
+    # Liberally taken from
+    # https://github.com/boghison/mixcloudscope/blob/master/cmake/FindXGettext.cmake

Does this need proper attribution? What's the copyright & licence on that file?

::: desktop/CMakeLists.txt
@@ +64,3 @@
     message (STATUS "Install Contractor contract: ON")
+    #    if (GETTEXT_FOUND)
+#        INTLTOOL_MERGE_CONTRACT (geary-attach.contract po)

Is this supposed to be commented out?

@@ +78,3 @@
 #            endif (DESKTOP_FILE_VALIDATE_FOUND)
 #        endif (DESKTOP_VALIDATE)
+#    endif (GETTEXT_FOUND)

And this?

::: po/nl.po
@@ +40,3 @@
 msgstr "E-mail verzenden en ontvangen"
 
+#: ../desktop/geary.appdata.xml.in:3

Is this an errant inclusion?

@@ +50,3 @@
 "duidelijke, moderne interface."
 
+#: ../desktop/geary.appdata.xml.in:4

Same with this?

Piotr, you're my go-to GNOME translation expert, would you mind reviewing this as well if you have a moment?

I can't comment on the CMake/build stuff, but I'm able to generate a correct POT file using <https://git.gnome.org/browse/gtk+/tree/make-pot> after removing [type: gettext/glade] tags from po/POTFILES.in.

X-GNOME-FullName in .desktop files doesn't get extracted and it's a known problem (see bug #770080 and bug #771641), but it's deprecated and we can just delete it.

The bigger problem is desktop/geary-attach.contract.in, which I don't know how to get translatable without intltool.

(In reply to Piotr Drąg from comment #4)
> I can't comment on the CMake/build stuff, but I'm able to generate a correct
> POT file using <https://git.gnome.org/browse/gtk+/tree/make-pot> after
> removing [type: gettext/glade] tags from po/POTFILES.in.
> 
> X-GNOME-FullName in .desktop files doesn't get extracted and it's a known
> problem (see bug #770080 and bug #771641), but it's deprecated and we can
> just delete it.

Thanks!

> The bigger problem is desktop/geary-attach.contract.in, which I don't know
> how to get translatable without intltool.

Yeah, this is support for custom Elementary OS UX. Given they've forked Geary, it might not be worth supporting any more, or maybe it should be a downstream packaging patch anyway.

Niels, do you want to send an email to the list to get some feedback and whether anyone is still using Geary on Elementary?

Created attachment 350524 [details] [review]
[Patch v2] Replace intltool with gettext

Got in contact with Elementary, and they were able to tell me that the Contract file should work flawlessly with gettext. I investigated it by manually invoking msgfmt, and saw that I forgot to remove the intltool-targeted underscores in the file.

Changes w.r.t. the previous patch:
* Patch is applicable on latest master
* Contract file is only merged and installed conditionally (the proper behaviour)
* POTFILES.in now removes the intltool-specific hints.

Thanks Piotr, Daniel (from Elementary) and Michael for the guidelines!

Now this should normally work, but can anyone check to make sure all goes well?

Patch rebased of current-ish master pushed and to branch wip/771643-replace-intltool. I'm just waiting on some feedback from gnome-i18n about what needs to happen with Damned Lies before merging this.

Oh one thing, we still need to provide a convenient way of generating po/geary.pot for translators that is compatible with Damned Lies, since they can no longer just run intltool, and "make pot_file" hasn't been working on master for ages it seems and continues to not work with this patch.

So either we should provide something like the make-pot script that Piotr mentioned exists for GTK+ and that uses the same xgettext args that Damned Lies does (maybe this means adding a po/Makevars file?), or fix the root Makefile and/or cmake build to provide a target that is compatible with whatever gets generated when using autotools — "make geary.pot" or similar.

In the last few months the situation has slightly changed, and just adding po/Makevars will allow Damned Lies to build a pot file.

As for the .contract file, you need a custom ITS rule.

Just pushed some updates to wip/771643-replace-intltool:

 - Added a po/Makevars
 - Re-enabled the "pot_file" make target to replace intltool-update
 - Worked around xgettext not recognising contract files by default

Seems to all work reasonably well - the generated geary.pot file matches l10n.g.o's pretty closely. I'll probably merge sometime in the next few days.

> +XGETTEXT_OPTIONS = --from-code=UTF-8 --add-comments --keyword=C_:1c,2 --keyword=NC_:1c,2

This should most likely be “--from-code=UTF-8 --keyword=_ --keyword=N_ --keyword=C_:1c,2 --keyword=NC_:1c,2 --keyword=g_dngettext:2,3 --add-comments”.

> +MSGID_BUGS_ADDRESS = gnome-i18n@gnome.org

Please use https://bugzilla.gnome.org/enter_bug.cgi?product=geary&keywords=I18N+L10N&component=internationalization instead, the translators mailing list is not the best choice for bug reports about Geary itself. :)

> +USE_MSGCTXT = no

Geary uses msgctxt, so it should be set to “yes”.

These is also X-GNOME-FullName in desktop/geary-autostart.desktop.in that you didn’t remove.

And replying to myself:

> As for the .contract file, you need a custom ITS rule.

ITS files are for translating custom XML files, so it obviously wouldn’t work.

Hey Piotr, thanks for the comments.

(In reply to Piotr Drąg from comment #11)
> > +XGETTEXT_OPTIONS = --from-code=UTF-8 --add-comments --keyword=C_:1c,2 --keyword=NC_:1c,2
> 
> This should most likely be “--from-code=UTF-8 --keyword=_ --keyword=N_
> --keyword=C_:1c,2 --keyword=NC_:1c,2 --keyword=g_dngettext:2,3
> --add-comments”.

I've added these in. From my reading from the Gettext docs theose additional keywords were already included by default for Vala source files, but maybe I'm missing something here.

> > +MSGID_BUGS_ADDRESS = gnome-i18n@gnome.org
> 
> Please use
> https://bugzilla.gnome.org/enter_bug.
> cgi?product=geary&keywords=I18N+L10N&component=internationalization instead,
> the translators mailing list is not the best choice for bug reports about
> Geary itself. :)

Good point, done. :)

> > +USE_MSGCTXT = no
> 
> Geary uses msgctxt, so it should be set to “yes”.

Ditto. :)

> These is also X-GNOME-FullName in desktop/geary-autostart.desktop.in that
> you didn’t remove.

Done!

I've updated the wip branch with these changes, will merge with master in a day or two.

Okay, this has been merged to master as commit d4ea866. Will keep this open for a few days in case there's any feedback.

Errr, actually commit 2a0fa4c because git - how does it work? ¯\_(ツ)_/¯

Created attachment 362844 [details] [review]
Translate GSchema

It’s a great opportunity to translate desktop/org.gnome.Geary.gschema.xml for our advanced users.

Cool - pushed as commit c6481c6. Thanks for those extra fixes you pushed to master as well.