Review Request 117052: Add ECMSetupQtTranslations
Aurélien Gâteau
agateau at kde.org
Thu Mar 27 10:22:20 UTC 2014
> 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-frameworks-devel/attachments/20140327/48a7a12c/attachment.html>
More information about the Kde-frameworks-devel
mailing list