Merge lp:~mterry/unity8/shutdown-dialog-on-resume into lp:unity8

Proposed by Michael Terry on 2015-10-21
Status: Merged
Approved by: Michael Terry on 2015-10-28
Approved revision: 2009
Merged at revision: 2035
Proposed branch: lp:~mterry/unity8/shutdown-dialog-on-resume
Merge into: lp:unity8
Diff against target: 179 lines (+51/-36)
5 files modified
plugins/Utils/windowkeysfilter.cpp (+15/-1)
plugins/Utils/windowkeysfilter.h (+7/-0)
qml/Components/PhysicalKeysMapper.qml (+16/-23)
qml/Shell.qml (+2/-2)
tests/qmltests/Components/tst_PhysicalKeysMapper.qml (+11/-10)
To merge this branch: bzr merge lp:~mterry/unity8/shutdown-dialog-on-resume
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Needs Fixing on 2015-10-27
Albert Astals Cid (community) 2015-10-21 Approve on 2015-10-23
Michael Zanetti (community) Needs Information on 2015-10-22
Review via email: mp+275240@code.launchpad.net

Commit message

Avoid showing the shutdown dialog when turning on the screen if your device is under heavy load.

Specifically, we actually watch the timestamp of input events as they come in to determine how long it's been. This means that if for whatever reason, processing of events get delayed, we don't misinterpret user input.

To test this, try running the following command and then turning the screen on and off again:
sudo cpulimit -l 1 -c 1 -p `ps ax | grep dbus-daemon | head -n 1 | awk '{print $1;}'`

Without this branch, you'll notice that at some point, you see the shutdown dialog in error. Because unity8 couldn't keep up with events and thought 2s passed between power-pressed and power-released events.

But if we watch the timestamps, we can avoid that particular fate.

Description of the change

* Are there any related MPs required for this MP to build/function as expected? Please list.
 https://code.launchpad.net/~unity-team/qtmir/1510571.ms-timestamp-compression/+merge/276112

 * 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?
 NA

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

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:2005
http://jenkins.qa.ubuntu.com/job/unity8-ci/6507/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/4754
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-touch/889
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/1219
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-wily/535
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/1114
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/1115
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/746
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/747
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/3837
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/4751
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/4751/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/24444
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-wily-mako/526
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/889
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/889/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/24443

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

review: Needs Fixing (continuous-integration)
Michael Terry (mterry) wrote :

Another option would be to expose the timestamp() value of the current event through to Qml and then use the timestamp of the autorepeated events to determine when two seconds have passed. If we don't trust the interval timing of the autorepeated events.

Albert Astals Cid (aacid) wrote :

property int powerKeyLongPressCount: 31 // about 2s
is something that depends on QStyleHints::keyboardAutoRepeatRate() and other things, those will probably won't change, but maybe we can add a test that actually checks that pressing and holidng the button triggers the dialog and takes between 1.5 and 2.5 seconds? just in case this varies wildly in the future with some update and we don't realize?

What do you think?

review: Needs Information
Michael Zanetti (mzanetti) wrote :

Very nice! This seems to work properly now.

 * 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.

The one AP failure seems not related to this.

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

yes

review: Approve
Michael Zanetti (mzanetti) wrote :

Oh, missed alberts comment. Yes, fair point... I was also thinking if API wise it wouldn't make sense to still specify it in ms, and internally divide the time into repeat-counts...

Michael Terry (mterry) wrote :

Ah great! I didn't know about QStyleHints::keyboardAutoRepeatRate(). Will specify time in ms in "API" and internally count ticks then.

Albert Astals Cid (aacid) wrote :

Note that before QStyleHints::keyboardAutoRepeatRate() there's another timeout for the first autorepeat to trigger, i don't know if there's a value for it or is hardcoded, worth investigating.

Michael Terry (mterry) wrote :

Ugh. keyboardAutoRepeatRate gives 30. Which is how many events happen per second. So 1000ms/30=33.3ms per autorepeat event. But that's not what I'm seeing. I'm seeing more like 40ms to 50ms.

And keyboardInputInterval *might* be the value used before Qt starts sending autorepeat events. It's not entirely clear if they use it for that. Which is 400ms for me. But I'm seeing a delay of 500ms.

So... I can't trust those values. But knowing that they exist, I'm not comfortable ignoring the settings either, like this MP does. I might look at using exposing event timestamps to Qml and comparing those as a more reliable strategy.

Michael Zanetti (mzanetti) :
review: Needs Information
Michael Terry (mterry) wrote :

OK, I've reworked this to use timestamps. Seems to work for me.

2008. By Michael Terry on 2015-10-22

remove an obsolete code change

PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:2008
http://jenkins.qa.ubuntu.com/job/unity8-ci/6521/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/4779
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-touch/904
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/1233
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-wily/549
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/1128
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/1129
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/760
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/761
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/3852
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/4776
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/4776/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/24485
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-wily-mako/533
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/904
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/904/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/24486

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

review: Needs Fixing (continuous-integration)
Albert Astals Cid (aacid) wrote :

It's weird because in r1942 i fixed a problem with pressing the power off button while the screen was off in which the first event we got was already an autorepeat.

From reading the code this would not work here, we would not set d.powerButtonPressStart but then i tried it and it seems to work, not sure if it's becuase i'm trying the Meizu vs the bq (that i don't have at hand right now) or something else changed.

I'll try testing the bq this afternoon.

Albert Astals Cid (aacid) wrote :

Tried with the bq and it also works fine, no idea what changed but nothing against from my side.

Albert Astals Cid (aacid) wrote :

 * 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.
enough

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

review: Approve
2009. By Michael Terry on 2015-10-27

Merge from trunk

Michael Terry (mterry) wrote :

Reports of this being unreliable seem true. Looks like Qt timestamps behave oddly. Am looking into it.

Michael Terry (mterry) wrote :

The unreliability was due to bug 1511076. Using branch https://code.launchpad.net/~mterry/qtmir/ms-compressed-timestamps/+merge/276048 fixes the problem in my testing. Will set this back to Approved, since no changes were required to this branch.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'plugins/Utils/windowkeysfilter.cpp'
2--- plugins/Utils/windowkeysfilter.cpp 2014-09-08 15:07:24 +0000
3+++ plugins/Utils/windowkeysfilter.cpp 2015-10-27 14:54:18 +0000
4@@ -21,7 +21,8 @@
5 #include <QQuickWindow>
6
7 WindowKeysFilter::WindowKeysFilter(QQuickItem *parent)
8- : QQuickItem(parent)
9+ : QQuickItem(parent),
10+ m_currentEventTimestamp(0)
11 {
12 connect(this, &QQuickItem::windowChanged,
13 this, &WindowKeysFilter::setupFilterOnWindow);
14@@ -36,7 +37,15 @@
15 if (event->type() == QEvent::KeyPress || event->type() == QEvent::KeyRelease) {
16 // Let QML see this event and decide if it does not want it
17 event->accept();
18+
19+ m_currentEventTimestamp = static_cast<QInputEvent*>(event)->timestamp();
20+ Q_EMIT currentEventTimestampChanged();
21+
22 QCoreApplication::sendEvent(this, event);
23+
24+ m_currentEventTimestamp = 0;
25+ Q_EMIT currentEventTimestampChanged();
26+
27 return event->isAccepted();
28 } else {
29 // Not interested
30@@ -56,3 +65,8 @@
31 m_filteredWindow = window;
32 }
33 }
34+
35+ulong WindowKeysFilter::currentEventTimestamp() const
36+{
37+ return m_currentEventTimestamp;
38+}
39
40=== modified file 'plugins/Utils/windowkeysfilter.h'
41--- plugins/Utils/windowkeysfilter.h 2015-04-30 09:31:51 +0000
42+++ plugins/Utils/windowkeysfilter.h 2015-10-27 14:54:18 +0000
43@@ -36,16 +36,23 @@
44 class WindowKeysFilter : public QQuickItem
45 {
46 Q_OBJECT
47+ Q_PROPERTY(ulong currentEventTimestamp READ currentEventTimestamp NOTIFY currentEventTimestampChanged)
48 public:
49 WindowKeysFilter(QQuickItem *parent = 0);
50
51 bool eventFilter(QObject *watched, QEvent *event) override;
52
53+ ulong currentEventTimestamp() const;
54+
55+Q_SIGNALS:
56+ void currentEventTimestampChanged();
57+
58 private Q_SLOTS:
59 void setupFilterOnWindow(QQuickWindow *window);
60
61 private:
62 QPointer<QQuickWindow> m_filteredWindow;
63+ ulong m_currentEventTimestamp;
64 };
65
66 #endif // UNITY_WINDOWKEYSFILTER_H
67
68=== modified file 'qml/Components/PhysicalKeysMapper.qml'
69--- qml/Components/PhysicalKeysMapper.qml 2015-09-02 09:30:32 +0000
70+++ qml/Components/PhysicalKeysMapper.qml 2015-10-27 14:54:18 +0000
71@@ -57,27 +57,20 @@
72
73 property bool altPressed: false
74 property bool altTabPressed: false
75- }
76-
77- Timer {
78- id: powerKeyLongPressTimer
79- interval: root.powerKeyLongPressTime
80- onTriggered: root.powerKeyLongPressed();
81- }
82-
83- function onKeyPressed(event) {
84- if ((event.key == Qt.Key_PowerDown || event.key == Qt.Key_PowerOff)
85- && (!event.isAutoRepeat || !powerKeyLongPressTimer.running)) {
86-
87- // FIXME: We only consider power key presses if the screen is
88- // on because of bugs 1410830/1409003. The theory is that when
89- // those bugs are encountered, there is a >2s delay between the
90- // power press event and the power release event, which causes
91- // the shutdown dialog to appear on resume. So to avoid that
92- // symptom while we investigate the root cause, we simply won't
93- // initiate any dialogs when the screen is off.
94- if (Powerd.status === Powerd.On) {
95- powerKeyLongPressTimer.restart();
96+
97+ property var powerButtonPressStart: 0
98+ }
99+
100+ function onKeyPressed(event, currentEventTimestamp) {
101+ if (event.key == Qt.Key_PowerDown || event.key == Qt.Key_PowerOff) {
102+ if (event.isAutoRepeat) {
103+ if (d.powerButtonPressStart > 0
104+ && currentEventTimestamp - d.powerButtonPressStart >= powerKeyLongPressTime) {
105+ d.powerButtonPressStart = 0;
106+ root.powerKeyLongPressed();
107+ }
108+ } else {
109+ d.powerButtonPressStart = currentEventTimestamp;
110 }
111 } else if ((event.key == Qt.Key_MediaTogglePlayPause || event.key == Qt.Key_MediaPlay) && !event.isAutoRepeat) {
112 event.accepted = callManager.handleMediaKey(false);
113@@ -113,9 +106,9 @@
114 }
115 }
116
117- function onKeyReleased(event) {
118+ function onKeyReleased(event, currentEventTimestamp) {
119 if (event.key == Qt.Key_PowerDown || event.key == Qt.Key_PowerOff) {
120- powerKeyLongPressTimer.stop();
121+ d.powerButtonPressStart = 0;
122 event.accepted = true;
123 } else if (event.key == Qt.Key_VolumeDown) {
124 if (!d.ignoreVolumeEvents) root.volumeDownTriggered();
125
126=== modified file 'qml/Shell.qml'
127--- qml/Shell.qml 2015-10-16 17:11:54 +0000
128+++ qml/Shell.qml 2015-10-27 14:54:18 +0000
129@@ -172,8 +172,8 @@
130 }
131
132 WindowKeysFilter {
133- Keys.onPressed: physicalKeysMapper.onKeyPressed(event);
134- Keys.onReleased: physicalKeysMapper.onKeyReleased(event);
135+ Keys.onPressed: physicalKeysMapper.onKeyPressed(event, currentEventTimestamp);
136+ Keys.onReleased: physicalKeysMapper.onKeyReleased(event, currentEventTimestamp);
137 }
138
139 HomeKeyWatcher {
140
141=== modified file 'tests/qmltests/Components/tst_PhysicalKeysMapper.qml'
142--- tests/qmltests/Components/tst_PhysicalKeysMapper.qml 2015-09-15 12:05:17 +0000
143+++ tests/qmltests/Components/tst_PhysicalKeysMapper.qml 2015-10-27 14:54:18 +0000
144@@ -67,25 +67,26 @@
145 Powerd.setStatus(data.status, Powerd.Unknown);
146 physicalKeysMapper.powerKeyLongPressTime = 500;
147
148- physicalKeysMapper.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: false });
149+ physicalKeysMapper.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: false }, 100);
150
151 // After the first key press the screen is always on
152 // and the rest of keypresses are auto repeat
153 Powerd.setStatus(Powerd.On, Powerd.Unknown);
154
155- for (var i = 0; i < 3; ++i) {
156- wait(10);
157- physicalKeysMapper.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: true});
158- }
159+ physicalKeysMapper.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: true}, 300);
160+ physicalKeysMapper.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: true}, 400);
161+ physicalKeysMapper.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: true}, 500);
162+ physicalKeysMapper.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: true}, 599);
163
164 // powerKeyLongPressed should not have been emitted yet.
165 compare(powerSpy.count, 0);
166
167- for (var i = 0; i < 10; ++i) {
168- wait(50);
169- physicalKeysMapper.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: true});
170- }
171-
172+ physicalKeysMapper.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: true}, 600);
173+
174+ compare(powerSpy.count, 1);
175+
176+ // Confirm we only emit once
177+ physicalKeysMapper.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: true}, 601);
178 compare(powerSpy.count, 1);
179 }
180

Subscribers

People subscribed via source and target branches