Add shortcuts for copying screenshots to clipboard
Needs ReviewPublic

Authored by kapillamba4 on Dec 2 2017, 8:38 PM.

Details

Reviewers
bgupta
ngraham
rkflx
Group Reviewers
Spectacle
Summary

Add 3 more shortcuts for copying to clipboard directly without saving the screenshot locally

  1. Full Screen screenshot to clipboard (Ctrl+Shift+Print)
  2. Active Window screenshot to clipboard (Ctrl+Meta+Print)
  3. Rectangular Region screenshot to clipboard (Ctrl+Print)

Originally below 3 shortcuts existed in spectacle:

  1. Full Screen screenshot (Shift+Print)
  2. Active Window screenshot (Meta+Print)
  3. Rectangular Region screenshot (Meta+Shift+Print)

On X11 clipboard content is owned by the application, Data copied to clipboard remains in the clipboard only as long as the parent application is running and is discarded when the application quits.

Made changes to ExportManager::doCopyToClipboard() to have screenshots persist in klipper clipboard manager even after spectacle application exits.

Added "x-kde-force-image-copy" mime type to screenshots being copied to clipboard, which forces klipper clipboard manager to remember this clipboard entry even if user has set "ignore images" option to true in klipper.

FEATURE: 375382

Test Plan

Tested all 6 shortcuts using dbus-monitor and checked the clipboard after using each key-combination.

Diff Detail

Repository
R166 Spectacle
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes

Hmm, depending on xclip seems like it might not be the best approach, both because it's another external dependency, and also because it's not Wayland compatible. I wonder how hard it would be to add the missing functionality to Klipper.

We can't do that.

But if xclip can take an image and have it persist after it closes, why can't we?

src/SpectacleCore.cpp
177

what happens if you press the shortcut with an existing spectacle open?

Also do you have a danger of making multiple connections if a user presses it twice?

Ok, if relying on xclip is acceptable, then I have no objection! We will need to come up with a Wayland plan, though. Also, marking the new dependency will probably require some CMake changes.

Ok, if relying on xclip is acceptable,

That's not what I meant.

I meant, look at how xclip saves an image after xclip exits by reading xclip code.

Oh! That makes more sense, sorry for the misunderstanding.

kapillamba4 added a comment.EditedFeb 3 2018, 10:03 PM

I think i will not be able to implement copy image to clipboard feature myself even after reading xclip source code, i had no idea of X11 or wayland before this. And yes there is a danger of making multiple connections (for non-gui/copy-to-clipboard shortcuts), i have just tested it. I think there must be someone workaround for this, I can look for it but i am not sure about implementing the copy image to clipboard feature for x11 myself.

And i don't think we need to find a seperate plan for wayland, this image persistence problem in QClipboard seems to exists only for x11 (i don't have the source right now but read about this somewhere at https://bugreports.qt.io)

rkflx added a subscriber: rkflx.Feb 7 2018, 10:41 AM

@kapillamba4 If I uncheck "Ignore images" in Klipper's settings (it's checked by default, but why?), clicking on Copy To Clipboard and closing Spectacle afterwards allows me to paste the screenshot.

Could you check whether it works then with your patch via DBus too?

In D9117#202346, @rkflx wrote:

@kapillamba4 If I uncheck "Ignore images" in Klipper's settings (it's checked by default, but why?), clicking on Copy To Clipboard and closing Spectacle afterwards allows me to paste the screenshot.

Could you check whether it works then with your patch via DBus too?

wow, it works. Didn't knew that klipper had this option. Should have read the klipper handbook first.
Thanks @rkflx, all six shortcuts work now, no need to use any external program. We should change the default checked state in klipper settings for 'ignore images' to unchecked.

The sources i remember for x11 QClipboard issue were this:
https://bugreports.qt.io/browse/QTBUG-11616
and
https://bugreports.qt.io/browse/QTBUG-16194

