Merge lp:~unity-team/unity8/fix-wakelocks into lp:unity8

Proposed by Michał Sawicz
Status: Merged
Approved by: Michał Sawicz
Approved revision: 2082
Merged at revision: 2082
Proposed branch: lp:~unity-team/unity8/fix-wakelocks
Merge into: lp:unity8
Prerequisite: lp:~dandrader/unity8/multiSurfaceApp
Diff against target: 213 lines (+61/-20)
8 files modified
qml/Stages/PhoneStage.qml (+6/-4)
qml/Stages/TabletStage.qml (+6/-4)
tests/mocks/Unity/Application/ApplicationInfo.cpp (+16/-0)
tests/mocks/Unity/Application/ApplicationInfo.h (+4/-0)
tests/mocks/Unity/Application/ApplicationManager.cpp (+2/-0)
tests/mocks/Unity/Application/ApplicationManager.h (+1/-1)
tests/plugins/Unity/Launcher/launchermodeltest.cpp (+2/-0)
tests/qmltests/tst_Shell.qml (+24/-11)
To merge this branch: bzr merge lp:~unity-team/unity8/fix-wakelocks
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Needs Fixing
Daniel d'Andrada Pending
Review via email: mp+279764@code.launchpad.net

This proposal supersedes a proposal from 2015-12-03.

Commit message

Let qtmir know which apps are exempt from the lifecycle management.
This way, it can manage its own wakelocks better (and stop preventing the system from deep sleeping).

Description of the change

Let qtmir know which apps are exempt from the lifecycle management, so it can manage its own wakelocks better (and stop preventing the system from deep sleeping).

 * Are there any related MPs required for this MP to build/function as expected? Please list.
 https://code.launchpad.net/~mterry/unity-api/fix-wakelocks/+merge/279476
 https://code.launchpad.net/~mterry/qtmir/fix-wakelocks/+merge/279481

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

 * 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?
 I'm on that team

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

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

In test_lifecyclePolicyExemption:

I don't see the point in changing app2 from dialer-app to gallery-app

review: Needs Fixing
Revision history for this message
Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

Likewise in test_lifecyclePolicyForNonTouchApp

Revision history for this message
Michael Terry (mterry) wrote : Posted in a previous version of this proposal

> I don't see the point in changing app2 from dialer-app to gallery-app

I realized when making this change that the test was flawed. dialer-app is a sidestage app, so in Tablet mode, it wouldn't actually trigger a Suspend for the main stage app anyway! :(

So I switched to two main stage apps.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

On 03/12/2015 18:17, Michael Terry wrote:
>> I don't see the point in changing app2 from dialer-app to gallery-app
> I realized when making this change that the test was flawed. dialer-app is a sidestage app, so in Tablet mode, it wouldn't actually trigger a Suspend for the main stage app anyway! :(
>
> So I switched to two main stage apps.

So worth adding a check in the test that ensures both apps are in the
main stage

Revision history for this message
Michael Terry (mterry) wrote : Posted in a previous version of this proposal

> So worth adding a check in the test that ensures both apps are in the
> main stage

I could, but that'd be a bit redundant with this check:
compare(app1.requestedState, ApplicationInfoInterface.RequestedSuspended);

Which is what we're really interested in. The test was poor before, because the checked requested state was the same as the running state, and so it would need to look for secondary characteristics to confirm the expected state but didn't.

But now that we compare for the suspended state, I don't think it's necessary to add further checks. But they'd be harmless to add if you like.

Revision history for this message
Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

Otherwise code looks ok. Didn't test yet.

review: Approve (code)
Revision history for this message
Michael Terry (mterry) wrote : Posted in a previous version of this proposal

I've updated the tests to have more comments and sanity checking.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

And fixes the bug

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
lp:~unity-team/unity8/fix-wakelocks updated
2082. By Michael Terry

Fix tests

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Michał Sawicz (saviq) wrote :

