Mir

Merge lp:~alan-griffiths/mir/fix-1717910 into lp:mir

fix-1717910
Merge into development-branch

Proposed by Alan Griffiths on 2017-09-18

Status:

Merged

Merged at revision:

4257

Proposed branch:

lp:~alan-griffiths/mir/fix-1717910

Merge into:

lp:mir

Diff against target:

116 lines (+38/-19)

4 files modified

debian/changelog (+2/-0)
examples/miral-kiosk/kiosk_window_manager.cpp (+30/-15)
examples/miral-kiosk/kiosk_window_manager.h (+5/-3)
include/miral/miral/canonical_window_manager.h (+1/-1)

To merge this branch:

bzr merge lp:~alan-griffiths/mir/fix-1717910

Critical

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Mir CI Bot	continuous-integration		Needs Fixing on 2017-09-20
Gerry Boland		2017-09-18	Approve on 2017-09-19
Review via email: mp+330924@code.launchpad.net

Commit Message

[miral-kiosk] Apply fullscreen logic when hidden windows are restored. (LP: #1717910)

Mir CI Bot (mir-ci-bot) wrote on 2017-09-18:

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3668/rebuild

review: Needs Fixing (continuous-integration)

Gerry Boland (gerboland) wrote on 2017-09-19:

No change. Try a command like this:

QT_LOGGING_RULES=qt.qpa.*=true QT_QPA_PLATFORM=ubuntumirclient /usr/lib/x86_64-linux-gnu/qt5/examples/webkitqml/flickrview/flickrview

Qt is getting the resize event from Mir, but we've never trusted the resize event from Mir as it seldom reflected the next buffer size - so instead we forces a redraw, which pulls a new buffer from Mir, and we check that buffer size to see if it changes. We're not getting any buffer size change.

OFC adopting the new buffer semantics stuff would probably rectify that. But that's non-trivial work

review: Needs Fixing

Alan Griffiths (alan-griffiths) wrote on 2017-09-19:

> No change. Try a command like this:
>
> QT_LOGGING_RULES=qt.qpa.*=true QT_QPA_PLATFORM=ubuntumirclient /usr/lib/x86_64
> -linux-gnu/qt5/examples/webkitqml/flickrview/flickrview

Thanks for that. I had been testing with...

$ miral-app -kiosk

...and that was improved by the changes so far.

Alan Griffiths (alan-griffiths) wrote on 2017-09-19:

This seems OK for these test cases:

$ miral-app -kiosk -launcher /usr/lib/x86_64-linux-gnu/qt5/examples/webkitqml/flickrview/flickrview

$ miral-app -kiosk

Note that this breaks the following case:

$ miral-app -kiosk -launcher 'gnome-terminal --app-id com.canonical.miral.Terminal'

But I don't think we should care.

Alan Griffiths (alan-griffiths) wrote on 2017-09-19:

> Note that this breaks the following case:
>
> $ miral-app -kiosk -launcher 'gnome-terminal --app-id
> com.canonical.miral.Terminal'
>
> But I don't think we should care.

Basically, this is the code that should work but replaces incorrect code used to work around a gtk-mir bug.

As gtk-mir is /1/ unmaintained and being disable going forward and /2/ already broken in multiple ways on Xenial I don't think we should worry.

Gerry Boland (gerboland) wrote on 2017-09-19:

There is something subtly wrong still. I see a resize flicker for the fancybrowser on startup. Looking at the WM trace, there is a non-fullscreen resize request by Qt of the window while it is hidden that Mir appears to be respecting. See https://pastebin.ubuntu.com/25572675/ with annotations

Mir CI Bot (mir-ci-bot) wrote on 2017-09-19:

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3669/rebuild

review: Approve (continuous-integration)

Alan Griffiths (alan-griffiths) wrote on 2017-09-19:

> There is something subtly wrong still. I see a resize flicker for the
> fancybrowser on startup. Looking at the WM trace, there is a non-fullscreen
> resize request by Qt of the window while it is hidden that Mir appears to be
> respecting. See https://pastebin.ubuntu.com/25572675/ with annotations

I don't see that here. But you're right - it doesn't make sense to respect resizing when hidden/hiding and change it once restored. Changed accordingly.

Gerry Boland (gerboland) wrote on 2017-09-19:

Thanks, working better now.

review: Approve

Mir CI Bot (mir-ci-bot) wrote on 2017-09-19:

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3670/rebuild

review: Approve (continuous-integration)

Mir CI Bot (mir-ci-bot) wrote on 2017-09-19:

review: Needs Fixing (continuous-integration)

Mir CI Bot (mir-ci-bot) wrote on 2017-09-20:

review: Needs Fixing (continuous-integration)

Mir CI Bot (mir-ci-bot) wrote on 2017-09-20:

review: Needs Fixing (continuous-integration)

Alan Griffiths (alan-griffiths) wrote on 2017-09-20:

12:13:07 The following tests FAILED:
12:13:07 11 - mir_acceptance_tests (Failed)
12:13:07 16 - mir_integration_tests (Failed)
12:13:07 22 - mir_unit_tests (Failed)
12:13:07 31 - mir_unit_tests_nested (Failed)

https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/5071/console

10:58:41 The following tests FAILED:
10:58:41 9 - mir_acceptance_tests (Failed)
10:58:41 14 - mir_integration_tests (Failed)
10:58:41 20 - mir_unit_tests (Failed)
10:58:41 29 - mir_unit_tests_nested (Failed)

https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/5071/arch=amd64,compiler=clang,platform=mesa,release=artful/console

Mir CI Bot (mir-ci-bot) wrote on 2017-09-20:

review: Needs Fixing (continuous-integration)

Alan Griffiths (alan-griffiths) wrote on 2017-09-20:

And again.

It seems valgrind is reporting errors on artful that were not there yesterday. (And have nothing to do with this MP as I can reproduce on trunk.)

Committing by hand

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:

Alan Griffiths

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === modified file 'debian/changelog'
 --- debian/changelog	2017-09-15 14:57:53 +0000
 +++ debian/changelog	2017-09-19 14:51:23 +0000
@@ -49,6 +49,8 @@
        . Make racy DragAndDrop test reliable. (LP: #1704780)
        . [libmiral] Define default window settings in one place and clamp the
          actual values to avoid ldiv0. (LP: #1717061)
++      . [miral-kiosk] Apply fullscreen logic when hidden windows are restored.
++        (LP: #1717910)
   -- Alan Griffiths <alan.griffiths@canonical.com>  Thu, 10 Aug 2017 11:54:30 +0000
 === modified file 'examples/miral-kiosk/kiosk_window_manager.cpp'
 --- examples/miral-kiosk/kiosk_window_manager.cpp	2017-08-21 15:05:23 +0000
 +++ examples/miral-kiosk/kiosk_window_manager.cpp	2017-09-19 14:51:23 +0000
@@ -1,5 +1,5 @@
  /*
-- * Copyright © 2016 Canonical Ltd.
++ * Copyright © 2016-2017 Canonical Ltd.
+  *
   * This program is free software: you can redistribute it and/or modify it
   * under the terms of the GNU General Public License version 2 or 3 as
@@ -128,21 +128,36 @@
+     }
+ }
--void KioskWindowManagerPolicy::advise_new_window(WindowInfo const& window_info)
--{
--    // We do this here, not in place_new_window() so that clients get a resize event.
--    // This shouldn't be necessary, but works better with the gtk-mir backend.
--    if (window_info.type() == mir_window_type_normal &&
--        !window_info.parent() &&
--        window_info.state() == mir_window_state_restored)
--    {
--        WindowSpecification specification;
--
--        specification.state() = mir_window_state_maximized;
--
++auto KioskWindowManagerPolicy::place_new_window(ApplicationInfo const& app_info, WindowSpecification const& request)
++-> WindowSpecification
++{
++    WindowSpecification specification = CanonicalWindowManagerPolicy::place_new_window(app_info, request);
++
++    if (specification.type() == mir_window_type_normal &&
++        (!specification.parent().is_set() || !specification.parent().value().lock()))
++    {
++        specification.state() = mir_window_state_maximized;
++        tools.place_and_size_for_state(specification, WindowInfo{});
++
++        if (!request.state().is_set() || request.state().value() != mir_window_state_restored)
++            specification.state() = request.state();
++    }
++
++    return specification;
++}
++
++void KioskWindowManagerPolicy::handle_modify_window(WindowInfo& window_info, WindowSpecification const& modifications)
++{
++    WindowSpecification specification = modifications;
++
++    if (window_info.type() == mir_window_type_normal && !window_info.parent())
++    {
++        specification.state() = mir_window_state_maximized;
          tools.place_and_size_for_state(specification, window_info);
--        tools.modify_window(window_info.window(), specification);
++
++        if (!modifications.state().is_set() || modifications.state().value() != mir_window_state_restored)
++            specification.state() = modifications.state();
+     }
--    CanonicalWindowManagerPolicy::advise_new_window(window_info);
++    CanonicalWindowManagerPolicy::handle_modify_window(window_info, specification);
+ }
 === modified file 'examples/miral-kiosk/kiosk_window_manager.h'
 --- examples/miral-kiosk/kiosk_window_manager.h	2017-08-21 15:05:23 +0000
 +++ examples/miral-kiosk/kiosk_window_manager.h	2017-09-19 14:51:23 +0000
@@ -1,5 +1,5 @@
  /*
-- * Copyright © 2016 Canonical Ltd.
++ * Copyright © 2016-2017 Canonical Ltd.
+  *
   * This program is free software: you can redistribute it and/or modify it
   * under the terms of the GNU General Public License version 2 or 3 as
@@ -30,13 +30,15 @@
  public:
      KioskWindowManagerPolicy(miral::WindowManagerTools const& tools, SwSplash const&);
++    auto place_new_window(miral::ApplicationInfo const& app_info, miral::WindowSpecification const& request)
++    -> miral::WindowSpecification override;
++
      void advise_focus_gained(miral::WindowInfo const& info) override;
--    virtual void advise_new_window(miral::WindowInfo const& window_info) override;
--
      bool handle_keyboard_event(MirKeyboardEvent const* event) override;
      bool handle_touch_event(MirTouchEvent const* event) override;
      bool handle_pointer_event(MirPointerEvent const* event) override;
++    void handle_modify_window(miral::WindowInfo& window_info, miral::WindowSpecification const& modifications) override;
  private:
      static const int modifier_mask =
 === modified file 'include/miral/miral/canonical_window_manager.h'
 --- include/miral/miral/canonical_window_manager.h	2017-08-21 14:18:55 +0000
 +++ include/miral/miral/canonical_window_manager.h	2017-09-19 14:51:23 +0000
@@ -31,7 +31,7 @@
      explicit CanonicalWindowManagerPolicy(WindowManagerTools const& tools);
--    auto place_new_window(
++    virtual auto place_new_window(
          ApplicationInfo const& app_info,
          WindowSpecification const& request_parameters)
          -> WindowSpecification override;

Mir

Merge lp:~alan-griffiths/mir/fix-1717910 into lp:mir

Commit Message

Description of the Change

Preview Diff

Subscribers