wow, it works. Didn't knew that klipper had this option. Should have read the klipper handbook first.
Thanks @rkflx, all six shortcuts work now, no need to use any external program. We should change the default checked state in klipper settings for 'ignore images' to unchecked.

Sounds great, and I would approve of that. Could you open another revision to do this? Thanks!

twist on your proposal; we special case it rather than changing the defaults.

We change klipper as follows:

diff --git a/klipper/klipper.cpp b/klipper/klipper.cpp
index 2905e6a3..db5b4c06 100644
--- a/klipper/klipper.cpp
+++ b/klipper/klipper.cpp
@@ -731,7 +731,7 @@ void Klipper::checkClipData( bool selectionMode )
         ; // ok
     else if( data->hasImage() )
     {
-        if( m_bIgnoreImages )
+        if( m_bIgnoreImages && !data->hasFormat("x-kde-force-image-copy"))
             return;
     }
     else // unknown, ignore

then in spectacle code, change ExportManager::doCopyToClipboard()

to

auto mime = new QMimeData();
mime->setPixmap(mSavePixmap)
mime->setData("x-kde-force-image-copy", ...);
qApp->clipBoard()->setMimeData(mime);

I've not tested this.

@davidedmundson that looks like it could work, but it might make more sense to just change the default in Klipper, because then all image-handling apps get the same behavior for free. I wonder what the original rationale was for using the current default, and whether that still hold true today in 2018.

The downside is memory usage.

A full HD picture can be easily 20mb uncompressed, and a gimp/krita workflow could easily see you copying that a dozen times in short succession. We'd be keeping all of them.

kapillamba4 added a comment.EditedFeb 8 2018, 8:09 AM

By multiple connections do you mean multiple instance of the application? If yes then we can change the StartupOption for KDBusService, but i don't see any of these 6 shortcuts creating multiple instance of this application.

diff --git a/src/Main.cpp b/src/Main.cpp
index b9b952c..e0dec92 100644
--- a/src/Main.cpp
+++ b/src/Main.cpp
@@ -144,7 +144,7 @@ int main(int argc, char **argv)
 
     // create the dbus connections
 
-    new KDBusService(KDBusService::Multiple, &core);
+    new KDBusService(KDBusService::Unique, &core);
 
     SpectacleDBusAdapter *dbusAdapter = new SpectacleDBusAdapter(&core);
     QObject::connect(&core, &SpectacleCore::grabFailed, dbusAdapter, &SpectacleDBusAdapter::ScreenshotFailed);

If we use Meta+shift+print shortcut repeatedly then the dbus method is invoked again and again, and this happens:

^ happens with or without this patch

wow, it works. Didn't knew that klipper had this option. Should have read the klipper handbook first.
Thanks @rkflx, all six shortcuts work now, no need to use any external program. We should change the default checked state in klipper settings for 'ignore images' to unchecked.

Sounds great, and I would approve of that. Could you open another revision to do this? Thanks!

I will open another revision soon.

@kapillamba4 Thanks, I'll look at it in a bit. Are you still planning to open a new Diff against Klipper, or should someone else?

In D9117#208026, @rkflx wrote:

@kapillamba4 Thanks, I'll look at it in a bit. Are you still planning to open a new Diff against Klipper, or should someone else?

I don't have any experience building, running or testing plasma-workspace components. I think someone else should open a revision.

rkflx requested changes to this revision.Feb 17 2018, 11:58 PM
In D9117#208026, @rkflx wrote:

@kapillamba4 Thanks, I'll look at it in a bit. Are you still planning to open a new Diff against Klipper, or should someone else?

I don't have any experience building, running or testing plasma-workspace components. I think someone else should open a revision.

Are you saying you did not test whether something you implemented actually works? Normally, that's not something your reviewer wants to hear… ;)

