<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/117052/">https://git.reviewboard.kde.org/r/117052/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 25th, 2014, 3:33 p.m. CET, <b>Alex Merry</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The docs need cleaning up, but I'd like to concentrate on the API first.
I'd rather this followed the convention of other file-generating macros in getting the user to provide a variable name to store the file(s) in, rather than always using ECM_QT_TRANSLATION_LOADER.
I also think that the installation should be handled by the caller. Provide a variable to store the qmfiles in, and require the caller to install them in a subdir <podir> of something in QStandardPaths::GenericDataLocation. That way, the project can make use of the user-configurable DATA_INSTALL_DIR from KDEInstallDirs, for example, or their own installation path code.
I could imagine that an option to specify the name of the loader file could potentially be useful to avoid clashes, but let's leave that until someone says they want it.</pre>
</blockquote>
<p>On March 26th, 2014, 9:36 a.m. CET, <b>Aurélien Gâteau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I made the macro install files itself to avoid developers getting it wrong: it is very easy to forget to install translation files or to install them in the wrong place. Most developers won't notice it because they don't test translations.
Same with the variable name: the function could be modified to accept the name of the variable as argument, but that adds complexity and more chances of getting it wrong. I assume we don't want to support the possibility to call ecm_setup_qt_translations() twice within the same framework. If you think this is a valid use-case, though, then the macro needs to be made more customizable.
Nevertheless, you are the maintainer, so I will make any changes you feel are needed. Can you give me the prototype of the macro you would like to see and how one should use it?</pre>
</blockquote>
<p>On March 27th, 2014, 12:09 a.m. CET, <b>Alex Merry</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Hmm, OK, I see your point. The macros in FindGettext do much the same thing with regards to installation.
With regards to the variable, I'd say that making the variable name explicit is likely to make it *less* error prone. If it's there in the function call, it's a little more obvious that you're suppose to make use of it, rather than having to know about a magic variable name (it has to be added to the target either way).
However, I think being able to specify the data dir could be important; if the user changes ${DATA_INSTALL_DIR} (and sets XDG_DATA_DIRS appropriately), the translations installation location should be changed accordingly.
So let's go with:
ecm_setup_qt_translations(<source_file_var>
PO_DIR <po_dir>
POT_NAME <pot_name>
[INSTALL_DATA_DIR <install_data_dir>]
[INSTALL_SUB_DIR <install_sub_dir>]
[CREATE_LOADER])
where <install_data_dir> defaults to share and must be set to something that will be found by QStandardPaths::GenericDataLocation.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I mostly agree, but I'd suggest two changes:
- The <source_file_var> is only needed if one uses CREATE_LOADER, therefore I would make it an argument of CREATE_LOADER.
- Having two arguments for the installation dir is unusual, what about merging them in an INSTALL_DESTINATION argument, which *must* be set to get translations installed (just like the gettext macros do)?</pre>
<br />
<p>- Aurélien</p>
<br />
<p>On March 25th, 2014, 11:48 a.m. CET, Aurélien Gâteau wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Build System, Extra Cmake Modules and KDE Frameworks.</div>
<div>By Aurélien Gâteau.</div>
<p style="color: grey;"><i>Updated March 25, 2014, 11:48 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
extra-cmake-modules
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This simplifies translation handling for frameworks using Qt translation system.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Here is a patch which make kbookmarks use it: http://agateau.com/tmp/kbookmarks-tr.diff . The necessary changes for the Messages.sh part of the patch are being reviewed for inclusion in the l10n-kde4 repository.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>modules/ECMTrLoader.cpp.in <span style="color: grey">(PRE-CREATION)</span></li>
<li>modules/ECMSetupQtTranslations.cmake <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/117052/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>