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

[Commit Hook]

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


This review has been submitted with commit 
4df6c4926ef1d01acea6a85dfc5bca3bea8a40df by Sebastian Kügler to branch 
frameworks.

- Commit Hook


On Aug. 7, 2013, 1:20 a.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111689/
> ---
> 
> (Updated Aug. 7, 2013, 1:20 a.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/src/desktoptojson/CMakeLists.txt PRE-CREATION 
>   staging/kservice/src/desktoptojson/kconfigtojson.h PRE-CREATION 
>   staging/kservice/src/desktoptojson/kconfigtojson.cpp PRE-CREATION 
>   staging/kservice/src/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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Commit Hook]

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

(Updated Aug. 7, 2013, 1:28 a.m.)


Status
--

This change has been marked as submitted.


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/src/desktoptojson/CMakeLists.txt PRE-CREATION 
  staging/kservice/src/desktoptojson/kconfigtojson.h PRE-CREATION 
  staging/kservice/src/desktoptojson/kconfigtojson.cpp PRE-CREATION 
  staging/kservice/src/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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Sebastian Kügler]

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

(Updated Aug. 7, 2013, 1:20 a.m.)


Review request for KDE Frameworks and David Faure.


Changes
---

Use QFileInfo, constness.

Since this patch already has a ship it, I'll do just that. If there are more 
cosmetical changes, we can always improve it later on. :)


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 (updated)
-

  staging/kservice/src/desktoptojson/CMakeLists.txt PRE-CREATION 
  staging/kservice/src/desktoptojson/kconfigtojson.h PRE-CREATION 
  staging/kservice/src/desktoptojson/kconfigtojson.cpp PRE-CREATION 
  staging/kservice/src/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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Kevin Krammer]

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



staging/kservice/src/desktoptojson/kconfigtojson.cpp


if you create a QFileInfo object you can ask it for exits and use its 
isAbsolute() check later on instead of assuming Unix paths in line 72


- Kevin Krammer


On Aug. 5, 2013, 11:29 p.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111689/
> ---
> 
> (Updated Aug. 5, 2013, 11:29 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/src/desktoptojson/CMakeLists.txt PRE-CREATION 
>   staging/kservice/src/desktoptojson/kconfigtojson.h PRE-CREATION 
>   staging/kservice/src/desktoptojson/kconfigtojson.cpp PRE-CREATION 
>   staging/kservice/src/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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: 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/#review37175
---



staging/kservice/src/desktoptojson/kconfigtojson.h


that should be const&,no?


- Milian Wolff


On Aug. 5, 2013, 11:29 p.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111689/
> ---
> 
> (Updated Aug. 5, 2013, 11:29 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/src/desktoptojson/CMakeLists.txt PRE-CREATION 
>   staging/kservice/src/desktoptojson/kconfigtojson.h PRE-CREATION 
>   staging/kservice/src/desktoptojson/kconfigtojson.cpp PRE-CREATION 
>   staging/kservice/src/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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Sebastian Kügler]

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

(Updated Aug. 5, 2013, 11:29 p.m.)


Review request for KDE Frameworks and David Faure.


Changes
---

put the options in the members, not the strings.


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 (updated)
-

  staging/kservice/src/desktoptojson/CMakeLists.txt PRE-CREATION 
  staging/kservice/src/desktoptojson/kconfigtojson.h PRE-CREATION 
  staging/kservice/src/desktoptojson/kconfigtojson.cpp PRE-CREATION 
  staging/kservice/src/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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[David Faure]

> On Aug. 2, 2013, 9:08 a.m., David Faure wrote:
> > staging/kservice/tools/desktoptojson/kconfigtojson.h, line 31
> > 
> >
> > I don't like file-static QStrings (global objects), and even less in 
> > headers (it creates an unused variable in any user of the header).
> > 
> > My suggestion: define the command-line options in the KConfigToJson 
> > constructor or runMain().
> > 
> > To go even further and avoid the need for shared strings completely: 
> > store the two QCommandLineOptions as members of the class, and pass them to 
> > isSet() and value(), instead of strings.
> >
> 
> Milian Wolff wrote:
> What's wrong with file-static QStrings that come out of a QStringLiteral? 
> This can/should imo be seen similar to a static const int or similar. If we 
> could rely on C++11 we'd write it as "constexpr QString foo = 
> QStringLiteral("bar");" after all, no?

Well maybe I still have bad memories of initialization issues with 
QString::null in qt3; creating global qstrings could give initialization-order 
troubles there.

In any case, global objects in a header file are bad, since they get duplicated 
in every .cpp file that includes the header (so much for sharing).

Sebas: my suggestion was to store the QCommandLineOption instances btw, rather 
than strings. But OK, whatever :-)


