Merge lp:~dobey/unity-scope-click/translated into lp:unity-scope-click

[dobey]

Add the _ macro definition.
Mark strings for translation.
Add build system support for translating the scope.
Add build dependency on intltool.

[Michal Hruby (mhr3)]

Isn't this missing calls to setlocale() and bindtextdomain()?

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:200
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/405/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-amd64-ci/306
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/303
        deb: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/303/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scope-click-ci/405/rebuild

review: Approve (continuous-integration)

[lp:~dobey/unity-scope-click/translated]

201. By dobey on 2014-04-04

Add setlocale/bindtextdomain calls to enable the translations at runtime.

[Alejandro J. Cura (alecu)]

Looks good to me. +1

review: Approve

[David Planella (dpm)]

Looks good to me two, just a couple of things:

216 + {"label", scopes::Variant(_("Yes Uninstall"))}

Should this not be "Yes, uninstall"?

I understand this adds i18n support to the scope itself, but how are the translations of the app's desktop files in the dash page that shows the list of all apps going to be handled?

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:201
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/411/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-amd64-ci/312
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/309
        deb: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/309/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scope-click-ci/411/rebuild

review: Approve (continuous-integration)

[Alejandro J. Cura (alecu)]

> Looks good to me two, just a couple of things:
>
> 216 + {"label", scopes::Variant(_("Yes Uninstall"))}
>
> Should this not be "Yes, uninstall"?
>
> I understand this adds i18n support to the scope itself, but how are the
> translations of the app's desktop files in the dash page that shows the list
> of all apps going to be handled?

For installed apps, we plan to take the values from the .desktop file.
For apps in the store, we plan to send a Language header in the http request to the store webservices.
Both will come in further branches.

[dobey (dobey)]

> Looks good to me two, just a couple of things:
>
> 216 + {"label", scopes::Variant(_("Yes Uninstall"))}
>
> Should this not be "Yes, uninstall"?

The purpose of this branch isn't to change or correct strings, only to add the _() to existing ones. Hopefully next week we'll be deleting the preview that has this string anyway, so it will just go away.

> I understand this adds i18n support to the scope itself, but how are the
> translations of the app's desktop files in the dash page that shows the list
> of all apps going to be handled?

Pulling translations from installed click packages, or from the store, is a much more difficult thing to do, and this branch does not do that. Really, we will need translations for clicks to be in the manifest, so that we can use the translations while offline. Loading the translation domain for every installed program is also very expensive, so we really need to have those translations directly in the .desktop file. We need to discuss implementation details about that on a wider scale, so we can then fix the translations to work for packages as well as the scope itself.

[Alejandro J. Cura (alecu)]

> 216 + {"label", scopes::Variant(_("Yes Uninstall"))}
>
> Should this not be "Yes, uninstall"?

That string is bug #1234211; it was like that on the design documents and we (devs) agree it looks horrible. I'll re-check again to see if we have a better wording.

[lp:~dobey/unity-scope-click/translated]

202. By dobey on 2014-04-04

Add use of dh-translations to package build.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:202
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/413/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-amd64-ci/314
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/311
        deb: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/311/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scope-click-ci/413/rebuild

review: Approve (continuous-integration)

[David Planella (dpm)]

That answered my questions, LGTM!

For translations to be exposed in the upstream project and shared with the source package, I recommend committing the .pot file to the source tree, as we do with other Ubuntu upstreams. Launchpad does not support .pot autogeneration for cmake projects, so to enable automatic translation imports and exports, the .pot file has to be present in the source tree for LP.

For translations in the source package, dh-translations should already take care of everything.

review: Approve

[dobey (dobey)]

> For translations to be exposed in the upstream project and shared with the
> source package, I recommend committing the .pot file to the source tree, as we
> do with other Ubuntu upstreams. Launchpad does not support .pot autogeneration
> for cmake projects, so to enable automatic translation imports and exports,
> the .pot file has to be present in the source tree for LP.
>
> For translations in the source package, dh-translations should already take
> care of everything.

We won't be using LP translations on the upstream project, only on the package. We don't need to have 2 places to pull and then manually try to consolidate them. So we'll just rely on dh-translations and pulling the .po files from the Ubuntu translations once they are translated.

