Unity 8

Merge lp:~dandrader/unity8/promptsZOrder into lp:unity8

promptsZOrder
Merge into trunk

Proposed by Daniel d'Andrada on 2016-06-01

Status:

Merged

Approved by:

Nick Dedekind on 2016-06-02

Approved revision:

2444

Merged at revision:

2449

Proposed branch:

lp:~dandrader/unity8/promptsZOrder

Merge into:

lp:unity8

Diff against target:

212 lines (+78/-21)

5 files modified

qml/Stages/ApplicationWindow.qml (+26/-10)
tests/mocks/Unity/Application/MirSurfaceListModel.cpp (+11/-1)
tests/mocks/Unity/Application/MirSurfaceListModel.h (+1/-0)
tests/qmltests/Stages/ApplicationCheckBox.qml (+1/-1)
tests/qmltests/Stages/tst_ApplicationWindow.qml (+39/-9)

To merge this branch:

bzr merge lp:~dandrader/unity8/promptsZOrder

Critical

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Unity8 CI Bot	continuous-integration		Needs Fixing on 2016-06-06
Nick Dedekind (community)		2016-06-01	Approve on 2016-06-02
Review via email: mp+296243@code.launchpad.net

Commit Message

Fix Z-ordering of prompt surfaces

And also update Unity.Application mock to prepend instead of append new
prompt surfaces, like qtmir does.

Description of the Change

* Are there any related MPs required for this MP to build/function as expected? Please list.
Not a requirement technically speaking (as in it works without it), but makes sense to have it as well:
https://code.launchpad.net/~dandrader/qtmir/fixFirstChangedOnPrepend/+merge/296313

* Did you perform an exploratory manual test run of your code change and any related functionality?
Yes

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
Not applicable

* If you changed the UI, has there been a design review?
Not applicable

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-06-01:

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

review: Needs Fixing (continuous-integration)

lp:~dandrader/unity8/promptsZOrder updated on 2016-06-02

2441. By Launchpad Translations on behalf of unity-team on 2016-06-02: Launchpad automatic translations update.

Nick Dedekind (nick-dedekind) wrote on 2016-06-02:

small comment, rest is fine.

review: Needs Fixing

Daniel d'Andrada (dandrader) wrote on 2016-06-02:

On 02/06/2016 08:15, Nick Dedekind wrote:
> prepending will always change "first". Might want to check qtmir to see if this matches if you copied.

Fixed. qtmir had the same problem even though I didn't copy-pasted from
it (see its url in the updated MP description).

lp:~dandrader/unity8/promptsZOrder updated on 2016-06-02

2442. By Daniel d'Andrada on 2016-06-02

Fix Z-ordering of prompt surfaces

And also update Unity.Application mock to prepend instead of append new
prompt surfaces, like qtmir does.

2443. By Daniel d'Andrada on 2016-06-02

Fix emission of firstChanged() on prepend

2444. By Daniel d'Andrada on 2016-06-02

Write regression test and fix/update existing one

Daniel d'Andrada (dandrader) wrote on 2016-06-02:

Wrote regression test and rebased on top of latest trunk

Nick Dedekind (nick-dedekind) wrote on 2016-06-02:

Fine now. Tests succeed.

review: Approve

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-06-03:

FAILED: Continuous integration, rev:2411
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/1365/
Executed test runs:
FAILURE: https://unity8-jenkins.ubuntu.com/job/build/1827/console
FAILURE: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/1848/console

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

review: Needs Fixing (continuous-integration)

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-06-05:

FAILED: Continuous integration, rev:2411
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/1396/
Executed test runs:
FAILURE: https://unity8-jenkins.ubuntu.com/job/build/1862/console
FAILURE: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/1888/console

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

review: Needs Fixing (continuous-integration)

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-06-06:

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

review: Needs Fixing (continuous-integration)

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

Mohammad Rakibul Hasan

Unity Team

 === modified file 'qml/Stages/ApplicationWindow.qml'
 --- qml/Stages/ApplicationWindow.qml	2016-05-17 19:56:05 +0000
 +++ qml/Stages/ApplicationWindow.qml	2016-06-02 12:31:17 +0000
@@ -121,15 +121,12 @@
          property bool surfaceOldEnoughToBeResized: false
