DevHeads.net

Review Request 116951: Fix KDBusServiceStarter::findServiceFor() not returning error string

Review request for kdelibs.

Repository: kdelibs

Description
When KDBusServiceStarter::findServiceFor() fails to start the requested service after it is found to not be running, it does not return the error string. This patch fixes that and makes it behave as in the apidox.

Diffs
kio/kio/kdbusservicestarter.cpp 90624fb

Diff: <a href="https://git.reviewboard.kde.org/r/116951/diff/" title="https://git.reviewboard.kde.org/r/116951/diff/">https://git.reviewboard.kde.org/r/116951/diff/</a>

Testing
Tested this scenario, and it now returns the error string.

Thanks,

David Jarvie

Comments

Re: Review Request 116951: Fix KDBusServiceStarter::findServiceF

By David Jarvie at 03/21/2014 - 09:39

(Updated March 21, 2014, 2:39 p.m.)

Review request for kdelibs.

Repository: kdelibs

Description
When KDBusServiceStarter::findServiceFor() fails to start the requested service after it is found to not be running, it does not return the error string. This patch fixes that and makes it behave as in the apidox.

Diffs (updated)
kio/kio/kdbusservicestarter.cpp 90624fb

Diff: <a href="https://git.reviewboard.kde.org/r/116951/diff/" title="https://git.reviewboard.kde.org/r/116951/diff/">https://git.reviewboard.kde.org/r/116951/diff/</a>

Testing
Tested this scenario, and it now returns the error string.

Thanks,

David Jarvie

Re: Review Request 116951: Fix KDBusServiceStarter::findServiceF

By David Jarvie at 03/21/2014 - 10:10

(Updated March 21, 2014, 3:10 p.m.)

Review request for kdelibs.

Changes
Fix null pointer access.

Repository: kdelibs

Description
When KDBusServiceStarter::findServiceFor() fails to start the requested service after it is found to not be running, it does not return the error string. This patch fixes that and makes it behave as in the apidox.

Diffs (updated)
kio/kio/kdbusservicestarter.cpp 90624fb

Diff: <a href="https://git.reviewboard.kde.org/r/116951/diff/" title="https://git.reviewboard.kde.org/r/116951/diff/">https://git.reviewboard.kde.org/r/116951/diff/</a>

Testing
Tested this scenario, and it now returns the error string.

Thanks,

David Jarvie

Re: Review Request 116951: Fix KDBusServiceStarter::findServiceF

By David Jarvie at 04/13/2014 - 17:41

(Updated April 13, 2014, 10:41 p.m.)

Review request for kdelibs.

Changes
Always output error in debug message.

Repository: kdelibs

Description
When KDBusServiceStarter::findServiceFor() fails to start the requested service after it is found to not be running, it does not return the error string. This patch fixes that and makes it behave as in the apidox.

Diffs (updated)
kio/kio/kdbusservicestarter.cpp 90624fb

Diff: <a href="https://git.reviewboard.kde.org/r/116951/diff/" title="https://git.reviewboard.kde.org/r/116951/diff/">https://git.reviewboard.kde.org/r/116951/diff/</a>

Testing
Tested this scenario, and it now returns the error string.

Thanks,

David Jarvie

Re: Review Request 116951: Fix KDBusServiceStarter::findServiceF

By David Jarvie at 04/14/2014 - 06:48

(Updated April 14, 2014, 11:48 a.m.)

Review request for kdelibs.

Changes
Fix indentation.

Note to self: when working on systems with different editor settings, check whitespace settings ;)

Repository: kdelibs

Description
When KDBusServiceStarter::findServiceFor() fails to start the requested service after it is found to not be running, it does not return the error string. This patch fixes that and makes it behave as in the apidox.

Diffs (updated)
kio/kio/kdbusservicestarter.cpp 90624fb

Diff: <a href="https://git.reviewboard.kde.org/r/116951/diff/" title="https://git.reviewboard.kde.org/r/116951/diff/">https://git.reviewboard.kde.org/r/116951/diff/</a>

Testing
Tested this scenario, and it now returns the error string.

Thanks,

David Jarvie

Re: Review Request 116951: Fix KDBusServiceStarter::findServiceF

By David Jarvie at 04/16/2014 - 15:25

(Updated April 16, 2014, 8:25 p.m.)

Status
This change has been marked as submitted.

Review request for kdelibs.

Repository: kdelibs

Description
When KDBusServiceStarter::findServiceFor() fails to start the requested service after it is found to not be running, it does not return the error string. This patch fixes that and makes it behave as in the apidox.

Diffs
kio/kio/kdbusservicestarter.cpp 90624fb

Diff: <a href="https://git.reviewboard.kde.org/r/116951/diff/" title="https://git.reviewboard.kde.org/r/116951/diff/">https://git.reviewboard.kde.org/r/116951/diff/</a>

Testing
Tested this scenario, and it now returns the error string.

Thanks,

David Jarvie

Re: Review Request 116951: Fix KDBusServiceStarter::findServiceF

By Commit Hook at 04/16/2014 - 15:25

This review has been submitted with commit fe8a0f456da5d3827e41c7754362844ebd11af84 by David Jarvie to branch KDE/4.12.

- Commit Hook

On April 14, 2014, 11:48 a.m., David Jarvie wrote:

Re: Review Request 116951: Fix KDBusServiceStarter::findServiceF

By David Faure at 04/15/2014 - 12:33

Ship it!

Ship It!

- David Faure

On April 14, 2014, 11:48 a.m., David Jarvie wrote:

Re: Review Request 116951: Fix KDBusServiceStarter::findServiceF

By Kevin Krammer at 04/14/2014 - 07:17

Looks good to me, but maybe let dfaure have a second look

- Kevin Krammer

On April 14, 2014, 11:48 a.m., David Jarvie wrote:

Re: Review Request 116951: Fix KDBusServiceStarter::findServiceF

By Kevin Krammer at 04/14/2014 - 01:15

kio/kio/kdbusservicestarter.cpp
<https://git.reviewboard.kde.org/r/116951/#comment38764>

indentation looks wrong but maybe that's the diff interface

kio/kio/kdbusservicestarter.cpp
<https://git.reviewboard.kde.org/r/116951/#comment38765>

same here

- Kevin Krammer

On April 13, 2014, 10:41 p.m., David Jarvie wrote:

Re: Review Request 116951: Fix KDBusServiceStarter::findServiceF

By David Faure at 04/11/2014 - 18:23

kio/kio/kdbusservicestarter.cpp
<https://git.reviewboard.kde.org/r/116951/#comment38613>

This would hide the error from the debug string, if the caller didn't specify an error pointer.

Better use a local var (say localError) and then
if (error)
*error = localError;

- David Faure

On March 21, 2014, 3:10 p.m., David Jarvie wrote:

Re: Review Request 116951: Fix KDBusServiceStarter::findServiceF

By Kevin Krammer at 03/21/2014 - 10:20

Looks good to me but maybe someone closer to KIO can confirm that

- Kevin Krammer

On March 21, 2014, 3:10 p.m., David Jarvie wrote:

Re: Review Request 116951: Fix KDBusServiceStarter::findServiceF

By Kevin Krammer at 03/21/2014 - 09:44

kio/kio/kdbusservicestarter.cpp
<https://git.reviewboard.kde.org/r/116951/#comment37656>

there is a check for error not being a null pointer in line 74, so it could pontentially be 0 here as well

- Kevin Krammer

On March 21, 2014, 2:39 p.m., David Jarvie wrote: