Merge lp:~mterry/unity-mir/no-focus into lp:unity-mir

[Michael Terry]

If no app has focus when we get a un-actionable onSessionFocused event, still re-set focus to the shell.

If no app has focus when we get a un-actionable onSessionFocused event, still re-set focus to the shell.

During testing of split greeter, Saviq noticed that the volume up/down buttons didn't work. Some digging discovered that the android input layer thought nothing was focused and that it only happens after maliit-server is running.

Digging into unity-mir, it seems we discard onSessionFocused events that don't relate to an application (like maliit-server was generating). But apparently, android-input is a little confused at that point, and there is code in the function for re-setting the focus for the focused app.

But if no app has focus, we did nothing. In the case where no app has focus, we should also re-set the focus onto the shell. This fixed the volume bug for me.

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

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Michał Sawicz (saviq)]

Could we have a test for this?

review: Needs Information

[Gerry Boland (gerboland)]

+1 on the test request

[lp:~mterry/unity-mir/no-focus]

225. By Michael Terry on 2014-05-23

Add tests

[Michael Terry (mterry)]

Tests added.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

 DLOG("Invalid application focused, discarding the event");
not correct any more, please update comment

Please add the checklist too
https://wiki.ubuntu.com/Process/Merges/Checklists/Unity-Mir

review: Needs Fixing

[Gerry Boland (gerboland)]

"Oo" <- as before please just make empty string

[Gerry Boland (gerboland)]

Testing this, everything seems ok

[lp:~mterry/unity-mir/no-focus]

226. By Michael Terry on 2014-05-26

simplify test

[Michael Terry (mterry)]

> DLOG("Invalid application focused, discarding the event");
> not correct any more, please update comment

I disagree. It means discarding the request to focus the new app. The code that follows is just rolling back any side effects of handling the request. I only added a special case to that code anyway, rather than change its meaning.

I could change the comment to "refocusing previous focus" or something like that. But it sounds like a wordier version of the "not doing anything" meaning we already have.

> Please add the checklist too
> https://wiki.ubuntu.com/Process/Merges/Checklists/Unity-Mir

Done.

> "Oo" <- as before please just make empty string

Done.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

> > DLOG("Invalid application focused, discarding the event");
> > not correct any more, please update comment
>
> I disagree. It means discarding the request to focus the new app.

Ok. Approved!

review: Approve

[Gerry Boland (gerboland)]

 * 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

=== 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:30:05 +0000
@@ -862,6 +862,8 @@
             DLOG("Invalid application focused, discarding the event");
             if (nullptr != m_focusedApplication)
                 focusApplication(m_focusedApplication->appId());
+            else
+                m_focusController->set_focus_to(nullptr);
         }
     }
 }

=== 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:30:05 +0000
@@ -142,6 +142,36 @@
     }
 }
 
+TEST_F(ApplicationManagerTests, focus_on_invalid_app_falls_back_to_focused_app)
+{
+    using namespace ::testing;
+    quint64 a_procId = 5921;
+    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));
+
+    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);
+
+    EXPECT_CALL(focusController, set_focus_to(first_session)).Times(1);
+
+    applicationManager.onSessionFocused(nullptr);
+}
+
+TEST_F(ApplicationManagerTests, focus_on_invalid_app_falls_back_to_default_session)
+{
+    using namespace ::testing;
+
+    std::shared_ptr<mir::scene::Session> null_session = nullptr;
+    EXPECT_CALL(focusController, set_focus_to(null_session)).Times(1);
+
+    applicationManager.onSessionFocused(nullptr);
+}
 
 TEST_F(ApplicationManagerTests,bug_case_1240400_second_dialer_app_fails_to_authorize_and_gets_mixed_up_with_first_one)
 {