I can work on the patch for Klipper, but it will need more time because when testing I noticed a problem with the approach as outlined in D9117#202448: For a FullHD screen and the default of 7 entries, taking a couple of screenshots increases memory consumption by ~70MB (difference in memory usage before/after for plasmawindowed, confirmed by comparing a Klipper restart with and without history restore). As Klipper is enabled by default and by default restores its entries on Plasma startup, this means we increase startup memory consumption by ~70MB for a typical setup. That's quite a chunk compared to the ~350MB a system with Plasma consumes in total otherwise. Also, I can imagine users won't be too happy if we plainly ignore their choice for Ignore images.

I pondered for quite a bit on what we can do from Spectacle's side to reduce copying to a subset of cases, but there is no elegant way really. I think we should fix the problem in Klipper by (only if Ignore images is enabled, of course)

  • replacing the entry instead of adding a new entry
  • hiding the entry from the user / remembering it only internally
  • not restoring the entry upon restart

@ngraham @davidedmundson I'll investigate if any of this is possible at all as soon as time allows, unless someone has a better idea or it's no big deal to waste memory.


As for the review, I'd need to test the patch first. However, I cannot get the new shortcuts to appear in kcmshell keys or kcmshell khotkeys. Any tips?

src/ExportManager.cpp
370

This does not compile, you'll need QStringLiteral around both string arguments.

Also, please describe in the summary how this mechanism is supposed to work and why you added it.

This revision now requires changes to proceed.Feb 17 2018, 11:58 PM
kapillamba4 added a comment.EditedFeb 18 2018, 5:50 AM

@rkflx, I didn't update DataCount in spectacle.khotkeys that's why you are not seeing the new shortcuts. I'll just update this patch. You will need to import the spectacle.khotkeys file to get the new shortcuts in kcmshell5 khotkeys

src/ExportManager.cpp
370

@rkflx I'll edit this line of code and add QStringLiteral around both string literals.

When we setImageData then format is "application/x-qt-image" ( using mimeData->formats() ) but setData() sets another mimeType "x-kde-force-image-copy" with same data returned for mimeType "application/x-qt-image".

Now if we call mimeData->formats() then we have a QStringList of 2 mime types:
"application/x-qt-image"
and
"x-kde-force-image-copy"

kapillamba4 added a comment.EditedFeb 18 2018, 6:05 AM

@rkflx, what do you think about adding a static variable inside takeNewScreenshotDBus() and takeNewScreenshot() methods to keep track of how many times these methods are called and prevent this D9117#202850 problem

rkflx added a comment.EditedFeb 18 2018, 8:00 AM

Thanks, compiles now, and the counter bump did the trick so I can review now. However there was a caveat: I needed to rm ~/.config/khotkeysrc and restart kded.

IOW, nobody upgrading to Spectacle 18.04 will get the new shortcuts. I'm afraid we cannot tell every user to manually import the new shortcuts from "somewhere". Can we find a better way, a trick or a fix to KHotKeys perhaps?

src/ExportManager.cpp
370

Also, please describe in the summary how this mechanism is supposed to work and why you added it.

Thanks for explanation, but I know very well what this code does ;) I meant that you should add it to the summary of this Diff, which will become the commit message. This way if someone git blames this odd-looking x-kde-force-image-copy, he'll know what it is for.

Also, I think you should describe it on a higher level, i.e. that Klipper ignores images by default, and therefore "Quit after Copy" or "Hotkey for capture to clipboard" do not work because without support by a clipboard manager on X11 access to the application's clipboard is lost as soon at the application exits. The new mimetype then will indicate to Klipper to always remember the new clipboard entry.

kapillamba4 added a comment.EditedFeb 18 2018, 12:31 PM

@rkflx, There is no need to import spectacle.khotkeys file seperately, Spectacle will install spectacle.khotkeys into /usr/share/khotkeys upon installation itself.

i removed ~/.config/khotkeysrc and then replaced /usr/share/khotkeys/spectacle.khotkeys
with the new desktop/spectacle.khotkeys file having 7 shortcuts, and after relogin i could see all 7 shortcuts in khotkeys.