--        property var focusedSurface: {
--            if (promptSurfacesRepeater.count == 0) {
--                return surfaceContainer;
--            } else {
--                return promptSurfacesRepeater.itemAt(promptSurfacesRepeater.count - 1);
--            }
--        }
++        property Item focusedSurface: promptSurfacesRepeater.count === 0 ? surfaceContainer
++                                                                         : promptSurfacesRepeater.first
          onFocusedSurfaceChanged: {
--            focusedSurface.focus = true;
++            if (focusedSurface) {
++                focusedSurface.focus = true;
++            }
+         }
+     }
@@ -158,6 +155,7 @@
          objectName: "screenshotImage"
          anchors.fill: parent
          antialiasing: !root.interactive
++        z: 1
          function take() {
              // Save memory by using a half-resolution (thus quarter size) screenshot.
@@ -175,6 +173,7 @@
          visible: active
          active: false
          anchors.fill: parent
++        z: screenshotImage.z + 1
          sourceComponent: Component {
              Splash {
                  id: splash
@@ -191,6 +190,7 @@
      SurfaceContainer {
          id: surfaceContainer
++        z: splashLoader.z + 1
          requestedWidth: root.requestedWidth
          requestedHeight: root.requestedHeight
          surfaceOrientationAngle: application && application.rotatesWindowContents ? root.surfaceOrientationAngle : 0
@@ -208,12 +208,28 @@
+             }
+         }
          delegate: SurfaceContainer {
--            interactive: index == (promptSurfacesRepeater.count - 1) && root.interactive
++            id: promptSurfaceContainer
++            interactive: index === 0 && root.interactive
              surface: model.surface
              width: root.width
              height: root.height
              isPromptSurface: true
--        }
++            z: surfaceContainer.z + (promptSurfacesRepeater.count - index)
++            property int index: model.index
++            onIndexChanged: updateFirst()
++            Component.onCompleted: updateFirst()
++            function updateFirst() {
++                if (index === 0) {
++                    promptSurfacesRepeater.first = promptSurfaceContainer;
++                }
++            }
++        }
++        onCountChanged: {
++            if (count === 0) {
++                first = null;
++            }
++        }
++        property Item first: null
+     }
      // SurfaceContainer size drives ApplicationWindow size
 === modified file 'tests/mocks/Unity/Application/MirSurfaceListModel.cpp'
 --- tests/mocks/Unity/Application/MirSurfaceListModel.cpp	2016-05-17 19:56:10 +0000
 +++ tests/mocks/Unity/Application/MirSurfaceListModel.cpp	2016-06-02 12:31:17 +0000
@@ -72,6 +72,16 @@
+     }
+ }
++void MirSurfaceListModel::prependSurface(MirSurface *surface)
++{
++    beginInsertRows(QModelIndex(), 0, 0);
++    m_surfaceList.prepend(surface);
++    connectSurface(surface);
++    endInsertRows();
++    Q_EMIT countChanged();
++    Q_EMIT firstChanged();
++}
++
  void MirSurfaceListModel::connectSurface(MirSurface *surface)
+ {
      connect(surface, &MirSurface::raiseRequested, this, [this, surface](){ this->raise(surface); });
@@ -143,7 +153,7 @@
              Mir::RestoredState,
              screenshotUrl);
--    appendSurface(surface);
++    prependSurface(surface);
      return surface;
+ }
 === modified file 'tests/mocks/Unity/Application/MirSurfaceListModel.h'
 --- tests/mocks/Unity/Application/MirSurfaceListModel.h	2016-05-17 19:25:23 +0000
 +++ tests/mocks/Unity/Application/MirSurfaceListModel.h	2016-06-02 12:31:17 +0000
@@ -49,6 +49,7 @@
      Q_INVOKABLE unity::shell::application::MirSurfaceInterface *createSurface();
  private:
++    void prependSurface(MirSurface *surface);
      void raise(MirSurface *surface);
      void moveSurface(int from, int to);
      void connectSurface(MirSurface *surface);
 === modified file 'tests/qmltests/Stages/ApplicationCheckBox.qml'
 --- tests/qmltests/Stages/ApplicationCheckBox.qml	2016-05-17 19:56:10 +0000
 +++ tests/qmltests/Stages/ApplicationCheckBox.qml	2016-06-02 12:31:17 +0000
