Add option to copy screenshot to clipboard in background mode
ClosedPublic

Authored by kdautovic on Mar 12 2019, 7:54 PM.

Details

Summary

FEATURE: 385559
BUG: 393708
FIXED-IN: 19.04.0

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
kdautovic requested review of this revision.Mar 12 2019, 7:54 PM
kdautovic created this revision.
davidre added a subscriber: davidre.EditedMar 12 2019, 8:19 PM

If you follow the chain what happens in background mode if mNotify is set or not you can see that SpectacleCore emits allDone() at the end of both. Because you never emit that signal, spectacle doesn't quit.

This only works if -b is also specified otherwise it is ignored. I could see 2 possible options to this instead of ignoring it. Either implicitly setting backgroundMode when -c is specified or copying the automatically taken screenshot at application launch to the clipboard.

src/SpectacleCore.h
84

Please use a more descriptive name, copyToClipboard should be fine.

I understand what you're saying, but in the code I added there's nothing stopping the

if (!mNotify) {
    emit allDone();
}

code from running, as far as I can tell. Correct me if I'm wrong.

As for the -b flag, I followed the way it's set up for the -n, -o and -d flags. If you'd still like to use one of the two options you specified, tell me which one and I'll update the code.

src/SpectacleCore.h
84

I initially thought of using the name copyToClipboard, but then I saw that the notify boolean was called mNotify, so I followed that. I'll rename it.

I understand what you're saying, but in the code I added there's nothing stopping the

if (!mNotify) {
    emit allDone();
}

code from running, as far as I can tell. Correct me if I'm wrong.

No, you're right the code still runs. The issue is that notify is true per default if not run with --nonotify.

As for the -b flag, I followed the way it's set up for the -n, -o and -d flags. If you'd still like to use one of the two options you specified, tell me which one and I'll update the code.

I see, I'm not using Spectacle from the shell and just added my thoughts on what would be clear behavior for the user. But if the other options already work like this then it's fine for me.

Speaking of notifications. If the default behavior is to emit a notification maybe we should notify here, too. What do you think?

src/SpectacleCore.h
84

IMO (I didn't write the code) the difference is that notify is verb. In my head I read if (notify) as "if we should notify". Clipboard is a noun and for me if (clipboard) looks like checking if the clipboard pointer is not null.

kdautovic added a comment.EditedMar 12 2019, 11:20 PM

Speaking of notifications. If the default behavior is to emit a notification maybe we should notify here, too. What do you think?

Yeah, I agree. Notify is probably called from ExportManager->doSave()? If so, I'll check it out tomorrow and update the code with the variable name change and the notification.

src/SpectacleCore.h
84

That makes sense. Okay, I'll rename it tomorrow.

davidre added inline comments.Mar 12 2019, 11:22 PM
src/SpectacleCore.cpp
218

It get's called through this connection.

kdautovic updated this revision to Diff 53781.Mar 13 2019, 11:38 AM

Renamed the mClipboard variable to copyToClipboard

kdautovic updated this revision to Diff 53783.Mar 13 2019, 12:21 PM

Added notifications for copying to clipboard.

I have trouble applying your patch. Could you please diff against master and not your last diff? You can also so use arc to post patches to Phabricator. Doing so also makes context available in phabricator. Being able to expand the file makes reviewing the changes much easier. Thanks!
https://community.kde.org/Infrastructure/Phabricator#Using_Arcanist_to_post_patches

kdautovic updated this revision to Diff 53786.Mar 13 2019, 1:26 PM
kdautovic updated this revision to Diff 53787.

Sorry, I was using format-patch and thought that updating the diff adds to the last diff instead of uploading a completely new diff. How do you usually update a diff?

If you use arc, then running arc diff on your beanch does all the work for you. Otherwise, click ActionsUpdate diff on this page, and paste the results of git diff master..<branchname>

Also more generally, this feature won't work unless Klipper has been configured to accept images, which is not the default setting to reduce memory usage. Imagine copying multiple 4k images; this could bloat startup memory by hundreds of megabytes. This was always the prior blocker to this work. Options:

  • Change Klipper's default settings to store images but don't remember images when rebooting
  • Add a metadata tag into the paste data from Spectacle and change Klipper to override its don't-accept-images-by-default setting for image paste data with this tag
  • Something else?

There's more information available in https://bugs.kde.org/show_bug.cgi?id=393708 Also you can see a prior attempt at this in D9117: Add shortcuts for copying screenshots to clipboard (abandoned, so don't worry that you're duplicating in-progress work; in fact, feel free to learn and copy from that diff!).

Shouldn't this work as it uses the same function as the "Copy to clipboard" button or am I missing something?

The existing Copy to Clipboard feature already suffers from this problem if you check the Quit after Save or Copy checkbox. What happens is that the image gets copied to the X11 clipboard while Spectacle is running, but once Spectacle quits, the pasted image is gone unless Klipper grabs it--which it doesn't do by default. It's a pre-existing bug, but that bug kills the feature introduced by this patch unless we finally get around to solving it. :)

This worked out of the box for me when I was testing. I didn't change any Klipper settings, unless I am also missing something.

It seems we were to fast in testing. I guess I tried copying somewhere while the notification was still displaying.

Yep. While Spectacle is still running, the pasted image is in the X11 clipboard. As soon as Spectacle quits (i.e. the notification disappears), the image is gone.

davidre added a comment.EditedMar 13 2019, 2:18 PM

