Merge lp:~dandrader/unity8/workaroundAnimatedSpriteBug into lp:unity8

Proposed by Daniel d'Andrada
Status: Merged
Approved by: Albert Astals Cid
Approved revision: 2444
Merged at revision: 2483
Proposed branch: lp:~dandrader/unity8/workaroundAnimatedSpriteBug
Merge into: lp:unity8
Prerequisite: lp:~dandrader/unity8/consecutiveCustomCursors
Diff against target: 60 lines (+2/-16)
2 files modified
plugins/Cursor/CursorImageInfo.cpp (+2/-12)
plugins/Cursor/CursorImageInfo.h (+0/-4)
To merge this branch: bzr merge lp:~dandrader/unity8/workaroundAnimatedSpriteBug
Reviewer Review Type Date Requested Status
Unity8 CI Bot continuous-integration Needs Fixing
Albert Astals Cid (community) Approve
Review via email: mp+296859@code.launchpad.net

Commit message

Work around AnimatedSprite infinite loop bug

https://bugreports.qt.io/browse/QTBUG-53937

Don't wait until the next event loop iteration before you update the other properties. If you do so, scenegraph thread will sync with an inconsistent state: new image URL + old cursor info. So you would risk having a frameWidth or frameHeight larger than the new image size, hitting the above mentioned bug.

The timer was there just to avoid (re)loading twice in case both
themeName and cursorName changes. But that's such a rare case that
it's not worth optimizing for it.

Description of the change

* Are there any related MPs required for this MP to build/function as expected? Please list.
No

* Did you perform an exploratory manual test run of your code change and any related functionality?
Yes.

The easiest way to test is:
$ bzr branch lp:~dandrader/+junk/animatedDemos
$ cd animatedDemos/CustomCursor
$ qmake && make
$ cd ..
$ qmlscene -I $PWD CursorShapes.qml --desktop_file_hint=SOME_EXISTING_FILE.desktop

Then move the cursor between a custom cursor slot (eg. boat or football) and a named cursor slot or the window border. With this patch, it will just work. With trunk (or with the prereq branch), it will hang.

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?

* If you changed the UI, has there been a design review?

To post a comment you must log in.
Revision history for this message
Albert Astals Cid (aacid) wrote :

 * Did you perform an exploratory manual test run of the code change and any related functionality?
Yes

 * Did CI run pass? If not, please explain why.
Waiting for CI to finish.

review: Approve
Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :

FAILED: Continuous integration, rev:2443
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/1446/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/1923
    UNSTABLE: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=vivid+overlay,testname=qmluitests.sh/985
    UNSTABLE: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=xenial+overlay,testname=qmluitests.sh/985
    UNSTABLE: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=yakkety,testname=qmluitests.sh/985
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/1949
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1885
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1885
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/1885
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/1876
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/1876/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1876
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1876/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=yakkety/1876
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=yakkety/1876/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/1876
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/1876/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1876
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1876/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=yakkety/1876
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=yakkety/1876/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/1876
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/1876/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/1876
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/1876/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=yakkety/1876
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=yakkety/1876/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/1446/rebuild

review: Needs Fixing (continuous-integration)
2444. By Daniel d'Andrada

Simplify solution

The timer was there just to avoid (re)loading twice in case both
themeName and cursorName changes. But that's such a rare case that
it's not worth optimizing for it.

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

Found a simpler solution. A fix/workaround that removes code is better than one that adds more. :)

See commit message for rationale.

Sorry for asking you to test it again.

Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :

FAILED: Continuous integration, rev:2444
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/1448/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/1926
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=vivid+overlay,testname=qmluitests.sh/987
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=xenial+overlay,testname=qmluitests.sh/987
    UNSTABLE: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=yakkety,testname=qmluitests.sh/987
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/1952
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1887
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1887
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/1887
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/1878
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/1878/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1878
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1878/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=yakkety/1878
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=yakkety/1878/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/1878
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/1878/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1878
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1878/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=yakkety/1878
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=yakkety/1878/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/1878
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/1878/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/1878
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/1878/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=yakkety/1878
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=yakkety/1878/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/1448/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Albert Astals Cid (aacid) wrote :

Nice :)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'plugins/Cursor/CursorImageInfo.cpp'
--- plugins/Cursor/CursorImageInfo.cpp 2016-05-19 19:44:37 +0000
+++ plugins/Cursor/CursorImageInfo.cpp 2016-06-09 10:54:48 +0000
@@ -19,17 +19,14 @@
19CursorImageInfo::CursorImageInfo(QObject *parent)19CursorImageInfo::CursorImageInfo(QObject *parent)
20 : QObject(parent)20 : QObject(parent)
21{21{
22 m_updateTimer.setInterval(0);
23 m_updateTimer.setSingleShot(true);
24 connect(&m_updateTimer, &QTimer::timeout, this, &CursorImageInfo::update);
25}22}
2623
27void CursorImageInfo::setCursorName(const QString &cursorName)24void CursorImageInfo::setCursorName(const QString &cursorName)
28{25{
29 if (cursorName != m_cursorName) {26 if (cursorName != m_cursorName) {
30 m_cursorName = cursorName;27 m_cursorName = cursorName;
28 update();
31 Q_EMIT cursorNameChanged();29 Q_EMIT cursorNameChanged();
32 scheduleUpdate();
33 }30 }
34}31}
3532
@@ -37,15 +34,8 @@
37{34{
38 if (m_themeName != themeName) {35 if (m_themeName != themeName) {
39 m_themeName = themeName;36 m_themeName = themeName;
37 update();
40 Q_EMIT themeNameChanged();38 Q_EMIT themeNameChanged();
41 scheduleUpdate();
42 }
43}
44
45void CursorImageInfo::scheduleUpdate()
46{
47 if (!m_updateTimer.isActive()) {
48 m_updateTimer.start();
49 }39 }
50}40}
5141
5242
=== modified file 'plugins/Cursor/CursorImageInfo.h'
--- plugins/Cursor/CursorImageInfo.h 2016-05-19 19:44:37 +0000
+++ plugins/Cursor/CursorImageInfo.h 2016-06-09 10:54:48 +0000
@@ -21,7 +21,6 @@
2121
22#include <QObject>22#include <QObject>
23#include <QString>23#include <QString>
24#include <QTimer>
2524
26class CursorImageInfo : public QObject25class CursorImageInfo : public QObject
27{26{
@@ -64,9 +63,6 @@
64 void update();63 void update();
6564
66private:65private:
67 void scheduleUpdate();
68 QTimer m_updateTimer;
69
70 QString m_themeName;66 QString m_themeName;
71 QString m_cursorName;67 QString m_cursorName;
7268

Subscribers

People subscribed via source and target branches