Unity 8

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

shutdown-dialog-on-resume
Merge into trunk

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

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	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

PS Jenkins bot (ps-jenkins) wrote on 2015-10-22:

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 on 2015-10-22:

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 on 2015-10-22:

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 on 2015-10-22:

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 on 2015-10-22:

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 on 2015-10-22:

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 on 2015-10-22:

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 on 2015-10-22:

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) on 2015-10-22:

review: Needs Information

Michael Terry (mterry) wrote on 2015-10-22:

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

lp:~mterry/unity8/shutdown-dialog-on-resume updated on 2015-10-22

2008. By Michael Terry on 2015-10-22: remove an obsolete code change

PS Jenkins bot (ps-jenkins) wrote on 2015-10-23:

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 on 2015-10-23:

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 on 2015-10-23:

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

Albert Astals Cid (aacid) wrote on 2015-10-23:

* 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

lp:~mterry/unity8/shutdown-dialog-on-resume updated on 2015-10-27

2009. By Michael Terry on 2015-10-27: Merge from trunk

PS Jenkins bot (ps-jenkins) wrote on 2015-10-27:

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

review: Needs Fixing (continuous-integration)

Michael Terry (mterry) wrote on 2015-10-28:

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

Michael Terry (mterry) wrote on 2015-10-28:

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

Subscribers

People subscribed via source and target branches

to all changes:

Michael Terry

Mohammad Rakibul Hasan

Unity Team

 === modified file 'plugins/Utils/windowkeysfilter.cpp'
 --- plugins/Utils/windowkeysfilter.cpp	2014-09-08 15:07:24 +0000
 +++ plugins/Utils/windowkeysfilter.cpp	2015-10-27 14:54:18 +0000
@@ -21,7 +21,8 @@
  #include <QQuickWindow>
  WindowKeysFilter::WindowKeysFilter(QQuickItem *parent)
--    : QQuickItem(parent)
++    : QQuickItem(parent),
++      m_currentEventTimestamp(0)
+ {
      connect(this, &QQuickItem::windowChanged,
              this, &WindowKeysFilter::setupFilterOnWindow);
@@ -36,7 +37,15 @@
      if (event->type() == QEvent::KeyPress || event->type() == QEvent::KeyRelease) {
          // Let QML see this event and decide if it does not want it
          event->accept();
++
++        m_currentEventTimestamp = static_cast<QInputEvent*>(event)->timestamp();
++        Q_EMIT currentEventTimestampChanged();
++
          QCoreApplication::sendEvent(this, event);
++
++        m_currentEventTimestamp = 0;
++        Q_EMIT currentEventTimestampChanged();
++
          return event->isAccepted();
      } else {
          // Not interested
@@ -56,3 +65,8 @@
          m_filteredWindow = window;
+     }
+ }
++
++ulong WindowKeysFilter::currentEventTimestamp() const
++{
++    return m_currentEventTimestamp;
++}
 === modified file 'plugins/Utils/windowkeysfilter.h'
 --- plugins/Utils/windowkeysfilter.h	2015-04-30 09:31:51 +0000
 +++ plugins/Utils/windowkeysfilter.h	2015-10-27 14:54:18 +0000
@@ -36,16 +36,23 @@
  class WindowKeysFilter : public QQuickItem
+ {
      Q_OBJECT
++    Q_PROPERTY(ulong currentEventTimestamp READ currentEventTimestamp NOTIFY currentEventTimestampChanged)
  public:
      WindowKeysFilter(QQuickItem *parent = 0);
      bool eventFilter(QObject *watched, QEvent *event) override;
++    ulong currentEventTimestamp() const;
++
++Q_SIGNALS:
++    void currentEventTimestampChanged();
++
  private Q_SLOTS:
      void setupFilterOnWindow(QQuickWindow *window);
  private:
      QPointer<QQuickWindow> m_filteredWindow;
++    ulong m_currentEventTimestamp;
  };
  #endif // UNITY_WINDOWKEYSFILTER_H
 === modified file 'qml/Components/PhysicalKeysMapper.qml'
 --- qml/Components/PhysicalKeysMapper.qml	2015-09-02 09:30:32 +0000
 +++ qml/Components/PhysicalKeysMapper.qml	2015-10-27 14:54:18 +0000
@@ -57,27 +57,20 @@
          property bool altPressed: false
          property bool altTabPressed: false
