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

Proposed by Michael Zanetti on 2017-04-04
Status: Needs review
Proposed branch: lp:~mzanetti/unity8/fix-focus-on-app-launch
Merge into: lp:unity8
Diff against target: 56 lines (+16/-1)
3 files modified
qml/Launcher/Launcher.qml (+1/-1)
qml/Shell.qml (+1/-0)
tests/qmltests/tst_Shell.qml (+14/-0)
To merge this branch: bzr merge lp:~mzanetti/unity8/fix-focus-on-app-launch
Reviewer Review Type Date Requested Status
Unity8 CI Bot continuous-integration Needs Fixing on 2017-04-04
Unity Team 2017-04-04 Pending
Review via email: mp+321868@code.launchpad.net

Commit message

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

Description of the change

 * 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

To post a comment you must log in.
Unity8 CI Bot (unity8-ci-bot) wrote :

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) 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?

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

"""
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) 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?
>
> --------------------------------
>
> """
> 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) 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.

Daniel d'Andrada (dandrader) 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 }

Michael Zanetti (mzanetti) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'qml/Launcher/Launcher.qml'
2--- qml/Launcher/Launcher.qml 2017-03-21 10:54:59 +0000
3+++ qml/Launcher/Launcher.qml 2017-04-04 14:29:06 +0000
4@@ -128,6 +128,7 @@
5 } else {
6 switchToNextState("")
7 }
8+ root.focus = false;
9 }
10
11 function fadeOut() {
12@@ -229,7 +230,6 @@
13 root.hide();
14 panel.highlightIndex = -2
15 event.accepted = true;
16- root.focus = false;
17 }
18 }
19
20
21=== modified file 'qml/Shell.qml'
22--- qml/Shell.qml 2017-03-24 14:04:50 +0000
23+++ qml/Shell.qml 2017-04-04 14:29:06 +0000
24@@ -174,6 +174,7 @@
25 } else {
26 startApp(appId);
27 }
28+ stage.focus = true;
29 }
30
31 function activateURL(url) {
32
33=== modified file 'tests/qmltests/tst_Shell.qml'
34--- tests/qmltests/tst_Shell.qml 2017-03-24 11:08:11 +0000
35+++ tests/qmltests/tst_Shell.qml 2017-04-04 14:29:06 +0000
36@@ -3170,6 +3170,20 @@
37 tryCompare(menuBarLoader.item, "visible", true);
38 }
39
40+ function test_launchFromDrawerPutsFocusOnStage() {
41+ loadShell("desktop");
42+ shell.usageScenario = "desktop";
43+ waitForRendering(shell);
44+ swipeAwayGreeter();
45+
46+ keyClick(Qt.Key_A, Qt.MetaModifier);
47+ typeString("browser");
48+ keyClick(Qt.Key_Enter);
49+
50+ var appRepeater = findChild(shell, "appRepeater");
51+ tryCompare(appRepeater.itemAt(0), "activeFocus", true);
52+ }
53+
54 function test_maximizedWindowAndMenuInPanel() {
55 loadShell("desktop");
56 shell.usageScenario = "desktop";

Subscribers

People subscribed via source and target branches