D22945: Fix --replace option

[Aleix Pol Gonzalez]

apol created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
apol requested review of this revision.

REVISION SUMMARY
  Wait for the other process to unregister before starting the new one.

TEST PLAN
  Reproduced the problem Noah mentioned and addressed it.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D22945

AFFECTED FILES
  shell/main.cpp

To: apol
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22945: Fix --replace option

[Kai Uwe Broulik]

broulik added a comment.


  Also can you split the crash fix and `--replace` option change please

INLINE COMMENTS

> main.cpp:136
>  corona->setShell(cliOptions.value(shellPluginOption));
> +QObject::connect(QCoreApplication::instance(), 
> &QCoreApplication::aboutToQuit, corona, &QObject::deleteLater);
>  

Can you explain? The `ShellCorona` is parented to the app, so is destroyed with 
it. Is the `deleteLater` even still processed at this point?

> main.cpp:182
> +
> +while 
> (QDBusConnection::sessionBus().interface()->isServiceRegistered(QStringLiteral("org.kde.plasmashell")))
>  {
> +QCoreApplication::processEvents(QEventLoop::AllEvents);

So this will just eat CPU until the other process quit?
Can we register `org.kde.plasmashell` service with `AllowReplace` flag so the 
new process can just take over?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D22945

To: apol
Cc: broulik, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22945: Fix --replace option

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> broulik wrote in main.cpp:136
> Can you explain? The `ShellCorona` is parented to the app, so is destroyed 
> with it. Is the `deleteLater` even still processed at this point?

aboutToQuit is called much earlier than the actual destruction.

> broulik wrote in main.cpp:182
> So this will just eat CPU until the other process quit?
> Can we register `org.kde.plasmashell` service with `AllowReplace` flag so the 
> new process can just take over?

We can do this change if we get D22946  in.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D22945

To: apol
Cc: broulik, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22945: Fix --replace option

[Aleix Pol Gonzalez]

apol updated this revision to Diff 63120.
apol added a comment.


  Split into two

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22945?vs=63109&id=63120

BRANCH
  replace

REVISION DETAIL
  https://phabricator.kde.org/D22945

AFFECTED FILES
  shell/main.cpp

To: apol
Cc: broulik, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22945: Fix --replace option

[Aleix Pol Gonzalez]

apol updated this revision to Diff 63134.
apol added a comment.


  x

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22945?vs=63120&id=63134

BRANCH
  replace

REVISION DETAIL
  https://phabricator.kde.org/D22945

AFFECTED FILES
  shell/main.cpp

To: apol
Cc: broulik, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22945: Fix --replace option

[Noah Davis]

ndavis added a comment.


  I can confirm that this fixes the bug. Thanks!

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D22945

To: apol
Cc: ndavis, broulik, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D22945: Fix --replace option

[Aleix Pol Gonzalez]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R120:8e61f49cc292: Fix --replace option (authored by apol).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D22945?vs=63134&id=63184#toc

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22945?vs=63134&id=63184

REVISION DETAIL
  https://phabricator.kde.org/D22945

AFFECTED FILES
  shell/main.cpp

To: apol
Cc: ndavis, broulik, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart