Review Request 117052: Add ECMSetupQtTranslations

[Aurélien Gâteau]

> On March 25, 2014, 3:33 p.m., Alex Merry wrote:
> > 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.
> 
> Aurélien Gâteau wrote:
>     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?
> 
> Alex Merry wrote:
>     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.

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)?


- Aurélien


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117052/#review54111
-----------------------------------------------------------


On March 25, 2014, 11:48 a.m., Aurélien Gâteau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117052/
> -----------------------------------------------------------
> 
> (Updated March 25, 2014, 11:48 a.m.)
> 
> 
> Review request for Build System, Extra Cmake Modules and KDE Frameworks.
> 
> 
> Repository: extra-cmake-modules
> 
> 
> Description
> -------
> 
> This simplifies translation handling for frameworks using Qt translation system.
> 
> 
> Diffs
> -----
> 
>   modules/ECMTrLoader.cpp.in PRE-CREATION 
>   modules/ECMSetupQtTranslations.cmake PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/117052/diff/
> 
> 
> Testing
> -------
> 
> 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.
> 
> 
> Thanks,
> 
> Aurélien Gâteau
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-buildsystem/attachments/20140327/48a7a12c/attachment.html>