FEATURE: 390415
FIXED-IN: 19.12.0
Add an option to save the screenshot to the clipboard after it has been taken.
davidre | |
ngraham |
Spectacle | |
VDG |
FEATURE: 390415
FIXED-IN: 19.12.0
Add an option to save the screenshot to the clipboard after it has been taken.
No Linters Available |
No Unit Test Coverage |
Buildable 15403 | |
Build 15421: arc lint + arc unit |
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.
src/Gui/SettingsDialog/GeneralOptionsPage.h | ||
---|---|---|
52 | member variables start with m mNothingToCopy, mCopySaveLocationToClipboard, etc |
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
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. :)
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:
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.
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 | ||
---|---|---|
340 | else? | |
349 | Why was this moved? |
Like the one on L159 in SpectacleCore.cpp ?
src/SpectacleCore.cpp | ||
---|---|---|
160 | enum the copy to clipboard, like this one? | |
340 | Continue with else for the if-else if.. above? Or you mean separate the check in this if with else for the !mCopySaveLocationToClipboard and !mCopyImageToClipboard variables? | |
349 | 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 | ||
---|---|---|
340 | mCopySaveLocationToClipboard || mCopyImageToClipboard is true when at least one of them is true. | |
349 | 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. |
src/SpectacleCore.cpp | ||
---|---|---|
269 | You would then call that here. |
Changed to enum and reverted the connect in L348 of SpectacleCore.cpp so spectacle -b -c works again.
src/SpectacleCore.cpp | ||
---|---|---|
275 | Call KSMainWindow::showInlineMessage here, right after lExportManager->doCopyToClipboard(false); | |
349 | 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.. |
src/SpectacleCore.cpp | ||
---|---|---|
350 | Oh you're right didn't think of that as I never use that feature myself. |
src/SpectacleCore.cpp | ||
---|---|---|
332 | Do we need the change here and in the if below now? We aren't calling doNotify anymore after auto copying right? |
src/SpectacleCore.cpp | ||
---|---|---|
332 | True, not needed anymore. Both are removed now. | |
350 | https://cgit.kde.org/spectacle.git/tree/src/Gui/KSMainWindow.h#n62 Is, unfortunately, private. |
src/SpectacleCore.cpp | ||
---|---|---|
350 | It seems I'm blind :D. |
Last nitpicks:
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 |
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 :)
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 :)
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. |
All done. I hope the names a re better now. Any recommendation for kde-dev best-practices?
Sorry for being annoying but I had a last look :)
Do you have commit access?
src/Gui/SettingsDialog/GeneralOptionsPage.cpp | ||
---|---|---|
69 | Remove comment |
Oh boy, I totally forgot about this one.. sorry :D
What exactly do you mean with Commit access?
There's the frameworks coding style which Spectacle does not follow entirely (There are also places where you can find a horrible style)
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 :)
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!