Approving per superseded.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'qml/Stages/PhoneStage.qml'
--- qml/Stages/PhoneStage.qml 2015-11-04 14:58:05 +0000
+++ qml/Stages/PhoneStage.qml 2015-12-08 14:40:30 +0000
@@ -426,14 +426,16 @@
426426
427 readonly property bool isDash: model.appId == "unity8-dash"427 readonly property bool isDash: model.appId == "unity8-dash"
428428
429 readonly property bool canSuspend: model.isTouchApp429 Binding {
430 && !isExemptFromLifecycle(model.appId)430 target: appDelegate.application
431 property: "exemptFromLifecycle"
432 value: !model.isTouchApp || isExemptFromLifecycle(model.appId)
433 }
431434
432 Binding {435 Binding {
433 target: appDelegate.application436 target: appDelegate.application
434 property: "requestedState"437 property: "requestedState"
435 value: !canSuspend438 value: (isDash && root.keepDashRunning)
436 || (isDash && root.keepDashRunning)
437 || (!root.suspended && appDelegate.focus)439 || (!root.suspended && appDelegate.focus)
438 ? ApplicationInfoInterface.RequestedRunning440 ? ApplicationInfoInterface.RequestedRunning
439 : ApplicationInfoInterface.RequestedSuspended441 : ApplicationInfoInterface.RequestedSuspended
440442
=== modified file 'qml/Stages/TabletStage.qml'
--- qml/Stages/TabletStage.qml 2015-11-04 14:58:05 +0000
+++ qml/Stages/TabletStage.qml 2015-12-08 14:40:30 +0000
@@ -612,14 +612,16 @@
612612
613 readonly property bool isDash: model.appId == "unity8-dash"613 readonly property bool isDash: model.appId == "unity8-dash"
614614
615 readonly property bool canSuspend: model.isTouchApp615 Binding {
616 && !isExemptFromLifecycle(model.appId)616 target: spreadTile.application
617 property: "exemptFromLifecycle"
618 value: !model.isTouchApp || isExemptFromLifecycle(model.appId)
619 }
617620
618 Binding {621 Binding {
619 target: spreadTile.application622 target: spreadTile.application
620 property: "requestedState"623 property: "requestedState"
621 value: !canSuspend624 value: (isDash && root.keepDashRunning)
622 || (isDash && root.keepDashRunning)
623 || (!root.suspended && (model.appId == priv.mainStageAppId625 || (!root.suspended && (model.appId == priv.mainStageAppId
624 || model.appId == priv.sideStageAppId))626 || model.appId == priv.sideStageAppId))
625 ? ApplicationInfoInterface.RequestedRunning627 ? ApplicationInfoInterface.RequestedRunning
626628
=== modified file 'tests/mocks/Unity/Application/ApplicationInfo.cpp'
--- tests/mocks/Unity/Application/ApplicationInfo.cpp 2015-12-08 14:40:30 +0000
+++ tests/mocks/Unity/Application/ApplicationInfo.cpp 2015-12-08 14:40:30 +0000
@@ -42,6 +42,7 @@
42 , m_rotatesWindowContents(false)42 , m_rotatesWindowContents(false)
43 , m_requestedState(RequestedRunning)43 , m_requestedState(RequestedRunning)
44 , m_isTouchApp(true)44 , m_isTouchApp(true)
45 , m_exemptFromLifecycle(false)
45 , m_manualSurfaceCreation(false)46 , m_manualSurfaceCreation(false)
46{47{
47}48}
@@ -60,6 +61,7 @@
60 , m_rotatesWindowContents(false)61 , m_rotatesWindowContents(false)
61 , m_requestedState(RequestedRunning)62 , m_requestedState(RequestedRunning)
62 , m_isTouchApp(true)63 , m_isTouchApp(true)
64 , m_exemptFromLifecycle(false)
63 , m_manualSurfaceCreation(false)65 , m_manualSurfaceCreation(false)
64{66{
65}67}
@@ -263,3 +265,17 @@
263 }265 }
264 }266 }
265}267}
268
269bool ApplicationInfo::exemptFromLifecycle() const
270{
271 return m_exemptFromLifecycle;
272}
273
274void ApplicationInfo::setExemptFromLifecycle(bool exemptFromLifecycle)
275{
276 if (m_exemptFromLifecycle != exemptFromLifecycle)
277 {
278 m_exemptFromLifecycle = exemptFromLifecycle;
279 Q_EMIT exemptFromLifecycleChanged(m_exemptFromLifecycle);
280 }
281}
266282
=== modified file 'tests/mocks/Unity/Application/ApplicationInfo.h'
--- tests/mocks/Unity/Application/ApplicationInfo.h 2015-12-08 14:40:30 +0000
+++ tests/mocks/Unity/Application/ApplicationInfo.h 2015-12-08 14:40:30 +0000
@@ -92,6 +92,9 @@
92 bool isTouchApp() const override;92 bool isTouchApp() const override;
93 void setIsTouchApp(bool isTouchApp); // only in mock93 void setIsTouchApp(bool isTouchApp); // only in mock
9494
95 bool exemptFromLifecycle() const override;
96 void setExemptFromLifecycle(bool) override;
97
95public:98public:
96 void setSession(Session* session);99 void setSession(Session* session);
97 Session* session() const { return m_session; }100 Session* session() const { return m_session; }
@@ -125,6 +128,7 @@
125 bool m_rotatesWindowContents;128 bool m_rotatesWindowContents;
126 RequestedState m_requestedState;129 RequestedState m_requestedState;
127 bool m_isTouchApp;130 bool m_isTouchApp;
131 bool m_exemptFromLifecycle;
128132
129 bool m_manualSurfaceCreation;133 bool m_manualSurfaceCreation;
130};134};
131135
=== modified file 'tests/mocks/Unity/Application/ApplicationManager.cpp'
--- tests/mocks/Unity/Application/ApplicationManager.cpp 2015-11-05 14:04:32 +0000
+++ tests/mocks/Unity/Application/ApplicationManager.cpp 2015-12-08 14:40:30 +0000
@@ -107,6 +107,8 @@
107 return app->focused();107 return app->focused();
108 case RoleIsTouchApp:108 case RoleIsTouchApp:
109 return app->isTouchApp();109 return app->isTouchApp();
110 case RoleExemptFromLifecycle:
111 return app->exemptFromLifecycle();
110 case RoleSession:112 case RoleSession:
111 return QVariant::fromValue(app->session());113 return QVariant::fromValue(app->session());
112 case RoleFullscreen:114 case RoleFullscreen:
113115
=== modified file 'tests/mocks/Unity/Application/ApplicationManager.h'
--- tests/mocks/Unity/Application/ApplicationManager.h 2015-10-01 17:43:10 +0000
+++ tests/mocks/Unity/Application/ApplicationManager.h 2015-12-08 14:40:30 +0000
@@ -43,7 +43,7 @@
43 static ApplicationManager *singleton();43 static ApplicationManager *singleton();
4444
45 enum MoreRoles {45 enum MoreRoles {
46 RoleSession = RoleIsTouchApp+1,46 RoleSession = RoleExemptFromLifecycle+1,
47 RoleFullscreen,47 RoleFullscreen,
48 };48 };
49 enum Flag {49 enum Flag {
5050
=== modified file 'tests/plugins/Unity/Launcher/launchermodeltest.cpp'
--- tests/plugins/Unity/Launcher/launchermodeltest.cpp 2015-11-06 13:27:15 +0000
+++ tests/plugins/Unity/Launcher/launchermodeltest.cpp 2015-12-08 14:40:30 +0000
@@ -59,6 +59,8 @@
59 Qt::ScreenOrientations supportedOrientations() const override { return Qt::PortraitOrientation; }59 Qt::ScreenOrientations supportedOrientations() const override { return Qt::PortraitOrientation; }
60 bool rotatesWindowContents() const override { return false; }60 bool rotatesWindowContents() const override { return false; }
61 bool isTouchApp() const override { return true; }61 bool isTouchApp() const override { return true; }
62 bool exemptFromLifecycle() const override { return false; }
63 void setExemptFromLifecycle(bool) override {}
6264
63 // Methods used for mocking (not in the interface)65 // Methods used for mocking (not in the interface)
64 void setFocused(bool focused) { m_focused = focused; Q_EMIT focusedChanged(focused); }66 void setFocused(bool focused) { m_focused = focused; Q_EMIT focusedChanged(focused); }
6567
=== modified file 'tests/qmltests/tst_Shell.qml'
--- tests/qmltests/tst_Shell.qml 2015-12-08 14:40:30 +0000
+++ tests/qmltests/tst_Shell.qml 2015-12-08 14:40:30 +0000
@@ -1857,21 +1857,28 @@
1857 loadShell(data.formFactor);1857 loadShell(data.formFactor);
1858 shell.usageScenario = data.usageScenario;1858 shell.usageScenario = data.usageScenario;
18591859
1860 // Add two main stage apps, the second in order to suspend the first.
1861 // LibreOffice has isTouchApp set to false by our mocks.
1860 var app1 = ApplicationManager.startApplication("libreoffice");1862 var app1 = ApplicationManager.startApplication("libreoffice");
1861 waitUntilAppWindowIsFullyLoaded(app1);1863 waitUntilAppWindowIsFullyLoaded(app1);
1862 var app2 = ApplicationManager.startApplication("dialer-app");1864 var app2 = ApplicationManager.startApplication("gallery-app");
1863 waitUntilAppWindowIsFullyLoaded(app2);1865 waitUntilAppWindowIsFullyLoaded(app2);
18641866
1865 // Make sure app1 is unfocused but still running1867 // Sanity checking
1866 compare(app1.session.lastSurface.activeFocus, false);1868 compare(app1.stage, ApplicationInfoInterface.MainStage);
1867 compare(app1.isTouchApp, false); // sanity check our mock, which sets this for us1869 compare(app2.stage, ApplicationInfoInterface.MainStage);
1868 compare(app1.requestedState, ApplicationInfoInterface.RequestedRunning);1870 verify(!app1.isTouchApp);
1871 verify(!app1.session.lastSurface.activeFocus);
1872
1873 // Make sure app1 is exempt with a requested suspend
1874 verify(app1.exemptFromLifecycle);
1875 compare(app1.requestedState, ApplicationInfoInterface.RequestedSuspended);
1869 }1876 }
18701877
1871 function test_lifecyclePolicyExemption_data() {1878 function test_lifecyclePolicyExemption_data() {
1872 return [1879 return [
1873 {tag: "phone", formFactor: "phone", usageScenario: "phone", suspendsApps: true},1880 {tag: "phone", formFactor: "phone", usageScenario: "phone"},
1874 {tag: "tablet", formFactor: "tablet", usageScenario: "tablet", suspendsApps: true}1881 {tag: "tablet", formFactor: "tablet", usageScenario: "tablet"}
1875 ]1882 ]
1876 }1883 }
18771884
@@ -1881,14 +1888,20 @@
18811888
1882 GSettingsController.setLifecycleExemptAppids(["webbrowser-app"]);1889 GSettingsController.setLifecycleExemptAppids(["webbrowser-app"]);
18831890
1891 // Add two main stage apps, the second in order to suspend the first
1884 var app1 = ApplicationManager.startApplication("webbrowser-app");1892 var app1 = ApplicationManager.startApplication("webbrowser-app");
1885 waitUntilAppWindowIsFullyLoaded(app1);1893 waitUntilAppWindowIsFullyLoaded(app1);
1886 var app2 = ApplicationManager.startApplication("dialer-app");1894 var app2 = ApplicationManager.startApplication("gallery-app");
1887 waitUntilAppWindowIsFullyLoaded(app2);1895 waitUntilAppWindowIsFullyLoaded(app2);
18881896
1889 // Make sure app1 is unfocused but still running1897 // Sanity checking
1890 compare(app1.session.lastSurface.activeFocus, false);1898 compare(app1.stage, ApplicationInfoInterface.MainStage);
1891 compare(app1.requestedState, ApplicationInfoInterface.RequestedRunning);1899 compare(app2.stage, ApplicationInfoInterface.MainStage);
1900 verify(!app1.session.lastSurface.activeFocus);
1901
1902 // Make sure app1 is exempt with a requested suspend
1903 verify(app1.exemptFromLifecycle);
1904 compare(app1.requestedState, ApplicationInfoInterface.RequestedSuspended);
1892 }1905 }
18931906
1894 function test_switchToStagedForcesLegacyAppClosing_data() {1907 function test_switchToStagedForcesLegacyAppClosing_data() {

Subscribers

People subscribed via source and target branches