Merge lp:~mzanetti/unity8/fix-focus-on-app-launch into lp:unity8

[Michael Zanetti]

properly put focus back on stage after launching something from the drawer

 * 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 UI, has there been a design review?
n/a

[Unity8 CI Bot (unity8-ci-bot)]

FAILED: Continuous integration, rev:2914
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/3666/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4867
    UNSTABLE: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=xenial+overlay,testname=qmluitests.sh/2982
    UNSTABLE: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=zesty,testname=qmluitests.sh/2982
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4895
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4706
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4706/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4706
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4706/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4706
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4706/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4706
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4706/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4706
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4706/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4706
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4706/artifact/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

[Daniel d'Andrada (dandrader)]

"""
root.focus = false;
"""

This looks redundant. I think code should be caring about whom to give focus to, not in explicitly unfocusing the item that's losing it as this is the side-effect of focusing someone else anyway. Is that really needed?

--------------------------------

"""
stage.focus = true;
"""

In some other focus-related branch you have the Stage focusing itself on its own accord. Would be good to consolidate that responsibility in Shell if feasible to avoid clashes. Ideally I think Shell logic should be the one deciding which Shell immediate child will get focus at any given time.

[Michael Zanetti (mzanetti)]

> """
> root.focus = false;
> """
>
> This looks redundant. I think code should be caring about whom to give focus
> to, not in explicitly unfocusing the item that's losing it as this is the
> side-effect of focusing someone else anyway. Is that really needed?
>
> --------------------------------
>
> """
> stage.focus = true;
> """
>
> In some other focus-related branch you have the Stage focusing itself on its
> own accord. Would be good to consolidate that responsibility in Shell if
> feasible to avoid clashes. Ideally I think Shell logic should be the one
> deciding which Shell immediate child will get focus at any given time.

yeah, you're certainly having a point there... the current situation isn't nice... however, it's not that simple... sometimes some things need to be focused by a GlobalShortcut which is buried somewhere inside components, other times something deep down in a component trigger a focus change into another component... I'd love to rework this somehow and am thinking how to best solve it, but that's gonna be a bigger thing...

[Michael Zanetti (mzanetti)]

> """
> root.focus = false;
> """
>
> This looks redundant. I think code should be caring about whom to give focus
> to, not in explicitly unfocusing the item that's losing it as this is the
> side-effect of focusing someone else anyway. Is that really needed?

yes... I just moved it a bit from where it was before... currently the launcher works in a way that when it is closed it unfocuses itself and in shell we have:

Launcher { onFocusChanged: if (!focus) stage.focus = true }

so that's kinda ok for here... I do certainly agree that in general this needs to be consolidated in a way eventually.

[Daniel d'Andrada (dandrader)]

On 04/04/2017 13:37, Michael Zanetti wrote:
>> """
>> root.focus = false;
>> """
>>
>> This looks redundant. I think code should be caring about whom to give focus
>> to, not in explicitly unfocusing the item that's losing it as this is the
>> side-effect of focusing someone else anyway. Is that really needed?
> yes... I just moved it a bit from where it was before... currently the launcher works in a way that when it is closed it unfocuses itself and in shell we have:
>
> Launcher { onFocusChanged: if (!focus) stage.focus = true }
>
> so that's kinda ok for here... I do certainly agree that in general this needs to be consolidated in a way eventually.

Maybe something like this would be a step in the right direction:

Launcher { onClosed: stage.focus = true }

[Michael Zanetti (mzanetti)]

On 04.04.2017 18:42, Daniel d'Andrada wrote:
> On 04/04/2017 13:37, Michael Zanetti wrote:
>>> """
>>> root.focus = false;
>>> """
>>>
>>> This looks redundant. I think code should be caring about whom to give focus
>>> to, not in explicitly unfocusing the item that's losing it as this is the
>>> side-effect of focusing someone else anyway. Is that really needed?
>> yes... I just moved it a bit from where it was before... currently the launcher works in a way that when it is closed it unfocuses itself and in shell we have:
>>
>> Launcher { onFocusChanged: if (!focus) stage.focus = true }
>>
>> so that's kinda ok for here... I do certainly agree that in general this needs to be consolidated in a way eventually.
>
> Maybe something like this would be a step in the right direction:
>
> Launcher { onClosed: stage.focus = true }
>
>

It doesn't necessarily only happen when closed... e.g. with alt+F1 you
focus it for keyboard navigation... when then you press esc the launcher
unfocuses itself and focus should go back to the stage even if the
launcher might not close because the setting says it should stay opened.

Unmerged revisions

2914. By Michael Zanetti on 2017-04-04

properly put focus back on stage after launching something from the drawer

=== modified file 'qml/Launcher/Launcher.qml'
--- qml/Launcher/Launcher.qml	2017-03-21 10:54:59 +0000
+++ qml/Launcher/Launcher.qml	2017-04-04 14:29:06 +0000
@@ -128,6 +128,7 @@
         } else {
             switchToNextState("")
         }
+        root.focus = false;
     }
 
     function fadeOut() {
@@ -229,7 +230,6 @@
             root.hide();
             panel.highlightIndex = -2
             event.accepted = true;
-            root.focus = false;
         }
     }
 
 
=== modified file 'qml/Shell.qml'
--- qml/Shell.qml	2017-03-24 14:04:50 +0000
+++ qml/Shell.qml	2017-04-04 14:29:06 +0000
@@ -174,6 +174,7 @@
         } else {
             startApp(appId);
         }
+        stage.focus = true;
     }
 
     function activateURL(url) {
 
=== modified file 'tests/qmltests/tst_Shell.qml'
--- tests/qmltests/tst_Shell.qml	2017-03-24 11:08:11 +0000
+++ tests/qmltests/tst_Shell.qml	2017-04-04 14:29:06 +0000
@@ -3170,6 +3170,20 @@
             tryCompare(menuBarLoader.item, "visible", true);
         }
 
+        function test_launchFromDrawerPutsFocusOnStage() {
+            loadShell("desktop");
+            shell.usageScenario = "desktop";
+            waitForRendering(shell);
+            swipeAwayGreeter();
+
+            keyClick(Qt.Key_A, Qt.MetaModifier);
+            typeString("browser");
+            keyClick(Qt.Key_Enter);
+
+            var appRepeater = findChild(shell, "appRepeater");
+            tryCompare(appRepeater.itemAt(0), "activeFocus", true);
+        }
+
         function test_maximizedWindowAndMenuInPanel() {
             loadShell("desktop");
             shell.usageScenario = "desktop";