- David


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


On Aug. 4, 2013, 3:58 a.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111689/
> ---
> 
> (Updated Aug. 4, 2013, 3:58 a.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/src/desktoptojson/CMakeLists.txt PRE-CREATION 
>   staging/kservice/src/desktoptojson/kconfigtojson.h PRE-CREATION 
>   staging/kservice/src/desktoptojson/kconfigtojson.cpp PRE-CREATION 
>   staging/kservice/src/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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Sebastian Kügler]

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

(Updated Aug. 4, 2013, 3:58 a.m.)


Review request for KDE Frameworks and David Faure.


Changes
---

constness. Remove unneeded member.


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 (updated)
-

  staging/kservice/src/desktoptojson/CMakeLists.txt PRE-CREATION 
  staging/kservice/src/desktoptojson/kconfigtojson.h PRE-CREATION 
  staging/kservice/src/desktoptojson/kconfigtojson.cpp PRE-CREATION 
  staging/kservice/src/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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Sebastian Kügler]

> On Aug. 3, 2013, 10:20 p.m., Milian Wolff wrote:
> > staging/kservice/src/desktoptojson/kconfigtojson.h, line 37
> > 
> >
> > why members? that's imo a "verschlimmbesserung". If at all, mark them 
> > as static const.

const: d'oh!

static: doesn't work, it asks me to move it outside of the class, which dfaure 
doesn't like. Otherwise, I'm getting QString::fromUtf8() not allowed there


- Sebastian


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


On Aug. 4, 2013, 3:58 a.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111689/
> ---
> 
> (Updated Aug. 4, 2013, 3:58 a.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/src/desktoptojson/CMakeLists.txt PRE-CREATION 
>   staging/kservice/src/desktoptojson/kconfigtojson.h PRE-CREATION 
>   staging/kservice/src/desktoptojson/kconfigtojson.cpp PRE-CREATION 
>   staging/kservice/src/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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Milian Wolff]

> On Aug. 2, 2013, 9:08 a.m., David Faure wrote:
> > staging/kservice/tools/desktoptojson/kconfigtojson.h, line 31
> > 
> >
> > I don't like file-static QStrings (global objects), and even less in 
> > headers (it creates an unused variable in any user of the header).
> > 
> > My suggestion: define the command-line options in the KConfigToJson 
> > constructor or runMain().
> > 
> > To go even further and avoid the need for shared strings completely: 
> > store the two QCommandLineOptions as members of the class, and pass them to 
> > isSet() and value(), instead of strings.
> >

What's wrong with file-static QStrings that come out of a QStringLiteral? This 
can/should imo be seen similar to a static const int or similar. If we could 
rely on C++11 we'd write it as "constexpr QString foo = QStringLiteral("bar");" 
after all, no?


- Milian


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


