<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/117823/">https://git.reviewboard.kde.org/r/117823/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On , <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;">(commenting my own RR to split the discussion from the description)
I expect this function to become the most used one from this CMake module (together with ecm_create_qm_loader()) and the ecm_create_qm_from_po_files() to be less relevant. It is odd to name the CMake module after the less relevant function.
What do you think of doing the following?
- Create a new ECMPoQmTools.cmake file, and do the following in it:
-- Add ecm_install_po_files_as_qm()
-- Move ecm_create_qm_loader() into it
-- Create a ecm_process_po_files_as_qm() function, which would have the same signature as gettext_process_po_files() (can be done now because it makes sense to pass the language as first argument)
- Modify ECMCreateQmFromPoFiles.cmake like this:
-- Revert the latest changes to ecm_create_qm_from_po_files()
-- Include ECMPoQmTools.cmake (so that ecm_create_qm_loader() is still available when including the old file)
-- Mark the whole module as deprecated by adding a 'message(DEPRECATION "ECMCreateQmFromPoFiles.cmake is deprecated. Use ECMPoQmTools.cmake instead.")' at the start of the module.</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;">Well, you can't use DEPRECATION, as that doesn't exist in CMake 2.8.12. Otherwise, I think this is sensible. I'm definitely in favour of following the gettext_process_po_files() signature.
I'm inclined to remove ECMCreateQmFromPoFiles.cmake entirely before 1.0, though (but maybe leave it in with an AUTHOR_WARNING for beta2, just to cause fewer issues for people updating only parts of their tree). It's not like anything other than some of the frameworks has made use of it.
One thing I will say is: unit tests, please! See tests/ECMInstallIconsTest/ for inspiration.</pre>
<br />
<p>- Alex</p>
<br />
<p>On April 28th, 2014, 9:07 a.m. UTC, 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 April 28, 2014, 9:07 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 patch adds a ecm_install_po_files_as_qm() function to ECMCreateQmFromPoFiles.cmake. This function makes it easy to install all the .po files of a Qt-translated framework.
The primary user of this function is the build system of the framework tarballs. The tarballs will ship with a po/ directory which follows the structure expected by ecm_install_po_files_as_qm(). The only necessary change is to add the following lines to the root CMakeLists.txt file:
if (IS_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/po")
ecm_install_po_files_as_qm(po)
endif()
Since the code checks for the presence of a po/ directory, it can be committed in the framework repository (no need for the tarball release scripts to alter the CMake files), making it easier for developers to reproduce the situation of a released tarball by creating/copying a po/ directory in their working copy.
We need this function (or something similar) because the way we are going to organize po files in the framework tarballs have changed: the current CMake code expects the po files to contain files named '<domain>-<lang>.po', but the final structure is going to be '<lang>/<domain>.po'.
This new function works similarly to ki18n_install() ( https://git.reviewboard.kde.org/r/117804/ ) and kdoctools_install() ( https://git.reviewboard.kde.org/r/117805/ )</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;">Modified kbookmarks to call ecm_install_po_files_as_qm(). Works as expected.</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/ECMCreateQmFromPoFiles.cmake <span style="color: grey">(7457fc5)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/117823/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>