@@ -156,7 +156,7 @@
              width: height
              height: promptsLabel.height * 0.7
              anchors.verticalCenter: parent.verticalCenter
--            onClicked: d.application.promptSurfaceList.get(d.application.promptSurfaceList.count - 1).close()
++            onClicked: d.application.promptSurfaceList.get(0).close()
              enabled: d.application && d.application.promptSurfaceList.count > 0
              Label { text: "➖"; anchors.centerIn: parent; enabled: parent.enabled }
+         }
 === modified file 'tests/qmltests/Stages/tst_ApplicationWindow.qml'
 --- tests/qmltests/Stages/tst_ApplicationWindow.qml	2016-05-17 19:25:23 +0000
 +++ tests/qmltests/Stages/tst_ApplicationWindow.qml	2016-06-02 12:31:17 +0000
@@ -106,7 +106,7 @@
                      enabled: parent.promptSurfaceList !== null && parent.promptSurfaceList.count > 0
                      activeFocusOnPress: false
                      text: "Remove"
--                    onClicked: { parent.promptSurfaceList.get(parent.promptSurfaceList.count - 1).close(); }
++                    onClicked: { parent.promptSurfaceList.get(0).close(); }
+                 }
                  Button {
@@ -449,20 +449,21 @@
              for (i = 0; i < 3; i++) {
                  promptSurfaceList.createSurface();
                  compare(promptSurfaces.count, i+1);
--                waitUntilSurfaceContainerStopsAnimating(promptSurfaces.itemAt(promptSurfaces.count - 1));
++                waitUntilSurfaceContainerStopsAnimating(promptSurfaces.itemAt(0));
+             }
--            for (i = 2; i >= 0; --i) {
--                var promptSurface = promptSurfaceList.get(i);
++            for (i = 3; i > 0; --i) {
++                var promptSurface = promptSurfaceList.get(0);
                  compare(promptSurface.activeFocus, true);
                  promptSurface.close();
--                tryCompareFunction(function() { return promptSurfaces.count; }, i);
++                promptSurface = null;
++                tryCompareFunction(function() { return promptSurfaces.count; }, i-1);
--                if (i > 0) {
--                    // active focus should have gone to the yongest remaining sibling
--                    var previousPromptSurface = promptSurfaceList.get(i-1);
--                    tryCompare(previousPromptSurface, "activeFocus", true);
++                if (promptSurfaces.count > 0) {
++                    // active focus should have gone to the new head of the list
++                    promptSurface = promptSurfaceList.get(0);
++                    tryCompare(promptSurface, "activeFocus", true);
                  } else {
                      // active focus should have gone to the application surface
                      tryCompare(applicationWindow.surface, "activeFocus", true);
@@ -498,5 +499,34 @@
              delegate.surface.close();
              tryCompare(promptSurfaces, "count", 0);
+         }
++
++        // Check that the z value of SurfaceContainers for prompt surfaces go from highest
++        // for index 0 to lowest for the last index in the prompt surface list.
++        // Regression test for https://bugs.launchpad.net/bugs/1586219
++        function test_promptSurfacesZOrdering() {
++            var promptSurfaceList = root.fakeApplication.promptSurfaceList;
++            var promptSurfaces = testCase.findChild(applicationWindow, "promptSurfacesRepeater");
++
++            promptSurfaceList.createSurface();
++
++            for (var i = 2; i <= 3; i++) {
++                promptSurfaceList.createSurface();
++                tryCompare(promptSurfaces, "count", i);
++                waitUntilSurfaceContainerStopsAnimating(promptSurfaces.itemAt(0));
++
++                for (var j = 1; j < promptSurfaces.count; j++) {
++                    var delegate = promptSurfaces.itemAt(j);
++                    var previousDelegate = promptSurfaces.itemAt(j-1);
++                    verify(previousDelegate.z > delegate.z);
++                }
++            }
++
++            // clean up
++            while (promptSurfaceList.count > 0) {
++                var currentCount = promptSurfaceList.count;
++                promptSurfaceList.get(0).close();
++                tryCompare(promptSurfaceList, "count", currentCount - 1);
++            }
++        }
+     }
+ }