Unity 8

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

superKeyPressFix
Merge into trunk

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

Status:

Merged

Approved by:

Daniel d'Andrada on 2016-11-29

Approved revision:

2662

Merged at revision:

2727

Proposed branch:

lp:~lukas-kde/unity8/superKeyPressFix

Merge into:

lp:unity8

Diff against target:

244 lines (+87/-18)

4 files modified

plugins/Utils/ElapsedTimer.h (+1/-1)
plugins/Utils/Timer.cpp (+5/-2)
plugins/Utils/Timer.h (+5/-6)
tests/plugins/Utils/WindowInputMonitorTest.cpp (+76/-9)

To merge this branch:

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

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Unity8 CI Bot	continuous-integration		Approve on 2016-11-28
Albert Astals Cid (community)		2016-10-07	Abstain on 2016-11-28
Daniel d'Andrada (community)			Approve on 2016-10-10
Review via email: mp+307977@code.launchpad.net

Commit Message

Fix the Super key not invoking the dash scope home

Description of the Change

Fix the Super key not invoking the dash scope home

lp:1607427 - super key not showing scopes home

Do not lie about a timer running, when it's not

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

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

review: Needs Fixing (continuous-integration)

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

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

review: Needs Fixing (continuous-integration)

Albert Astals Cid (aacid) wrote on 2016-10-10:

I don't see how this makes a difference

Previously we had single shot, which means the timeout signal gets emitted and then it stops itself, so basically we get emitActivatedIfNoTouchesAround called once.

Now we have no single show but we still stop the timer when emitActivatedIfNoTouchesAround gets called, which is only from the timeout signal of the timer.

So basically the same is happening before and after the patch?

review: Needs Information

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

Yeah, I also don't get it. A timer being singleshot means that it times out only once and then stops itself. Unlike a regular timer which will timeout every QTimer::interval() milliseconds until you manually stop() it.

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

Yeah, the core of this problem seems to be the fact that even a single shot timer, although it fires only once, is never stopped.

The fake timer was doing a manual stop() in that case; if you remove this discrepancy, then the regression test passes with this branch, and fails in the real environment - resulting in the bug, as described in lp:1607427.

In other words, the functionality works exactly once - and then stops working. The key press handler has this:

...
&& !m_activationTimer->isRunning()

meaning that if the timer is never stopped, it will never register any subsequent Super key presses.

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

Also, without this branch, any long press of the Super key would break this

Albert Astals Cid (aacid) wrote on 2016-10-10:

I think the problem lies in the Timer wrapping, i'll suggest a different MR in a few minutes

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

On 10/10/2016 10:33, Lukáš Tinkl wrote:
> Yeah, the core of this problem seems to be the fact that even a single shot timer, although it fires only once, is never stopped.
>
> The fake timer was doing a manual stop() in that case; if you remove this discrepancy, then the regression test passes with this branch, and fails in the real environment - resulting in the bug, as described in lp:1607427.
>
> In other words, the functionality works exactly once - and then stops working. The key press handler has this:
>
> ...
> && !m_activationTimer->isRunning()
>
> meaning that if the timer is never stopped, it will never register any subsequent Super key presses.

That's all very strange. qtimer.cpp has this:

"""

void QTimer::timerEvent(QTimerEvent *e)
{
     if (e->timerId() == id) {
         if (single)
             stop();
         emit timeout(QPrivateSignal());
     }
}

"""

ie, a singleshot timer stops itself on timeout...

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

