Merge lp:~dandrader/unity-mir/improve_osk_ipc into lp:unity-mir

[Daniel d'Andrada]

Make UbuntuKeyboardInfo more robust

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:114
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/105/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-saucy-amd64-ci/47
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-saucy-armhf-ci/105
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-saucy-armhf-ci/105/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-saucy-i386-ci/105

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/unity-mir-ci/105/rebuild

review: Approve (continuous-integration)

[Michał Sawicz (saviq)]

Can we please use $XDG_RUNTIME_DIR and mode 600 for this? Or Jamie will eat us alive ;)

IIUC, this will still bail if the connection isn't available within 50s. It might be good enough, but if there's an easy way to get notified on changes to the socket, maybe we should add it? So that if it shows up after the 50s, we will try again?

review: Needs Fixing

[Michał Sawicz (saviq)]

I stopped maliit-server, but the keyboard dimensions were not reset - can't use the bottom part of the shell...

review: Needs Fixing

[Michał Sawicz (saviq)]

Stopped when it was on screen, btw.

[Michał Sawicz (saviq)]

It also seems much more difficult to dismiss the keyboard - the 2GU or something, at the top of the keyboard, that caused the original bug, were probably meant for this?

review: Needs Information

[Daniel d'Andrada (dandrader)]

> I stopped maliit-server, but the keyboard dimensions were not reset - can't
> use the bottom part of the shell... Stopped when it was on screen, btw.

That's an unrelated bug.

There are two different kinds of OSK information handled by unity8:

1 - the OSK geometry, which is what being dealt with here
2 - whether the OSK is visible or not

They are orthogonal to each other. So the problem when you kill maliit-server while the OSK is being displayed is that unity8-mir somehow doesn't notice or gets notified of its disappearance and hence doesn't disable OSK's InputFilterArea. Let's not mix bugs!

[Michał Sawicz (saviq)]

On 11.10.2013 15:54, Daniel d'Andrada wrote:
> They are orthogonal to each other. So the problem when you kill maliit-server while the OSK is being displayed is that unity8-mir somehow doesn't notice or gets notified of its disappearance and hence doesn't disable OSK's InputFilterArea. Let's not mix bugs!

Of course, sorry - that's Qt.inputMethod then.

[Daniel d'Andrada (dandrader)]

> IIUC, this will still bail if the connection isn't available within 50s.

Right.

> It might be good enough, but if there's an easy way to get notified on changes to
> the socket, maybe we should add it?

The only thing I can think of is watching (through inotify) the directory where the socket file is expected to appear. But that will wake up unity8 for every file change in this directory and not only the socket one.

> So that if it shows up after the 50s, we
> will try again?

I think we could simply throttle the retry frequency as time passes (frequent at first and occasionally later on) and finally give up at a much later point in time (say, 30 minutes).

[Michał Sawicz (saviq)]

On 11.10.2013 16:16, Daniel d'Andrada wrote:
>> IIUC, this will still bail if the connection isn't available within 50s.
> Right.
>
>> >It might be good enough, but if there's an easy way to get notified on changes to
>> >the socket, maybe we should add it?
> The only thing I can think of is watching (through inotify) the directory where the socket file is expected to appear. But that will wake up unity8 for every file change in this directory and not only the socket one.
>
>> >So that if it shows up after the 50s, we
>> >will try again?
> I think we could simply throttle the retry frequency as time passes (frequent at first and occasionally later on) and finally give up at a much later point in time (say, 30 minutes).

Actually after a fix in upstart jobs, maliit-server will *always* be
running when unity8 is, so we should be good there. Let's leave as is.

[lp:~dandrader/unity-mir/improve_osk_ipc]

115. By Daniel d'Andrada on 2013-10-11

Use XDG_RUNTIME_DIR instead of /tmp

[Daniel d'Andrada (dandrader)]

> Can we please use $XDG_RUNTIME_DIR and mode 600 for this? Or Jamie will eat us
> alive ;)

Done.

[Michał Sawicz (saviq)]

review: Approve

=== modified file 'src/modules/Unity/Application/ubuntukeyboardinfo.cpp'
--- src/modules/Unity/Application/ubuntukeyboardinfo.cpp	2013-10-09 15:24:16 +0000
+++ src/modules/Unity/Application/ubuntukeyboardinfo.cpp	2013-10-11 15:35:13 +0000
@@ -16,58 +16,85 @@
 
 #include "ubuntukeyboardinfo.h"
 
-#include <QTimer>
+#include <QDir>
 
 namespace {
     const int gConnectionAttemptIntervalMs = 5000;
-    const int gMaxFailedAttempts = 10;
+    const int gMaxConsecutiveAttempts = 10;
     const char gServerName[] = "ubuntu-keyboard-info";
 }
 
 UbuntuKeyboardInfo::UbuntuKeyboardInfo(QObject *parent)
     : QObject(parent),
-    m_failedAttempts(0),
+    m_consecutiveAttempts(0),
     m_lastWidth(0),
     m_lastHeight(0)
 {
-    connect(&m_socket, &QLocalSocket::connected, this, &UbuntuKeyboardInfo::onConnected);
-    connect(&m_socket, &QLocalSocket::disconnected, this, &UbuntuKeyboardInfo::onDisconnected);
+    connect(&m_socket, &QLocalSocket::stateChanged, this, &UbuntuKeyboardInfo::onSocketStateChanged);
     connect(&m_socket, &QIODevice::readyRead,
             this, &UbuntuKeyboardInfo::readAllBytesFromSocket);
 
+    buildSocketFilePath();
+
     typedef void (QLocalSocket::*MemberFunctionType)(QLocalSocket::LocalSocketError);
     MemberFunctionType funcPointer = &QLocalSocket::error;
     connect(&m_socket, funcPointer,
             this, &UbuntuKeyboardInfo::onSocketError);
 
+    m_connectionRetryTimer.setInterval(gConnectionAttemptIntervalMs);
+    m_connectionRetryTimer.setSingleShot(true);
+    connect(&m_connectionRetryTimer, &QTimer::timeout,
+            this, &UbuntuKeyboardInfo::tryConnectingToServer);
     tryConnectingToServer();
 }
 
+UbuntuKeyboardInfo::~UbuntuKeyboardInfo()
+{
+    // Make sure we don't get onSocketStateChanged() called during
+    // destruction.
+    m_socket.disconnect(this);
+}
+
 void UbuntuKeyboardInfo::tryConnectingToServer()
 {
-    m_socket.connectToServer(gServerName, QIODevice::ReadOnly);
-}
-
-void UbuntuKeyboardInfo::onConnected()
-{
-    m_failedAttempts = 0;
-}
-
-void UbuntuKeyboardInfo::onDisconnected()
-{
-    qWarning("UbuntuKeyboardInfo - disconnected");
+    ++m_consecutiveAttempts;
+    Q_ASSERT(!m_socketFilePath.isEmtpy());
+    m_socket.connectToServer(m_socketFilePath, QIODevice::ReadOnly);
+}
+
+void UbuntuKeyboardInfo::onSocketStateChanged(QLocalSocket::LocalSocketState socketState)
+{
+    switch (socketState) {
+    case QLocalSocket::UnconnectedState:
+        retryConnection();
+        break;
+    case QLocalSocket::ConnectedState:
+        m_consecutiveAttempts = 0;
+        break;
+    default:
+        break;
+    }
 }
 
 void UbuntuKeyboardInfo::onSocketError(QLocalSocket::LocalSocketError socketError)
 {
+    Q_UNUSED(socketError);
+    qWarning() << "UbuntuKeyboardInfo - socket error:" << m_socket.errorString();
+}
+
+void UbuntuKeyboardInfo::retryConnection()
+{
     // Polling every gConnectionAttemptIntervalMs. Not the best approach but could be worse.
-    Q_UNUSED(socketError);
-    ++m_failedAttempts;
-    if (m_failedAttempts < gMaxFailedAttempts) {
-        QTimer::singleShot(gConnectionAttemptIntervalMs, this, SLOT(tryConnectingToServer()));
+    if (m_consecutiveAttempts < gMaxConsecutiveAttempts) {
+        if (!m_connectionRetryTimer.isActive()) {
+            m_connectionRetryTimer.start();
+        }
     } else {
-        qCritical() << "Failed to connect to" << gServerName << "after"
-                    << m_failedAttempts << "failed attempts";
+        qCritical() << "Failed to connect to" << m_socketFilePath << "after"
+                    << m_consecutiveAttempts << "failed attempts";
+
+        // it shouldn't be running, but just to be sure.
+        m_connectionRetryTimer.stop();
     }
 }
 
@@ -101,3 +128,15 @@
         }
     }
 }
