Simplify copying to clipboard removing one method call
Needs ReviewPublic

Authored by aprcela on Aug 28 2019, 9:54 PM.

Details

Reviewers
davidre
Summary

Simplify copying to clipboard removing one method call
removed toImage

Test Plan

Copy to clipboard works the same (both via GUI & CLI)

Diff Detail

Repository
R166 Spectacle
Branch
B411263
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15802
Build 15820: arc lint + arc unit
aprcela created this revision.Aug 28 2019, 9:54 PM
Restricted Application added a project: Spectacle. · View Herald TranscriptAug 28 2019, 9:54 PM
Restricted Application added a subscriber: Spectacle. · View Herald Transcript
aprcela requested review of this revision.Aug 28 2019, 9:54 PM
aprcela updated this revision to Diff 64899.Aug 28 2019, 9:55 PM

Remove include of QMimeData

aprcela edited the test plan for this revision. (Show Details)

Apparently, this is slower than setImage() because:

Note that this is slower than setImage() because it needs to convert the QPixmap to a QImage first.

https://doc.qt.io/qt-5/qclipboard.html#setPixmap

Since we are doing the conversion via data->setImageData(mSavePixmap.toImage()); I assume that we end up with the same speed.
What's a good way to test the speed difference?

davidre requested changes to this revision.Aug 29 2019, 6:30 AM

We need the x-kde-force-image-copy hint for Klipper.

This revision now requires changes to proceed.Aug 29 2019, 6:30 AM
broulik added a subscriber: broulik.EditedAug 29 2019, 6:38 AM

setPixmap does exactly the same thing, from Qt's source code:

void QClipboard::setPixmap(const QPixmap &pixmap, Mode mode)
{
    QMimeData *data = new QMimeData;
    data->setImageData(pixmap);
    setMimeData(data, mode);
}

and you lost the x-kde-force-image-copy hint, so I don't think this patch is neccessary.

What you can try is just not converting toImage() first because it seems Qt can do that internally/implicitly.

From qmimedata.cpp

// images and pixmaps are interchangeable
if ((type == QVariant::Pixmap && data.type() == QVariant::Image)
    || (type == QVariant::Image && data.type() == QVariant::Pixmap))
    return data;
aprcela updated this revision to Diff 64921.Aug 29 2019, 8:22 AM

Revert back to QMimeData. Removed toImage() sine QT converts internally

setPixmap does exactly the same thing, from Qt's source code:

void QClipboard::setPixmap(const QPixmap &pixmap, Mode mode)
{
    QMimeData *data = new QMimeData;
    data->setImageData(pixmap);
    setMimeData(data, mode);
}

and you lost the x-kde-force-image-copy hint, so I don't think this patch is neccessary.

What you can try is just not converting toImage() first because it seems Qt can do that internally/implicitly.

From qmimedata.cpp

// images and pixmaps are interchangeable
if ((type == QVariant::Pixmap && data.type() == QVariant::Image)
    || (type == QVariant::Image && data.type() == QVariant::Pixmap))
    return data;

Reverted back.
And removed toImage() - works without it. Thanks broulik!

toImage() from qtpixmap.cpp:

QImage QPixmap::toImage() const
{
    if (isNull())
        return QImage();

    return data->toImage();
}

Not much computing power saved. Worth it?

aprcela retitled this revision from Simplify copying to clipboard by using setPixmap() to Simplify copying to clipboard removing one method call.Aug 29 2019, 8:38 AM
aprcela edited the summary of this revision. (Show Details)