Add option to additionally save screenshot to clipboard
ClosedPublic

Authored by aprcela on Aug 14 2019, 10:55 PM.

Details

Summary

FEATURE: 390415
FIXED-IN: 19.12.0

Add an option to save the screenshot to the clipboard after it has been taken.

Test Plan

Diff Detail

Repository
R166 Spectacle
Branch
arcpatch-D23162
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15392
Build 15410: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes

Did some variable renaming.
Mostly from variables containing CopyToClipboard to CopySaveLocationToClipboard. So it's more clear what is saved to the clipboard (image or location).
Also: from mCopyFileToClipboard to mCopyImageToClipboard

Did this:
mCopyImageToClipboard = SpectacleConfig::instance()->copyImageToClipboard();
so it properly gets the current value, before this, it would take the value true/false once and use it all the time. Even after the user changed the setting while using Spectacle.

ngraham added inline comments.Aug 16 2019, 2:19 PM
src/Gui/SettingsDialog/GeneralOptionsPage.h
52

member variables start with m

mNothingToCopy, mCopySaveLocationToClipboard, etc

aprcela updated this revision to Diff 63878.Aug 16 2019, 5:33 PM
aprcela marked 2 inline comments as done.
  • Rename some 'file' variables to 'image'
aprcela marked an inline comment as done.Aug 16 2019, 5:34 PM

Fixed, sorry..

Hi guys, first time here, I just wanted to thank you for this super useful tool, I take a lot of screenshots and on kubuntu this is my one & only tool to do that, also I was starting to develop this same feature but I think that aprcela took the lead here! Anyway I considered developing this feature because on Windows I also take a lot of screenshot and there I use Greenshot, one useful feature of that software is that it lets you define your output, be it save directly to disk, copy to clipboard(ahem... ;)) or send to printer. It's possible to take into consideration this output feature of Greenshot to try to develop something similar on Spectacle? sorry for this long post and if this is not the right site to ask or comment this sorry, please show me where I could do that and I'll humbly delete this post and rant on wherever you point me

[...] one useful feature of that software is that it lets you define your output, be it save directly to disk, copy to clipboard(ahem... ;))

Just to give the game away a bit, after this patch lands, I was planning on asking @aprcela if he wanted to tackle that next. :) But in general I find it's best not to do that until after a triumph, such as a hefty patch landing, so for now let's continue with this one. :)

@ngraham , @jborrero

Since I was a long time user of ShareX (until I abandoned Windows), there's a lot I'm planning to add to spectacle :)
This will be on my list of next things to do.
Maybe discuss this further on bugs.kde.org ?

I don't think Copy file location to clipboard after saving belongs into the "After taking screenshot:" button group. It's confusing that it specifies two different points in time.

aprcela added a comment.EditedAug 19 2019, 7:51 AM

I don't think Copy file location to clipboard after saving belongs into the "After taking screenshot:" button group. It's confusing that it specifies two different points in time.

That one bugs me too.
To make more sense, Don't change the clipboard content and Copy image to clipboard should definitely go into the same button group.
Which means that Copy file location to clipboard after saving goes into a separate group.

There's one inconsistency tho, when a user activates both, we get the case 4) i mentioned up above. Which is very esoteric. See here:

1st put the screenshot in the clipboard, after it has been saved replace the clipboard content with the file location

This seems like a pretty esoteric use case. What would be the purpose? Can you envision why someone would want to do this. Also note that this isn't currently a feature that exists, so we're not debating taking it away from anyone, but rather proposing to add it in the first place. That makes the bar for adding it much higher. :)

One approach would be:
Two groups. First one called After taking screenshot: and containing Don't change the clipboard content and Copy image to clipboard.
Second group called After saving image: with a checkbox Copy file location to clipboard after saving and placed in Save, rather than General (as it used to be)

Take this patch also into consideration: https://phabricator.kde.org/D23210

I don't think Copy file location to clipboard after saving belongs into the "After taking screenshot:" button group. It's confusing that it specifies two different points in time.

That one bugs me too.
To make more sense, Don't change the clipboard content and Copy image to clipboard should definitely go into the same button group.
Which means that Copy file location to clipboard after saving goes into a separate group.

There's one inconsistency tho, when a user activates both, we get the case 4) i mentioned up above. Which is very esoteric. See here:

1st put the screenshot in the clipboard, after it has been saved replace the clipboard content with the file location

This seems like a pretty esoteric use case. What would be the purpose? Can you envision why someone would want to do this. Also note that this isn't currently a feature that exists, so we're not debating taking it away from anyone, but rather proposing to add it in the first place. That makes the bar for adding it much higher. :)

One approach would be:
Two groups. First one called After taking screenshot: and containing Don't change the clipboard content and Copy image to clipboard.
Second group called After saving image: with a checkbox Copy file location to clipboard after saving and placed in Save, rather than General (as it used to be)

Take this patch also into consideration: https://phabricator.kde.org/D23210

Good idea.
With D23210 in mind however I would probably do

After taking  screenshot:  o Do Nothing
                           o Copy to clipboard
                           o Save Image

And have the other checkbox in the Save category as you said.

