Created attachment 276995 [details] [review]
version 0 (untested!)

I think it's a trivial change to POTFILES.in, but it should preferably go with i18n support in gterminal.vala.

I created a patch, but haven't tested it yet, might not work :)

I didn't add i18n to gterminal since I had problems with include order (config.h wasn't being included first...)

Review of attachment 276995 [details] [review]:

If it works, ok to commit with the comments fixed.

::: src/Makefile.am
@@ +193,3 @@
 # See bug #710862 about -Wsuggest-attribute=format
 gterminal_CFLAGS = \
+	-DGETTEXT_PACKAGE=\"$(GETTEXT_PACKAGE)\" \

This should go to gterminal_CPPFLAGS not CFLAGS.

::: src/gterminal.vala
@@ +42,3 @@
       var group = new GLib.OptionGroup ("output",
+                                        N_("Output options:"),
+                                        _("Show output options"),

N_ here too.

@@ +94,3 @@
     private static const OptionEntry[] entries = {
       { "app-id", 0, OptionFlags.HIDDEN, OptionArg.CALLBACK, (void*) option_app_id,
+        N_("Server application ID"), "ID" },

N_("ID")

@@ +101,3 @@
     {
       var group = new GLib.OptionGroup ("global",
+                                        _("Global options:"),

N_

@@ +182,2 @@
       { "fd", 0, 0, OptionArg.STRING_ARRAY, ref pass_fds,
+        N_("Forward file descriptor"), "FD" },

N_("FD")

@@ +215,1 @@
         "GEOMETRY" },

N_

@@ +222,3 @@
     {
       var group = new GLib.OptionGroup ("window",
+                                        _("Window options:"),

N_

And so one for all the rest too :-)

(Oh how much I hate that bugzilla doesn't auto-cc myself if I attach a patch... I completely forgot about this issue.)

Created attachment 278166 [details] [review]
v2

Partially tested.  How to print any of the _N() strings?  --help doesn't work.

N_() just marks the string for extraction by xgettext. GOption does the translation internally, that's what the set_translation_domain calls are for.

Looks good code-wise, but does it actually build? When I tried to add i18n to this before, it broke because config.h was not the first thing included in the valac-generated C ?

(In reply to comment #6)
> Looks good code-wise, but does it actually build?

It does, and I've checked some run-time translated strings (e.g. error parsing cmd line) that are indeed translated. I have valac 0.22.1.

> When I tried to add i18n to
> this before, it broke because config.h was not the first thing included in the
> valac-generated C ?

I had this problem at my first attempt too, when (following Google's advices) I tried to define GETTEXT_PACKAGE in the source code. Moving it to Makefile fixed it.

Right. Howver config.h still is included in the wrong place, which can have undesired effects (e.g. when using largefiles, which isn't relevant for gterminal). 

So please commit, thanks!

Looks like this broke the Continuous build: http://build.gnome.org/continuous/buildmaster/builds/2014/06/14/16/build/log-gnome-terminal.txt

Reverting for now.

Looks like it's vala bug 725995.  I can't reproduce with gcc, but can with clang.

I have no idea how to wor around the issue.  Let's hope vala folks fix it before gterminal becomes mature :)

Created attachment 278460 [details] [review]
-Werror workaround

According to bug 725995 comment 2, this might be a suitable workaround.

Colin: Could you please confirm that it's compiling in Continuous Build again?

(In reply to comment #12)
> Colin: Could you please confirm that it's compiling in Continuous Build again?

Feel free to try pushing a "git revert" of https://git.gnome.org/browse/gnome-continuous/commit/?id=a50cf3c91cb61f35d4eb25191ef21a02639833fe

Build 20140614.29 succeeded, I think we're okay.