Merge lp:~mzanetti/unity8/fix-focus-on-app-launch into lp:unity8
| 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 |
| Related bugs: |
| 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:
|
|||
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
| 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
FAILED: Continuous integration, rev:2914
https:/
Executed test runs:
SUCCESS: https:/
UNSTABLE: https:/
UNSTABLE: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/