On Aug. 3, 2013, 10:07 p.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111689/
> ---
> 
> (Updated Aug. 3, 2013, 10:07 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/src/desktoptojson/CMakeLists.txt PRE-CREATION 
>   staging/kservice/src/desktoptojson/kconfigtojson.h PRE-CREATION 
>   staging/kservice/src/desktoptojson/kconfigtojson.cpp PRE-CREATION 
>   staging/kservice/src/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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: 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/#review37039
---



staging/kservice/src/desktoptojson/kconfigtojson.h


why members? that's imo a "verschlimmbesserung". If at all, mark them as 
static const.


- Milian Wolff


On Aug. 3, 2013, 10:07 p.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111689/
> ---
> 
> (Updated Aug. 3, 2013, 10:07 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/src/desktoptojson/CMakeLists.txt PRE-CREATION 
>   staging/kservice/src/desktoptojson/kconfigtojson.h PRE-CREATION 
>   staging/kservice/src/desktoptojson/kconfigtojson.cpp PRE-CREATION 
>   staging/kservice/src/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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Sebastian Kügler]

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

(Updated Aug. 3, 2013, 10:07 p.m.)


Review request for KDE Frameworks and David Faure.


Changes
---

Moved strings into members, addressed other comments.


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 (updated)
-

  staging/kservice/src/desktoptojson/CMakeLists.txt PRE-CREATION 
  staging/kservice/src/desktoptojson/kconfigtojson.h PRE-CREATION 
  staging/kservice/src/desktoptojson/kconfigtojson.cpp PRE-CREATION 
  staging/kservice/src/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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[David Faure]

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



staging/kservice/tools/desktoptojson/kconfigtojson.h


I don't like file-static QStrings (global objects), and even less in 
headers (it creates an unused variable in any user of the header).

My suggestion: define the command-line options in the KConfigToJson 
constructor or runMain().

To go even further and avoid the need for shared strings completely: store 
the two QCommandLineOptions as members of the class, and pass them to isSet() 
and value(), instead of strings.




staging/kservice/tools/desktoptojson/kconfigtojson.cpp


;; -> ;



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


;; -> ;



staging/kservice/tools/desktoptojson/main.cpp


It makes no difference in this particular case, but it's always better to 
create the app first, and then use any other Qt API => swap the first two lines.


- David Faure


On Aug. 1, 2013, 6:59 p.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111689/
> ---
> 
> (Updated Aug. 1, 2013, 6:59 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Kevin Ottens]

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



staging/kservice/tools/CMakeLists.txt


Should be under src



staging/kservice/tools/desktoptojson/kconfigtojson.h


Why those default parameters? The implementation code actually doesn't seem 
ready to be called with no parameters, and is never called that way.
You probably want to drop the default values here.



staging/kservice/tools/desktoptojson/main.cpp


Why 2.0 and not 1.0? :-)



- Kevin Ottens


On Aug. 1, 2013, 6:59 p.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111689/
> ---
> 
> (Updated Aug. 1, 2013, 6:59 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: 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/#review36947
---

Ship it!


awesome, I like it now :)

- Milian Wolff


On Aug. 1, 2013, 6:59 p.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111689/
> ---
> 
> (Updated Aug. 1, 2013, 6:59 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Milian Wolff]

> On Aug. 1, 2013, 6:14 p.m., Milian Wolff wrote:
> > staging/kservice/tools/desktoptojson/kconfigtojson.cpp, line 60
> > 
> >
> > just do return convert(...);
> 
> Nicolás Alvarez wrote:
> No, that would give the opposite return value. To have the same behavior 
> you need return !convert(...), or return convert(...) ? 0 : 1;

Thanks :)


- Milian


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


On Aug. 1, 2013, 6:59 p.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111689/
> ---
> 
> (Updated Aug. 1, 2013, 6:59 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Sebastian Kügler]

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

(Updated Aug. 1, 2013, 6:59 p.m.)


Review request for KDE Frameworks and David Faure.


Changes
---

Fixed indentation in header.


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 (updated)
-

  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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Sebastian Kügler]

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

(Updated Aug. 1, 2013, 6:56 p.m.)


Review request for KDE Frameworks and David Faure.


Changes
---

Updated patch.

Also, licensing is LGPL2.1+ as per kdelibs policy, not 3.x.


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 (updated)
-

  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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Nicolás Alvarez]

> On Aug. 1, 2013, 6:14 p.m., Milian Wolff wrote:
> > staging/kservice/tools/desktoptojson/kconfigtojson.cpp, line 60
> > 
> >
> > just do return convert(...);

No, that would give the opposite return value. To have the same behavior you 
need return !convert(...), or return convert(...) ? 0 : 1;


- Nicolás


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


