Merge lp://qastaging/~mzanetti/unity-mir/screenshotting-focusing-api into lp://qastaging/unity-mir

FAILED: Continuous integration, rev:179
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/275/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/138/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/139/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/138/console

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

=== removed file 'src/modules/Unity/Application/ApplicationImage.qml'
This will break SurfaceFlinger support. We good with that?

> === removed file 'src/modules/Unity/Application/ApplicationImage.qml'
> This will break SurfaceFlinger support. We good with that?

I asked around about that in January and answer from Kevin was yes. I'll get a confirmation that we're good to merge this now (effectively breaking SF support).

+void ApplicationManager::screenshotUpdated()
+ if (!m_nextFocusedAppId.isEmpty()) {
+ Q_EMIT focusRequested(m_nextFocusedAppId);
+ m_nextFocusedAppId.clear();
+ }
I don't follow this. Why emit a focusRequest after screenshot is updated? Is it just the best time to do it?

+ engine->addImageProvider(QLatin1String("application"), new ApplicationScreenshotProvider(appManager));

If I see a QUrl like "application://calculator-app" - is that a url for a url dispatcher? Or a url for that app's screenshot? I'm just a tad worried about duplicate purposes of urls of that form. Thought?

> > === removed file 'src/modules/Unity/Application/ApplicationImage.qml'
> > This will break SurfaceFlinger support. We good with that?
>
> I asked around about that in January and answer from Kevin was yes. I'll get a
> confirmation that we're good to merge this now (effectively breaking SF
> support).

Confirmed. Good to go.

> +void ApplicationManager::screenshotUpdated()
> + if (!m_nextFocusedAppId.isEmpty()) {
> + Q_EMIT focusRequested(m_nextFocusedAppId);
> + m_nextFocusedAppId.clear();
> + }
> I don't follow this. Why emit a focusRequest after screenshot is updated? Is
> it just the best time to do it?

In order to make the QML code less complex, when someone calls requestFocusApplication() (formerly activateApplication()), I first update the screenshot, and then emit the requestFocus() signal. So the QML code has everything in place to start animating, instead of having to do the updateScreenshot() stuff from there.

> + engine->addImageProvider(QLatin1String("application"), new
> ApplicationScreenshotProvider(appManager));
>
> If I see a QUrl like "application://calculator-app" - is that a url for a url
> dispatcher? Or a url for that app's screenshot?

that's a url-dispatcher url :)

the imageprovider's url is

image://application/calculator-app/12345678

> I'm just a tad worried about
> duplicate purposes of urls of that form. Thought?

After clearing the confusion, does it still sound too similar to you?

> After clearing the confusion, does it still sound too similar to you?
/me idiot. You're right. Plz ignore

> > +void ApplicationManager::screenshotUpdated()
> > + if (!m_nextFocusedAppId.isEmpty()) {
> > + Q_EMIT focusRequested(m_nextFocusedAppId);
> > + m_nextFocusedAppId.clear();
> > + }
> > I don't follow this. Why emit a focusRequest after screenshot is updated? Is
> > it just the best time to do it?
>
> In order to make the QML code less complex, when someone calls
> requestFocusApplication() (formerly activateApplication()), I first update the
> screenshot, and then emit the requestFocus() signal. So the QML code has
> everything in place to start animating, instead of having to do the
> updateScreenshot() stuff from there.

Okay am understanding the why. But this method is strange to read, so it needs at least a comment justifying it. Might a second slot, connected to Application::screenshotChanged make the logic a little more obvious?

Also using "sender()" to get the signal originator always feels dangerous to me, I'm not definite if the pointer returned always exists, does it?

FAILED: Continuous integration, rev:180
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/286/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/149/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/150/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/149/console

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

> Okay am understanding the why. But this method is strange to read, so it needs
> at least a comment justifying it. Might a second slot, connected to
> Application::screenshotChanged make the logic a little more obvious?
>
> Also using "sender()" to get the signal originator always feels dangerous to
> me, I'm not definite if the pointer returned always exists, does it?

I've added some comments describing why this happens here. You are right about the sender() thing. Thinking about it, it could happen that the Application is gone by the time this signal is delivered, given that the signal emission happens in a mir thread and hence the connection a queued one. - I've fixed it to make sure the we don't crash.

Ok, that'll do. I just need to test this with your unity8 work, before I can approve

FAILED: Continuous integration, rev:181
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/287/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/150/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/151/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/150/console

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

FAILED: Continuous integration, rev:182
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/291/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/154/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/155/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/154/console

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

FAILED: Continuous integration, rev:183
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/292/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/155/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/156/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/155/console

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

since I managed to break ApplicationManager easily in the past - it would be nice if you could add tests that cover suspend / resume and the changes to focusing..

FAILED: Continuous integration, rev:184
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/293/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/156/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/157/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/156/console

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

FAILED: Continuous integration, rev:185
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/295/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/158/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/159/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/158/console

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

> since I managed to break ApplicationManager easily in the past - it would be
> nice if you could add tests that cover suspend / resume and the changes to
> focusing..

Absolutely. Test for the suspended added.

Btw. I noticed that the QtTests in there are not executed and broken/outdated. I tried to add a test for the requestFocus, but as we're using google test to test a Qt api instead of QtTest here I can't use QSignalSpy to verify it emits focusRequested.

FAILED: Continuous integration, rev:186
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/297/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/160/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/161/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/160/console

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

Bump?

FAILED: Continuous integration, rev:187
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/299/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/162/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/163/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/162/console

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

turns out, QSignalSpy does work after all. No idea why I failed to get anything into it on my first attempt.

All the comments are fixed now. Good to go?

FAILED: Continuous integration, rev:188
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/300/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/163/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/164/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/163/console

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

LGTM

* Did you perform an exploratory manual test run of the code change and any related functionality?
Yes, works fine
* Did CI run pass? If not, please explain why.
MR depends on this landing first: https://code.launchpad.net/~mzanetti/unity-api/new-screenshot-and-focusing-api

+1 on the plugin installation dirs.