Somehow there's still a bug. If I make install to /usr from the master branch, Print will call the Spectacle GUI just fine. However with your patch, kded will only print the usual DBus message without the GUI appearing and I have to killall spectacle afterwards.

That might be rooted in how you test Spectacle. Make sure to use install to the proper place (you might have to set -DCMAKE_INSTALL_PREFIX=/usr for cmake) instead of only compiling the sources, otherwise the DBus call will reach the wrong binary. If you rebase on master, you'll even get a different button layout so you can tell the difference.


i removed ~/.config/khotkeysrc and then replaced /usr/share/khotkeys/spectacle.khotkeys with the new desktop/spectacle.khotkeys file having 7 shortcuts, and after relogin i could see all 7 shortcuts in khotkeys.

That's how upgrades work in general, except for an important detail: You cannot remove ~/.config/khotkeysrc, because this would destroy user data (and also we cannot tell everybody to remove their config file).


@rkflx, what do you think about adding a static variable inside takeNewScreenshotDBus() and takeNewScreenshot() methods to keep track of how many times these methods are called and prevent this D9117#202850 problem

Not sure if a static is the best way, but I think it makes sense to simply ignore DBus hotkeys if taking a screenshot is currently in progress. In practice that should be quite fast, so it would only be relevant for "Rectangular Region" captures, still better solve it generically.

In any case, please open a separate Diff for that. And make sure to add BUG: 381778 ;)

Also, please setup Arcanist to upload the patch, it makes it so much easier to review because it will include your authorship information and the dreaded "Context not available" will be gone from Phabricator. Thanks!

dbus/org.kde.Spectacle.xml
101

There were 5 DBus methods before, but you only add 3. In principle we should support all 5 also for the clipboard case, even if we don't add shortcuts for them. Also, some users (e.g. in Bug 390415) want to save and copy simultaneously.

I wonder if it would be better to not add new methods and instead extend the existing ones with two optional parameters: method(..., saveToFile=true, copyToClipboard=false). Could you research whether DBus supports such optional parameters? We'd still have to ensure that existing users of the DBus API don't have to change their calls.

desktop/spectacle.khotkeys
611

The naming should be more in line with what is on the button in GUI. Idea:

Copy Screenshot of Full Screen to Clipboard

Also, I think we should now emphasize for the older shortcuts that those save to a file. Idea:

Save Screenshot of Full Screen to File

Adapt the other shortcuts accordingly, and improve the XML in that regard too.

src/ExportManager.cpp
371–372

Use QApplication::clipboard()->setMimeData.

src/SpectacleCore.cpp
186

Those connects look wrong. Why would you call them everytime you take a screenshot via DBus? Spectacle can already be running and accept DBus calls, it's not true in general that it only starts when the hotkey is pressed and exits immediately.

src/SpectacleCore.h
52

Is there any reason you need to introduce a new method here? AFAIK Spectacle could be called from DBus before and you only a some new variants to this.

In general I feel because of this change you have to move a lot of things around, potentially introducing many bugs (did not look into this too much yet, though, so take this comment with a grain of salt).

kapillamba4 edited the summary of this revision. (Show Details)Feb 18 2018, 12:54 PM
kapillamba4 edited the summary of this revision. (Show Details)Feb 18 2018, 12:58 PM
kapillamba4 added inline comments.Feb 18 2018, 1:49 PM
src/SpectacleCore.cpp
186

I didn't think of this. We can use Qt::UniqueConnection to prevent duplicate connections or else multiple connections will lead to multiple calls to slots.

What we want to know is if user wants to save the image or copy it to clipboard which is indicated by shouldSave variable, then only we connect appropriate signals and slots.

src/SpectacleCore.h
52

I can merge SpectacleCore::takeNewScreenshotDBus() back into SpectacleCore::takeNewScreenshot() but i think that it will only make a small difference in SpectacleCore::takeNewScreenshot(), which is:

