FEATURE: 385559
BUG: 393708
FIXED-IN: 19.04.0
Details
- Reviewers
ngraham - Group Reviewers
Spectacle - Commits
- R166:ec5e918743e7: Add option to copy screenshot to clipboard in background mode
Diff Detail
- Repository
- R166 Spectacle
- Lint
Lint Skipped - Unit
Unit Tests Skipped
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 ↗ | (On Diff #53758) | 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 ↗ | (On Diff #53758) | 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. |
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 ↗ | (On Diff #53758) | 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. |
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 ↗ | (On Diff #53758) | That makes sense. Okay, I'll rename it tomorrow. |
src/SpectacleCore.cpp | ||
---|---|---|
218 | It get's called through this connection. |
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
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 Actions → Update 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.
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.
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
src/ExportManager.cpp | ||
---|---|---|
501 | In general we prefer to use enums rather than straight bool arguments. |
src/ExportManager.cpp | ||
---|---|---|
501 | 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. |
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.
src/Main.cpp | ||
---|---|---|
71 ↗ | (On Diff #54879) | 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? |
src/Main.cpp | ||
---|---|---|
71 ↗ | (On Diff #54879) | Ah darn, I misread the schedule. :( It won't happen again. |
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 and string freezes" == "broken feature freeze and broken string freeze"
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
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?
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?
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.