> On 10/10/2016 10:33, Lukáš Tinkl wrote:
> > Yeah, the core of this problem seems to be the fact that even a single shot
> timer, although it fires only once, is never stopped.
> >
> > The fake timer was doing a manual stop() in that case; if you remove this
> discrepancy, then the regression test passes with this branch, and fails in
> the real environment - resulting in the bug, as described in lp:1607427.
> >
> > In other words, the functionality works exactly once - and then stops
> working. The key press handler has this:
> >
> > ...
> > && !m_activationTimer->isRunning()
> >
> > meaning that if the timer is never stopped, it will never register any
> subsequent Super key presses.
>
>
> That's all very strange. qtimer.cpp has this:
>
> """
>
> void QTimer::timerEvent(QTimerEvent *e)
> {
> if (e->timerId() == id) {
> if (single)
> stop();
> emit timeout(QPrivateSignal());
> }
> }
>
> """
>
>
> ie, a singleshot timer stops itself on timeout...

Yeah, but our (your :p) Utils/Timer.cpp doesn't and that's the problem really

Albert Astals Cid (aacid) wrote on 2016-10-10:

I think this is a better fix, actually fixing Timer::isRunning not to lie

http://paste.ubuntu.com/23303197/

What do you think?

Albert Astals Cid (aacid) wrote on 2016-10-10:

Forgot to initialize m_isRunning http://paste.ubuntu.com/23303206/

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

> Forgot to initialize m_isRunning http://paste.ubuntu.com/23303206/

Now that makes sense. Proper fix.

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

Pushed Albert's change

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

Looks good to me.

* Did you perform an exploratory manual test run of the code change and any related functionality?
No, but fix is simple, and so is the code being fixed. That, plus the regression test, makes me confident enough to approve it already.

* Did CI run pass? If not, please explain why.
No results yet. May reassess once it comes.

review: Approve

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

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

review: Needs Fixing (continuous-integration)

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

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

review: Needs Fixing (continuous-integration)

Albert Astals Cid (aacid) wrote on 2016-11-28:

Abstaining since code is partially mine

review: Abstain

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

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

review: Needs Fixing (continuous-integration)

lp:~lukas-kde/unity8/superKeyPressFix updated on 2016-11-28

2662. By Lukáš Tinkl on 2016-11-28: don't return garbage when the timer isn't running, or never been started

Lukáš Tinkl (lukas-kde) wrote on 2016-11-28:

Submitted a fix for what I think is the culprit why it can occasionally break
(quite visible when testing the appDrawer branch):

QElapsedTimer::elapsed() returns garbage when the timer has never been started or not running

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

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

review: Approve (continuous-integration)

lp:~lukas-kde/unity8/superKeyPressFix updated on 2016-11-29

2663. By Lukáš Tinkl on 2016-11-29: make the FakeElapsedTimer more realistic; adapt the test

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 'plugins/Utils/ElapsedTimer.h'
 --- plugins/Utils/ElapsedTimer.h	2015-04-22 10:40:54 +0000
 +++ plugins/Utils/ElapsedTimer.h	2016-11-29 15:38:24 +0000
@@ -41,7 +41,7 @@
  public:
      void start() override { m_timer.start(); }
      qint64 msecsSinceReference() const override { return m_timer.msecsSinceReference(); }
--    qint64 elapsed() const override { return m_timer.elapsed(); }
++    qint64 elapsed() const override { return m_timer.isValid() ? m_timer.elapsed() : 0; }
  private:
      QElapsedTimer m_timer;
  };
 === modified file 'plugins/Utils/Timer.cpp'
 --- plugins/Utils/Timer.cpp	2015-04-22 10:40:54 +0000
 +++ plugins/Utils/Timer.cpp	2016-11-29 15:38:24 +0000
@@ -37,13 +37,16 @@
  void Timer::start()
+ {
      m_timer.start();
--    AbstractTimer::start();
+ }
  void Timer::stop()
+ {
      m_timer.stop();
--    AbstractTimer::stop();
++}
++
++bool Timer::isRunning() const
++{
++    return m_timer.isActive();
+ }
  bool Timer::isSingleShot() const
 === modified file 'plugins/Utils/Timer.h'
 --- plugins/Utils/Timer.h	2015-04-22 10:40:54 +0000
 +++ plugins/Utils/Timer.h	2016-11-29 15:38:24 +0000
