Review Request 126185: Make the KAppTemplate CMake module global

[Alex Merry]

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


We're getting there :-).


kde-modules/KDEInstallDirs.cmake (line 516)
<https://git.reviewboard.kde.org/r/126185/#comment61140>

    This variable should be documented.



kde-modules/KDEInstallDirs.cmake (line 518)
<https://git.reviewboard.kde.org/r/126185/#comment61139>

    The (optional) 5th argument to this macro is a compatibility variable name. Since KTEMPLATES_INSTALL_DIR didn't exist before, it shouldn't exist now, so this argument should be removed.



kde-modules/KDETemplateGenerator.cmake (line 5)
<https://git.reviewboard.kde.org/r/126185/#comment61144>

    I think it probably is more accurate to say that it "packages" the templates, rather than "generates" them. The module name should be changed to match as well.
    
    You should also note that the templates are not just packaged but installed.



kde-modules/KDETemplateGenerator.cmake (lines 7 - 10)
<https://git.reviewboard.kde.org/r/126185/#comment61145>

    Heh, I think I see where you copied the documentation formatting from. This paragraph should go.



kde-modules/KDETemplateGenerator.cmake (line 12)
<https://git.reviewboard.kde.org/r/126185/#comment61146>

    "function" (singular - there is only one).



kde-modules/KDETemplateGenerator.cmake (line 14)
<https://git.reviewboard.kde.org/r/126185/#comment61142>

    OK, what needs to be in the subdirectory? are template1, template2 etc the subdriectory names? The packages file name?
    
    The following sentence suggests this command only creates a single template tarball, but the signature here has multiple arguments.
    
    Also, the command doesn't need to include the word "template" twice. I suggest kde_add_app_template.
    
    Finally, the signature now includes the TEMPLATES keyword.



kde-modules/KDETemplateGenerator.cmake (lines 16 - 17)
<https://git.reviewboard.kde.org/r/126185/#comment61141>

    These are separate sentences, and should have the appropriate punctuation (capital letters and full stops).



kde-modules/KDETemplateGenerator.cmake (line 20)
<https://git.reviewboard.kde.org/r/126185/#comment61143>

    We're way past 5.10 by now...



kde-modules/KDETemplateGenerator.cmake (lines 38 - 48)
<https://git.reviewboard.kde.org/r/126185/#comment61147>

    This looks like you copied it out of KDECompilerSettings as well. It doesn't seem to be relevant here.



kde-modules/KDETemplateGenerator.cmake (line 52)
<https://git.reviewboard.kde.org/r/126185/#comment61148>

    Honestly, I'd just use ARG as the prefix - you're in a function, it's not going to leak anyway. That means you can refer to ARG_TEMPLATES, which is both shorter and more descriptive.
    
    You should also check for unparsed arguments, and for the TEMPLATES argument being missing or empty. See, eg, ECMAddAppIcon.cmake for how to do this.



kde-modules/KDETemplateGenerator.cmake (line 84)
<https://git.reviewboard.kde.org/r/126185/#comment61149>

    Use ``KDE_INSTALL_KTEMPLATESDIR`` instead of ``KTEMPLATES_INSTALL_DIR``. You also need to document that this variable needs to be set before using the command.
    
    My preference would actually be to make it explicit in the signature that installation will happen, by having the usage be something like
    
        kde_package_app_templates(
            TEMPLATES
                foo
                bar
                baz
            INSTALL_DIR
                "${KDE_INSTALL_KTEMPLATESDIR}"
        )
    
    My view is that this makes it explicit what will happen when you're reading the code - it will package the templates and install them. But I'm not going to force your hand on this if you'd rather not do it that way.


- Alex Merry


On Dec. 9, 2015, 12:12 p.m., Marco Martin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126185/
> -----------------------------------------------------------
> 
> (Updated Dec. 9, 2015, 12:12 p.m.)
> 
> 
> Review request for Build System, KDE Frameworks, Plasma, Aleix Pol Gonzalez, and Simon Wächter.
> 
> 
> Repository: extra-cmake-modules
> 
> 
> Description
> -------
> 
> templates are very useful as teaching tool in order to make
> a minimal application that uses a certain framework.
> templates in the KAppTemplate repository will always get forgotten
> (plus kapptemplate is not really necessary as they work in kdevelop as well)
> An ideal situation would be frameworks having templates in their own repos
> with templates of barebone apps using the main framework features.
> In order to do that, the cmake stuff needed in order to correctly install
> a template needs to be ported to a place avaiable to all frameworks
> 
> 
> Diffs
> -----
> 
>   kde-modules/KDETemplateGenerator.cmake PRE-CREATION 
>   kde-modules/KDEInstallDirs.cmake b7cd34d 
> 
> Diff: https://git.reviewboard.kde.org/r/126185/diff/
> 
> 
> Testing
> -------
> 
> done some templates installed by plasma-framework
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-buildsystem/attachments/20151212/5f8df222/attachment-0001.html>