Review Request 117052: Add ECMSetupQtTranslations

[Alex Merry]

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



modules/ECMSetupQtTranslations.cmake
<https://git.reviewboard.kde.org/r/117052/#comment38003>

    That's quite an uninformative first line :-)
    
    The general doc style of CMake modules that provide only one function appears to be just to go straight into the documentation.  So start with the prototype, then the descrption of it (the second para here), and detail arguments.



modules/ECMSetupQtTranslations.cmake
<https://git.reviewboard.kde.org/r/117052/#comment38004>

    Lowercase



modules/ECMSetupQtTranslations.cmake
<https://git.reviewboard.kde.org/r/117052/#comment38005>

    generates -> generate



modules/ECMSetupQtTranslations.cmake
<https://git.reviewboard.kde.org/r/117052/#comment38015>

    I think the name should somehow indicate that it's for integrating a gettext-based workflow into Qt's l10n support.  A macro to facilitate a Linguist-based workflow would also be reasonable (not that KDE would have much use for it).



modules/ECMSetupQtTranslations.cmake
<https://git.reviewboard.kde.org/r/117052/#comment38006>

    I prefer saying that a parameter "is" something to "must be".  eg: "PO_DIR is the path to the directory containing .p files".
    
    Save "must" for extra constraints, like the location of POT_NAME.



modules/ECMSetupQtTranslations.cmake
<https://git.reviewboard.kde.org/r/117052/#comment38007>

    You may as well just say <source_file_var>



modules/ECMSetupQtTranslations.cmake
<https://git.reviewboard.kde.org/r/117052/#comment38008>

    Just a nitpick, but the variable normally ends _SRCS



modules/ECMSetupQtTranslations.cmake
<https://git.reviewboard.kde.org/r/117052/#comment38009>

    License!



modules/ECMSetupQtTranslations.cmake
<https://git.reviewboard.kde.org/r/117052/#comment38010>

    Is it worth filing a Qt bug?  If there already is one, please put the link in the comment.



modules/ECMSetupQtTranslations.cmake
<https://git.reviewboard.kde.org/r/117052/#comment38011>

    FindGettext calls the target pofiles.  Is it worth making this target qmfiles for consistency?



modules/ECMSetupQtTranslations.cmake
<https://git.reviewboard.kde.org/r/117052/#comment38013>

    Why is this a macro, rather than a function?  Don't forget the PARENT_SCOPE argument of set()



modules/ECMSetupQtTranslations.cmake
<https://git.reviewboard.kde.org/r/117052/#comment38012>

    Please use the style
      set(ecm_sqt_options)
      set(ecm_sqt_oneValueArgs ARGS HERE)
      set(ecm_sqt_multiValueArgs)
      cmake_parse_arguments(ECM_SQT "${ecm_sqt_options}" "${ecm_sqt_oneValueArgs}" "${ecm_sqt_multiValueArgs}" ${ARGN})
    as this makes it much clearer what's going on.
    
    Also, make sure global variables start with ECM_ (so ECM_SQT here), so we can guarantee not to clash with any other packages.



modules/ECMTrLoader.cpp.in
<https://git.reviewboard.kde.org/r/117052/#comment38014>

    QLatin1String() when you're using +


- Alex Merry


On March 27, 2014, 3:26 p.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 27, 2014, 3:26 p.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/ECMSetupQtTranslations.cmake PRE-CREATION 
>   modules/ECMTrLoader.cpp.in 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/d4c61a0c/attachment-0001.html>