+
+void UbuntuKeyboardInfo::buildSocketFilePath()
+{
+    char *xdgRuntimeDir = getenv("XDG_RUNTIME_DIR");
+
+    if (xdgRuntimeDir) {
+        m_socketFilePath = QDir(xdgRuntimeDir).filePath(gServerName);
+    } else {
+        m_socketFilePath = QDir("/tmp").filePath(gServerName);
+    }
+
+}

=== modified file 'src/modules/Unity/Application/ubuntukeyboardinfo.h'
--- src/modules/Unity/Application/ubuntukeyboardinfo.h	2013-10-09 15:24:16 +0000
+++ src/modules/Unity/Application/ubuntukeyboardinfo.h	2013-10-11 15:35:13 +0000
@@ -18,6 +18,7 @@
 #define UBUNTU_KEYBOARD_INFO_H
 
 #include <QLocalSocket>
+#include <QTimer>
 
 // Temporary solution to get information about the onscreen keyboard
 // This shouldn't be needed once the OSK is a properly sized surface
@@ -28,6 +29,7 @@
     Q_PROPERTY(qreal height READ height NOTIFY heightChanged)
 public:
     UbuntuKeyboardInfo(QObject *parent = 0);
+    virtual ~UbuntuKeyboardInfo();
     qreal width() const { return m_lastWidth; }
     qreal height() const { return m_lastHeight; }
 
@@ -37,8 +39,7 @@
 
 private Q_SLOTS:
     void tryConnectingToServer();
-    void onConnected();
-    void onDisconnected();
+    void onSocketStateChanged(QLocalSocket::LocalSocketState socketState);
     void onSocketError(QLocalSocket::LocalSocketError socketError);
     void readAllBytesFromSocket();
 
@@ -50,12 +51,18 @@
         qint32 keyboardHeight;
     };
     void readInfoFromSocket();
+    void retryConnection();
+    void buildSocketFilePath();
 
-    int m_failedAttempts;
+    int m_consecutiveAttempts;
 
     QLocalSocket m_socket;
     qint32 m_lastWidth;
     qint32 m_lastHeight;
+    QTimer m_connectionRetryTimer;
+
+    // Path to the socket file created by ubuntu-keyboard
+    QString m_socketFilePath;
 };
 
 #endif // UBUNTU_KEYBOARD_INFO_H