Differential D29487

Refactor Spectacle to run in single instance mode with KDBusService::Unique
ClosedPublic
Actions

Authored by meven on May 6 2020, 5:24 PM.

Details

Reviewers
davidre
bport
davidedmundson
apol
ngraham
Commits
R166:dec0e6cef23a: Refactor Spectacle to run in single instance mode with KDBusService::Unique
Summary

Since Spectacle got DBus activation support in 09cd11881d828da35c46c48da79f2d988e6a78cc, it has been run in multiple instance mode.
But it has been shaddowed by using qbus ee862d161a480408338d00b8826c915f7a97575c in the desktop file to mimic singleton behavior.

This refactors Spectacle to run in true single instance mode using KDBusService::Unique.
This is to allow Spectacle to have its executable in its Exec desktop Entry, to allow KWin to match the executable to the service file, itself to allow screenshots under Wayland without intermediate click.

The listed bugs are fixed incidently except for 414739 where a simple fix was added in screenShotUpdated.

BUG: 420477
BUG: 414739

CCBUG: 412186

Test Plan

Start spectacle, alternatively, from shortcut, command line.
Take screenshots, from alternative ways.

There is always at most one Spectacle window unless launched with --new-instance.

Diff Detail

Repository
R166 Spectacle
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
meven created this revision.May 6 2020, 5:24 PM
Restricted Application added a project: Spectacle. · View Herald TranscriptMay 6 2020, 5:24 PM
Restricted Application added a subscriber: Spectacle. · View Herald Transcript
meven requested review of this revision.May 6 2020, 5:24 PM
anthonyfieroni added a subscriber: anthonyfieroni.May 6 2020, 5:33 PM
anthonyfieroni added inline comments.
src/Main.cpp
81–86

Just use second function

atexit(deleteQApplication);
KDBusService service(KDBusService::Unique, lCore); // it does not need to be heap allocation
atexit(noopfunc);
anthonyfieroni added a comment.EditedMay 6 2020, 5:37 PM

I don't see why qApp is not deleted here
https://api.kde.org/frameworks-api/frameworks-apidocs/frameworks/kdbusaddons/html/kdbusservice_8cpp_source.html#l00099
Probably because it can be allocated on stack.

meven updated this revision to Diff 82176.May 7 2020, 7:55 AM
meven marked an inline comment as done.

Store QApplication on the stack

broulik added a subscriber: broulik.May 7 2020, 9:23 AM
broulik added inline comments.
src/SpectacleCore.cpp
98

This leaks

src/SpectacleDBusAdapter.h
48

Are these DBus-exported?

davidre added a comment.EditedMay 7 2020, 9:45 AM

Just a heads up you don't need to use the weird lvariable, theParam code style. See discussion in https://phabricator.kde.org/D21042

(I personally change it back in every line I need to touch)

anthonyfieroni added inline comments.May 7 2020, 10:46 AM
src/Main.cpp
41

s_needToDeleteQApplication is not needed any more.

42

Deleting application allocated on stack is UB. Keep app on heap.

meven planned changes to this revision.EditedMay 7 2020, 10:54 AM
meven marked 2 inline comments as done.

This patch currently implies removing the option to have multipe instances of spectacle, and we have a setting to have this upon pressing the Print key.
So should I keep the multiple instance mode ? @davidre @ngraham
It is feasible.
Any time the commandline with the background option or with the setting "Open a new Spectacle window" is set I would start a new Gui instance.

My main goal is to have "/usr/bin/spectacle" in desktop file field "Exec" anyway.

src/SpectacleDBusAdapter.h
48

They are

davidre added a comment.EditedMay 11 2020, 8:19 AM

I would like to keep the option. We currently just start a new `Qprocess with "spectacle" would that still work?

I guess on app start you would check if there is an instance on the bus already and then do the right thing?

meven marked 2 inline comments as done.May 11 2020, 9:24 AM
In D29487#668034, @davidre wrote:

I would like to keep the option. We currently just start a new `Qprocess with "spectacle" would that still work?

I guess on app start you would check if there is an instance on the bus already and then do the right thing?

Because the "no parameter case with gui already started" case is the simple shortcut PrintKey, I need to reuse the instance to have correct behavior.
I am thinking about adding an option "-i/--new-instance" passed for the option to start a new GUI application without dbus hooks and KDBusService::Multiple in this case.

Right now my patch leaks zombie process when launching through Dbus activated shortcuts :/

meven updated this revision to Diff 82539.May 11 2020, 3:15 PM
meven marked 2 inline comments as done.

Add --new-instance option, clean deletion of app

meven updated this revision to Diff 82660.May 12 2020, 1:45 PM

Move SpectacleCore on the stack to avoid crashes, small refactoring

