> 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
>
>
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
