D22946: Include API to generically implement --replace arguments

This revision was automatically updated to reflect the committed changes. Closed by commit R271:75cf9e495edd: Include API to generically implement --replace arguments (authored by apol). REPOSITORY R271 KDBusAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22946?vs=70043&id=7004

D22946: Include API to generically implement --replace arguments

davidedmundson accepted this revision. This revision is now accepted and ready to land. REPOSITORY R271 KDBusAddons BRANCH arcpatch-D22946 REVISION DETAIL https://phabricator.kde.org/D22946 To: apol, #frameworks, davidedmundson Cc: davidedmundson, broulik, kossebau, kde-frameworks-devel,

D22946: Include API to generically implement --replace arguments

apol updated this revision to Diff 70043. apol added a comment. Remove wrong comment REPOSITORY R271 KDBusAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22946?vs=69997&id=70043 BRANCH arcpatch-D22946 REVISION DETAIL https://phabricator.kde.org/D22946 AFFECTED FILES

D22946: Include API to generically implement --replace arguments

apol updated this revision to Diff 69997. apol added a comment. Made the code less "racey" REPOSITORY R271 KDBusAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22946?vs=69147&id=69997 BRANCH arcpatch-D22946 REVISION DETAIL https://phabricator.kde.org/D22946 AFFECTED F

D22946: Include API to generically implement --replace arguments

davidedmundson added a comment. 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

D22946: Include API to generically implement --replace arguments

apol marked an inline comment as done. REPOSITORY R271 KDBusAddons REVISION DETAIL https://phabricator.kde.org/D22946 To: apol, #frameworks, davidedmundson Cc: davidedmundson, broulik, kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D22946: Include API to generically implement --replace arguments

apol updated this revision to Diff 69147. apol added a comment. Rebase and get inspiration from 5bf091ee07ac44ed1bf1e75a4d07847edb86c5d6 as suggested by David REPOSITORY R271 KDBusAddons CHANGES SINCE LAST UPDATE

D22946: Include API to generically implement --replace arguments

davidedmundson requested changes to this revision. This revision now requires changes to proceed. REPOSITORY R271 KDBusAddons REVISION DETAIL https://phabricator.kde.org/D22946 To: apol, #frameworks, davidedmundson Cc: davidedmundson, broulik, kossebau, kde-frameworks-devel, LeGast00n, GB_2,

D22946: Include API to generically implement --replace arguments

davidedmundson added inline comments. INLINE COMMENTS > kdbusservice.cpp:129 > +d->registered = bus->registerService(d->serviceName > + , (options & Replace) ? > QDBusConnectionInterface::ReplaceExistingService : > QDBusConnectionInterface::Do

D22946: Include API to generically implement --replace arguments

apol added a comment. ping REPOSITORY R271 KDBusAddons REVISION DETAIL https://phabricator.kde.org/D22946 To: apol, #frameworks Cc: broulik, kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D22946: Include API to generically implement --replace arguments

apol added a comment. 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? REPOSITORY R271 KDBusAddons REVISION DETAIL https://phabricator.kde.org/D22946 To: a

D22946: Include API to generically implement --replace arguments

apol added a dependent revision: D22967: Let KDBusService deal with the replace option. REPOSITORY R271 KDBusAddons REVISION DETAIL https://phabricator.kde.org/D22946 To: apol, #frameworks Cc: broulik, kossebau, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22946: Include API to generically implement --replace arguments

apol updated this revision to Diff 63124. apol added a comment. More documentation. Don't wait for the original process to finish. REPOSITORY R271 KDBusAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22946?vs=63116&id=63124 BRANCH master REVISION DETAIL https://phabr

D22946: Include API to generically implement --replace arguments

kossebau added inline comments. INLINE COMMENTS > apol wrote in 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. By just reading the API dox, I would have assumed one can replace any "unique

D22946: Include API to generically implement --replace arguments

apol updated this revision to Diff 63116. apol added a comment. error handling REPOSITORY R271 KDBusAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22946?vs=63115&id=63116 BRANCH master REVISION DETAIL https://phabricator.kde.org/D22946 AFFECTED FILES src/kdbusservi

D22946: Include API to generically implement --replace arguments

apol marked an inline comment as done. apol added inline comments. INLINE COMMENTS > kossebau wrote in 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? I'd say this

D22946: Include API to generically implement --replace arguments

kossebau added inline comments. INLINE COMMENTS > 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. REPOSITORY R271 KDBusAddons REVISION DETAIL https://phabrica

D22946: Include API to generically implement --replace arguments

kossebau added inline comments. INLINE COMMENTS > kdbusservice.h:121 > + * Indicates that if there's already a unique service running, to be > quit and replaced > + * with our own. > + * A unique service which has a /MainApplication object implementing a rg.qtproject.Qt

D22946: Include API to generically implement --replace arguments

apol updated this revision to Diff 63115. apol added a comment. Argument -> flag REPOSITORY R271 KDBusAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22946?vs=63114&id=63115 BRANCH master REVISION DETAIL https://phabricator.kde.org/D22946 AFFECTED FILES src/kdbusser

D22946: Include API to generically implement --replace arguments

apol edited the summary of this revision. REPOSITORY R271 KDBusAddons REVISION DETAIL https://phabricator.kde.org/D22946 To: apol, #frameworks Cc: broulik, kossebau, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22946: Include API to generically implement --replace arguments

apol marked 3 inline comments as done. REPOSITORY R271 KDBusAddons REVISION DETAIL https://phabricator.kde.org/D22946 To: apol, #frameworks Cc: broulik, kossebau, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22946: Include API to generically implement --replace arguments

broulik added inline comments. INLINE COMMENTS > kossebau wrote in kdbusservice.h:151 > Please no "bool" type argument, this makes places calling harder to > understand -> https://community.kde.org/Policies/Library_Code_Policy#Flags especially given there is an `options` flag already, you can j

D22946: Include API to generically implement --replace arguments

kossebau added a comment. 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? INLINE COMMENTS

D22946: Include API to generically implement --replace arguments

apol created this revision. apol added a reviewer: Frameworks. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. apol requested review of this revision. REVISION SUMMARY It's quite wide-spread in KDE applications, and having to implement it in every applicat