meven added a comment.May 12 2020, 2:43 PM

Patch is ready for review.

The zombie process when activating with dbus is pre-existant and despite my effor, I didn't find a solution.

meven edited the test plan for this revision. (Show Details)May 12 2020, 2:44 PM
davidre added a comment.May 14 2020, 8:03 AM

I didn't review the entire code yet but seems much cleaner. A very welcome refactor!
I noticed a flaw however in the current code if the first instance of spectacle is started with --new-instance a second one will always be started when the shortcut is pressed regardless of the setting.

desktop/org.kde.spectacle.desktop.cmake
143

Is there a reason that we hardcode the path instead of "spectacle"?

src/Main.cpp
85

whitespace

src/SpectacleCore.cpp
52

Can you do

{
}
meven updated this revision to Diff 82913.May 15 2020, 9:55 AM
meven marked 3 inline comments as done.

Spacing

davidre added a comment.May 25 2020, 9:34 AM

Ping. Did you have time to look into

I noticed a flaw however in the current code if the first instance of spectacle is started with --new-instance a second one will always be started when the shortcut is pressed regardless of the setting.

meven added a comment.May 25 2020, 10:34 AM
In D29487#673803, @davidre wrote:

Ping. Did you have time to look into

I noticed a flaw however in the current code if the first instance of spectacle is started with --new-instance a second one will always be started when the shortcut is pressed regardless of the setting.

Well it is not a flaw, it is by design.
--new-instance launches an instance without dbus connection and unique instance.
The first one without --new-instance will be the dbus unique instance.

desktop/org.kde.spectacle.desktop.cmake
143

KWin with D29407 will find executable path taking screenshot using their Exec field in their desktop file.
And since screenshot is considered sensitive information in Wayland, we are expecting absolute path in Exec field to make sure the files match.
This then allow KWin to authorize or not the file to take screenshots if it has a special key in its desktop file.
This bit will be added to spectacle in D29408 after this refactoring is done.

You'd get Error calling KWin DBus interface: "org.kde.kwin.Screenshot.Error.NoAuthorized" "The process is not authorized to take a screenshot"

ngraham requested changes to this revision.Jun 1 2020, 3:59 AM

Hmm, for me this breaks the option to open a new instance when the PrintScreen key is pressed and an existing instance is already running. It only works if I launch Spectacle manually using the --new-instance flag. The feature should work regardless of how the app is launched.

This revision now requires changes to proceed.Jun 1 2020, 3:59 AM
meven added a comment.Jun 2 2020, 1:07 PM
In D29487#674322, @ngraham wrote:

Hmm, for me this breaks the option to open a new instance when the PrintScreen key is pressed and an existing instance is already running. It only works if I launch Spectacle manually using the --new-instance flag. The feature should work regardless of how the app is launched.

Last time I checked it worked (2 months ago...)
Perhaps your installed org.kde.spectacle.desktop was not updated
Will check.

davidre accepted this revision.Jun 3 2020, 12:20 PM
In D29487#673809, @meven wrote:
In D29487#673803, @davidre wrote:

Ping. Did you have time to look into

I noticed a flaw however in the current code if the first instance of spectacle is started with --new-instance a second one will always be started when the shortcut is pressed regardless of the setting.

Well it is not a flaw, it is by design.
--new-instance launches an instance without dbus connection and unique instance.
The first one without --new-instance will be the dbus unique instance.

Alright I guess. Should we maybe even hide it from the user visible options then?

In D29487#674322, @ngraham wrote:

Hmm, for me this breaks the option to open a new instance when the PrintScreen key is pressed and an existing instance is already running. It only works if I launch Spectacle manually using the --new-instance flag. The feature should work regardless of how the app is launched

Works for me too.

I think we can do this now

ngraham resigned from this revision.Jun 3 2020, 10:28 PM

All right, maybe it was a local config issue on my side. I'll make noise if it stays broken. :)

This revision is now accepted and ready to land.Jun 3 2020, 10:28 PM
meven edited the summary of this revision. (Show Details)Jun 5 2020, 8:37 AM
meven updated this revision to Diff 83220.Jun 5 2020, 9:11 AM

Fix build

Closed by commit R166:dec0e6cef23a: Refactor Spectacle to run in single instance mode with KDBusService::Unique (authored by meven). · Explain WhyJun 5 2020, 9:31 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 83221

View Options

desktop/org.kde.spectacle.desktop.cmake

Loading...
View Options

src/Main.cpp

Loading...
View Options

src/SpectacleCore.h

Loading...
View Options

src/SpectacleCore.cpp

Loading...
View Options

src/SpectacleDBusAdapter.h

Loading...
View Options

src/SpectacleDBusAdapter.cpp

Loading...