Merge lp:~nick-dedekind/qtubuntu/1493530-mimeData into lp:qtubuntu

[Nick Dedekind]

Added check for valid mime data.

Added check for valid mime data.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:281
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/246/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/37
        deb: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/37/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/246/rebuild

review: Approve (continuous-integration)

[Albert Astals Cid (aacid)]

Would make sense to move the null check inside serialzeMimeData in case it's called from somewhere else in the future?

review: Needs Information

[Daniel d'Andrada (dandrader)]

> Would make sense to move the null check inside serialzeMimeData in case it's
> called from somewhere else in the future?

I think a Q_ASSERT() at most as serialzeMimeData() is an internal (private) function.

[Daniel d'Andrada (dandrader)]

When it gets a null mimeData it should also clear the global (D-Bus) clipboard.

review: Needs Fixing

[Nick Dedekind (nick-dedekind)]

> When it gets a null mimeData it should also clear the global (D-Bus)
> clipboard.

I'm not so sure we should be clearing it if it's null. If a client tries to copy an item which isn't able to set mime data, we shouldn't clear the last item in the clipboard.

I believe the last valid clipboard item should persist until another valid item is set. We don't ever "empty" the clipboard as I understand. For this reason we should actually also put the change inside the if block as well.

[lp:~nick-dedekind/qtubuntu/1493530-mimeData]

282. By Nick Dedekind on 2015-11-09

merged with trunk

283. By Nick Dedekind on 2015-11-09

moved changed signal to if block

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:283
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/271/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/62
        deb: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/62/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/271/rebuild

review: Approve (continuous-integration)

[Daniel d'Andrada (dandrader)]

> > When it gets a null mimeData it should also clear the global (D-Bus)
> > clipboard.
>
> I'm not so sure we should be clearing it if it's null. If a client tries to
> copy an item which isn't able to set mime data, we shouldn't clear the last
> item in the clipboard.
>
> I believe the last valid clipboard item should persist until another valid
> item is set. We don't ever "empty" the clipboard as I understand. For this
> reason we should actually also put the change inside the if block as well.

Ok

review: Approve

=== modified file 'src/ubuntumirclient/clipboard.cpp'
--- src/ubuntumirclient/clipboard.cpp	2015-08-20 15:37:29 +0000
+++ src/ubuntumirclient/clipboard.cpp	2015-11-09 10:12:48 +0000
@@ -141,6 +141,8 @@
 
 QByteArray UbuntuClipboard::serializeMimeData(QMimeData *mimeData) const
 {
+    Q_ASSERT(mimeData != nullptr);
+
     const QStringList formats = mimeData->formats();
     const int formatCount = qMin(formats.size(), maxFormatsCount);
     const int headerSize = sizeof(int) + (formatCount * 4 * sizeof(int));
@@ -244,13 +246,15 @@
         delete mPendingGetContentsCall.data();
     }
 
-    QByteArray serializedMimeData = serializeMimeData(mimeData);
-    if (!serializedMimeData.isEmpty()) {
-        setDBusClipboardContents(serializedMimeData);
+    if (mimeData != nullptr) {
+        QByteArray serializedMimeData = serializeMimeData(mimeData);
+        if (!serializedMimeData.isEmpty()) {
+            setDBusClipboardContents(serializedMimeData);
+        }
+
+        mMimeData = mimeData;
+        emitChanged(QClipboard::Clipboard);
     }
-
-    mMimeData = mimeData;
-    emitChanged(QClipboard::Clipboard);
 }
 
 bool UbuntuClipboard::supportsMode(QClipboard::Mode mode) const