[David Planella (dpm)]

Note that there is no need to do manual work if you enable translations in
the upstream project. With translations sharing, translations done either
upstream or on the source package flow in either direction. Thus developers
don't even have to care. Adding automatic imports and exports gives the
benefit of having translations committed automatically upstream, regardless
of where they are done.

On the contrary, if you only enable translations in the source package
that's where you'll have to do manual work to get those translations back
to upstream: you'll need to request an export from LP, wait for an e-mail
with a zipped export and then manually commit those .po files to upstream.

Thus my recommendation is always to go for the first, which for developers
it's just a matter of committing the .pot file and then the rest works.

Cheers,
David.

On Wed, Apr 9, 2014 at 4:02 PM, Rodney Dawes <email address hidden>wrote:

> The proposal to merge lp:~dobey/unity-scope-click/translated into
> lp:unity-scope-click has been updated.
>
> Status: Needs review => Approved
>
> For more details, see:
>
> https://code.launchpad.net/~dobey/unity-scope-click/translated/+merge/214182
> --
>
> https://code.launchpad.net/~dobey/unity-scope-click/translated/+merge/214182
> You are reviewing the proposed merge of
> lp:~dobey/unity-scope-click/translated into lp:unity-scope-click.
>

[dobey (dobey)]

On Wed, 2014-04-09 at 14:27 +0000, David Planella wrote:
> Note that there is no need to do manual work if you enable translations in
> the upstream project. With translations sharing, translations done either
> upstream or on the source package flow in either direction. Thus developers
> don't even have to care. Adding automatic imports and exports gives the
> benefit of having translations committed automatically upstream, regardless
> of where they are done.

Unfortunately, it means keeping a generated file in version control, a
file which can oft easily result in conflicts when working on the
project. For projects with very little development, and/or very few
contributors, it's an OK option, because the potential for conflict is
limited, and it means getting new translations without worry. However,
it still means Launchpad is committing directly to the tree, without
warning, and that itself can result in annoyances that result in "manual
work" to resolve conflicts or criss-cross merge problems. Also, it means
having complex rules in the build system, so that the .pot file is
always properly updated in the source tree, as the template is generated
in the build tree, not the source tree, and the recommended way to use
cmake is to do builds under a separate build directory.

Even with the pot file in the tree, and Launchpad automatically
importing that, I don't think we'd want to have Launchpad committing
directly to the tree, so we'd still need to do manual imports of the
translations to the tree.

[David Planella (dpm)]

I don't want to keep arguing on this one, so I'll just add for the sake of
completion that Launchpad only commits .po files to the tree, it doesn't
update any other files.

On Wed, Apr 9, 2014 at 4:45 PM, Rodney Dawes <email address hidden>wrote:

> On Wed, 2014-04-09 at 14:27 +0000, David Planella wrote:
> > Note that there is no need to do manual work if you enable translations
> in
> > the upstream project. With translations sharing, translations done either
> > upstream or on the source package flow in either direction. Thus
> developers
> > don't even have to care. Adding automatic imports and exports gives the
> > benefit of having translations committed automatically upstream,
> regardless
> > of where they are done.
>
> Unfortunately, it means keeping a generated file in version control, a
> file which can oft easily result in conflicts when working on the
> project. For projects with very little development, and/or very few
> contributors, it's an OK option, because the potential for conflict is
> limited, and it means getting new translations without worry. However,
> it still means Launchpad is committing directly to the tree, without
> warning, and that itself can result in annoyances that result in "manual
> work" to resolve conflicts or criss-cross merge problems. Also, it means
> having complex rules in the build system, so that the .pot file is
> always properly updated in the source tree, as the template is generated
> in the build tree, not the source tree, and the recommended way to use
> cmake is to do builds under a separate build directory.
>
> Even with the pot file in the tree, and Launchpad automatically
> importing that, I don't think we'd want to have Launchpad committing
> directly to the tree, so we'd still need to do manual imports of the
> translations to the tree.
>
>
>
> --
>
> https://code.launchpad.net/~dobey/unity-scope-click/translated/+merge/214182
> You are reviewing the proposed merge of
> lp:~dobey/unity-scope-click/translated into lp:unity-scope-click.
>

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2014-03-24 14:04:38 +0000
+++ CMakeLists.txt	2014-04-04 16:41:17 +0000
@@ -36,6 +36,7 @@
 add_subdirectory(autopilot)
 add_subdirectory(scope)
 add_subdirectory(data)
