D22684: [Klipper] Fix clipboard history management
ngraham added a comment. Thanks for the fix! REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22684 To: pdabrowski, #plasma, #plasma_workspaces, ngraham, davidedmundson Cc: GB_2, davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D22684: [Klipper] Fix clipboard history management
This revision was automatically updated to reflect the committed changes. Closed by commit R120:423308f854c4: [Klipper] Fix clipboard history management (authored by pdabrowski, committed by ngraham). REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22684?vs=69856=71695 REVISION DETAIL https://phabricator.kde.org/D22684 AFFECTED FILES applets/clipboard/contents/ui/ClipboardPage.qml applets/clipboard/contents/ui/ImageItemDelegate.qml klipper/autotests/CMakeLists.txt klipper/historyimageitem.cpp klipper/historyimageitem.h klipper/historyitem.cpp klipper/historyitem.h klipper/historymodel.cpp klipper/historymodel.h klipper/klipper.cpp klipper/klipper.h To: pdabrowski, #plasma, #plasma_workspaces, ngraham, davidedmundson Cc: GB_2, davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D22684: [Klipper] Fix clipboard history management
ngraham added a comment. David acked this in person at the meeting. Landing it. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22684 To: pdabrowski, #plasma, #plasma_workspaces, ngraham, davidedmundson Cc: GB_2, davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D22684: [Klipper] Fix clipboard history management
GB_2 added a comment. Ping. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22684 To: pdabrowski, #plasma, #plasma_workspaces, ngraham, davidedmundson Cc: GB_2, davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D22684: [Klipper] Fix clipboard history management
pdabrowski added a comment. 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... REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22684 To: pdabrowski, #plasma, #plasma_workspaces, ngraham, davidedmundson Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D22684: [Klipper] Fix clipboard history management
pdabrowski added inline comments. INLINE COMMENTS > davidedmundson wrote in historyimageitem.cpp:45 > what's this square about? > > Also if "bpp" is bits per pixel, we need to put it in i18n() for translation The square indicates that the clipboard contains an image, not a literal string like "1920x1080 32bpp". > i18n() Done. > ngraham wrote in historyimageitem.cpp:65 > Does this have to be a pixmap? Can it just be a QIcon instead? This is overridden from HistoryItem parent class. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22684 To: pdabrowski, #plasma, #plasma_workspaces, ngraham, davidedmundson Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D22684: [Klipper] Fix clipboard history management
ngraham accepted this revision. ngraham added a comment. This revision is now accepted and ready to land. @davidedmundson, still good to go? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22684 To: pdabrowski, #plasma, #plasma_workspaces, ngraham, davidedmundson Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D22684: [Klipper] Fix clipboard history management
pdabrowski updated this revision to Diff 69856. pdabrowski marked 3 inline comments as done. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22684?vs=69775=69856 REVISION DETAIL https://phabricator.kde.org/D22684 AFFECTED FILES applets/clipboard/contents/ui/ClipboardPage.qml applets/clipboard/contents/ui/ImageItemDelegate.qml klipper/autotests/CMakeLists.txt klipper/historyimageitem.cpp klipper/historyimageitem.h klipper/historyitem.cpp klipper/historyitem.h klipper/historymodel.cpp klipper/historymodel.h klipper/klipper.cpp klipper/klipper.h To: pdabrowski, #plasma, #plasma_workspaces, ngraham, davidedmundson Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D22684: [Klipper] Fix clipboard history management
davidedmundson added inline comments.
INLINE COMMENTS
> historyimageitem.cpp:45
> if ( m_text.isNull() ) {
> -m_text = QStringLiteral( "%1x%2x%3 %4" )
> +m_text = QStringLiteral( "▨ %1x%2 %3bpp" )
> .arg( m_data.width() )
what's this square about?
Also if "bpp" is bits per pixel, we need to put it in i18n() for translation
> historymodel.h:70
> int m_maxSize;
> +bool m_displayImages;
> QMutex m_mutex;
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.
REPOSITORY
R120 Plasma Workspace
REVISION DETAIL
https://phabricator.kde.org/D22684
To: pdabrowski, #plasma, #plasma_workspaces, ngraham, davidedmundson
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas,
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D22684: [Klipper] Fix clipboard history management
ngraham added inline comments.
INLINE COMMENTS
> historyimageitem.cpp:65
>
> +const QPixmap& HistoryImageItem::image() const {
> +if (m_model->displayImages()) {
Does this have to be a pixmap? Can it just be a QIcon instead?
> historyimageitem.cpp:70
> +static QPixmap imageIcon(
> +QIcon::fromTheme(QStringLiteral("viewimage")).pixmap(QSize(48, 48))
> +);
use the `view-preview` icon instead
REPOSITORY
R120 Plasma Workspace
REVISION DETAIL
https://phabricator.kde.org/D22684
To: pdabrowski, #plasma, #plasma_workspaces, ngraham, davidedmundson
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas,
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D22684: [Klipper] Fix clipboard history management
pdabrowski added a comment. Rebased. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22684 To: pdabrowski, #plasma, #plasma_workspaces, ngraham, davidedmundson Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D22684: [Klipper] Fix clipboard history management
pdabrowski updated this revision to Diff 69775. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22684?vs=63147=69775 REVISION DETAIL https://phabricator.kde.org/D22684 AFFECTED FILES applets/clipboard/contents/ui/ClipboardPage.qml applets/clipboard/contents/ui/ImageItemDelegate.qml klipper/historyimageitem.cpp klipper/historyimageitem.h klipper/historyitem.cpp klipper/historyitem.h klipper/historymodel.h klipper/klipper.cpp klipper/klipper.h To: pdabrowski, #plasma, #plasma_workspaces, ngraham, davidedmundson Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D22684: [Klipper] Fix clipboard history management
ngraham added a comment. @pdabrowski can you rebase this? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22684 To: pdabrowski, #plasma, #plasma_workspaces, ngraham, davidedmundson Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D22684: [Klipper] Fix clipboard history management
ngraham added a comment. @davidedmundson are you still good with this? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22684 To: pdabrowski, #plasma, #plasma_workspaces, ngraham, davidedmundson Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D22684: [Klipper] Fix clipboard history management
pdabrowski added a comment. Bump. This privacy issue (#409366) is still unresolved in latest Plasma. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22684 To: pdabrowski, #plasma, #plasma_workspaces, ngraham, davidedmundson Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D22684: [Klipper] Fix clipboard history management
pdabrowski added a comment. New, much better solution. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22684 To: pdabrowski, #plasma, #plasma_workspaces, ngraham, davidedmundson Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D22684: [Klipper] Fix clipboard history management
pdabrowski updated this revision to Diff 63147. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22684?vs=63145=63147 REVISION DETAIL https://phabricator.kde.org/D22684 AFFECTED FILES applets/clipboard/contents/ui/ClipboardPage.qml applets/clipboard/contents/ui/ImageItemDelegate.qml klipper/historyimageitem.cpp klipper/historyimageitem.h klipper/historyitem.cpp klipper/historyitem.h klipper/historymodel.h klipper/klipper.cpp klipper/klipper.h To: pdabrowski, #plasma, #plasma_workspaces, ngraham, davidedmundson Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D22684: [Klipper] Fix clipboard history management
pdabrowski updated this revision to Diff 63145. pdabrowski edited the summary of this revision. pdabrowski edited the test plan for this revision. pdabrowski added a comment. New, much better solution. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22684?vs=62444=63145 REVISION DETAIL https://phabricator.kde.org/D22684 AFFECTED FILES applets/clipboard/contents/ui/ImageItemDelegate.qml klipper/historyimageitem.cpp klipper/historyimageitem.h klipper/historyitem.cpp klipper/historyitem.h klipper/historymodel.h klipper/klipper.cpp klipper/klipper.h To: pdabrowski, #plasma, #plasma_workspaces, ngraham, davidedmundson Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D22684: [Klipper] Fix clipboard history management
pdabrowski updated this revision to Diff 62444. pdabrowski retitled this revision from "[Klipper] Fix clipboard history clearing" to "[Klipper] Fix clipboard history management". pdabrowski edited the summary of this revision. pdabrowski added a comment. This revision is now accepted and ready to land. new diff: proper fix for clipboard history management REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22684?vs=62410=62444 REVISION DETAIL https://phabricator.kde.org/D22684 AFFECTED FILES klipper/klipper.cpp klipper/klipper.h klipper/tray.cpp klipper/tray.h To: pdabrowski, #plasma, #plasma_workspaces, ngraham, davidedmundson Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