Good idea.
With D23210 in mind however I would probably do

After taking  screenshot:  o Do Nothing
                            o Copy to clipboard
                            o Save Image

And have the other checkbox in the Save category as you said.

Like this? And the 3rd radiobutton Save image would be added in D23210

aprcela updated this revision to Diff 64025.Aug 19 2019, 10:53 AM
  • Reorder buttons
ngraham accepted this revision as: VDG.Aug 19 2019, 7:43 PM

Brilliant; that UI makes much more sense. UI and functionality looks good to me now. @davidre, would you like to do the remaining code review bits on behalf of Spectacle?

In general I'm fine with the approach taken here if we would just add this one setting. However we want to add at least one more button to this group. I think using an enum here and having only a single config entry would be better. Compare the printkeyactionrunning group.

src/SpectacleCore.cpp
337

else?

346

Why was this moved?

In general I'm fine with the approach taken here if we would just add this one setting. However we want to add at least one more button to this group. I think using an enum here and having only a single config entry would be better. Compare the printkeyactionrunning group.

Like the one on L159 in SpectacleCore.cpp ?

src/SpectacleCore.cpp
159

enum the copy to clipboard, like this one?

337

Continue with else for the if-else if.. above?
The upper one sets the text, while this one sets the action and url for the file.
I left it separated as it was.

Or you mean separate the check in this if with else for the !mCopySaveLocationToClipboard and !mCopyImageToClipboard variables?

346

Because if this is where it was before, it would close Spectacle after the notification disappears:

original:

moved:

For the enum. You would define it inside SpectacleConfig and then save the respective value instead of a config entry for each option.

src/SpectacleCore.cpp
337

mCopySaveLocationToClipboard || mCopyImageToClipboard is true when at least one of them is true.
!mCopySaveLocationToClipboard && !mCopyImageToClipboard is true when both of them is false.
So I thought it could be made the else of the if in Line 321. Now I see it already has an else if and an else.
You can leave it as is

346

I understand you want to show feedback when copying. With moving this you broke the case when we want to show a notifcation after copying and then exit i.e. spectacle -b -c. I would also prefer if we could display a Inline message instead like when you use the button to copy to clipbboard.
For that we could reuse KSMainWindow::sendToClipboard which is currently private.

davidre added inline comments.Aug 20 2019, 10:34 AM
src/SpectacleCore.cpp
268

You would then call that here.

aprcela updated this revision to Diff 64187.Aug 21 2019, 9:57 AM
aprcela marked 8 inline comments as done.
  • Use enum for copyToClipboard
  • Revert connect to fix

Changed to enum and reverted the connect in L348 of SpectacleCore.cpp so spectacle -b -c works again.

src/SpectacleCore.cpp
274

Call KSMainWindow::showInlineMessage here, right after lExportManager->doCopyToClipboard(false);

346

Reverted, works again.

If we would use KSMainWindow::sendToClipboard than we have the problem when a user has both Quit after Save or copy and Copy image to clipboard activated, than Spectacle will immediately close after the image has been saved to the clipboard..
I'd rather go and make the KSMainWindow::showInlineMessage public and call it after lExportManager->doCopyToClipboard(false);

davidre added inline comments.Aug 21 2019, 10:08 AM
src/SpectacleCore.cpp
347

Oh you're right didn't think of that as I never use that feature myself.
Good news: showInlineMessage is already public

davidre added inline comments.Aug 21 2019, 10:14 AM
src/SpectacleCore.cpp
329

Do we need the change here and in the if below now? We aren't calling doNotify anymore after auto copying right?

aprcela updated this revision to Diff 64188.Aug 21 2019, 10:24 AM
aprcela marked 3 inline comments as done.
  • Remove unnecessary check
aprcela added inline comments.Aug 21 2019, 10:26 AM
src/SpectacleCore.cpp
329

True, not needed anymore. Both are removed now.

347

https://cgit.kde.org/spectacle.git/tree/src/Gui/KSMainWindow.h#n62

Is, unfortunately, private.
I assume it won't just work by moving it to the public group?

davidre added inline comments.Aug 21 2019, 12:10 PM
src/SpectacleCore.cpp
347

It seems I'm blind :D.
Just move it to the public group.

aprcela updated this revision to Diff 64195.Aug 21 2019, 12:37 PM
  • Move showInlineMessage() and MessageDuration to public
aprcela marked 4 inline comments as done.Aug 21 2019, 12:38 PM

Done. Tested & it's working.

src/SpectacleCore.cpp
347

done :)

Last nitpicks:

  • Since with your other patch this setting will control what happens after a Screenshot is taken and does not control what to copy to the clipboard, could you rename some variables accordingly?
  • Write to GeneralConfig instead of GuiConfig. I plan to move some settings around so the place where they are configured corresponds to the their group.
src/Gui/SettingsDialog/GeneralOptionsPage.cpp
63

This one

src/Gui/SettingsDialog/GeneralOptionsPage.h
51

Also of course the group

src/SpectacleConfig.cpp
372