if(mStartMode != DBusMode) {  ...  } else { ... }
kapillamba4 added inline comments.Feb 18 2018, 1:54 PM
src/SpectacleCore.cpp
186

I thought qt prevents multiple duplicate connections by default

kapillamba4 added inline comments.Feb 18 2018, 2:00 PM
src/SpectacleCore.h
52

Also we would have to add another parameter shouldSave to SpectacleCore::takeNewScreenshot() and make appropriate changes everywhere for SpectacleCore::takeNewScreenshot()

rkflx added inline comments.Feb 18 2018, 2:09 PM
src/SpectacleCore.cpp
186

In general it works like this:

  • At application startup, wire together signals and slots with connect.
  • After this, when the application is running, react to signals by code paths in the slots, which is where the magic happens. No re-wiring necessary.

If you need to use different connections depending on what the user is doing, your code design might be wrong. From a high level, it should look like this:

takeScreenshot(options) {
  if(option1)
    action1;
  if(option2 && specialInternalState)
    action2;
}
src/SpectacleCore.h
52

That's preferable to simply copying the code around. Also, default parameters can help. Perhaps it would look similar to what I proposed in D9117#inline-50329.

kapillamba4 added inline comments.Feb 18 2018, 2:15 PM
src/SpectacleCore.cpp
186

I tried this in the first place but the codebase is such that it will require alot of changes if we don't use connects inside the function.

rkflx requested changes to this revision.Feb 18 2018, 2:34 PM
In D9117#208750, @rkflx wrote:

Somehow there's still a bug. If I make install to /usr from the master branch, Print will call the Spectacle GUI just fine. However with your patch, kded will only print the usual DBus message without the GUI appearing and I have to killall spectacle afterwards.

Can anybody reproduce?

src/SpectacleCore.cpp
186

Workflow before:

setOptions
takeScreenshot
(always)
  doSave

Workflow after:

setOptions
takeScreenshot
if(save)
  doSave
if(copy)
  doCopy

The structure of the code should reflect that workflow. It might need adaptations beyond takeNewScreenshot, of course. I'm sure with patience you'll solve this eventually ;)

This revision now requires changes to proceed.Feb 18 2018, 2:34 PM
kapillamba4 added a comment.EditedFeb 19 2018, 6:27 PM
In D9117#208821, @rkflx wrote:
In D9117#208750, @rkflx wrote:

Somehow there's still a bug. If I make install to /usr from the master branch, Print will call the Spectacle GUI just fine. However with your patch, kded will only print the usual DBus message without the GUI appearing and I have to killall spectacle afterwards.

Can anybody reproduce?

I was working on a branch i made when i created this revision, maybe some change broke the previous patch, i am sorry for that.

Now all shortcuts work, also added dbus support for all 5 methods with shouldSave and shouldCopy arguments.
I think savemode enum should be in SpectacleCore instead of ImageGrabber?

I was initially thinking of adding shouldSave and shouldCopy parameters to signals and slots so that they reach SpectacleCore::screenshotUpdated slot but i think this is cleaner way. What do you guys think?

ngraham requested changes to this revision.Feb 20 2018, 4:10 AM

FWIW, I also do not get the new hotkeys on upgrade, even when I deploy everything to /usr and then reboot. This needs to be fixed or else current users won't get the new feature.

This revision now requires changes to proceed.Feb 20 2018, 4:10 AM
kapillamba4 added a comment.EditedFeb 20 2018, 7:03 PM

FWIW, I also do not get the new hotkeys on upgrade, even when I deploy everything to /usr and then reboot. This needs to be fixed or else current users won't get the new feature.

I don't why spectacle khotkeys are not automatically imported after spectacle installation. I will try to find a fix for this in here or in khotkeys.

And will also setup archanist.

rkflx added a comment.EditedFeb 22 2018, 9:42 PM