Code structure wise I would prefer to decouple doing the notification from the ExportManager meaning emitting a signal copiedToClipboard and then connecting it to doNotify like done above. But then I saw doSave has also this parameter and emits forceNotify (although I found only one place where this is set to true). So we have mutliple ways we trigger a notifiacation: Emitting forceNotify() from the ExportManager and connecting the imageSaved signal to doNotify. I already stated my preference but can also see the argument for this being the responsibility of the ExportManager (but then I would also argue that the doNotify method should be moved). In my opinion having only one place being responsible makes the code clearer.
Anyways sorry for the ramble. I guess if I find the time I will submit a refactoring diff.

How much effort would it take to change the default Klipper behavior?

How much effort would it take to change the default Klipper behavior?

My apologies for the delayed reply. Not actually that much effort, actually. Here's an example of how you'd do it: https://phabricator.kde.org/D9117#202448

kdautovic set the repository for this revision to R166 Spectacle.Mar 24 2019, 5:22 PM

Thanks, let's continue once that lands. :)

kdautovic updated this revision to Diff 54712.Mar 24 2019, 6:57 PM

Update doCopyToClipboard function with the "x-kde-force-image-copy" mime type hint.

ngraham added inline comments.Mar 24 2019, 7:09 PM
src/ExportManager.cpp
501 ↗(On Diff #54712)

In general we prefer to use enums rather than straight bool arguments.

kdautovic added inline comments.Mar 24 2019, 7:12 PM
src/ExportManager.cpp
501 ↗(On Diff #54712)

I used bool because notify was already defined as bool and refactoring as enum would take some more work that's unrelated to this revision.

Fair enough. Let's refactor in another revision.

kdautovic updated this revision to Diff 54715.Mar 24 2019, 7:20 PM

Use arcanist to get context

ngraham edited the summary of this revision. (Show Details)Mar 26 2019, 6:15 PM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the summary of this revision. (Show Details)Mar 26 2019, 6:28 PM
ngraham accepted this revision.Mar 26 2019, 6:32 PM

Thanks, this works great. Spectacle actually does exit correctly after using the -c option, but it just takes it a few seconds because it has to be running while the notification is visible. I think that's fine.

This revision is now accepted and ready to land.Mar 26 2019, 6:32 PM
This revision was automatically updated to reflect the committed changes.
cfeck added a subscriber: cfeck.Mar 26 2019, 7:58 PM
cfeck added inline comments.
src/Main.cpp
71

Applications/19.04 branch was already frozen on Mar 21 for new features/translations, see https://community.kde.org/Schedules/Applications/19.04_Release_Schedule

Did you ask for an exception?

ngraham added inline comments.Mar 26 2019, 8:06 PM
src/Main.cpp
71

Ah darn, I misread the schedule. :( It won't happen again.

pino added a subscriber: pino.Mar 26 2019, 9:56 PM

Because of the broken feature and string freezes, the commit was reverted in Applications/19.04, while still part of master (so it will be part of Applications 19.08).

Broken feature? What doesn't work? I tested it out and it worked fine for me.

pino added a comment.Mar 26 2019, 10:04 PM

Broken feature? What doesn't work? I tested it out and it worked fine for me.

"broken feature and string freezes" == "broken feature freeze and broken string freeze"

davidre added a comment.EditedApr 2 2019, 7:44 AM

Testing master using quit after copy works but doing spectacle -b -c does not. Can you confirm?
Also copying and then manually quitting doesn't add the image to clipper.
See also my comment at D20180#442070

Testing master using quit after copy works but doing spectacle -b -c does not. Can you confirm?

Darn, it worked before but now seems broken :(

Also copying and then manually quitting doesn't add the image to clipper.

Oooh, yeah that's a bug. I guess we should always add x-kde-force-image-copy to the metadata of a copied screenshot? Wanna do that on the Applications/19.04 branch?

Oooh, yeah that's a bug. I guess we should always add x-kde-force-image-copy to the metadata of a copied screenshot? Wanna do that on the Applications/19.04 branch?

That's where my confusion here and in D20180 is coming from. Isn't it always added unconditionally in ExportManager::doCopyToClipboard? And both the button regardless of quit after copy and spectacle -b -c go through that. Or am I missing a place where copying is done?

pino removed a subscriber: pino.Apr 2 2019, 6:00 PM

Oooh, yeah that's a bug. I guess we should always add x-kde-force-image-copy to the metadata of a copied screenshot? Wanna do that on the Applications/19.04 branch?

That's where my confusion here and in D20180 is coming from. Isn't it always added unconditionally in  `ExportManager::doCopyToClipboard`? And both the button regardless of quit after copy and spectacle -b -c go through that.  Or am I missing a place where copying is done?

That's what I think, too. As far as I'm aware, this is the only place that does copying, and it should always be called. Can anyone confirm?

I plan on fixing this before the beta release, or at least the full release. Anyone have any ideas what might be causing this?

I plan on fixing this before the beta release, or at least the full release. Anyone have any ideas what might be causing this?

The -b -c bug has been fixed in:
https://phabricator.kde.org/D23522
Which is merged into master

I saw that this patch exists, but a bit too late...

Now we have another problem:

https://bugs.kde.org/show_bug.cgi?id=411263

If spectacle is launched with --nonotify, like: spectacle -b -c -n
The image doesn't get saved into the clipboard. Whereas spectacle -b -c works fine.
I'm looking now for a few days into it and can't figure out why.