Merge lp:~mterry/unity-mir/dont-hide-focused-apps into lp:unity-mir

[Michael Terry]

Separate suspend logic from visibility logic, to allow top app to remain visible even when suspended.

Separate suspend logic from visibility logic, to allow top app to remain visible even when suspended.

When testing the split greeter, Saviq noticed that the user session would go black if an app was open while the greeter was up (sliding the greeter lets you peek at user session).

This is because unity-mir hides() app sessions when they are suspended to optimized compositing. But the top app should remain visible for cross-session compositing like the greeter wants to do.

So don't automatically hide() when suspending, but instead only hide() if we are unfocusing an app to bring another app above it.

(We still automatically show() when resuming because we want to ensure that the app is visible now.)

== Checklist ==
 * 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

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

[lp:~mterry/unity-mir/dont-hide-focused-apps]

222. By Michael Terry on 2014-05-19

Separate suspend logic from visibility logic, to allow top app to remain visible even when suspended.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:222
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/344/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-amd64-ci/8
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-armhf-ci/8
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-armhf-ci/8/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-i386-ci/8

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

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

It reads strange that you call setVisible(true) in Application, but setVisible(false) in ApplicationManager. Could you make this more consistent please?

I'd appreciate you adding a test or two, to verify nothing breaks.

review: Needs Fixing

[lp:~mterry/unity-mir/dont-hide-focused-apps]

223. By Michael Terry on 2014-05-22

Move setVisible call to application manager

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:223
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/351/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-amd64-ci/15
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-armhf-ci/15
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-armhf-ci/15/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-i386-ci/15

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

review: Approve (continuous-integration)

[lp:~mterry/unity-mir/dont-hide-focused-apps]

224. By Michael Terry on 2014-05-23

Add some tests

[Michael Terry (mterry)]

OK, setVisible calls moved out, and a couple tests added.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:224
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/352/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-amd64-ci/16
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-armhf-ci/16
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-armhf-ci/16/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-i386-ci/16

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

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

+ std::shared_ptr<mir::scene::Session> first_session = std::make_shared<MockSession>("Oo", a_procId);
The "Oo" not needed, jut an empty string will do

+ EXPECT_EQ(true, the_app->visible());
Could you please switch the arguments around?

Am testing now

[lp:~mterry/unity-mir/dont-hide-focused-apps]

225. By Michael Terry on 2014-05-26

Simplify test

[Michael Terry (mterry)]

> The "Oo" not needed, jut an empty string will do

Ah, OK. Fixed. Just copy and paste junk.

> Could you please switch the arguments around?

Really? Looking at the other tests, it seems common to have a "static value" on left and the interesting call on right.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:225
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/360/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-amd64-ci/24
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-armhf-ci/24
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-armhf-ci/24/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-i386-ci/24

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

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

> > Could you please switch the arguments around?
>
> Really? Looking at the other tests, it seems common to have a "static value"
> on left and the interesting call on right.
Yeah sorry I know, I want to standardize things to the "value to check" and "correct value" order - see the newer tests I added recently.

[lp:~mterry/unity-mir/dont-hide-focused-apps]

226. By Michael Terry on 2014-05-26

Swap test value placement

[Michael Terry (mterry)]

Done.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:226
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/363/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-amd64-ci/27
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-armhf-ci/27
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-armhf-ci/27/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-utopic-i386-ci/27

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

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

Thank you. Approved!

 * Did you perform an exploratory manual test run of the code change and any related functionality?
Y
 * Did CI run pass? If not, please explain why.
Y

review: Approve

=== modified file 'src/modules/Unity/Application/application.cpp'
--- src/modules/Unity/Application/application.cpp	2014-05-19 12:36:27 +0000
+++ src/modules/Unity/Application/application.cpp	2014-05-26 12:21:08 +0000
@@ -223,9 +223,6 @@
         case Application::Suspended:
             if (m_state == Application::Running) {
                 session()->set_lifecycle_state(mir_lifecycle_state_will_suspend);
-
-                session()->hide();
-
                 m_suspendTimer->start(3000);
             }
             break;
@@ -236,7 +233,6 @@
             if (m_state == Application::Suspended) {
                 resume();
                 session()->set_lifecycle_state(mir_lifecycle_state_resumed);
-                session()->show();
             } else if (m_state == Application::Stopped) {
                 respawn();
                 state = Application::Starting;

=== modified file 'src/modules/Unity/Application/application_manager.cpp'
--- src/modules/Unity/Application/application_manager.cpp	2014-05-19 15:30:15 +0000
+++ src/modules/Unity/Application/application_manager.cpp	2014-05-26 12:21:08 +0000
@@ -345,20 +345,22 @@
     }
 }
 
-void ApplicationManager::suspendApplication(Application *application)
+bool ApplicationManager::suspendApplication(Application *application)
 {
     if (application == nullptr)
-        return;
+        return false;
 
     updateScreenshot(application->appId());
 
     DLOG("ApplicationManager::suspend(this=%p, application(%p)->appId(%s) )",this, application, qPrintable(application->appId()));
     // Present in exceptions list, return.
     if (!m_lifecycleExceptions.filter(application->appId().section('_',0,0)).empty())
-        return;
+        return false;
 
     if (application->state() == Application::Running)
         application->setState(Application::Suspended);
+
+    return true;
 }
 
 void ApplicationManager::resumeApplication(Application *application)
@@ -366,8 +368,10 @@
     if (application == nullptr)
         return;
 
-    if (application->state() != Application::Running)
+    if (application->state() != Application::Running) {
+        application->setVisible(true);
         application->setState(Application::Running);
+    }
 }
 
 bool ApplicationManager::focusApplication(const QString &inputAppId)