@kapillamba4 Sorry I haven't got around to review your latest changes yet, please bear with me.

I only tried it briefly, and I could now take a screenshot straight to the clipboard with a single keypress. Good job!

However, there was no feedback at all. I think this should show a notification, just like when you save to a file. As far as I understand, users could turn off those notifications in their global settings if they don't like them. What do you think?


As for the update mechanism, I found https://forum.kde.org/viewtopic.php?f=17&t=88836. Could you have a look? Perhaps there is an official way we are supposed to use for this.

kapillamba4 added a comment.EditedFeb 28 2018, 2:16 PM
In D9117#211878, @rkflx wrote:

As for the update mechanism, I found https://forum.kde.org/viewtopic.php?f=17&t=88836. Could you have a look? Perhaps there is an official way we are supposed to use for this.

@rkflx, I looked at https://forum.kde.org/viewtopic.php?f=17&t=88836, khotkeys_update does not exist anymore. There is no dbus method for khotkeys to update a shortcut group.
I think a bash script can be used to parse khotkeysrc files and remove all spectacle entries using it, but that will be a very ugly job and not a right way to do. One way is to rename spectacle.khotkeys to spectacle02.khotkeys and "importId" in spectacle.khotkeys to spectacle02, this will create another screenshot shortcuts group inside khotkeys on installation.

Old shortcuts group will not work because, those shortcuts use old dbus methods which used different number of arguments.

In D9117#211878, @rkflx wrote:

@kapillamba4 Sorry I haven't got around to review your latest changes yet, please bear with me.

I only tried it briefly, and I could now take a screenshot straight to the clipboard with a single keypress. Good job!

However, there was no feedback at all. I think this should show a notification, just like when you save to a file. As far as I understand, users could turn off those notifications in their global settings if they don't like them. What do you think?

How about adding a screenshot sound like /usr/share/sounds/freedesktop/stereo/camera-shutter.oga instead of a notification, i have not tried it though.

rkflx added a comment.Feb 28 2018, 7:38 PM

@kapillamba4 Thanks for your patience! ;)

@rkflx, I looked at https://forum.kde.org/viewtopic.php?f=17&t=88836, khotkeys_update does not exist anymore. There is no dbus method for khotkeys to update a shortcut group.
I think a bash script can be used to parse khotkeysrc files and remove all spectacle entries using it, but that will be a very ugly job and not a right way to do. One way is to rename spectacle.khotkeys to spectacle02.khotkeys and "importId" in spectacle.khotkeys to spectacle02, this will create another screenshot shortcuts group inside khotkeys on installation.

We have kconf_update as the official update mechanism, and it also supports scripts. However I don't know yet whether it's applicable to our situation, as I don't understand yet where the configs you are talking about are coming from. If you'd enlighten me in that regard, maybe I could help you better.

Old shortcuts will not work because, those shortcuts use old dbus methods which used different number of arguments.

Too bad. Then we might have to duplicate some parts, i.e. on the DBus side provide the old func(1, 2) as well as the new func(1, 2, 3), and in Spectacle forward the old to the new function by setting the missing parameters.

Another option would be to wait a bit, because we might have to rework more things and perhaps do a huge incompatible change for the 18.08 release (not sure yet, see also D10879).

How about adding a screenshot sound like /usr/share/sounds/freedesktop/stereo/camera-shutter.oga instead of a notification, i have not tried it though.

Personally I'm not a fan of sounds on the desktop ;) I think we should show a visual notification. Nevertheless, if we use KNotification I believe users can just configure the type of notification they want, see kcmshell5 kcmnotify.

kapillamba4 updated this revision to Diff 28745.EditedMar 5 2018, 7:15 PM

@rkflx, Added kconf_update :)

kconf_update will remove old shortcuts on installation of spectacle, Still need to relogin after installation for new screenshots to appear.
Please apply this patch on master:af6efd

