QtMir

Merge lp:~lukas-kde/qtmir/fix-app-request-focus-lp1672337 into lp:qtmir

fix-app-request-focus-lp1672337
Merge into trunk

Proposed by Lukáš Tinkl on 2017-03-22

Status:

Merged

Approved by:

Daniel d'Andrada on 2017-03-27

Approved revision:

624

Merged at revision:

626

Proposed branch:

lp:~lukas-kde/qtmir/fix-app-request-focus-lp1672337

Merge into:

lp:qtmir

Diff against target:

22 lines (+9/-3)

1 file modified

src/modules/Unity/Application/application.cpp (+9/-3)

To merge this branch:

bzr merge lp:~lukas-kde/qtmir/fix-app-request-focus-lp1672337

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Daniel d'Andrada (community)		2017-03-22	Approve on 2017-03-27
Unity8 CI Bot (community)	continuous-integration		Approve on 2017-03-25
Review via email: mp+320626@code.launchpad.net

Commit message

Raise (activate) windows when an app focus is requested

Description of the change

Raise (activate) windows when an app focus is requested

Fixes lp:1672337

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-24:

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

review: Approve (continuous-integration)

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-03-24:

1- you should call surface->requestFocus(), not surface->active(). Shell is the one who ultimately decides to activate a surface. And that also allows shell to do any needed scene preparation before calling surface->activate()

2- Surface list doesn't contain null surfaces. So there's no reason to check for null.

3- Call the temp surface something more accurate, like surface, instead of "iface"

4 - No need for this code anymore:

"""
auto surface = static_cast<MirSurfaceInterface*>(m_proxySurfaceList->get(0));
surface->requestFocus();
"""

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

Having said that, this looks like something that miral should be doing. When you minimize the top-level, the child should *at least* loose focus. But I don't see that happening in the logs.

review: Needs Fixing

lp:~lukas-kde/qtmir/fix-app-request-focus-lp1672337 updated on 2017-03-24

623. By Lukáš Tinkl on 2017-03-24: address review comments

Revision history for this message

Lukáš Tinkl (lukas-kde) wrote on 2017-03-24:

> 1- you should call surface->requestFocus(), not surface->active(). Shell is
> the one who ultimately decides to activate a surface. And that also allows
> shell to do any needed scene preparation before calling surface->activate()
>
> 2- Surface list doesn't contain null surfaces. So there's no reason to check
> for null.
>
> 3- Call the temp surface something more accurate, like surface, instead of
> "iface"
>
> 4 - No need for this code anymore:
>
> """
> auto surface = static_cast<MirSurfaceInterface*>(m_proxySurfaceList->get(0));
> surface->requestFocus();
> """

Done

>
> Having said that, this looks like something that miral should be doing. When
> you minimize the top-level, the child should *at least* loose focus. But I
> don't see that happening in the logs.

Yeah, Gerry had an idea of a more future-proof solution (for later), most likely involving miral

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-24:

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

review: Approve (continuous-integration)

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-03-24:

"""
INFO_MSG << "() - Requesting focus for toplevel app surface";
"""

s/toplevel/most recent top-level

"""
// raise the whole tree beneath
"""

Don't think that comment applies.

lp:~lukas-kde/qtmir/fix-app-request-focus-lp1672337 updated on 2017-03-24

624. By Lukáš Tinkl on 2017-03-24: fix comments/debug

Revision history for this message

Lukáš Tinkl (lukas-kde) wrote on 2017-03-24:

> """
> INFO_MSG << "() - Requesting focus for toplevel app surface";
> """
>
> s/toplevel/most recent top-level
>
>
> """
> // raise the whole tree beneath
> """
>
> Don't think that comment applies.

Done

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-24:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-25:

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

review: Approve (continuous-integration)

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-03-27:

Not sure we would need to do that if miral behaved properly in the use case mentioned in the bug but I guess this change won't hurt either.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Daniel d'Andrada

Lukáš Tinkl

Mir development team

 === modified file 'src/modules/Unity/Application/application.cpp'
 --- src/modules/Unity/Application/application.cpp	2017-02-07 20:59:34 +0000
 +++ src/modules/Unity/Application/application.cpp	2017-03-24 21:21:21 +0000
@@ -869,9 +869,15 @@
  void Application::requestFocus()
+ {
      if (m_proxySurfaceList->rowCount() > 0) {
--        INFO_MSG << "() - Requesting focus for most recent app surface";
--        auto surface = static_cast<MirSurfaceInterface*>(m_proxySurfaceList->get(0));
--        surface->requestFocus();
++        INFO_MSG << "() - Requesting focus for most recent toplevel app surface";
++
++        for (int i = 0; i < m_proxySurfaceList->count(); ++i) {
++            auto surface = static_cast<MirSurfaceInterface*>(m_proxySurfaceList->get(i));
++            if (!surface->parentSurface()) {
++                surface->requestFocus();
++                break;
++            }
++        }
      } else {
          INFO_MSG << "() - emitting focusRequested()";
          Q_EMIT focusRequested();