Merge lp:~dandrader/unity-api/lifecycle into lp:unity-api

[Daniel d'Andrada]

Needs to land along with:
https://code.launchpad.net/~dandrader/qtmir/lifecycle/+merge/230092
https://code.launchpad.net/~dandrader/unity8/lifecycle/+merge/230090

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:154
http://jenkins.qa.ubuntu.com/job/unity-api-ci/251/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-api-utopic-amd64-ci/86/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-api-utopic-armhf-ci/86/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-api-utopic-i386-ci/86/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-api-ci/251/rebuild

review: Needs Fixing (continuous-integration)

[Gerry Boland (gerboland)]

I dislike this API as it only makes sense when an Application having 1 surface, which is not going to be true in future.

Why can't we just have the screenshot being obtained by an image provider? Why would Application save its screenshot internally at all? It was useful before QtComp, as it was a cacheing system that made sense at the time, but now I don't see the benefit at all.

Aside: in adding new methods, you need to add them to this QML test: test/qmltest/unity/shell/application/tst_Application.qml

review: Needs Fixing

[Michael Zanetti (mzanetti)]

> I dislike this API as it only makes sense when an Application having 1
> surface, which is not going to be true in future.

+1

>
> Why can't we just have the screenshot being obtained by an image provider? Why
> would Application save its screenshot internally at all? It was useful before
> QtComp, as it was a cacheing system that made sense at the time, but now I
> don't see the benefit at all.

Well, we need to update it at some point. Where would you save the screenshot otherwise? IMO the Application object is the right place to do so. Actually IMO it should be completely transparent to QML... The MirSurfaceItem should either paint the surface of the screenshot. Not requiring QML to do anything to switch between the two...

>
> Aside: in adding new methods, you need to add them to this QML test:
> test/qmltest/unity/shell/application/tst_Application.qml

+1

[Michał Sawicz (saviq)]

> > I dislike this API as it only makes sense when an Application having 1
> > surface, which is not going to be true in future.
>
> +1

With more than one surface, we'll need to save surface state and screenshot for all the surfaces I'd think.

> > Why can't we just have the screenshot being obtained by an image provider?
> Why
> > would Application save its screenshot internally at all? It was useful
> before
> > QtComp, as it was a cacheing system that made sense at the time, but now I
> > don't see the benefit at all.
>
> Well, we need to update it at some point. Where would you save the screenshot
> otherwise? IMO the Application object is the right place to do so. Actually
> IMO it should be completely transparent to QML... The MirSurfaceItem should
> either paint the surface of the screenshot. Not requiring QML to do anything
> to switch between the two...

Agreed, the screenshot(s) should be taken on suspension without the shell ever really knowing, so no public API needed ideally?

[Daniel d'Andrada (dandrader)]

> > Why can't we just have the screenshot being obtained by an image provider?
> Why
> > would Application save its screenshot internally at all? It was useful
> before
> > QtComp, as it was a cacheing system that made sense at the time, but now I
> > don't see the benefit at all.
>
> Well, we need to update it at some point. Where would you save the screenshot
> otherwise? IMO the Application object is the right place to do so. Actually
> IMO it should be completely transparent to QML... The MirSurfaceItem should
> either paint the surface of the screenshot. Not requiring QML to do anything
> to switch between the two...

You have to animate (e.g crossfade) between the surface and the screenshot, and that's done in QML.
Unless you want to move that QML code from unity8 to qtmir, which I don't think it makes sense as unity8 is the guy that takes care of such UI stuff.

[Daniel d'Andrada (dandrader)]

> Why can't we just have the screenshot being obtained by an image provider? Why
> would Application save its screenshot internally at all? It was useful before
> QtComp, as it was a cacheing system that made sense at the time, but now I
> don't see the benefit at all.

I don't get your comment. That what is happening already. The image provider is just an API between C++ and QML so that QML can use an URL to refer to an QImage held in the C++ side.

[Michael Zanetti (mzanetti)]

I'd still say we shouldn't need those update methods... The screenshot should be automatically updated before the app is put into suspend and discarded when we have a surface again.

Also do we really need a crossfading between screenshot and surface? Ideally the image shouldn't change when switching from surface to screenshot. The other way round I guess the app just initializes from start again. Not sure we need a crossfade even there as inside apps the screens usually also don't use a crossfade when changing.

review: Needs Information

[Daniel d'Andrada (dandrader)]

> I'd still say we shouldn't need those update methods... The screenshot should
> be automatically updated before the app is put into suspend and discarded when
> we have a surface again.

Talked to Gerry. There's another way of doing it that doesn't require updateScreenshot() and discardScreenshot() API but still lets unity8 decide when to get it and when to drop it. Working on it now.

> Also do we really need a crossfading between screenshot and surface? Ideally
> the image shouldn't change when switching from surface to screenshot. The
> other way round I guess the app just initializes from start again.

We need to crossfade as there's no guarantee that the screenshot we have will match the contents of the app surface once it's restarted. So we crossfade to mitigate any sudden, dramatic, change.

> Not sure we need a crossfade even there as inside apps the screens usually also don't use
> a crossfade when changing.

Didn't get your comment. But if what you mean is that, for example, some application doesn't animate when switching between tabs or something, then it's the apps problem, not ours. Ie, if some app is doing a bad job it doesn't mean that we should follow suite.

[Michael Zanetti (mzanetti)]

> > I'd still say we shouldn't need those update methods... The screenshot
> should
> > be automatically updated before the app is put into suspend and discarded
> when
> > we have a surface again.
>
> Talked to Gerry. There's another way of doing it that doesn't require
> updateScreenshot() and discardScreenshot() API but still lets unity8 decide
> when to get it and when to drop it. Working on it now.

But why do you want the shell to be in charge of that? It's not that the shell is the OOM killer. The killing happens behind the scenes, why not just make things work behind the scenes then?

>
> > Also do we really need a crossfading between screenshot and surface? Ideally
> > the image shouldn't change when switching from surface to screenshot. The
> > other way round I guess the app just initializes from start again.
>
> We need to crossfade as there's no guarantee that the screenshot we have will
> match the contents of the app surface once it's restarted. So we crossfade to
> mitigate any sudden, dramatic, change.

Again. We're using sudden, dramatic, life-threatening changes everywhere in the apps. From splash to app, when pushing a page to the PageStack inside the app. Doing a cross fade here would be inconsistent and tell the user that something is different, while the whole point of this is to make the app killing and restarting invisible to the user. Did design request a crossfade here? If not I don't think there should be one.

>
> > Not sure we need a crossfade even there as inside apps the screens usually
> also don't use
> > a crossfade when changing.
>
> Didn't get your comment. But if what you mean is that, for example, some
> application doesn't animate when switching between tabs or something, then
> it's the apps problem, not ours. Ie, if some app is doing a bad job it doesn't
> mean that we should follow suite.

My point is that we should do what the app does as we want to trick the user into thinking he is in the app. Trying to be more clever than the app itself here only makes the cheating more visible.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:155
http://jenkins.qa.ubuntu.com/job/unity-api-ci/252/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-api-utopic-amd64-ci/87/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-api-utopic-armhf-ci/87/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-api-utopic-i386-ci/87/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-api-ci/252/rebuild

review: Needs Fixing (continuous-integration)

[Michael Zanetti (mzanetti)]

> > Didn't get your comment. But if what you mean is that, for example, some
> > application doesn't animate when switching between tabs or something, then
> > it's the apps problem, not ours. Ie, if some app is doing a bad job it
> doesn't
> > mean that we should follow suite.

Also, its not that *some* app doesn't animate, its that all of the apps don't do so because the SDK doesn't do it.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:156
http://jenkins.qa.ubuntu.com/job/unity-api-ci/254/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-api-utopic-amd64-ci/89/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-api-utopic-armhf-ci/89/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-api-utopic-i386-ci/89/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-api-ci/254/rebuild

review: Needs Fixing (continuous-integration)

[Daniel d'Andrada (dandrader)]

Updated.

[Gerry Boland (gerboland)]

review: Approve

[Michael Zanetti (mzanetti)]

there are test failures

review: Needs Fixing

[Daniel d'Andrada (dandrader)]

> there are test failures

Fixed. Should have paid attention to Jenkins posts here :/

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:157
http://jenkins.qa.ubuntu.com/job/unity-api-ci/255/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-utopic-amd64-ci/90
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-utopic-armhf-ci/90
        deb: http://jenkins.qa.ubuntu.com/job/unity-api-utopic-armhf-ci/90/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-utopic-i386-ci/90

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-api-ci/255/rebuild

review: Approve (continuous-integration)

[Michael Zanetti (mzanetti)]

ack

review: Approve

[Gerry Boland (gerboland)]

Commit message & debian changelog entry do not match what this MR does.

review: Needs Fixing

[Michał Sawicz (saviq)]

FWIW if there's a changelog entry, no need for commit message.

[Michał Sawicz (saviq)]

review: Needs Fixing

[lp:~dandrader/unity-api/lifecycle]

158. By Daniel d'Andrada on 2014-08-21

Don't touch this stuff, whatever it's used for

[lp:~dandrader/unity-api/lifecycle]

159. By Daniel d'Andrada on 2014-08-21

Updated debian/changelog

[Daniel d'Andrada (dandrader)]

> Commit message & debian changelog entry do not match what this MR does.

Fixed.

[Michał Sawicz (saviq)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:159
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~dandrader/unity-api/lifecycle/+merge/230091/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/unity-api-ci/260/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-utopic-amd64-ci/95
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-utopic-armhf-ci/95
        deb: http://jenkins.qa.ubuntu.com/job/unity-api-utopic-armhf-ci/95/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-utopic-i386-ci/95

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-api-ci/260/rebuild

review: Needs Fixing (continuous-integration)

=== modified file 'debian/changelog'
--- debian/changelog	2014-08-06 19:32:56 +0000
+++ debian/changelog	2014-08-21 14:35:01 +0000
@@ -1,3 +1,9 @@
+unity-api (7.89) UNRELEASED; urgency=medium
+
+  * Remove all mentions to screenshotting from the API
+
+ -- Daniel d'Andrada <[e-mail]>  Fri, 21 Aug 2014 11:33:00 -0300
+
 unity-api (7.88+14.10.20140806-0ubuntu1) utopic; urgency=medium
 
   [ Michal Hruby ]

=== modified file 'include/unity/shell/application/ApplicationInfoInterface.h'
--- include/unity/shell/application/ApplicationInfoInterface.h	2013-12-11 11:04:11 +0000
+++ include/unity/shell/application/ApplicationInfoInterface.h	2014-08-21 14:35:01 +0000
@@ -97,13 +97,6 @@
      */
     Q_PROPERTY(bool focused READ focused NOTIFY focusedChanged)
 
-    /**
-     * @brief The URL of the app's screenshot to be used with the image provider.
-     *
-     * Holds the URL for the app's screenshot. This URL will change whenever the screenshot updates.
-     */
-    Q_PROPERTY(QUrl screenshot READ screenshot NOTIFY screenshotChanged)
-
 protected:
     /// @cond
     ApplicationInfoInterface(const QString &appId, QObject* parent = 0): QObject(parent) { Q_UNUSED(appId) }
@@ -152,7 +145,6 @@
     virtual Stage stage() const = 0;
     virtual State state() const = 0;
     virtual bool focused() const = 0;
-    virtual QUrl screenshot() const = 0;
     /// @endcond
 
 Q_SIGNALS:
@@ -163,7 +155,6 @@
     void stageChanged(Stage stage);
     void stateChanged(State state);
     void focusedChanged(bool focused);
-    void screenshotChanged(const QUrl &screenshot);
     /// @endcond
 };
 

=== modified file 'include/unity/shell/application/ApplicationManagerInterface.h'
--- include/unity/shell/application/ApplicationManagerInterface.h	2014-03-10 12:40:25 +0000
+++ include/unity/shell/application/ApplicationManagerInterface.h	2014-08-21 14:35:01 +0000
@@ -77,7 +77,6 @@
         m_roleNames.insert(RoleStage, "stage");
         m_roleNames.insert(RoleState, "state");
         m_roleNames.insert(RoleFocused, "focused");
-        m_roleNames.insert(RoleScreenshot, "screenshot");
 
         connect(this, SIGNAL(rowsInserted(QModelIndex, int, int)), SIGNAL(countChanged()));
         connect(this, SIGNAL(rowsRemoved(QModelIndex, int, int)), SIGNAL(countChanged()));
@@ -100,7 +99,6 @@
         RoleStage,
         RoleState,
         RoleFocused,
-        RoleScreenshot,
     };
 
     /// @cond
@@ -185,18 +183,6 @@
       */
     Q_INVOKABLE virtual bool stopApplication(const QString &appId) = 0;
 
-    /**
-     * @brief Update the screenshot for an application.
-     *
-     * NOTE: Normally the ApplicationManager will update screenshots unfocusing or focusing apps,
-     * However, in cases where you need to show the screenshot while the application is still focused,
-     * you can request the ApplicationManager to update it now.
-     *
-     * @param appId The application for which the screenshot should be updated.
-     * @returns True if the screenshot update operation was scheduled successfully, false otherwise (i.e. the given appId could not be found)
-     */
-    Q_INVOKABLE virtual bool updateScreenshot(const QString &appId) = 0;
-
 Q_SIGNALS:
     /// @cond
     void countChanged();

=== modified file 'include/unity/shell/application/CMakeLists.txt'
--- include/unity/shell/application/CMakeLists.txt	2014-03-04 10:50:44 +0000
+++ include/unity/shell/application/CMakeLists.txt	2014-08-21 14:35:01 +0000
@@ -7,8 +7,8 @@
 
 set(UNITY_API_LIB_HDRS ${UNITY_API_LIB_HDRS} ${headers} ${internal_headers} PARENT_SCOPE)
 
-set(VERSION 2)
-set(PKGCONFIG_NAME "unity-shell-appliction")
+set(VERSION 3)
+set(PKGCONFIG_NAME "unity-shell-application")
 set(PKGCONFIG_DESCRIPTION "Unity shell Application APIs")
 set(PKGCONFIG_REQUIRES "Qt5Core")
 set(PKGCONFIG_FILE unity-shell-application.pc)

=== modified file 'test/qmltest/mocks/plugins/Unity/Application/Mocks/MockApplicationInfo.cpp'
--- test/qmltest/mocks/plugins/Unity/Application/Mocks/MockApplicationInfo.cpp	2013-12-11 11:28:22 +0000
+++ test/qmltest/mocks/plugins/Unity/Application/Mocks/MockApplicationInfo.cpp	2014-08-21 14:35:01 +0000
@@ -54,11 +54,6 @@
     return m_icon;
 }
 
-QUrl MockApplicationInfo::screenshot() const
-{
-    return m_screenshot;
-}
-
 ApplicationInfoInterface::Stage MockApplicationInfo::stage() const
 {
     return m_stage;

=== modified file 'test/qmltest/mocks/plugins/Unity/Application/Mocks/MockApplicationInfo.h'
--- test/qmltest/mocks/plugins/Unity/Application/Mocks/MockApplicationInfo.h	2013-12-11 11:28:22 +0000
+++ test/qmltest/mocks/plugins/Unity/Application/Mocks/MockApplicationInfo.h	2014-08-21 14:35:01 +0000
@@ -36,7 +36,6 @@
     QString name() const;
     QString comment() const;
     QUrl icon() const;
-    QUrl screenshot() const;
 
     ApplicationInfoInterface::Stage stage() const;
     void setStage(ApplicationInfoInterface::Stage stage);
@@ -52,7 +51,6 @@
     QString m_name;
     QString m_comment;
     QUrl m_icon;
-    QUrl m_screenshot;
     ApplicationInfoInterface::Stage m_stage;
     ApplicationInfoInterface::State m_state;
     bool m_focused;

=== modified file 'test/qmltest/mocks/plugins/Unity/Application/Mocks/MockApplicationManager.cpp'
--- test/qmltest/mocks/plugins/Unity/Application/Mocks/MockApplicationManager.cpp	2014-05-23 08:35:43 +0000
+++ test/qmltest/mocks/plugins/Unity/Application/Mocks/MockApplicationManager.cpp	2014-08-21 14:35:01 +0000
@@ -66,8 +66,6 @@
         return item->state();
     case RoleFocused:
         return item->focused();
-    case RoleScreenshot:
-        return item->screenshot();
     }
 
     return QVariant();
@@ -138,9 +136,3 @@
     Q_UNUSED(appId)
     return true;
 }
-
-bool MockApplicationManager::updateScreenshot(const QString &appId)
-{
-    Q_UNUSED(appId)
-    return true;
-}

=== modified file 'test/qmltest/mocks/plugins/Unity/Application/Mocks/MockApplicationManager.h'
--- test/qmltest/mocks/plugins/Unity/Application/Mocks/MockApplicationManager.h	2014-03-10 12:34:16 +0000
+++ test/qmltest/mocks/plugins/Unity/Application/Mocks/MockApplicationManager.h	2014-08-21 14:35:01 +0000
@@ -57,8 +57,6 @@
 
     Q_INVOKABLE bool stopApplication(const QString &appId);
 
-    Q_INVOKABLE bool updateScreenshot(const QString &appId);
-
 private:
     QList<MockApplicationInfo*> m_list;
 };

=== modified file 'test/qmltest/unity/shell/application/tst_Application.qml'
--- test/qmltest/unity/shell/application/tst_Application.qml	2014-03-10 12:34:16 +0000
+++ test/qmltest/unity/shell/application/tst_Application.qml	2014-08-21 14:35:01 +0000
@@ -69,7 +69,6 @@
                         { enum: "RoleStage" },
                         { enum: "RoleState" },
                         { enum: "RoleFocused" },
-                        { enum: "RoleScreenshot" },
                     ];
         }
 
@@ -89,7 +88,6 @@
                         { tag: "ApplicationManager.roles[stage]", role: "stage", type: "number" },
                         { tag: "ApplicationManager.roles[state]", role: "state", type: "number" },
                         { tag: "ApplicationManager.roles[focused]", role: "focused", type: "boolean" },
-                        { tag: "ApplicationManager.roles[screenshot]", role: "screenshot", type: "object" },
                     ];
         }
 
@@ -113,7 +111,6 @@
                 { tag: "ApplicationManager.methods[unfocusCurrentApplication]", method: "unfocusCurrentApplication" },
                 { tag: "ApplicationManager.methods[startApplication]", method: "startApplication" },
                 { tag: "ApplicationManager.methods[stopApplication]", method: "stopApplication" },
-                { tag: "ApplicationManager.methods[updateScreenshot]", method: "updateScreenshot" },
             ];
         }
 
@@ -146,7 +143,6 @@
                 { tag: "ApplicationInfo.properties[stage]", property: "stage", type: "number" },
                 { tag: "ApplicationInfo.properties[state]", property: "state", type: "number" },
                 { tag: "ApplicationInfo.properties[focused]", property: "focused", type: "boolean" },
-                { tag: "ApplicationInfo.properties[screenshot]", property: "screenshot", type: "object" },
             ];
         }