KNS: Deprecate isRemote method and handle parse error properly
ClosedPublic

Authored by alex on May 5 2020, 7:40 PM.

Details

Summary

The isRemote check and the FIXMEs are removed, instead the result from the empty target check is properly handled.

The same check as in isRemote is done in the Installation::readConfig method (line 122).
If this fails a warning is given and in the engine is an error emitted. This error gets shown in the GUI.
Then there are no other checks needed.

Test Plan

Delete the line TargetDir from the dolphin servicemenu knsrc file.
(Dolphin repository src/settings/services/servicemenu.knsrc).

Before:


After, an error message is shown:

Diff Detail

Repository
R304 KNewStuff
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
alex created this revision.May 5 2020, 7:40 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 5 2020, 7:40 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
alex requested review of this revision.May 5 2020, 7:40 PM
leinir requested changes to this revision.May 5 2020, 7:59 PM
leinir added inline comments.
src/core/installation.h
76

i'm afraid this is an exported class, at most we can mark this as deprecated (with a bit of explanation), we can't remove it...

This revision now requires changes to proceed.May 5 2020, 7:59 PM
alex updated this revision to Diff 82024.May 5 2020, 8:13 PM
alex retitled this revision from KNS: Remove isRemote method and handle parse error properly to KNS: Deprecate isRemote method and handle parse error properly.

Make isRemote deprecated

leinir requested changes to this revision.May 6 2020, 1:20 PM

Apart from these couple of details, it looks pretty good :) (i'd say just fix and commit, but one of them's a tiny bit larger than just a typo fix ;) )

src/core/engine.cpp
174

"Failed to read config file" is more a specific issue with setting up the installation handler, so... "Could not initialise the installation handler for %1. This is a critical error and should be reported to the application author" would probably be a little clearer... It is /supposed/ to not happen, and is really quite critical, so the best we can reasonably do is direct the blame to the appropriate location.

src/core/installation.h
78

Obsolete, not obsulete :) (just a nitpick, really :) )

This revision now requires changes to proceed.May 6 2020, 1:20 PM
alex updated this revision to Diff 82098.May 6 2020, 1:45 PM
alex marked an inline comment as done.

Typo and error message

If the error message is on two lines they aren't very readable, but
that is an issue for another day :-).

alex marked 2 inline comments as done.May 6 2020, 1:46 PM
leinir accepted this revision.May 7 2020, 9:57 AM

Sorted, nicely done :) Makes the code just a touch simpler as well, which is always good :)

This revision is now accepted and ready to land.May 7 2020, 9:57 AM
This revision was automatically updated to reflect the committed changes.