+add_subdirectory(po)
 
 # Custom targets for the tests
 add_custom_target (test

=== modified file 'debian/control'
--- debian/control	2014-04-02 14:44:39 +0000
+++ debian/control	2014-04-04 16:41:17 +0000
@@ -3,7 +3,9 @@
 Priority: optional
 Build-Depends: cmake (>= 2.8.10),
                debhelper (>= 9),
+               dh-translations,
                google-mock,
+               intltool,
                libglib2.0-dev (>= 2.32),
                libjsoncpp-dev,
                libubuntu-download-manager-client-dev,

=== modified file 'debian/rules'
--- debian/rules	2014-03-24 19:18:13 +0000
+++ debian/rules	2014-04-04 16:41:17 +0000
@@ -13,7 +13,7 @@
 DEB_HOST_MULTIARCH ?= $(shell dpkg-architecture -qDEB_HOST_MULTIARCH)
 
 %:
-	dh $@ --buildsystem cmake --fail-missing
+	dh $@ --buildsystem cmake --with translations --fail-missing
 
 override_dh_auto_configure:
 	dh_auto_configure -- -DCMAKE_VERBOSE_MAKEFILE=ON

=== added directory 'po'
=== added file 'po/CMakeLists.txt'
--- po/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ po/CMakeLists.txt	2014-04-04 16:41:17 +0000
@@ -0,0 +1,36 @@
+include(FindGettext)
+find_program(GETTEXT_XGETTEXT_EXECUTABLE xgettext)
+find_program(INTLTOOL_UPDATE intltool-update)
+
+set(GETTEXT_PACKAGE ${PROJECT_NAME})
+set(POT_FILE ${GETTEXT_PACKAGE}.pot)
+execute_process(
+    COMMAND cat ${CMAKE_CURRENT_SOURCE_DIR}/LINGUAS
+    OUTPUT_VARIABLE LINGUAS
+    OUTPUT_STRIP_TRAILING_WHITESPACE
+)
+
+# Creates POTFILES
+add_custom_target(POTFILES ALL
+    COMMENT "Generating POTFILES"
+    COMMAND ${CMAKE_CURRENT_SOURCE_DIR}/genpotfiles.sh ${CMAKE_SOURCE_DIR}
+)
+
+# Creates the .pot file containing the translations template
+add_custom_target(${POT_FILE} ALL
+    COMMENT "Generating translation template"
+    COMMAND XGETTEXT="${GETTEXT_XGETTEXT_EXECUTABLE}" srcdir="${CMAKE_CURRENT_SOURCE_DIR}" ${INTLTOOL_UPDATE} --gettext-package ${GETTEXT_PACKAGE} --pot
+    DEPENDS POTFILES
+)
+
+# Builds the binary translations catalog for each language
+# it finds source translations (*.po) for
+foreach(LANG ${LINGUAS})
+    list(APPEND PO_FILES "${LANG}.po")
+    gettext_process_po_files(${LANG} ALL PO_FILES "${LANG}.po")
+    set(INSTALL_DIR ${CMAKE_INSTALL_LOCALEDIR}/${LANG}/LC_MESSAGES)
+    install(FILES ${CMAKE_CURRENT_BINARY_DIR}/${LANG}.gmo
+            DESTINATION ${INSTALL_DIR}
+            RENAME ${DOMAIN}.mo
+    )
+endforeach(LANG)

=== added file 'po/LINGUAS'
=== added file 'po/POTFILES.in'
--- po/POTFILES.in	1970-01-01 00:00:00 +0000
+++ po/POTFILES.in	2014-04-04 16:41:17 +0000
@@ -0,0 +1,1 @@
+scope/click/preview.cpp

=== added file 'po/genpotfiles.sh'
--- po/genpotfiles.sh	1970-01-01 00:00:00 +0000
+++ po/genpotfiles.sh	2014-04-04 16:41:17 +0000
@@ -0,0 +1,6 @@
+#!/bin/sh
+
+sed '/^#/d
+	 s/^[[].*] *//
+	 /^[ 	]*$/d' \
+      "`dirname ${0}`/POTFILES.in" | sed '$!s/$/ \\/' > POTFILES

=== modified file 'scope/click/CMakeLists.txt'
--- scope/click/CMakeLists.txt	2014-04-01 13:27:09 +0000
+++ scope/click/CMakeLists.txt	2014-04-04 16:41:17 +0000
@@ -4,6 +4,11 @@
 pkg_check_modules(JSON_CPP REQUIRED jsoncpp)
 pkg_check_modules(LIBURL_DISPATCHER REQUIRED url-dispatcher-1)
 
+add_definitions(
+  -DGETTEXT_PACKAGE=\"${PROJECT_NAME}\"
+  -DGETTEXT_LOCALEDIR=\"${CMAKE_INSTALL_LOCALEDIR}\"
+)
+
 add_library(${SCOPE_LIB_UNVERSIONED} SHARED
   download-manager.cpp
   index.cpp

=== added file 'scope/click/click-i18n.h'
--- scope/click/click-i18n.h	1970-01-01 00:00:00 +0000
+++ scope/click/click-i18n.h	2014-04-04 16:41:17 +0000
@@ -0,0 +1,37 @@
+/*
+ * Copyright (C) 2014 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3, as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranties of
+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+ * PURPOSE.  See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * In addition, as a special exception, the copyright holders give
+ * permission to link the code of portions of this program with the
+ * OpenSSL library under certain conditions as described in each
+ * individual source file, and distribute linked combinations
+ * including the two.
+ * You must obey the GNU General Public License in all respects
+ * for all of the code used other than OpenSSL.  If you modify
+ * file(s) with this exception, you may extend this exception to your
+ * version of the file(s), but you are not obligated to do so.  If you
+ * do not wish to do so, delete this exception statement from your
+ * version.  If you delete this exception statement from all source
+ * files in the program, then also delete it here.
+ */
+
+#ifndef CLICKI18N_H
+#define CLICKI18N_H
+
+#include <libintl.h>
+
+#define _(value) dgettext(GETTEXT_PACKAGE, value)
+
+#endif

=== modified file 'scope/click/preview.cpp'
--- scope/click/preview.cpp	2014-04-02 12:01:34 +0000
+++ scope/click/preview.cpp	2014-04-04 16:41:17 +0000
@@ -41,6 +41,8 @@
 #include <iostream>
 #include <sstream>
 
+#include "click-i18n.h"
+
 namespace click {
 
 // Preview base class
@@ -155,18 +157,18 @@
 
 scopes::PreviewWidgetList Preview::downloadErrorWidgets()
 {
-    return errorWidgets(scopes::Variant("Download Error"),
-                        scopes::Variant("Download or install failed. Please try again."),
+    return errorWidgets(scopes::Variant(_("Download Error")),
+                        scopes::Variant(_("Download or install failed. Please try again.")),
                         scopes::Variant(click::Preview::Actions::CLOSE_PREVIEW), // TODO see bug LP: #1289434
-                        scopes::Variant("Close"));
+                        scopes::Variant(_("Close")));
 }
 
 scopes::PreviewWidgetList Preview::loginErrorWidgets()
 {
-    return errorWidgets(scopes::Variant("Login Error"),
-                        scopes::Variant("Please log in to your Ubuntu One account."),
+    return errorWidgets(scopes::Variant(_("Login Error")),
+                        scopes::Variant(_("Please log in to your Ubuntu One account.")),
                         scopes::Variant(click::Preview::Actions::OPEN_ACCOUNTS),
-                        scopes::Variant("Go to Accounts"));
+                        scopes::Variant(_("Go to Accounts")));
 }
 
 scopes::PreviewWidgetList Preview::errorWidgets(const scopes::Variant& title,
@@ -299,12 +301,12 @@
     builder.add_tuple(
         {
             {"id", scopes::Variant(click::Preview::Actions::OPEN_CLICK)},
-            {"label", scopes::Variant("Open")}
+            {"label", scopes::Variant(_("Open"))}
         });
     builder.add_tuple(
         {
             {"id", scopes::Variant(click::Preview::Actions::UNINSTALL_CLICK)},
-            {"label", scopes::Variant("Uninstall")}
+            {"label", scopes::Variant(_("Uninstall"))}
         });
     buttons.add_attribute_value("actions", builder.end());
     widgets.push_back(buttons);
@@ -358,20 +360,20 @@
     scopes::PreviewWidgetList widgets;
 
     scopes::PreviewWidget header("hdr", "header");
-    header.add_attribute_value("title", scopes::Variant("Confirmation"));
+    header.add_attribute_value("title", scopes::Variant(_("Confirmation")));
     header.add_attribute_value("subtitle",
-                         scopes::Variant("Uninstalling this app will delete all the related information. Are you sure you want to uninstall?")); // TODO: wording needs review. see bug LP: #1234211
+                               scopes::Variant(_("Uninstalling this app will delete all the related information. Are you sure you want to uninstall?"))); // TODO: wording needs review. see bug LP: #1234211
     widgets.push_back(header);
 
     scopes::PreviewWidget buttons("buttons", "actions");
     scopes::VariantBuilder builder;
     builder.add_tuple({
        {"id", scopes::Variant(click::Preview::Actions::CLOSE_PREVIEW)}, // TODO: see bug LP: #1289434
-       {"label", scopes::Variant("Not anymore")}
+       {"label", scopes::Variant(_("Not anymore"))}
     });
     builder.add_tuple({
        {"id", scopes::Variant(click::Preview::Actions::CONFIRM_UNINSTALL)},
-       {"label", scopes::Variant("Yes Uninstall")}
+       {"label", scopes::Variant(_("Yes Uninstall"))}
     });
     buttons.add_attribute_value("actions", builder.end());
     widgets.push_back(buttons);
@@ -410,7 +412,7 @@
     builder.add_tuple(
         {
             {"id", scopes::Variant(click::Preview::Actions::INSTALL_CLICK)},
-            {"label", scopes::Variant("Install")},
+            {"label", scopes::Variant(_("Install"))},
             {"download_url", scopes::Variant(details.download_url)}
         });
     buttons.add_attribute_value("actions", builder.end());

=== modified file 'scope/click/query.cpp'
--- scope/click/query.cpp	2014-04-03 13:08:47 +0000
+++ scope/click/query.cpp	2014-04-04 16:41:17 +0000
@@ -50,6 +50,8 @@
 #include<set>
 #include<sstream>
 
+#include "click-i18n.h"
+
 namespace
 {
 
@@ -136,7 +138,7 @@
           replyProxy(replyProxy),
           installedApplications(installedApplications),
           renderer(categoryTemplate),
-          category(replyProxy->register_category("appstore", "Available", "", renderer)),
+          category(replyProxy->register_category("appstore", _("Available"), "", renderer)),
           queryUrl(queryUri) {
     }
 
@@ -246,7 +248,7 @@
                                std::string categoryTemplate)
 {
     scopes::CategoryRenderer rdr(categoryTemplate);
-    auto cat = replyProxy->register_category("local", "My apps", "", rdr);
+    auto cat = replyProxy->register_category("local", _("My apps"), "", rdr);
 
     // cat might be null when the underlying query got cancelled.
     if (!cat)

=== modified file 'scope/click/scope.cpp'
--- scope/click/scope.cpp	2014-03-21 04:31:34 +0000
+++ scope/click/scope.cpp	2014-04-04 16:41:17 +0000
@@ -39,6 +39,8 @@
 #include <QSharedPointer>
 #include <url-dispatcher.h>
 
+#include "click-i18n.h"
+
 namespace
 {
 click::Interface& clickInterfaceInstance()
@@ -82,6 +84,10 @@
 
 int click::Scope::start(std::string const&, scopes::RegistryProxy const&)
 {
+    setlocale(LC_ALL, "");
+    bindtextdomain(GETTEXT_PACKAGE, GETTEXT_LOCALEDIR);
+    bind_textdomain_codeset(GETTEXT_PACKAGE, "UTF-8");
+
     return VERSION;
 }