kapillamba4 updated this revision to Diff 28746.Mar 5 2018, 7:19 PM
kapillamba4 updated this revision to Diff 28748.Mar 5 2018, 7:23 PM

This is fantastic, I can confirm that with the latest changes and a log-out/reboot, the new shortcuts appear and work properly.

I agree with @rkflx that we need some sort of feedback here. FWIW I would be in favor of a subtle picture-taking sound (that's what macOS's screenshot tool does), but a notification pop-up would be fine too.

cfeck retitled this revision from Add shortcuts for copying screenshots to clipboard. to Add shortcuts for copying screenshots to clipboard.Mar 30 2018, 3:11 AM
cfeck edited the summary of this revision. (Show Details)
cfeck added a subscriber: cfeck.Mar 30 2018, 3:13 AM
cfeck added inline comments.
src/SpectacleCore.cpp
156

Missing space. Please review all code changes for correct white space, or use a code formatting tool.

Format Code and update patch to work with latest commit b50dc95

kapillamba4 marked 17 inline comments as done.Mar 30 2018, 6:47 PM

Tested all shortcuts

rkflx added a comment.Mar 30 2018, 9:07 PM

@kapillamba4 Are you still planning to add some sort of notification, or should I review the code now? (Won't be done before next week anyway…)

kapillamba4 added a comment.EditedMar 30 2018, 9:16 PM
In D9117#237295, @rkflx wrote:

@kapillamba4 Are you still planning to add some sort of notification, or should I review the code now? (Won't be done before next week anyway…)

@rkflx, I think i will add notification option in https://phabricator.kde.org/D11203 (a checkbox in configure menu of spectacle to toggle show/hide notification on copying to clipboard) after this revision

rkflx added a comment.Mar 30 2018, 9:19 PM

I think i will add notification in https://phabricator.kde.org/D11203 (a checkbox in configure menu of spectacle to toggle show/hide notification on copying to clipboard) after this revision

In that case it would be best to create a new Diff entirely. Also, please don't add a checkbox to Spectacle. As I wrote above, the official way to configure this is via the existing button on the notification (which opens the respective KCM).

kapillamba4 added a comment.EditedMar 30 2018, 9:55 PM
In D9117#237297, @rkflx wrote:

I think i will add notification in https://phabricator.kde.org/D11203 (a checkbox in configure menu of spectacle to toggle show/hide notification on copying to clipboard) after this revision

In that case it would be best to create a new Diff entirely. Also, please don't add a checkbox to Spectacle. As I wrote above, the official way to configure this is via the existing button on the notification (which opens the respective KCM).

I assume you mean adding spectacle event source in kcmnotify and adding option to toggle notification on/off for copying to clipboard? I'll look into it

rkflx added a comment.Mar 31 2018, 8:55 AM

I assume you mean adding spectacle event source in kcmnotify

There is already a New Screenshot Saved event, you'd only need to add an additional event for New Screenshot Copied to Clipboard.

adding option to toggle notification on/off for copying to clipboard

I don't think you'll need to add an option. The notification should default to "Show a message in a popup". Users who want to change settings can just click on the Configure icon in the notification, which will open the KCM.

What's the status of this patch?

rkflx added a comment.May 1 2018, 1:02 PM

What's the status of this patch?

It's on my to-review list (but in general I find some of the code still a bit odd, so sorry it's not done yet).

Also, it would be good to add the notification.

rkflx resigned from this revision.Aug 25 2018, 6:42 AM

Sorry to say, but in light of recent events I lost all motivation to further contribute to Plasma. Someone else should take over.

What is the status of this patch? @rkflx is not around any more, but maybe someone else can take this review?

ngraham added a subscriber: davidre.Mar 5 2019, 3:12 PM

This extends the old .khotkeys approach; maybe @davidre wants to tackle this once the port to KGlobalAccel is finished?

Yes I think this would be a good and practical feature to have.