Unity 8

Merge lp:~lukas-kde/unity8/fixPanelMaxState into lp:unity8

fixPanelMaxState
Merge into trunk

Proposed by Lukáš Tinkl on 2016-01-07

Status:

Merged

Approved by:

Michael Zanetti on 2016-01-08

Approved revision:

2110

Merged at revision:

2130

Proposed branch:

lp:~lukas-kde/unity8/fixPanelMaxState

Merge into:

lp:unity8

Diff against target:

90 lines (+36/-3)

2 files modified

qml/Stages/DesktopStage.qml (+7/-3)
tests/qmltests/Stages/tst_DesktopStage.qml (+29/-0)

To merge this branch:

bzr merge lp:~lukas-kde/unity8/fixPanelMaxState

Undecided

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Needs Fixing on 2016-01-08
Michael Zanetti (community)		2016-01-07	Approve on 2016-01-08
Review via email: mp+281856@code.launchpad.net

Commit message

Fix panel drop shadow and click-to-focus of maximized apps

Description of the change

Fix recent desktop stage regressions:

1. the panel drop shadow is always displayed (should only be visible when there's a maximized app in background)
2. with a max'd app in background, clicking the panel no longer brings that app into foreground

* Are there any related MPs required for this MP to build/function as expected? Please list.

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

Yes

* Did you make sure that your branch does not contain spurious tags?

Yes

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

Yes

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

Yes (this is to align with the intended design)

lp:~lukas-kde/unity8/fixPanelMaxState updated on 2016-01-07

2109. By Lukáš Tinkl on 2016-01-07: add a regression test for the drop shadow stuff

Revision history for this message

Lukáš Tinkl (lukas-kde) wrote on 2016-01-07:

Added a regression test for the drop shadow stuff

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-01-07:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/7027/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-01-07:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/7029/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2016-01-08:

It fixes the interaction with the panel, so that's good. Two remarks:

====

+ property int foregroundMaximizedAppIdIndex: -1

I know this has been there before, but please rename it to foregroundMaximizedAppId, as it has nothing to do with an index.

====

it fixes the issue, but there is something wrong with the dropshadow still. If there is a maximized app, but not in foreground, the panel's dropshadow is visible. However, comparing that with unity7, the dropshadow seems to be *always* behind any window. It effectively only drops a shadow onto the wallpaper.

review: Needs Fixing

Revision history for this message

Lukáš Tinkl (lukas-kde) wrote on 2016-01-08:

> It fixes the interaction with the panel, so that's good. Two remarks:
>
> ====
>
> + property int foregroundMaximizedAppIdIndex: -1
>
> I know this has been there before, but please rename it to
> foregroundMaximizedAppId, as it has nothing to do with an index.

Shouldn't it rather be foregroundMaximizedAppIndex? It is an index in fact, it has nothing to do with "appId" in fact.

>
> it fixes the issue, but there is something wrong with the dropshadow still. If
> there is a maximized app, but not in foreground, the panel's dropshadow is
> visible. However, comparing that with unity7, the dropshadow seems to be
> *always* behind any window. It effectively only drops a shadow onto the
> wallpaper.

I believe that's the correct behavior, the drop shadow should be visible only if there's a maximized app in the background (https://docs.google.com/presentation/d/1O9B6d4gm3X_u7460o_iTYORTWJ7Td7aN7jLvo2O9A4w/edit?hl=en#slide=id.ge5d9cfc49_0_17)

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2016-01-08:

On 08.01.2016 12:37, Lukáš Tinkl wrote:
>> It fixes the interaction with the panel, so that's good. Two remarks:
>>
>> ====
>>
>> + property int foregroundMaximizedAppIdIndex: -1
>>
>> I know this has been there before, but please rename it to
>> foregroundMaximizedAppId, as it has nothing to do with an index.
>
> Shouldn't it rather be foregroundMaximizedAppIndex? It is an index in fact, it has nothing to do with "appId" in fact.

ok, if it is an index, fine. but the current mix of IdIndex is broken in
any case :)

>
>>
>> it fixes the issue, but there is something wrong with the dropshadow still. If
>> there is a maximized app, but not in foreground, the panel's dropshadow is
>> visible. However, comparing that with unity7, the dropshadow seems to be
>> *always* behind any window. It effectively only drops a shadow onto the
>> wallpaper.
>
> I believe that's the correct behavior, the drop shadow should be visible only if there's a maximized app in the background (https://docs.google.com/presentation/d/1O9B6d4gm3X_u7460o_iTYORTWJ7Td7aN7jLvo2O9A4w/edit?hl=en#slide=id.ge5d9cfc49_0_17)
>

ok then. wfm, just wanted to be sure this is what's wanted.

lp:~lukas-kde/unity8/fixPanelMaxState updated on 2016-01-08

2110. By Lukáš Tinkl on 2016-01-08: rename internal variable to foregroundMaximizedAppIndex

Revision history for this message

Lukáš Tinkl (lukas-kde) wrote on 2016-01-08:

>
>
> On 08.01.2016 12:37, Lukáš Tinkl wrote:
> >> It fixes the interaction with the panel, so that's good. Two remarks:
> >>
> >> ====
> >>
> >> + property int foregroundMaximizedAppIdIndex: -1
> >>
> >> I know this has been there before, but please rename it to
> >> foregroundMaximizedAppId, as it has nothing to do with an index.
> >
> > Shouldn't it rather be foregroundMaximizedAppIndex? It is an index in fact,
> it has nothing to do with "appId" in fact.
>
> ok, if it is an index, fine. but the current mix of IdIndex is broken in
> any case :)

Done

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2016-01-08:

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

yes

* Did CI run pass? If not, please explain why.

unrelated failures, no new regressions from this branch

* Did you make sure that the branch does not contain spurious tags?

yes, stripped now

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-01-08:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/7034/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:

Lukáš Tinkl

Mohammad Rakibul Hasan

Unity Team

 === modified file 'qml/Stages/DesktopStage.qml'
 --- qml/Stages/DesktopStage.qml	2015-11-30 17:27:47 +0000
 +++ qml/Stages/DesktopStage.qml	2016-01-08 13:29:28 +0000
@@ -124,16 +124,20 @@
          onFocusedAppDelegateChanged: updateForegroundMaximizedApp();
          property int foregroundMaximizedAppZ: -1
++        property int foregroundMaximizedAppIndex: -1 // for stuff like drop shadow and focusing maximized app by clicking panel
          function updateForegroundMaximizedApp() {
              var tmp = -1;
++            var tmpAppId = -1;
              for (var i = appRepeater.count - 1; i >= 0; i--) {
                  var item = appRepeater.itemAt(i);
                  if (item && item.visuallyMaximized) {
++                    tmpAppId = i;
                      tmp = Math.max(tmp, item.normalZ);
+                 }
+             }
              foregroundMaximizedAppZ = tmp;
++            foregroundMaximizedAppIndex = tmpAppId;
+         }
          function indexOf(appId) {
@@ -176,8 +180,8 @@
          onMinimize: priv.focusedAppDelegate && priv.focusedAppDelegate.minimize();
          onMaximize: priv.focusedAppDelegate // don't restore minimized apps when double clicking the panel
                      && priv.focusedAppDelegate.restoreFromMaximized();
--        onFocusMaximizedApp: if (priv.foregroundMaximizedAppIdIndex != -1) {
--                                 ApplicationManager.focusApplication(appRepeater.itemAt(priv.foregroundMaximizedAppIdIndex).appId);
++        onFocusMaximizedApp: if (priv.foregroundMaximizedAppIndex != -1) {
++                                 ApplicationManager.focusApplication(appRepeater.itemAt(priv.foregroundMaximizedAppIndex).appId);
+                              }
+     }
@@ -206,7 +210,7 @@
      Binding {
          target: PanelState
          property: "dropShadow"
--        value: priv.focusedAppDelegate && !priv.focusedAppDelegate.maximized && priv.foregroundMaximizedAppIdIndex !== -1
++        value: priv.focusedAppDelegate && !priv.focusedAppDelegate.maximized && priv.foregroundMaximizedAppIndex !== -1
+     }
      Component.onDestruction: {
 === modified file 'tests/qmltests/Stages/tst_DesktopStage.qml'
 --- tests/qmltests/Stages/tst_DesktopStage.qml	2015-12-08 15:37:51 +0000
 +++ tests/qmltests/Stages/tst_DesktopStage.qml	2016-01-08 13:29:28 +0000
@@ -25,6 +25,7 @@
  import ".." // For EdgeBarrierControls
  import "../../../qml/Stages"
  import "../../../qml/Components"
++import "../../../qml/Components/PanelState"
  Item {
      id: root
@@ -500,5 +501,33 @@
              tryCompare(dialerAppDelegate, "visible", true);
              tryCompare(facebookAppDelegate, "visible", false);
+         }
++
++        function test_dropShadow() {
++            killAllRunningApps();
++
++            // verify the drop shadow is not visible initially
++            verify(PanelState.dropShadow == false);
++
++            // start an app, maximize it
++            var facebookApp = startApplication("facebook-webapp");
++            var facebookAppDelegate = findChild(desktopStage, "appDelegate_facebook-webapp");
++            facebookAppDelegate.maximize();
++
++            // verify the drop shadow is still not visible
++            verify(PanelState.dropShadow == false);
++
++            // start a foreground app, not maximized
++            var dialerApp = startApplication("dialer-app");
++            var dialerAppDelegate = findChild(desktopStage, "appDelegate_dialer-app");
++
++            // verify the drop shadow becomes visible
++            verify(PanelState.dropShadow == true);
++
++            // close the maximized app
++            ApplicationManager.stopApplication("facebook-webapp");
++
++            // verify the drop shadow is gone
++            verify(PanelState.dropShadow == false);
++        }
+     }
+ }