We don't need this whole logic here. The print key stuff needed it because one Focusing the window only works on X11 right now and not on Wayland. Just return static_cast<SpectacleConfig::CopyImageToClipboardSetting>(mGuiConfig.readEntry(QStringLiteral("copyImageToClipboard"), clipboardAction); (Or split in two lines if it is to long).

src/SpectacleConfig.h
58

The enum name and enum values

135

The method names

aprcela added a comment.EditedAug 21 2019, 1:31 PM

Last nitpicks:

  • Since with your other patch this setting will control what happens after a Screenshot is taken and does not control what to copy to the clipboard, could you rename some variables accordingly?

is this renaming ok:
CopyImageToClipboardSetting -> AfterTakingScreenshotAction (everything, including enum)

enum CopyImageToClipboardSetting : int { DoNotChangeClipboard = 0, CopyImageToClipboard }
to:
enum AfterTakingScreenshotAction : int { NoClipboardChange = 0, CopyImageToClipboard }

nothingToCopy -> doNothing

CopyToClipboardGroup -> mAfterTakingScreenshotGroup

  • Write to GeneralConfig instead of GuiConfig. I plan to move some settings around so the place where they are configured corresponds to the their group.

As far as I can see, nothing of the stuff I changed and added gets written to GuiConfig. Can you link to something?
NVM, found it :)

Last nitpicks:

  • Since with your other patch this setting will control what happens after a Screenshot is taken and does not control what to copy to the clipboard, could you rename some variables accordingly?

is this renaming ok:
CopyImageToClipboardSetting -> AfterTakingScreenshotAction (everything, including enum)

enum CopyImageToClipboardSetting : int { DoNotChangeClipboard = 0, CopyImageToClipboard }
to:
enum AfterTakingScreenshotAction : int { NoClipboardChange = 0, CopyImageToClipboard }

nothingToCopy -> doNothing

CopyToClipboardGroup -> mAfterTakingScreenshotGroup

Yes I was thinking of something like that! NoClipboardChange should also probably be something like doNothing like the button.

  • Write to GeneralConfig instead of GuiConfig. I plan to move some settings around so the place where they are configured corresponds to the their group.

As far as I can see, nothing of the stuff I changed and added gets written to GuiConfig. Can you link to something?
NVM, found it :)

aprcela updated this revision to Diff 64203.Aug 21 2019, 1:42 PM
  • Rename variables, methods and enum. Remove unnecessary logic

Almost good to go now :)

src/SpectacleConfig.cpp
366

We forgot the actual config key, and I guess clipboardAction should be doNothing. The key could. just be afterTakingScreenshot.

aprcela marked 7 inline comments as done.Aug 21 2019, 1:46 PM

All done. I hope the names a re better now. Any recommendation for kde-dev best-practices?

aprcela updated this revision to Diff 64205.Aug 21 2019, 1:51 PM
  • Setting variable renamed
aprcela marked an inline comment as done.Aug 21 2019, 1:52 PM

I hope that's it :)

Sorry for being annoying but I had a last look :)

Do you have commit access?

src/Gui/SettingsDialog/GeneralOptionsPage.cpp
69

Remove comment

aprcela updated this revision to Diff 64208.Aug 21 2019, 2:00 PM
aprcela marked an inline comment as done.
  • Setting variable renamed, remove commented code

Sorry for being annoying but I had a last look :)

Do you have commit access?

Oh boy, I totally forgot about this one.. sorry :D

What exactly do you mean with Commit access?

All done. I hope the names a re better now. Any recommendation for kde-dev best-practices?

There's the frameworks coding style which Spectacle does not follow entirely (There are also places where you can find a horrible style)

Sorry for being annoying but I had a last look :)

Do you have commit access?

Oh boy, I totally forgot about this one.. sorry :D

What exactly do you mean with Commit access?

To push to KDE repositories you need to have a developer account. If you don't have one I can commit for you.

davidre accepted this revision.Aug 21 2019, 2:05 PM
This revision is now accepted and ready to land.Aug 21 2019, 2:05 PM

To push to KDE repositories you need to have a developer account. If you don't have one I can commit for you.

No, I don't have a dev account. Only this, bug.kde and the kde forum.
I would appreciate that :)

ngraham accepted this revision.Aug 21 2019, 2:12 PM

Very nice work.

Looks like you have some non-UNIX line endings that our commit hookscript complains about when I try to land the patch:

remote: Audit failure - Commit 10a29ef1849c50547c13adc2416a02dfb82b4378 - End of Line Style (non-Unix): src/Gui/SettingsDialog/SaveOptionsPage.cpp
remote: Push declined - commits failed audit

Can you fix it?

aprcela updated this revision to Diff 64211.Aug 21 2019, 2:24 PM
  • Fix end of line

Looks like you have some non-UNIX line endings that our commit hookscript complains about when I try to land the patch:

remote: Audit failure - Commit 10a29ef1849c50547c13adc2416a02dfb82b4378 - End of Line Style (non-Unix): src/Gui/SettingsDialog/SaveOptionsPage.cpp
remote: Push declined - commits failed audit

Can you fix it?

Weird, used kdevelop all the time. I fixed it now. Can you test again please? Thanks!

There we go, much better. Congrats on the nice big patch landed!

This revision was automatically updated to reflect the committed changes.