[RFC] Make notifications responsibility of SpectacleCore
AbandonedPublic

Authored by davidre on Apr 1 2019, 5:37 PM.

Details

Summary

Currently notifications are triggered in two ways. Either by triggering it through ExportManager by supplying an argument in doSave
and doCopyToClipboard which emitted forceNotify or directly by SpectacleCore. I propose moving this responsibility solely to
SpectacleCore since it is much clearer where notifications are triggered. For example notification where triggered in KSMainWindow
which also quitted the whole program. This allows us to remove this behavior from KSMainWindow and simplify the Code a bit. To do
this I had to add a new signal to ExportManager which is emitted after an Image is copied to the Clipboard.

Test Plan

Should work as before.

Diff Detail

Repository
R166 Spectacle
Branch
arcpatch-D20180
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15339
Build 15357: arc lint + arc unit
davidre created this revision.Apr 1 2019, 5:37 PM
Restricted Application added a project: Spectacle. · View Herald TranscriptApr 1 2019, 5:37 PM
Restricted Application added a subscriber: Spectacle. · View Herald Transcript
davidre requested review of this revision.Apr 1 2019, 5:37 PM
davidre edited reviewers, added: Spectacle; removed: VDG.Apr 1 2019, 5:38 PM
ngraham requested changes to this revision.Apr 1 2019, 7:55 PM
ngraham added a subscriber: ngraham.

Thanks, a nice refactoring. However in testing this broke the "quit after copy" feature; now the image is no longer added to klipper when this setting is used.

This revision now requires changes to proceed.Apr 1 2019, 7:55 PM
davidre added a comment.EditedApr 2 2019, 7:49 AM

You're right. There seems to be some magic involved. If I wait 250 ms and directly quit after hiding like KSMainWindow instead of showing the notification it works. Testing master I also found that spectacle -b -c does not work and if you copy to the clipboard and then exit manually the image doesn't appear Klipper.
Now to figure out the reason...

davidre updated this revision to Diff 61645.EditedJul 12 2019, 12:15 PM

rebase
Now that we fixed Klipper this should work

The image still does not show up in Klipper

src/SpectacleCore.cpp
391–395

break line

For me it is the same behavior as master:

  • If you copy and quit on copy is checked it shows up in the history
  • If you copy and then exit manually it doesn't show up in the history but you can still paste it (for example into KolourPaint)

Notice that you have to check "Avoid empty clipboard" in Klipper settings. I think we came to the conclusion that this is the same behavior as for text.

davidre updated this revision to Diff 61655.Jul 12 2019, 1:12 PM
davidre marked an inline comment as done.

Break line

Nope, exit-on-copy doesn't put it in the history, but it works on master

Yes you're right, I think I retestet the wrong version. However pasting still works.
It seems related to shortly quitting after copying. If I add
QTimer::singleShot(250, &QApplication::quit); to imageExported it appears in the history, but then we can't show the notification.

Ping? Maybe @davidedmundson has insight why it is only added to the history if the app quits

davidre added a comment.EditedAug 6 2019, 5:16 AM

Something to look out for https://phabricator.kde.org/D22684

I would still like to land this even if we lose the image inside Klipper. It doesn't appear there everytime anyway and pasting still works as mentioned earlier.

nicolasfella accepted this revision.Tue, Aug 20, 11:11 AM
davidre updated this revision to Diff 64109.Tue, Aug 20, 12:28 PM

Remove qDebug()

ngraham resigned from this revision.Tue, Aug 20, 4:37 PM

I'll defer to you on the code here, with the caveat that it's on the you-break-it-you-fix-it basis. :)

This revision is now accepted and ready to land.Tue, Aug 20, 5:44 PM
davidre abandoned this revision.Wed, Aug 21, 12:28 PM

Landing this would make D23162 harder and the architecture while here simpler worse for that case. I will do an alternative patch after it is landed