[Klipper] Fix clipboard history management
ClosedPublic

Authored by pdabrowski on Jul 23 2019, 2:01 PM.

Details

Summary

Fixed clearing the clipboard when clearing history
(bug #409366 caused by 3bd6ac34ed74e3b86bfb6b29818b726baf505f20)

Properly synchronize clipboard content and history
(deleting last item in history didn't clear the clipboard).

Keep last image in history even if we are ignoring images, but without preview
(so that we can still clear history, and the tooltip says truth about clipboard content).

Fixed tray icon tooltip text.

Do not scale small images up in clipboard Plasmoid.

BUG: 409366

Test Plan

Spectacle -> [Copy to Clipboard]
Paste into Dolphin (clipboard content)

Deleting single items from clipboard

Clearing the entire history

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
pdabrowski created this revision.Jul 23 2019, 2:01 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJul 23 2019, 2:01 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
pdabrowski requested review of this revision.Jul 23 2019, 2:01 PM
pdabrowski updated this revision to Diff 62398.Jul 23 2019, 2:16 PM
pdabrowski updated this revision to Diff 62410.Jul 23 2019, 3:04 PM

new diff: much better patch

ngraham retitled this revision from [Plasma Workspace][Klipper] Fixed clipboard history clearing (#409366) to [Plasma Workspace][Klipper] Fixed clipboard history clearing.Jul 23 2019, 5:26 PM
ngraham edited the summary of this revision. (Show Details)

Thanks for the patch!

A more descriptive Description section would be appreciated: you could explain what the issue is and how specifically this patch fixes it. In general that improves the likelihood of quick review. :)

ngraham retitled this revision from [Plasma Workspace][Klipper] Fixed clipboard history clearing to [Klipper] Fix clipboard history clearing.Jul 23 2019, 5:29 PM
ngraham edited the summary of this revision. (Show Details)
pdabrowski edited the summary of this revision. (Show Details)Jul 23 2019, 5:58 PM
davidedmundson accepted this revision.Jul 23 2019, 6:07 PM
This revision is now accepted and ready to land.Jul 23 2019, 6:07 PM
pdabrowski planned changes to this revision.Jul 23 2019, 6:14 PM

Weak pointer might not work well for this after all.
I need to have a second look at this patch.

pdabrowski retitled this revision from [Klipper] Fix clipboard history clearing to [Klipper] Fix clipboard history management.
pdabrowski edited the summary of this revision. (Show Details)

new diff: proper fix for clipboard history management

This revision is now accepted and ready to land.Jul 23 2019, 10:45 PM
pdabrowski requested review of this revision.Jul 23 2019, 10:46 PM
pdabrowski updated this revision to Diff 63145.Aug 5 2019, 8:19 PM
pdabrowski edited the summary of this revision. (Show Details)
pdabrowski edited the test plan for this revision. (Show Details)
This comment was removed by pdabrowski.
pdabrowski edited the summary of this revision. (Show Details)Aug 5 2019, 8:23 PM
pdabrowski updated this revision to Diff 63147.Aug 5 2019, 9:01 PM

New, much better solution.

pdabrowski edited the summary of this revision. (Show Details)Aug 5 2019, 9:22 PM
pdabrowski edited the summary of this revision. (Show Details)Aug 5 2019, 10:14 PM
pdabrowski edited the summary of this revision. (Show Details)

Bump.
This privacy issue (#409366) is still unresolved in latest Plasma.

@davidedmundson are you still good with this?

@pdabrowski can you rebase this?

ngraham added inline comments.Nov 15 2019, 2:54 PM
klipper/historyimageitem.cpp
65

Does this have to be a pixmap? Can it just be a QIcon instead?

70

use the view-preview icon instead

davidedmundson added inline comments.Nov 15 2019, 3:13 PM
klipper/historyimageitem.cpp
45

what's this square about?

Also if "bpp" is bits per pixel, we need to put it in i18n() for translation

klipper/historymodel.h
70

It's best to initialise this.

I know setDisplayImages will always be called by the app, but it makes the class more re-usable and robust.

pdabrowski updated this revision to Diff 69856.Nov 16 2019, 7:28 PM
pdabrowski marked 3 inline comments as done.
ngraham accepted this revision.Nov 16 2019, 11:31 PM

@davidedmundson, still good to go?

This revision is now accepted and ready to land.Nov 16 2019, 11:31 PM
pdabrowski added inline comments.Nov 17 2019, 8:03 AM
klipper/historyimageitem.cpp
45

The square indicates that the clipboard contains an image, not a literal string like "1920x1080 32bpp".

i18n()

Done.

65

This is overridden from HistoryItem parent class.

Is this OK now?

I think the privacy issue caused by that reported regression (#409366) should really be fixed, one way or another. Most people are probably totally unaware their clipboard still contains the last content after they command it to be cleared...

David acked this in person at the meeting. Landing it.

This revision was automatically updated to reflect the committed changes.

Thanks for the fix!