Include API to generically implement --replace arguments
ClosedPublic

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
apol created this revision.Aug 5 2019, 12:03 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 5 2019, 12:03 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
apol requested review of this revision.Aug 5 2019, 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.Aug 5 2019, 1:00 PM
apol edited the summary of this revision. (Show Details)
apol updated this revision to Diff 63115.Aug 5 2019, 1:04 PM

Argument -> flag

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

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.Aug 5 2019, 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.Aug 5 2019, 1:16 PM

error handling

kossebau added inline comments.Aug 5 2019, 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.Aug 5 2019, 2:15 PM

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

apol added a comment.Aug 6 2019, 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?

davidedmundson added inline comments.
src/kdbusservice.cpp
193

I don't like this part very much. It means we're registering the name whilst the former app is potentially still running.

I would suggest rebasing on Harald's 5bf091ee07ac44ed1bf1e75a4d07847edb86c5d6 change.

It'll allow us to do this all in a race-free way.

We can register the name org.kde.plasmashell in a queued manner
then call quit on the current process, and then call waitForRegistration()

and it'll be better than the current approach plasma does.

davidedmundson requested changes to this revision.Aug 21 2019, 10:17 AM
This revision now requires changes to proceed.Aug 21 2019, 10:17 AM
apol updated this revision to Diff 69147.Nov 1 2019, 1:17 AM

Rebase and get inspiration from 5bf091ee07ac44ed1bf1e75a4d07847edb86c5d6 as suggested by David

apol marked an inline comment as done.Nov 1 2019, 1:22 AM

I think I didn't explain myself properly. There's a way to do a version that has no race conditions.

Order of events needs to be

  1. We try to register our service name with the queued flag
  2. We see if we succeeded to register it immediately
  3. If it fails, we send a non-blocking quit method to whoever currently owns that name
  4. and then wait for our application to be given the service name we've already requested

all except 3 exist already.

src/kdbusservice.cpp
171

This is still racey

181

QDBusConnectionInterface::serviceUnregistered is very different to QDBusServiceWatcher::serviceUnregistered

QDBusConnectionInterface::serviceUnregistered will be called when our process loses the service name it had.

Our process hasn't even requested a service name yet, so this won't happen.

apol updated this revision to Diff 69997.Nov 19 2019, 3:14 PM

Made the code less "racey"

apol updated this revision to Diff 70043.Nov 20 2019, 10:13 AM

Remove wrong comment

davidedmundson accepted this revision.Nov 20 2019, 10:14 AM
This revision is now accepted and ready to land.Nov 20 2019, 10:14 AM
This revision was automatically updated to reflect the committed changes.