@@ -30,18 +30,16 @@
+ {
      Q_OBJECT
  public:
--    AbstractTimer(QObject *parent) : QObject(parent), m_isRunning(false) {}
++    AbstractTimer(QObject *parent) : QObject(parent) {}
      virtual int interval() const = 0;
      virtual void setInterval(int msecs) = 0;
--    virtual void start() { m_isRunning = true; }
--    virtual void stop() { m_isRunning = false; }
--    bool isRunning() const { return m_isRunning; }
++    virtual void start() = 0;
++    virtual void stop() = 0;
++    virtual bool isRunning() const = 0;
      virtual bool isSingleShot() const = 0;
      virtual void setSingleShot(bool value) = 0;
  Q_SIGNALS:
      void timeout();
--private:
--    bool m_isRunning;
  };
  /** A QTimer wrapper */
@@ -55,6 +53,7 @@
      void setInterval(int msecs) override;
      void start() override;
      void stop() override;
++    bool isRunning() const override;
      bool isSingleShot() const override;
      void setSingleShot(bool value) override;
  private:
 === modified file 'tests/plugins/Utils/WindowInputMonitorTest.cpp'
 --- tests/plugins/Utils/WindowInputMonitorTest.cpp	2016-03-21 19:35:46 +0000
 +++ tests/plugins/Utils/WindowInputMonitorTest.cpp	2016-11-29 15:38:24 +0000
@@ -25,14 +25,15 @@
  public:
      static qint64 msecsSinceEpoch;
--    FakeElapsedTimer() { start(); }
++    FakeElapsedTimer() {}
--    void start() override { m_msecsSinceReference = msecsSinceEpoch; }
++    void start() override { m_msecsSinceReference = msecsSinceEpoch; m_valid = true; }
      qint64 msecsSinceReference() const override { return m_msecsSinceReference; }
--    qint64 elapsed() const override { return msecsSinceEpoch - m_msecsSinceReference; }
++    qint64 elapsed() const override { return m_valid ? msecsSinceEpoch - m_msecsSinceReference : qrand(); }
  private:
      qint64 m_msecsSinceReference;
++    bool m_valid{false};
  };
  qint64 FakeElapsedTimer::msecsSinceEpoch = 0;
@@ -48,11 +49,14 @@
      int interval() const override;
      void setInterval(int msecs) override;
      void start() override;
++    void stop() override;
++    bool isRunning() const override;
      bool isSingleShot() const override;
      void setSingleShot(bool value) override;
  private:
      int m_interval;
      bool m_singleShot;
++    bool m_isRunning;
      qint64 m_nextTimeoutTime;
  };
@@ -85,6 +89,8 @@
      void tapWhileTouching();
      void multipleHomeKeys();
++    void repeatedSuperPress();
++
  private:
      void passTime(qint64 timeSpanMs);
@@ -98,11 +104,10 @@
  void WindowInputMonitorTest::cleanup()
+ {
--     delete m_fakeTimerFactory;
++    delete m_fakeTimerFactory;
      m_fakeTimerFactory = nullptr;
+ }
--
  void WindowInputMonitorTest::passTime(qint64 timeSpanMs)
