Include API to generically implement --replace arguments
Needs ReviewPublic

Authored by apol on Mon, Aug 5, 12:03 PM.

Details

Reviewers
None
Group Reviewers
Frameworks
Summary

It's quite wide-spread in KDE applications, and having to implement it in every
application isn't very efficient.
This adds an option value to suggest this replace so applications can share the
implementation.

Test Plan

Ported plasmashell to it, works.

Diff Detail

Repository
R271 KDBusAddons
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14764
Build 14782: arc lint + arc unit
apol created this revision.Mon, Aug 5, 12:03 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMon, Aug 5, 12:03 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
apol requested review of this revision.Mon, Aug 5, 12:03 PM

Just some API style nitpicks while this was by-passing. Not dealt with KDBusService recently, so cannot really comment otherwise. QCoreApplication::processEvents() in the constructor though makes me feel uncomfortable, any chance to make this async?

src/kdbusservice.h
160

@since missing

161

Please no "bool" type argument, this makes places calling harder to understand -> https://community.kde.org/Policies/Library_Code_Policy#Flags

broulik added inline comments.
src/kdbusservice.h
161

especially given there is an options flag already, you can just add another flag for this

apol marked 3 inline comments as done.Mon, Aug 5, 1:00 PM
apol edited the summary of this revision. (Show Details)
apol updated this revision to Diff 63115.Mon, Aug 5, 1:04 PM

Argument -> flag

kossebau added inline comments.Mon, Aug 5, 1:08 PM
src/kdbusservice.cpp
119

What if the quit fails? No error handling?

I fear for having such logic in the library it needs to be more elaborared IMHO.

src/kdbusservice.h
121

A unique service which has a /MainApplication object implementing a rg.qtproject.Qt.QCoreApplication interface,...

Any chance to make this non-Qt-only?

apol marked an inline comment as done.Mon, Aug 5, 1:16 PM
apol added inline comments.
src/kdbusservice.h
121

I'd say this is for applications using KDBusService really and it's not like this is the only usage of this org.qtproject.* here.

apol updated this revision to Diff 63116.Mon, Aug 5, 1:16 PM

error handling

kossebau added inline comments.Mon, Aug 5, 1:48 PM
src/kdbusservice.h
121

By just reading the API dox, I would have assumed one can replace any "unique service" with the well-known name, not just services using the same toolkit/libaries. Only when I saw the implementation, it became obvious.

apol updated this revision to Diff 63124.Mon, Aug 5, 2:15 PM

More documentation.
Don't wait for the original process to finish.

apol added a comment.Tue, Aug 6, 2:00 PM

I wonder if it would make sense to include an enum NoOption=0 value, so that we don't need to cast the 0 into the enum (see D22967). Thoughts?