--    }
--
--    Timer {
--        id: powerKeyLongPressTimer
--        interval: root.powerKeyLongPressTime
--        onTriggered: root.powerKeyLongPressed();
--    }
--
--    function onKeyPressed(event) {
--        if ((event.key == Qt.Key_PowerDown || event.key == Qt.Key_PowerOff)
--            && (!event.isAutoRepeat || !powerKeyLongPressTimer.running)) {
--
--            // FIXME: We only consider power key presses if the screen is
--            // on because of bugs 1410830/1409003.  The theory is that when
--            // those bugs are encountered, there is a >2s delay between the
--            // power press event and the power release event, which causes
--            // the shutdown dialog to appear on resume.  So to avoid that
--            // symptom while we investigate the root cause, we simply won't
--            // initiate any dialogs when the screen is off.
--            if (Powerd.status === Powerd.On) {
--                powerKeyLongPressTimer.restart();
++
++        property var powerButtonPressStart: 0
++    }
++
++    function onKeyPressed(event, currentEventTimestamp) {
++        if (event.key == Qt.Key_PowerDown || event.key == Qt.Key_PowerOff) {
++            if (event.isAutoRepeat) {
++                if (d.powerButtonPressStart > 0
++                        && currentEventTimestamp - d.powerButtonPressStart >= powerKeyLongPressTime) {
++                    d.powerButtonPressStart = 0;
++                    root.powerKeyLongPressed();
++                }
++            } else {
++                d.powerButtonPressStart = currentEventTimestamp;
+             }
          } else if ((event.key == Qt.Key_MediaTogglePlayPause || event.key == Qt.Key_MediaPlay) && !event.isAutoRepeat) {
              event.accepted = callManager.handleMediaKey(false);
@@ -113,9 +106,9 @@
+         }
+     }
--    function onKeyReleased(event) {
++    function onKeyReleased(event, currentEventTimestamp) {
          if (event.key == Qt.Key_PowerDown || event.key == Qt.Key_PowerOff) {
--            powerKeyLongPressTimer.stop();
++            d.powerButtonPressStart = 0;
              event.accepted = true;
          } else if (event.key == Qt.Key_VolumeDown) {
              if (!d.ignoreVolumeEvents) root.volumeDownTriggered();
 === modified file 'qml/Shell.qml'
 --- qml/Shell.qml	2015-10-16 17:11:54 +0000
 +++ qml/Shell.qml	2015-10-27 14:54:18 +0000
@@ -172,8 +172,8 @@
+     }
      WindowKeysFilter {
--        Keys.onPressed: physicalKeysMapper.onKeyPressed(event);
--        Keys.onReleased: physicalKeysMapper.onKeyReleased(event);
++        Keys.onPressed: physicalKeysMapper.onKeyPressed(event, currentEventTimestamp);
++        Keys.onReleased: physicalKeysMapper.onKeyReleased(event, currentEventTimestamp);
+     }
      HomeKeyWatcher {
 === modified file 'tests/qmltests/Components/tst_PhysicalKeysMapper.qml'
 --- tests/qmltests/Components/tst_PhysicalKeysMapper.qml	2015-09-15 12:05:17 +0000
 +++ tests/qmltests/Components/tst_PhysicalKeysMapper.qml	2015-10-27 14:54:18 +0000
@@ -67,25 +67,26 @@
          Powerd.setStatus(data.status, Powerd.Unknown);
          physicalKeysMapper.powerKeyLongPressTime = 500;
--        physicalKeysMapper.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: false });
++        physicalKeysMapper.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: false }, 100);
          // After the first key press the screen is always on
          // and the rest of keypresses are auto repeat
          Powerd.setStatus(Powerd.On, Powerd.Unknown);
--        for (var i = 0; i < 3; ++i) {
--            wait(10);
--            physicalKeysMapper.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: true});
--        }
++        physicalKeysMapper.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: true}, 300);
++        physicalKeysMapper.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: true}, 400);
++        physicalKeysMapper.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: true}, 500);
++        physicalKeysMapper.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: true}, 599);
          // powerKeyLongPressed should not have been emitted yet.
          compare(powerSpy.count, 0);
--        for (var i = 0; i < 10; ++i) {
--            wait(50);
--            physicalKeysMapper.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: true});
--        }
--
++        physicalKeysMapper.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: true}, 600);
++
++        compare(powerSpy.count, 1);
++
++        // Confirm we only emit once
++        physicalKeysMapper.onKeyPressed({ key: Qt.Key_PowerDown, isAutoRepeat: true}, 601);
          compare(powerSpy.count, 1);
+     }