+ {
      qint64 finalTime = FakeElapsedTimer::msecsSinceEpoch + timeSpanMs;
@@ -133,7 +138,7 @@
      QFETCH(int, silenceAfterTap);
      QFETCH(int, expectedActivatedCount);
      QFETCH(Qt::Key, key);
--    WindowInputMonitor homeKeyWatcher(m_fakeTimerFactory->create(), new FakeElapsedTimer);
++    WindowInputMonitor homeKeyWatcher(m_fakeTimerFactory->create(this), new FakeElapsedTimer);
      QSignalSpy activatedSpy(&homeKeyWatcher, &WindowInputMonitor::homeKeyActivated);
      QVERIFY(activatedSpy.isValid());
@@ -194,7 +199,7 @@
  void WindowInputMonitorTest::tapWhileTouching()
+ {
--    WindowInputMonitor homeKeyWatcher(m_fakeTimerFactory->create(), new FakeElapsedTimer);
++    WindowInputMonitor homeKeyWatcher(m_fakeTimerFactory->create(this), new FakeElapsedTimer);
      QSignalSpy activatedSpy(&homeKeyWatcher, &WindowInputMonitor::homeKeyActivated);
      QVERIFY(activatedSpy.isValid());
@@ -229,7 +234,7 @@
   */
  void WindowInputMonitorTest::multipleHomeKeys()
+ {
--    WindowInputMonitor homeKeyWatcher(m_fakeTimerFactory->create(), new FakeElapsedTimer);
++    WindowInputMonitor homeKeyWatcher(m_fakeTimerFactory->create(this), new FakeElapsedTimer);
      QSignalSpy activatedSpy(&homeKeyWatcher, &WindowInputMonitor::homeKeyActivated);
      QVERIFY(activatedSpy.isValid());
@@ -258,12 +263,64 @@
      QCOMPARE(activatedSpy.count(), 1);
+ }
++// regression test for lp:1607427
++void WindowInputMonitorTest::repeatedSuperPress()
++{
++    WindowInputMonitor homeKeyWatcher(m_fakeTimerFactory->create(this), new FakeElapsedTimer);
++    QSignalSpy activatedSpy(&homeKeyWatcher, &WindowInputMonitor::homeKeyActivated);
++    QVERIFY(activatedSpy.isValid());
++
++    // 1st try
++    passTime(1000);
++    {
++        QKeyEvent keyEvent(QEvent::KeyPress, Qt::Key_Super_L, Qt::NoModifier);
++        homeKeyWatcher.update(&keyEvent);
++    }
++    passTime(50);
++    {
++        QKeyEvent keyEvent(QEvent::KeyRelease, Qt::Key_Super_L, Qt::NoModifier);
++        homeKeyWatcher.update(&keyEvent);
++    }
++    passTime(1000);
++    QCOMPARE(activatedSpy.count(), 1);
++
++    // a touch event in between
++    {
++        QTouchEvent touchEvent(QEvent::TouchBegin);
++        homeKeyWatcher.update(&touchEvent);
++    }
++    {
++        QTouchEvent touchEvent(QEvent::TouchUpdate);
++        homeKeyWatcher.update(&touchEvent);
++    }
++    {
++        QTouchEvent touchEvent(QEvent::TouchEnd);
++        homeKeyWatcher.update(&touchEvent);
++    }
++
++    passTime(1000);
++    // 2nd try
++    activatedSpy.clear();
++    {
++        QKeyEvent keyEvent(QEvent::KeyPress, Qt::Key_Super_L, Qt::NoModifier);
++        homeKeyWatcher.update(&keyEvent);
++    }
++    passTime(50);
++    {
++        QKeyEvent keyEvent(QEvent::KeyRelease, Qt::Key_Super_L, Qt::NoModifier);
++        homeKeyWatcher.update(&keyEvent);
++    }
++    passTime(1000);
++    QCOMPARE(activatedSpy.count(), 1);
++}
++
  /////////////////////////////////// FakeTimer //////////////////////////////////
  FakeTimer::FakeTimer(QObject *parent)
      : UnityUtil::AbstractTimer(parent)
      , m_interval(0)
      , m_singleShot(true)
++    , m_isRunning(false)
+ {
+ }
@@ -285,10 +342,20 @@
  void FakeTimer::start()
+ {
--    AbstractTimer::start();
++    m_isRunning = true;
      m_nextTimeoutTime = FakeElapsedTimer::msecsSinceEpoch + (qint64)interval();
+ }
++void FakeTimer::stop()
++{
++    m_isRunning = false;
++}
++
++bool FakeTimer::isRunning() const
++{
++    return m_isRunning;
++}
++
  int FakeTimer::interval() const
+ {
      return m_interval;