On Aug. 1, 2013, 6:03 p.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111689/
> ---
> 
> (Updated Aug. 1, 2013, 6:03 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: 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/#review36933
---


nearly there! :)


staging/kservice/tools/desktoptojson/kconfigtojson.h


this is not being used here, QString include should be enough I think.



staging/kservice/tools/desktoptojson/kconfigtojson.h


is this the coding style of KDELibs?

class Foo
{
public:
Foo();
};

I assumed this is it:

class Foo
{
public:
Foo();
};

But the coding style does not tell which one it is. Maybe David Faure or so 
should say what is preferred.



staging/kservice/tools/desktoptojson/kconfigtojson.h


no need for virtual, actually no need for the dtor at all.



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


then don't pass them along at all.



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


just do return convert(...);



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


same as below, check for the invalidity first, then return early. move the 
rest out of the conditional



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


personally, I prefer to reduce the conditional clutter by doing things like 
this:

if (...) {
  var = asdf();
  if (!valid(var)) {
return false;
  }
  do_stuff();
}



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


merge with above to else if?


- Milian Wolff


On Aug. 1, 2013, 6:03 p.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111689/
> ---
> 
> (Updated Aug. 1, 2013, 6:03 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Kevin Krammer]

> On Aug. 1, 2013, 4:48 p.m., Kevin Krammer wrote:
> > staging/kservice/tools/desktoptojson/kconfigtojson.cpp, line 57
> > 
> >
> > Do we want the output/errors to be translated?
> 
> Sebastian Kügler wrote:
> It's really a helper tool for the build process, so no. I've removed the 
> translations from main.cpp as well.

I see.
I am not expert on CMake, so the questions becomes: is this the right way to 
ensure it does not end up in /usr/bin or something?
But rather in a place where it can be found by the build system when needed?


- Kevin


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


On Aug. 1, 2013, 6:03 p.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111689/
> ---
> 
> (Updated Aug. 1, 2013, 6:03 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Sebastian Kügler]

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

(Updated Aug. 1, 2013, 6:03 p.m.)


Review request for KDE Frameworks and David Faure.


Changes
---

Licensing header: lgpl3+ kde e.V. as recommended in kdelibs licensing policy


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 (updated)
-

  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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Sebastian Kügler]

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

(Updated Aug. 1, 2013, 5:49 p.m.)


Review request for KDE Frameworks and David Faure.


Changes
---

Also share the strings in main.cpp.


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 (updated)
-

  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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Sebastian Kügler]

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

(Updated Aug. 1, 2013, 5:44 p.m.)


Review request for KDE Frameworks and David Faure.


Changes
---

Addressed comments, thanks!


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 (updated)
-

  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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Sebastian Kügler]

> On Aug. 1, 2013, 4:48 p.m., Kevin Krammer wrote:
> > staging/kservice/tools/desktoptojson/kconfigtojson.cpp, line 57
> > 
> >
> > Do we want the output/errors to be translated?

It's really a helper tool for the build process, so no. I've removed the 
translations from main.cpp as well.


- Sebastian


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


On Aug. 1, 2013, 4:32 p.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111689/
> ---
> 
> (Updated Aug. 1, 2013, 4:32 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/desktoptojson/CMakeLists.txt PRE-CREATION 
>   staging/kservice/tools/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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Kevin Krammer]

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



staging/kservice/tools/desktoptojson/kconfigtojson.h


sorry for more style nitpicking:
int &argc, char **argv



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


int &argc



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


Do we want the output/errors to be translated?



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


since the purpose of those to containers is lookup, maybe QSet 
instead?
Not that it matters a lot given there are only two keys, just saying :)


- Kevin Krammer


On Aug. 1, 2013, 4:32 p.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111689/
> ---
> 
> (Updated Aug. 1, 2013, 4:32 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/desktoptojson/CMakeLists.txt PRE-CREATION 
>   staging/kservice/tools/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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Sebastian Kügler]

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

(Updated Aug. 1, 2013, 4:32 p.m.)


Review request for KDE Frameworks and David Faure.


Changes
---