@@ -644,6 +648,7 @@
     // If app Stopped, trust that upstart-app-launch respawns it itself, and AppManager will
     // be notified of that through the onProcessStarting slot. Else resume.
     if (application->state() == Application::Suspended) {
+        application->setVisible(true);
         application->setState(Application::Running);
     }
 }
@@ -910,7 +915,8 @@
     // set state of previously focused app to suspended
     if (m_focusedApplication && m_lifecycleExceptions.filter(m_focusedApplication->appId().section('_',0,0)).empty()) {
         Application *lastApplication = applicationForStage(application->stage());
-        suspendApplication(lastApplication);
+        if (suspendApplication(lastApplication))
+            lastApplication->setVisible(false);
     }
 
     if (application->stage() == Application::MainStage)
@@ -920,6 +926,7 @@
 
     m_focusedApplication = application;
     m_focusedApplication->setFocused(true);
+    m_focusedApplication->setVisible(true);
     m_focusedApplication->setState(Application::Running);
     move(m_applications.indexOf(application), 0);
     Q_EMIT focusedApplicationIdChanged();

=== modified file 'src/modules/Unity/Application/application_manager.h'
--- src/modules/Unity/Application/application_manager.h	2014-05-19 15:30:15 +0000
+++ src/modules/Unity/Application/application_manager.h	2014-05-26 12:21:08 +0000
@@ -109,7 +109,7 @@
     unitymir::Application* findApplicationWithPid(const qint64 pid);
 
     // Internal helpers
-    void suspendApplication(Application *application);
+    bool suspendApplication(Application *application);
     void resumeApplication(Application *application);
     int panelHeight();
     QSize displaySize() const { return m_displaySize; }

=== modified file 'tests/application_manager_test.cpp'
--- tests/application_manager_test.cpp	2014-05-20 10:21:20 +0000
+++ tests/application_manager_test.cpp	2014-05-26 12:21:08 +0000
@@ -474,6 +474,81 @@
 
 }
 
+TEST_F(ApplicationManagerTests,suspend_does_not_hide)
+{
+    using namespace ::testing;
+    quint64 a_procId = 5921;
+    const char an_app_id[] = "some_app";
+    QByteArray a_cmd("/usr/bin/app1 --desktop_file_hint=some_app");
+    std::shared_ptr<mir::scene::Surface> aSurface(nullptr);
+
+    ON_CALL(procInfo,command_line(_)).WillByDefault(Return(a_cmd));
+
+    ON_CALL(appController,appIdHasProcessId(_,_)).WillByDefault(Return(false));
+
+    bool authed = true;
+
+    std::shared_ptr<mir::scene::Session> first_session = std::make_shared<MockSession>("", a_procId);
+    applicationManager.authorizeSession(a_procId, authed);
+
+    applicationManager.onSessionStarting(first_session);
+    applicationManager.onSessionCreatedSurface(first_session.get(), aSurface);
+
+    Application * the_app = applicationManager.findApplication(an_app_id);
+
+    EXPECT_EQ(the_app->visible(), true);
+    EXPECT_EQ(the_app->state(), Application::Running);
+
+    applicationManager.setSuspended(true);
+
+    EXPECT_EQ(the_app->visible(), true);
+    EXPECT_EQ(the_app->state(), Application::Suspended);
+}
+
+TEST_F(ApplicationManagerTests,focus_hides_old_app)
+{
+    using namespace ::testing;
+    quint64 first_procId = 5921;
+    quint64 second_procId = 5922;
+    const char first_appId[] = "app1";
+    const char second_appId[] = "app2";
+    std::shared_ptr<mir::scene::Surface> aSurface(nullptr);
+    QByteArray first_cmdLine("/usr/bin/app1 --desktop_file_hint=app1");
+    QByteArray second_cmdLine("/usr/bin/app2--desktop_file_hint=app2");
+
+    EXPECT_CALL(procInfo,command_line(first_procId))
+        .Times(1)
+        .WillOnce(Return(first_cmdLine));
+
+    ON_CALL(appController,appIdHasProcessId(_,_)).WillByDefault(Return(false));
+
+    EXPECT_CALL(procInfo,command_line(second_procId))
+        .Times(1)
+        .WillOnce(Return(second_cmdLine));
+
+    bool authed = true;
+
+    std::shared_ptr<mir::scene::Session> first_session = std::make_shared<MockSession>("", first_procId);
+    std::shared_ptr<mir::scene::Session> second_session = std::make_shared<MockSession>("", second_procId);
+    applicationManager.authorizeSession(first_procId, authed);
+    applicationManager.authorizeSession(second_procId, authed);
+    applicationManager.onSessionStarting(first_session);
+    applicationManager.onSessionStarting(second_session);
+
+    Application * first_app = applicationManager.findApplication(first_appId);
+    Application * second_app = applicationManager.findApplication(second_appId);
+
+    // Confirm that both start visible, but also mark first_app as the
+    // active app by creating a surface for its session.
+    applicationManager.onSessionCreatedSurface(first_session.get(), aSurface);
+    EXPECT_EQ(first_app->visible(), true);
+    EXPECT_EQ(second_app->visible(), true);
+
+    applicationManager.onSessionCreatedSurface(second_session.get(), aSurface);
+    EXPECT_EQ(first_app->visible(), false);
+    EXPECT_EQ(second_app->visible(), true);
+}
+
 TEST_F(ApplicationManagerTests,requestFocusApplication)
 {
     using namespace ::testing;