Change default Klipper behavior to accept images
ClosedPublic

Authored by kdautovic on Mar 18 2019, 1:07 PM.

Details

Summary

Change the default Klipper behavior in order for https://phabricator.kde.org/D19715 to work.

Klipper will save images to clipboard, but unless Ignore Images is unchecked in the options, it won't save them to clipboard history.

CCBUG: 393708

Diff Detail

Repository
R120 Plasma Workspace
Lint
Lint Skipped
Unit
Unit Tests Skipped
kdautovic created this revision.Mar 18 2019, 1:07 PM
Restricted Application edited projects, added Plasma; removed Plasma: Workspaces. · View Herald TranscriptMar 18 2019, 1:07 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
kdautovic requested review of this revision.Mar 18 2019, 1:07 PM
davidedmundson requested changes to this revision.Mar 18 2019, 1:33 PM
davidedmundson added a subscriber: davidedmundson.

You're just going to burn through memory like crazy. We can't do this.
It also doesn't properly fix the klipper issue as it then breaks for anyone who has changed the setting.

There is a proposal by me last time this came up:
https://phabricator.kde.org/D9117#202448

This revision now requires changes to proceed.Mar 18 2019, 1:33 PM

The problem I see with that is that memory usage wasn't solved. Regardless of whether we use my approach or your approach, they're going to use the same amount of memory if people copy screenshots to clipboard. Your approach is just going to give the data a different MIME type.

This way, all image handling applications get the same behavior.

With all of this in mind, perhaps the best solution would be to never save images in history, so that once something else is copied, the image is forgotten.

It means only spectacle fills memory, not doing work in gimp.

The issue is that spectacle closes, but we need klipper to copy the image in advance as it doesn't know that spectacle is about to close. It's unique to this situation.

Your approach is just going to give the data a different MIME type.

Additional mime hint, not a different mime type.

But you're right my idea could be expanded upon to only keep the image when it's the top of the stack, that would work nicely.

kdautovic updated this revision to Diff 54222.EditedMar 18 2019, 2:17 PM

Added a special case for not saving history when the data is an image and m_bIgnoreImages is set. This allows copying images to clipboard, and keeping the current default behavior of Klipper to not save images in history when m_bIgnoreImages is set.

This also allows to enable saving images to history by simply changing the Klipper settings.

kdautovic edited the summary of this revision. (Show Details)Mar 18 2019, 2:31 PM
davidedmundson added a comment.EditedMar 18 2019, 3:02 PM

We don't use goto.

Whilst I think doing this makes sense, it's not the complete solution.

We won't get this far as we filter out the images on line 753, before this code.

This comment was removed by kdautovic.
kdautovic updated this revision to Diff 54237.Mar 18 2019, 3:44 PM

Remove the image filter which stopped copying images to clipboard.

kdautovic updated this revision to Diff 54239.Mar 18 2019, 3:48 PM

Removed the use of goto.

Is this good enough to merge?

This is definitely better. The part with the history saving is neat and that part is all good to go.

But it still does a huge image copy to plasma every time you copy and paste an image around in gimp. It's very expensive, both in memory and the blocking X calls for something not needed as you're not closing the app.

I would still like to have the mime type hint solution on line 743.

@kdautovic are you planning on working on this some more? Looks like we're really close!

Yeah, I'll add the mime hint.

kdautovic updated this revision to Diff 54692.Mar 24 2019, 5:20 PM

Add the "x-kde-force-image-copy" mime type hint.

kdautovic updated this revision to Diff 54694.Mar 24 2019, 5:21 PM

Formatting.

davidedmundson added inline comments.Mar 24 2019, 5:38 PM
klipper/klipper.cpp
744
bIgnoreImages || !data-> ...
kdautovic added inline comments.Mar 24 2019, 5:51 PM
klipper/klipper.cpp
744

But bIgnoreImages is used later in the code, when determining whether to save images to history or not. Is it really necessary here? If it's added here then we're back to square one because copying screenshots to clipboard won't work with the default behavior of Klipper.

kdautovic updated this revision to Diff 54711.Mar 24 2019, 6:52 PM

Change QString to QStringLiteral

kdautovic updated this revision to Diff 54717.Mar 24 2019, 7:23 PM

Use arcanist to get context

Any updates? This is buildable and works. Is it acceptable to merge?

davidedmundson added inline comments.Mar 26 2019, 5:16 PM
klipper/klipper.cpp
744

We want:

  1. if ignore images is false and it's from spectacle - we load the image in the cache and the history
  2. if ignore images is false and it's not from spectacle - we load the image in the cache and the history
  3. if ignoreImages is true and it's from spectacle, we load the image, but don't keep in history
  4. if ignoreImages is true and it's not from spectacle we throw it away.

I think this patch is going to break case 2 as we now return early.

But you're right that my comment was wrong, I meant.

if (m_ignoreImage && !data->
    return;
kdautovic updated this revision to Diff 54871.Mar 26 2019, 5:45 PM

Add m_bIgnoreImage to get the correct behavior

This revision is now accepted and ready to land.Mar 26 2019, 5:51 PM
ngraham edited the summary of this revision. (Show Details)Mar 26 2019, 6:07 PM
This revision was automatically updated to reflect the committed changes.