Merge lp:~dandrader/ubuntu-keyboard/improve_kbd_info_ipc into lp:ubuntu-keyboard

[Daniel d'Andrada]

Various improvements to ubuntu-keyboard-info socket server

Make it more robust

Would be good to have https://code.launchpad.net/~dandrader/unity-mir/improve_osk_ipc/+merge/190417 in as well.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:74
http://jenkins.qa.ubuntu.com/job/ubuntu-keyboard-ci/147/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-keyboard-saucy-amd64-ci/148
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-keyboard-saucy-armhf-ci/147
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-keyboard-saucy-armhf-ci/147/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-keyboard-saucy-i386-ci/147

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-keyboard-ci/147/rebuild

review: Approve (continuous-integration)

[Michał Sawicz (saviq)]

Only thing - please put the socket in mode 600 in $XDG_RUNTIME_DIR, as mentioned in https://code.launchpad.net/~dandrader/unity-mir/improve_osk_ipc/+merge/190417/comments/437227

review: Needs Fixing

[Günter Schwann (schwann)]

tests/dynamic-layout does not compile:
/home/phablet/ubuntu-keyboard/tests/dynamic-layout/../../src/plugin/libubuntu-keyboard-plugin.so: undefined reference to `vtable for UbuntuApplicationApiWrapper'

And when starting maliit-server, I get a crash:
WARNING: bool MIMPluginManagerPrivate::loadPlugin(const QDir&, const QString&) Error loading plugin from "/usr/lib/maliit/plugins/libubuntu-keyboard-plugin.so" "Cannot load library /usr/lib/maliit/plugins/libubuntu-keyboard-plugin.so: (/usr/lib/maliit/plugins/libubuntu-keyboard-plugin.so: undefined symbol: _ZTV27UbuntuApplicationApiWrapper)"

review: Needs Fixing

[Günter Schwann (schwann)]

My build and run error was a problem in the build script - solved.
Code does, apart from the comment from saviq, looking good.

review: Approve

[lp:~dandrader/ubuntu-keyboard/improve_kbd_info_ipc]

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

Use XDG_RUNTIME_DIR instead of /tmp

[Daniel d'Andrada (dandrader)]

> Only thing - please put the socket in mode 600 in $XDG_RUNTIME_DIR, as
> mentioned in https://code.launchpad.net/~dandrader/unity-
> mir/improve_osk_ipc/+merge/190417/comments/437227

Done.

[Michał Sawicz (saviq)]

review: Approve

=== modified file 'src/plugin/ubuntuapplicationapiwrapper.cpp'
--- src/plugin/ubuntuapplicationapiwrapper.cpp	2013-10-09 15:28:52 +0000
+++ src/plugin/ubuntuapplicationapiwrapper.cpp	2013-10-11 15:34:57 +0000
@@ -25,6 +25,8 @@
   #define HAVE_UBUNTU_PLATFORM_API
 #endif
 
+#include <QDir>
+#include <QFile>
 #include <QtGlobal>
 #include <QByteArray>
 
@@ -41,14 +43,34 @@
     }
 
     if (m_runningOnMir) {
-        connect(&m_localServer, &QLocalServer::newConnection,
-                this, &UbuntuApplicationApiWrapper::onNewConnection);
-        bool ok = m_localServer.listen(gServerName);
-        if (!ok) {
-            qWarning() << "UbuntuApplicationApiWrapper: failed to listen for connections on"
-                       << gServerName;
+        startLocalServer();
+    }
+}
+
+void UbuntuApplicationApiWrapper::startLocalServer()
+{
+    QString socketFilePath = buildSocketFilePath();
+
+    {
+        QFile socketFile(socketFilePath);
+        if (socketFile.exists()) {
+            // It's a leftover from a previous run of maliit-server. let's get rid of it.
+            // The other possibility is that another instance of maliit-server is already
+            // running, but that's absolutely unsupported.
+            if (!socketFile.remove()) {
+                qWarning() << "UbuntuApplicationApiWrapper: unable to remove pre-existing"
+                           << socketFilePath ;
+            }
         }
     }
+
+    connect(&m_localServer, &QLocalServer::newConnection,
+            this, &UbuntuApplicationApiWrapper::onNewConnection);
+    bool ok = m_localServer.listen(socketFilePath);
+    if (!ok) {
+        qWarning() << "UbuntuApplicationApiWrapper: failed to listen for connections on"
+                   << socketFilePath;
+    }
 }
 
 void UbuntuApplicationApiWrapper::reportOSKVisible(const int x, const int y, const int width, const int height)
@@ -96,6 +118,13 @@
     struct SharedInfo sharedInfo;
     sharedInfo.keyboardWidth = width;
     sharedInfo.keyboardHeight = height;
+
+    if (sharedInfo == m_lastInfoShared) {
+        // do not flood our listener with redundant info. This also means that
+        // were are getting called unnecessarily
+        return;
+    }
+
     const qint64 sharedInfoSize = sizeof(struct SharedInfo);
     qint64 bytesWritten = m_clientConnection->write(reinterpret_cast<char *>(&sharedInfo),
                                                     sharedInfoSize);
@@ -108,32 +137,54 @@
         qWarning() << "UbuntuApplicationApiWrapper: tried to write" << sharedInfoSize << "bytes"
                       "but only" << bytesWritten << "went through";
     }
+
+    m_lastInfoShared = sharedInfo;
 }
 
 void UbuntuApplicationApiWrapper::onNewConnection()
 {
-    if (m_clientConnection)
+    QLocalSocket *newConnection = m_localServer.nextPendingConnection();
+
+    if (m_clientConnection) {
+        qWarning() << "UbuntuApplicationApiWrapper: Refusing incoming connection as we "
+                      "already have an active one.";
+        delete newConnection;
         return; // ignore it. for simplicity we care to serve only one client (unity8-mir)
+    }
+    m_clientConnection = newConnection;
+    m_lastInfoShared.reset();
 
-    m_clientConnection = m_localServer.nextPendingConnection();
     connect(m_clientConnection, &QLocalSocket::disconnected,
             this, &UbuntuApplicationApiWrapper::onClientDisconnected);
-
-    typedef void (QLocalSocket::*MemberFunctionType)(QLocalSocket::LocalSocketError);
-    MemberFunctionType funcPointer = &QLocalSocket::error;
-    connect(m_clientConnection, funcPointer,
-            this, &UbuntuApplicationApiWrapper::onClientError);
 }
 
 void UbuntuApplicationApiWrapper::onClientDisconnected()
 {
-    delete m_clientConnection;
-    m_clientConnection = 0;
-}
-
-void UbuntuApplicationApiWrapper::onClientError(QLocalSocket::LocalSocketError socketError)
-{
-    Q_UNUSED(socketError);
-    delete m_clientConnection;
-    m_clientConnection = 0;
+    m_clientConnection->deleteLater();
+    m_clientConnection = 0;
+}
+
+QString UbuntuApplicationApiWrapper::buildSocketFilePath() const
+{
+    char *xdgRuntimeDir = getenv("XDG_RUNTIME_DIR");
+
+    if (xdgRuntimeDir) {
+        return QDir(xdgRuntimeDir).filePath(gServerName);
+    } else {
+        return QDir("/tmp").filePath(gServerName);
+    }
+}
+
+// ------------------------------- SharedInfo ----------------------------
+
+bool UbuntuApplicationApiWrapper::SharedInfo::operator ==(const struct SharedInfo &other)
+{
+    return keyboardWidth == other.keyboardWidth
+        && keyboardHeight == other.keyboardHeight;
+}
+
+void UbuntuApplicationApiWrapper::SharedInfo::reset()
+{
+    keyboardWidth = 0;
+    keyboardHeight = 0;
 }

=== modified file 'src/plugin/ubuntuapplicationapiwrapper.h'
--- src/plugin/ubuntuapplicationapiwrapper.h	2013-10-09 15:28:52 +0000
+++ src/plugin/ubuntuapplicationapiwrapper.h	2013-10-11 15:34:57 +0000
@@ -44,7 +44,6 @@
 private Q_SLOTS:
     void onNewConnection();
     void onClientDisconnected();
-    void onClientError(QLocalSocket::LocalSocketError socketError);
 
 private:
     // NB! Must match the definition in unity-mir. Not worth creating a shared header
@@ -52,12 +51,18 @@
     struct SharedInfo {
         qint32 keyboardWidth;
         qint32 keyboardHeight;
+
+        bool operator ==(const struct SharedInfo &other);
+        void reset();
     };
     void sendInfoToClientConnection(int width, int height);
+    void startLocalServer();
+    QString buildSocketFilePath() const;
 
     bool m_runningOnMir;
     QLocalServer m_localServer;
     QLocalSocket *m_clientConnection;
+    struct SharedInfo m_lastInfoShared;
 };
 
 #endif // UBUNTUAPPLICATIONAPIWRAPPER_H