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
davidedmundson |
Plasma: Workspaces |
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
Automatic diff as part of commit; lint not applicable. |
Automatic diff as part of commit; unit tests not applicable. |
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
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.
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.
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 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!
klipper/klipper.cpp | ||
---|---|---|
744 | bIgnoreImages || !data-> ... |
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. |
klipper/klipper.cpp | ||
---|---|---|
744 | We want:
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; |