Re: Review Request 125766: Changed the manual command line parsing to use QCommandLineParser.

[Antonio Larrosa Jimenez]

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

(Updated Nov. 27, 2015, 12:07 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Repository: kded


Description
---

Changed the manual command line parsing to use QCommandLineParser instead of
parsing it manually. Also, now kded5 --version and kded5 --help show the
version information and help on options.

There's a new human readable text ("Check cache validity") that maybe
should be inside i18n, but since kded doesn't user i18n at all and
I wasn't sure it's desired to do so, I just left it untranslated.

Also, unset SESSION_MANAGER environment variable instead of setting it empty.
This removes a warning from Qt when parsing the (wrong, empty)
value of SESSION_MANAGER


Diffs
-

  src/kded.cpp 6929d7d3f24d3556f1b227d5a9bfb5b02b1c295e 

Diff: https://git.reviewboard.kde.org/r/125766/diff/


Testing
---

I tested kded5 runs, and the --check, --version and --help options work as 
expected.
Also, now it doesn't show a warning every time it's run. I didn't really check 
it's
not doing any session management, but SESSION_MANAGER is only used in one place 
in qtbase,
at 
[https://github.com/qtproject/qtbase/blob/dev/src/plugins/platforms/xcb/qxcbsessionmanager.cpp#L350](https://github.com/qtproject/qtbase/blob/dev/src/plugins/platforms/xcb/qxcbsessionmanager.cpp#L350)
and reading that, I think unsetting the environment variable makes more sense.


Thanks,

Antonio Larrosa Jimenez

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

Re: Review Request 125766: Changed the manual command line parsing to use QCommandLineParser.

[Antonio Larrosa Jimenez]

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

(Updated Nov. 27, 2015, 11:52 a.m.)


Review request for KDE Frameworks.


Changes
---

Fixed the issue David mentioned about setupAppInfo and added a comment about 
the removed optimization (using QCoreApplication in some cases -> using 
QApplication always)


Repository: kded


Description
---

Changed the manual command line parsing to use QCommandLineParser instead of
parsing it manually. Also, now kded5 --version and kded5 --help show the
version information and help on options.

There's a new human readable text ("Check cache validity") that maybe
should be inside i18n, but since kded doesn't user i18n at all and
I wasn't sure it's desired to do so, I just left it untranslated.

Also, unset SESSION_MANAGER environment variable instead of setting it empty.
This removes a warning from Qt when parsing the (wrong, empty)
value of SESSION_MANAGER


Diffs (updated)
-

  src/kded.cpp 6929d7d3f24d3556f1b227d5a9bfb5b02b1c295e 

Diff: https://git.reviewboard.kde.org/r/125766/diff/


Testing
---

I tested kded5 runs, and the --check, --version and --help options work as 
expected.
Also, now it doesn't show a warning every time it's run. I didn't really check 
it's
not doing any session management, but SESSION_MANAGER is only used in one place 
in qtbase,
at 
[https://github.com/qtproject/qtbase/blob/dev/src/plugins/platforms/xcb/qxcbsessionmanager.cpp#L350](https://github.com/qtproject/qtbase/blob/dev/src/plugins/platforms/xcb/qxcbsessionmanager.cpp#L350)
and reading that, I think unsetting the environment variable makes more sense.


Thanks,

Antonio Larrosa Jimenez

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

Re: Review Request 125766: Changed the manual command line parsing to use QCommandLineParser.

[David Faure]

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

Ship it!



src/kded.cpp (line 684)


Yep, someone should contribute a 
QGuiApplication::disableSessionManagement()...



src/kded.cpp (line 687)


this could move to setupAppInfo, or you could remove that separate method 
now that it's called only from one place. It just looks a bit inconsistent now.



src/kded.cpp 


The reason the code was the way it was, was to only create a 
QCoreApplication rather than a QApplication when --check is called, since this 
was done at KDE startup where time matters and core-only is sufficient for this.

This being said, I'm not sure --check is still used anywhere. I did some 
searching and I can't find where it's used (neither in kdelibs4+startkde4 or in 
KF5+startkde5).

So I guess I'll forget about the --check optimization (we can keep it for 
compat but we don't have to make it super efficient then)


- David Faure


On Oct. 23, 2015, 5:58 p.m., Antonio Larrosa Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125766/
> ---
> 
> (Updated Oct. 23, 2015, 5:58 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kded
> 
> 
> Description
> ---
> 
> Changed the manual command line parsing to use QCommandLineParser instead of
> parsing it manually. Also, now kded5 --version and kded5 --help show the
> version information and help on options.
> 
> There's a new human readable text ("Check cache validity") that maybe
> should be inside i18n, but since kded doesn't user i18n at all and
> I wasn't sure it's desired to do so, I just left it untranslated.
> 
> Also, unset SESSION_MANAGER environment variable instead of setting it empty.
> This removes a warning from Qt when parsing the (wrong, empty)
> value of SESSION_MANAGER
> 
> 
> Diffs
> -
> 
>   src/kded.cpp 6929d7d3f24d3556f1b227d5a9bfb5b02b1c295e 
> 
> Diff: https://git.reviewboard.kde.org/r/125766/diff/
> 
> 
> Testing
> ---
> 
> I tested kded5 runs, and the --check, --version and --help options work as 
> expected.
> Also, now it doesn't show a warning every time it's run. I didn't really 
> check it's
> not doing any session management, but SESSION_MANAGER is only used in one 
> place in qtbase,
> at 
> [https://github.com/qtproject/qtbase/blob/dev/src/plugins/platforms/xcb/qxcbsessionmanager.cpp#L350](https://github.com/qtproject/qtbase/blob/dev/src/plugins/platforms/xcb/qxcbsessionmanager.cpp#L350)
> and reading that, I think unsetting the environment variable makes more sense.
> 
> 
> Thanks,
> 
> Antonio Larrosa Jimenez
> 
>

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

Re: Review Request 125766: Changed the manual command line parsing to use QCommandLineParser.

[Antonio Larrosa Jimenez]

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

(Updated Oct. 23, 2015, 5:58 p.m.)


Review request for KDE Frameworks.


Changes
---

Used markup so that the link in the "Testing Done" box is really a link


Repository: kded


Description
---

Changed the manual command line parsing to use QCommandLineParser instead of
parsing it manually. Also, now kded5 --version and kded5 --help show the
version information and help on options.

There's a new human readable text ("Check cache validity") that maybe
should be inside i18n, but since kded doesn't user i18n at all and
I wasn't sure it's desired to do so, I just left it untranslated.

Also, unset SESSION_MANAGER environment variable instead of setting it empty.
This removes a warning from Qt when parsing the (wrong, empty)
value of SESSION_MANAGER


Diffs
-

  src/kded.cpp 6929d7d3f24d3556f1b227d5a9bfb5b02b1c295e 

Diff: https://git.reviewboard.kde.org/r/125766/diff/


Testing (updated)
---

I tested kded5 runs, and the --check, --version and --help options work as 
expected.
Also, now it doesn't show a warning every time it's run. I didn't really check 
it's
not doing any session management, but SESSION_MANAGER is only used in one place 
in qtbase,
at 
[https://github.com/qtproject/qtbase/blob/dev/src/plugins/platforms/xcb/qxcbsessionmanager.cpp#L350](https://github.com/qtproject/qtbase/blob/dev/src/plugins/platforms/xcb/qxcbsessionmanager.cpp#L350)
and reading that, I think unsetting the environment variable makes more sense.


Thanks,

Antonio Larrosa Jimenez

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

Re: Review Request 125766: Changed the manual command line parsing to use QCommandLineParser.

[Antonio Larrosa Jimenez]

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

(Updated Oct. 23, 2015, 4:50 p.m.)


Review request for KDE Frameworks.


Changes
---

Use qunsetenv instead of putenv and qstrdup. Thanks Aleix for reminding me of 
the Qt global functions for environment variable handling


Repository: kded


Description
---

Changed the manual command line parsing to use QCommandLineParser instead of
parsing it manually. Also, now kded5 --version and kded5 --help show the
version information and help on options.

There's a new human readable text ("Check cache validity") that maybe
should be inside i18n, but since kded doesn't user i18n at all and
I wasn't sure it's desired to do so, I just left it untranslated.

Also, unset SESSION_MANAGER environment variable instead of setting it empty.
This removes a warning from Qt when parsing the (wrong, empty)
value of SESSION_MANAGER


Diffs (updated)
-

  src/kded.cpp 6929d7d3f24d3556f1b227d5a9bfb5b02b1c295e 

Diff: https://git.reviewboard.kde.org/r/125766/diff/


Testing
---

I tested kded5 runs, and the --check, --version and --help options work as 
expected.
Also, now it doesn't show a warning every time it's run. I didn't really check 
it's
not doing any session management, but SESSION_MANAGER is only used in one place 
in qtbase,
at 
https://github.com/qtproject/qtbase/blob/dev/src/plugins/platforms/xcb/qxcbsessionmanager.cpp#L350
and reading that, I think unsetting the environment variable makes more sense.


Thanks,

Antonio Larrosa Jimenez

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

Re: Review Request 125766: Changed the manual command line parsing to use QCommandLineParser.

[Antonio Larrosa Jimenez]

> On Oct. 23, 2015, 3:14 p.m., Aleix Pol Gonzalez wrote:
> > src/kded.cpp, line 686
> > 
> >
> > Maybe it should set up KAboutData as well?

It's a QApplication, not a KApplication, so the only purpose of KAboutData 
would be calling KAboutData::setApplicationData, for it to call 
QApplication::setApplicationName/Version itself instead of directly, isn't it?


- Antonio


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


On Oct. 23, 2015, 2:36 p.m., Antonio Larrosa Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125766/
> ---
> 
> (Updated Oct. 23, 2015, 2:36 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kded
> 
> 
> Description
> ---
> 
> Changed the manual command line parsing to use QCommandLineParser instead of
> parsing it manually. Also, now kded5 --version and kded5 --help show the
> version information and help on options.
> 
> There's a new human readable text ("Check cache validity") that maybe
> should be inside i18n, but since kded doesn't user i18n at all and
> I wasn't sure it's desired to do so, I just left it untranslated.
> 
> Also, unset SESSION_MANAGER environment variable instead of setting it empty.
> This removes a warning from Qt when parsing the (wrong, empty)
> value of SESSION_MANAGER
> 
> 
> Diffs
> -
> 
>   src/kded.cpp 6929d7d3f24d3556f1b227d5a9bfb5b02b1c295e 
> 
> Diff: https://git.reviewboard.kde.org/r/125766/diff/
> 
> 
> Testing
> ---
> 
> I tested kded5 runs, and the --check, --version and --help options work as 
> expected.
> Also, now it doesn't show a warning every time it's run. I didn't really 
> check it's
> not doing any session management, but SESSION_MANAGER is only used in one 
> place in qtbase,
> at 
> https://github.com/qtproject/qtbase/blob/dev/src/plugins/platforms/xcb/qxcbsessionmanager.cpp#L350
> and reading that, I think unsetting the environment variable makes more sense.
> 
> 
> Thanks,
> 
> Antonio Larrosa Jimenez
> 
>

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

Re: Review Request 125766: Changed the manual command line parsing to use QCommandLineParser.

[Aleix Pol Gonzalez]

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



src/kded.cpp (line 684)


Use qputenv, also then the strdup isn't necessary.



src/kded.cpp (line 686)


Maybe it should set up KAboutData as well?


- Aleix Pol Gonzalez


On Oct. 23, 2015, 4:36 p.m., Antonio Larrosa Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125766/
> ---
> 
> (Updated Oct. 23, 2015, 4:36 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kded
> 
> 
> Description
> ---
> 
> Changed the manual command line parsing to use QCommandLineParser instead of
> parsing it manually. Also, now kded5 --version and kded5 --help show the
> version information and help on options.
> 
> There's a new human readable text ("Check cache validity") that maybe
> should be inside i18n, but since kded doesn't user i18n at all and
> I wasn't sure it's desired to do so, I just left it untranslated.
> 
> Also, unset SESSION_MANAGER environment variable instead of setting it empty.
> This removes a warning from Qt when parsing the (wrong, empty)
> value of SESSION_MANAGER
> 
> 
> Diffs
> -
> 
>   src/kded.cpp 6929d7d3f24d3556f1b227d5a9bfb5b02b1c295e 
> 
> Diff: https://git.reviewboard.kde.org/r/125766/diff/
> 
> 
> Testing
> ---
> 
> I tested kded5 runs, and the --check, --version and --help options work as 
> expected.
> Also, now it doesn't show a warning every time it's run. I didn't really 
> check it's
> not doing any session management, but SESSION_MANAGER is only used in one 
> place in qtbase,
> at 
> https://github.com/qtproject/qtbase/blob/dev/src/plugins/platforms/xcb/qxcbsessionmanager.cpp#L350
> and reading that, I think unsetting the environment variable makes more sense.
> 
> 
> Thanks,
> 
> Antonio Larrosa Jimenez
> 
>

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

Re: Review Request 125766: Changed the manual command line parsing to use QCommandLineParser.

[Aleix Pol Gonzalez]

> On Oct. 23, 2015, 5:09 p.m., Aleix Pol Gonzalez wrote:
> > Ship It!

Eh sorry I pressed that by fault, I don't know how it got confirmed. My actual 
review follows.


- Aleix


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


On Oct. 23, 2015, 4:36 p.m., Antonio Larrosa Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125766/
> ---
> 
> (Updated Oct. 23, 2015, 4:36 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kded
> 
> 
> Description
> ---
> 
> Changed the manual command line parsing to use QCommandLineParser instead of
> parsing it manually. Also, now kded5 --version and kded5 --help show the
> version information and help on options.
> 
> There's a new human readable text ("Check cache validity") that maybe
> should be inside i18n, but since kded doesn't user i18n at all and
> I wasn't sure it's desired to do so, I just left it untranslated.
> 
> Also, unset SESSION_MANAGER environment variable instead of setting it empty.
> This removes a warning from Qt when parsing the (wrong, empty)
> value of SESSION_MANAGER
> 
> 
> Diffs
> -
> 
>   src/kded.cpp 6929d7d3f24d3556f1b227d5a9bfb5b02b1c295e 
> 
> Diff: https://git.reviewboard.kde.org/r/125766/diff/
> 
> 
> Testing
> ---
> 
> I tested kded5 runs, and the --check, --version and --help options work as 
> expected.
> Also, now it doesn't show a warning every time it's run. I didn't really 
> check it's
> not doing any session management, but SESSION_MANAGER is only used in one 
> place in qtbase,
> at 
> https://github.com/qtproject/qtbase/blob/dev/src/plugins/platforms/xcb/qxcbsessionmanager.cpp#L350
> and reading that, I think unsetting the environment variable makes more sense.
> 
> 
> Thanks,
> 
> Antonio Larrosa Jimenez
> 
>

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

Re: Review Request 125766: Changed the manual command line parsing to use QCommandLineParser.

[Aleix Pol Gonzalez]

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

Ship it!


Ship It!

- Aleix Pol Gonzalez


On Oct. 23, 2015, 4:36 p.m., Antonio Larrosa Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125766/
> ---
> 
> (Updated Oct. 23, 2015, 4:36 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kded
> 
> 
> Description
> ---
> 
> Changed the manual command line parsing to use QCommandLineParser instead of
> parsing it manually. Also, now kded5 --version and kded5 --help show the
> version information and help on options.
> 
> There's a new human readable text ("Check cache validity") that maybe
> should be inside i18n, but since kded doesn't user i18n at all and
> I wasn't sure it's desired to do so, I just left it untranslated.
> 
> Also, unset SESSION_MANAGER environment variable instead of setting it empty.
> This removes a warning from Qt when parsing the (wrong, empty)
> value of SESSION_MANAGER
> 
> 
> Diffs
> -
> 
>   src/kded.cpp 6929d7d3f24d3556f1b227d5a9bfb5b02b1c295e 
> 
> Diff: https://git.reviewboard.kde.org/r/125766/diff/
> 
> 
> Testing
> ---
> 
> I tested kded5 runs, and the --check, --version and --help options work as 
> expected.
> Also, now it doesn't show a warning every time it's run. I didn't really 
> check it's
> not doing any session management, but SESSION_MANAGER is only used in one 
> place in qtbase,
> at 
> https://github.com/qtproject/qtbase/blob/dev/src/plugins/platforms/xcb/qxcbsessionmanager.cpp#L350
> and reading that, I think unsetting the environment variable makes more sense.
> 
> 
> Thanks,
> 
> Antonio Larrosa Jimenez
> 
>

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