Review Request 111689: desktoptojson -- convert .desktop files to .json for plugin metadata

[Milian Wolff]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111689/#review36906
-----------------------------------------------------------


Some more nitpicks from my side - sorry Sebas I hope you don't think I'm too pedantic :) But I hope others read this as well and start following the new best-practices for Qt5 codebases.


staging/kservice/tools/desktoptojson/kconfigtojson.cpp
<http://git.reviewboard.kde.org/r/111689/#comment27217>

    remove



staging/kservice/tools/desktoptojson/kconfigtojson.cpp
<http://git.reviewboard.kde.org/r/111689/#comment27218>

    use <QCommandLineParser> or whatever its called, to follow the style of the includes below



staging/kservice/tools/desktoptojson/kconfigtojson.cpp
<http://git.reviewboard.kde.org/r/111689/#comment27216>

    both are not used anymore, or?



staging/kservice/tools/desktoptojson/kconfigtojson.cpp
<http://git.reviewboard.kde.org/r/111689/#comment27208>

    endl instead of "\n" and no nead for .toLocal8Bit().constData()
    
    just:
    
    QTextStream out(stdout)
    out << msg << endl;



staging/kservice/tools/desktoptojson/kconfigtojson.cpp
<http://git.reviewboard.kde.org/r/111689/#comment27209>

    use intializer list
    
    KConfigToJson::KConfigToJson(...)
        : m_parser(parser)
    {
    }



staging/kservice/tools/desktoptojson/kconfigtojson.cpp
<http://git.reviewboard.kde.org/r/111689/#comment27210>

    that's not debug, that's an error message, or?
    
    I really suggest you get rid of the coutput free function and instead add these two at the top:
    
    static QTextStream cout(stdout);
    static QTextStream cerr(stderr);
    
    Then use them wherever appropriate, e.g. here:
    
    cerr << "Failed to resolve filenames" << m_inFile << m_outFile << endl;



staging/kservice/tools/desktoptojson/kconfigtojson.cpp
<http://git.reviewboard.kde.org/r/111689/#comment27211>

    if you use a static cout as I showed above, the QStringLiteral won't be necessary.



staging/kservice/tools/desktoptojson/kconfigtojson.cpp
<http://git.reviewboard.kde.org/r/111689/#comment27212>

    share the QStringLiteral, i.e. at the top add
    
    static const QString INPUT = QStringLiteral("input");
    
    Then use it here and above. Do the same for "output" below.



staging/kservice/tools/desktoptojson/kconfigtojson.cpp
<http://git.reviewboard.kde.org/r/111689/#comment27213>

    QStringLiteral for both



staging/kservice/tools/desktoptojson/kconfigtojson.cpp
<http://git.reviewboard.kde.org/r/111689/#comment27214>

    use QStringLiteral for the values



staging/kservice/tools/desktoptojson/main.cpp
<http://git.reviewboard.kde.org/r/111689/#comment27215>

    I'm not sure about the style guide in KDELibs, but aren't the <KLocalizedString> kind of includes preferred? Same for the Qt files.


- Milian Wolff


On July 30, 2013, 7:45 p.m., Sebastian Kügler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111689/
> -----------------------------------------------------------
> 
> (Updated July 30, 2013, 7:45 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Description
> -------
> 
> Small program which takes a .desktop file and converts it to json. This is useful to convert plugins which have their metadata in .desktop files (i.e. all KDE plugins) to Qt's new plugin system.
> 
> 
> Diffs
> -----
> 
>   staging/kservice/tools/CMakeLists.txt PRE-CREATION 
>   staging/kservice/tools/desktoptojson/CMakeLists.txt PRE-CREATION 
>   staging/kservice/tools/desktoptojson/kconfigtojson.h PRE-CREATION 
>   staging/kservice/tools/desktoptojson/kconfigtojson.cpp PRE-CREATION 
>   staging/kservice/tools/desktoptojson/main.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/111689/diff/
> 
> 
> Testing
> -------
> 
> Converted metadata of several plugins and used them from QPluginLoader -- works.
> 
> 
> Thanks,
> 
> Sebastian Kügler
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20130801/21e558bc/attachment.html>