All issues addressed, thanks Kevin and Milian for the reviews! 


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 (updated)
-

  staging/kservice/tools/desktoptojson/CMakeLists.txt PRE-CREATION 
  staging/kservice/tools/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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Sebastian Kügler]

> On Aug. 1, 2013, 12:07 p.m., Milian Wolff wrote:
> > 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.

No problem at all, in fact I much appreciate the thorough review, as indeed 
this is a good way to transfer knowledge about best practises. :)


> On Aug. 1, 2013, 12:07 p.m., Milian Wolff wrote:
> > staging/kservice/tools/desktoptojson/main.cpp, line 20
> > 
> >
> > I'm not sure about the style guide in KDELibs, but aren't the 
> >  kind of includes preferred? Same for the Qt files.

There's no header KLocalizedString, only klocalizedstring.h exists. I'll leave 
that for now, as it needs addressing elsewhere first.


> On Aug. 1, 2013, 12:07 p.m., Milian Wolff wrote:
> > staging/kservice/tools/desktoptojson/kconfigtojson.cpp, line 26
> > 
> >
> > use  or whatever its called, to follow the style of 
> > the includes below

There's no CamelCase header for QCommandLineParser. I'll leave it as-is here.


- Sebastian


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


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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: 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


remove



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


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



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


both are not used anymore, or?



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


endl instead of "\n" and no nead for .toLocal8Bit().constData()

just:

QTextStream out(stdout)
out << msg << endl;



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


use intializer list

KConfigToJson::KConfigToJson(...)
: m_parser(parser)
{
}



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


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


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



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


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


QStringLiteral for both



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


use QStringLiteral for the values



staging/kservice/tools/desktoptojson/main.cpp


I'm not sure about the style guide in KDELibs, but aren't the 
 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Kevin Krammer]

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



staging/kservice/tools/desktoptojson/kconfigtojson.h


maybe place the & and ** at the argument name to be consistent with other 
arguments?



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


Maybe use QStringLiteral? I.e. try to make it build with NO_CAST_FROM_ASCII?



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


different style in & placement in header/source


- Kevin Krammer


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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Sebastian Kügler]

---
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.


Changes
---

Thanks for the review. All issues have been addressed in this version.


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 (updated)
-

  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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: 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/#review36503
---



staging/kservice/tools/desktoptojson/kconfigtojson.h


If you stick to using a dptr (doesn't make much sense, but well), use a 
QScopedPointer. This should become the standard way to handle this pattern.



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


why this function at all? it looks like a poor-man's qDebug/kDebug?

If you want to ensure the output is always readable, I suggest adding a 
global/class-local QTextStream out(stdout); which you then use like 
qDebug/kDebug.



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


here and below: These strings should use QStringLiteral, and be shared 
wherever possible.



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


imo, just run d->parser->process(*this) directly and run quit() afterwards.



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


this and the one below should probably be static.



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


why not directly file.write(jdoc.toJson())?



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


will be done automatically when the function's scope is left.


- Milian Wolff


On July 25, 2013, 4:10 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 25, 2013, 4:10 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Sune Vuorela]

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



staging/kservice/tools/desktoptojson/kconfigtojson.h


No need to have it inherit anything



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


maybe qPrintable(msg) instead



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


As such there is no need for a d-pointer, but it also doesn't hurt.



staging/kservice/tools/desktoptojson/kconfigtojson.cpp


Timer usage could be skipped with no eventloop running



staging/kservice/tools/desktoptojson/main.cpp


I don't think you need to 'new' it here. just let the scoping handle the 
deletion



staging/kservice/tools/desktoptojson/main.cpp


Just do it as a QCoreApplication first and then create a KConfigToJson 
afterwards



staging/kservice/tools/desktoptojson/main.cpp


and just return kconfigtojson.runMain(); here


For inspiration for doing simple command line tools, you could also take a look 
at https://codereview.qt-project.org/#change,60560,patchset=5 which has a 
simpler 'top down' structure which is how CLI-apps usually are.

- Sune Vuorela


On July 25, 2013, 4:10 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 25, 2013, 4:10 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Sebastian Kügler]

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

(Updated July 25, 2013, 4:10 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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel