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.
Details
- Reviewers
davidedmundson - Group Reviewers
Frameworks - Commits
- R271:75cf9e495edd: Include API to generically implement --replace arguments
Ported plasmashell to it, works.
Diff Detail
- Repository
- R271 KDBusAddons
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 14759 Build 14777: arc lint + arc unit
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 | ||
---|---|---|
157 | @since missing | |
158 | Please no "bool" type argument, this makes places calling harder to understand -> https://community.kde.org/Policies/Library_Code_Policy#Flags |
src/kdbusservice.h | ||
---|---|---|
158 | especially given there is an options flag already, you can just add another flag for this |
src/kdbusservice.cpp | ||
---|---|---|
119 ↗ | (On Diff #63115) | 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 ↗ | (On Diff #63115) | A unique service which has a /MainApplication object implementing a rg.qtproject.Qt.QCoreApplication interface,... Any chance to make this non-Qt-only? |
src/kdbusservice.h | ||
---|---|---|
121 ↗ | (On Diff #63115) | 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. |
src/kdbusservice.h | ||
---|---|---|
121 ↗ | (On Diff #63115) | 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. |
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?
src/kdbusservice.cpp | ||
---|---|---|
129 ↗ | (On Diff #63115) | 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 and it'll be better than the current approach plasma does. |
Rebase and get inspiration from 5bf091ee07ac44ed1bf1e75a4d07847edb86c5d6 as suggested by David
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
- We try to register our service name with the queued flag
- We see if we succeeded to register it immediately
- If it fails, we send a non-blocking quit method to whoever currently owns that name
- and then wait for our application to be given the service name we've already requested
all except 3 exist already.
src/kdbusservice.cpp | ||
---|---|---|
124–125 ↗ | (On Diff #63115) | This is still racey |
134 ↗ | (On Diff #63115) | 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. |