Qt5 / KDE5 refactor create_SalInstance

Move most common code into Qt5Instance (static) functions.
Not much saved with regard to LOC, but since the code was
95% C'n'P, I hope it'll be less error prone this way.

Also handle all QApplication argument pointers via
std::unique_ptr from the beginning.

Change-Id: I87b2f571d398db7ccce3e740f16205b251be4ced
Reviewed-on: https://gerrit.libreoffice.org/69578
Tested-by: Jenkins
Reviewed-by: Aleksei Nikiforov <darktemplar@basealt.ru>

vcl/inc/qt5/Qt5Instance.hxx

@@ -27,12 +27,21 @@
 
 #include <QtCore/QObject>
 
+#include <cstdlib>
 #include <functional>
+#include <memory>
+#include <vector>
 
 class QApplication;
 class SalYieldMutex;
 class SalFrame;
 
+struct StdFreeCStr
+{
+    void operator()(char* arg) const noexcept { std::free(arg); }
+};
+using FreeableCStr = std::unique_ptr<char[], StdFreeCStr>;
+
 class VCLPLUG_QT5_PUBLIC Qt5Instance : public QObject,
                                        public SalGenericInstance,
                                        public SalUserEventList
@@ -44,9 +53,8 @@ class VCLPLUG_QT5_PUBLIC Qt5Instance : public QObject,
     const bool m_bUseCairo;
     std::unordered_map<OUString, css::uno::Reference<css::uno::XInterface>> m_aClipboards;
 
-public:
     std::unique_ptr<QApplication> m_pQApplication;
-    std::unique_ptr<char* []> m_pFakeArgvFreeable;
+    std::vector<FreeableCStr> m_pFakeArgvFreeable;
     std::unique_ptr<char* []> m_pFakeArgv;
     std::unique_ptr<int> m_pFakeArgc;
 
@@ -61,9 +69,17 @@ Q_SIGNALS:
     void deleteObjectLaterSignal(QObject* pObject);
 
 public:
-    explicit Qt5Instance(bool bUseCairo = false);
+    explicit Qt5Instance(std::unique_ptr<QApplication>& pQApp, bool bUseCairo = false);
     virtual ~Qt5Instance() override;
 
+    // handle common SalInstance setup
+    static void AllocFakeCmdlineArgs(std::unique_ptr<char* []>& rFakeArgv,
+                                     std::unique_ptr<int>& rFakeArgc,
+                                     std::vector<FreeableCStr>& rFakeArgvFreeable);
+    void MoveFakeCmdlineArgs(std::unique_ptr<char* []>& rFakeArgv, std::unique_ptr<int>& rFakeArgc,
+                             std::vector<FreeableCStr>& rFakeArgvFreeable);
+    static std::unique_ptr<QApplication> CreateQApplication(int& nArgc, char** pArgv);
+
     void RunInMainThread(std::function<void()> func);
 
     virtual SalFrame* CreateFrame(SalFrame* pParent, SalFrameStyleFlags nStyle) override;

vcl/qt5/Qt5Instance.cxx

@@ -200,10 +200,11 @@ void Qt5Instance::ImplRunInMain()
     (void)this; // suppress unhelpful [loplugin:staticmethods]; can't be static
 }
 
-Qt5Instance::Qt5Instance(bool bUseCairo)
+Qt5Instance::Qt5Instance(std::unique_ptr<QApplication>& pQApp, bool bUseCairo)
     : SalGenericInstance(std::make_unique<Qt5YieldMutex>())
     , m_postUserEventId(-1)
     , m_bUseCairo(bUseCairo)
+    , m_pQApplication(std::move(pQApp))
 {
     ImplSVData* pSVData = ImplGetSVData();
     if (bUseCairo)
@@ -232,8 +233,6 @@ Qt5Instance::~Qt5Instance()
     // force freeing the QApplication before freeing the arguments,
     // as it uses references to the provided arguments!
     m_pQApplication.reset();
-    for (int i = 0; i < *m_pFakeArgc; i++)
-        free(m_pFakeArgvFreeable[i]);
 }
 
 void Qt5Instance::deleteObjectLater(QObject* pObject) { pObject->deleteLater(); }
@@ -441,18 +440,16 @@ Reference<XInterface> Qt5Instance::CreateDropTarget()
     return Reference<XInterface>(static_cast<cppu::OWeakObject*>(new Qt5DropTarget()));
 }
 
-extern "C" {
-VCLPLUG_QT5_PUBLIC SalInstance* create_SalInstance()
+void Qt5Instance::AllocFakeCmdlineArgs(std::unique_ptr<char* []>& rFakeArgv,
+                                       std::unique_ptr<int>& rFakeArgc,
+                                       std::vector<FreeableCStr>& rFakeArgvFreeable)
 {
     OString aVersion(qVersion());
     SAL_INFO("vcl.qt5", "qt version string is " << aVersion);
 
-    QApplication* pQApplication;
-    char** pFakeArgvFreeable = nullptr;
-
-    int nFakeArgc = 2;
     const sal_uInt32 nParams = osl_getCommandArgCount();
     OString aDisplay;
+    sal_uInt32 nDisplayValueIdx = 0;
     OUString aParam, aBin;
 
     for (sal_uInt32 nIdx = 0; nIdx < nParams; ++nIdx)
@@ -460,66 +457,85 @@ VCLPLUG_QT5_PUBLIC SalInstance* create_SalInstance()
         osl_getCommandArg(nIdx, &aParam.pData);
         if (aParam != "-display")
             continue;
-        if (!pFakeArgvFreeable)
-        {
-            pFakeArgvFreeable = new char*[nFakeArgc + 2];
-            pFakeArgvFreeable[nFakeArgc++] = strdup("-display");
-        }
-        else
-            free(pFakeArgvFreeable[nFakeArgc]);
-
         ++nIdx;
-        osl_getCommandArg(nIdx, &aParam.pData);
-        aDisplay = OUStringToOString(aParam, osl_getThreadTextEncoding());
-        pFakeArgvFreeable[nFakeArgc] = strdup(aDisplay.getStr());
+        nDisplayValueIdx = nIdx;
     }
-    if (!pFakeArgvFreeable)
-        pFakeArgvFreeable = new char*[nFakeArgc];
-    else
-        nFakeArgc++;
 
     osl_getExecutableFile(&aParam.pData);
     osl_getSystemPathFromFileURL(aParam.pData, &aBin.pData);
     OString aExec = OUStringToOString(aBin, osl_getThreadTextEncoding());
-    pFakeArgvFreeable[0] = strdup(aExec.getStr());
-    pFakeArgvFreeable[1] = strdup("--nocrashhandler");
 
-    char** pFakeArgv = new char*[nFakeArgc];
+    std::vector<FreeableCStr> aFakeArgvFreeable;
+    aFakeArgvFreeable.reserve(4);
+    aFakeArgvFreeable.emplace_back(strdup(aExec.getStr()));
+    aFakeArgvFreeable.emplace_back(strdup("--nocrashhandler"));
+    if (nDisplayValueIdx)
+    {
+        aFakeArgvFreeable.emplace_back(strdup("-display"));
+        osl_getCommandArg(nDisplayValueIdx, &aParam.pData);
+        aDisplay = OUStringToOString(aParam, osl_getThreadTextEncoding());
+        aFakeArgvFreeable.emplace_back(strdup(aDisplay.getStr()));
+    }
+    rFakeArgvFreeable.swap(aFakeArgvFreeable);
+
+    const int nFakeArgc = rFakeArgvFreeable.size();
+    rFakeArgv.reset(new char*[nFakeArgc]);
     for (int i = 0; i < nFakeArgc; i++)
-        pFakeArgv[i] = pFakeArgvFreeable[i];
+        rFakeArgv[i] = rFakeArgvFreeable[i].get();
+
+    rFakeArgc.reset(new int);
+    *rFakeArgc = nFakeArgc;
+}
 
-    char* session_manager = nullptr;
+void Qt5Instance::MoveFakeCmdlineArgs(std::unique_ptr<char* []>& rFakeArgv,
+                                      std::unique_ptr<int>& rFakeArgc,
+                                      std::vector<FreeableCStr>& rFakeArgvFreeable)
+{
+    m_pFakeArgv = std::move(rFakeArgv);
+    m_pFakeArgc = std::move(rFakeArgc);
+    m_pFakeArgvFreeable.swap(rFakeArgvFreeable);
+}
+
+std::unique_ptr<QApplication> Qt5Instance::CreateQApplication(int& nArgc, char** pArgv)
+{
+    QApplication::setAttribute(Qt::AA_DisableHighDpiScaling);
+
+    FreeableCStr session_manager;
     if (getenv("SESSION_MANAGER") != nullptr)
     {
-        session_manager = strdup(getenv("SESSION_MANAGER"));
+        session_manager.reset(strdup(getenv("SESSION_MANAGER")));
         unsetenv("SESSION_MANAGER");
     }
 
-    QApplication::setAttribute(Qt::AA_DisableHighDpiScaling);
-
-    int* pFakeArgc = new int;
-    *pFakeArgc = nFakeArgc;
-    pQApplication = new QApplication(*pFakeArgc, pFakeArgv);
+    std::unique_ptr<QApplication> pQApp = std::make_unique<QApplication>(nArgc, pArgv);
 
     if (session_manager != nullptr)
     {
         // coverity[tainted_string] - trusted source for setenv
-        setenv("SESSION_MANAGER", session_manager, 1);
-        free(session_manager);
+        setenv("SESSION_MANAGER", session_manager.get(), 1);
     }
 
     QApplication::setQuitOnLastWindowClosed(false);
+    return pQApp;
+}
 
+extern "C" {
+VCLPLUG_QT5_PUBLIC SalInstance* create_SalInstance()
+{
     static const bool bUseCairo = (nullptr != getenv("SAL_VCL_QT5_USE_CAIRO"));
-    Qt5Instance* pInstance = new Qt5Instance(bUseCairo);
 
-    // initialize SalData
-    new Qt5Data(pInstance);
+    std::unique_ptr<char* []> pFakeArgv;
+    std::unique_ptr<int> pFakeArgc;
+    std::vector<FreeableCStr> aFakeArgvFreeable;
+    Qt5Instance::AllocFakeCmdlineArgs(pFakeArgv, pFakeArgc, aFakeArgvFreeable);
 
-    pInstance->m_pQApplication.reset(pQApplication);
-    pInstance->m_pFakeArgvFreeable.reset(pFakeArgvFreeable);
-    pInstance->m_pFakeArgv.reset(pFakeArgv);
-    pInstance->m_pFakeArgc.reset(pFakeArgc);
+    std::unique_ptr<QApplication> pQApp
+        = Qt5Instance::CreateQApplication(*pFakeArgc, pFakeArgv.get());
+
+    Qt5Instance* pInstance = new Qt5Instance(pQApp, bUseCairo);
+    pInstance->MoveFakeCmdlineArgs(pFakeArgv, pFakeArgc, aFakeArgvFreeable);
+
+    new Qt5Data(pInstance);
 
     return pInstance;
 }

vcl/unx/kde5/KDE5SalInstance.cxx

@@ -38,8 +38,8 @@
 
 using namespace com::sun::star;
 
-KDE5SalInstance::KDE5SalInstance()
-    : Qt5Instance(true)
+KDE5SalInstance::KDE5SalInstance(std::unique_ptr<QApplication>& pQApp)
+    : Qt5Instance(pQApp, true)
 {
     ImplSVData* pSVData = ImplGetSVData();
     pSVData->maAppData.mxToolkitName = OUString("kde5");
@@ -88,81 +88,19 @@ KDE5SalInstance::createFolderPicker(const uno::Reference<uno::XComponentContext>
 extern "C" {
 VCLPLUG_KDE5_PUBLIC SalInstance* create_SalInstance()
 {
-    OString aVersion(qVersion());
-    SAL_INFO("vcl.qt5", "qt version string is " << aVersion);
+    std::unique_ptr<char* []> pFakeArgv;
+    std::unique_ptr<int> pFakeArgc;
+    std::vector<FreeableCStr> aFakeArgvFreeable;
+    Qt5Instance::AllocFakeCmdlineArgs(pFakeArgv, pFakeArgc, aFakeArgvFreeable);
 
-    QApplication* pQApplication;
-    char** pFakeArgvFreeable = nullptr;
+    std::unique_ptr<QApplication> pQApp
+        = Qt5Instance::CreateQApplication(*pFakeArgc, pFakeArgv.get());
 
-    int nFakeArgc = 2;
-    const sal_uInt32 nParams = osl_getCommandArgCount();
-    OString aDisplay;
-    OUString aParam, aBin;
-
-    for (sal_uInt32 nIdx = 0; nIdx < nParams; ++nIdx)
-    {
-        osl_getCommandArg(nIdx, &aParam.pData);
-        if (aParam != "-display")
-            continue;
-        if (!pFakeArgvFreeable)
-        {
-            pFakeArgvFreeable = new char*[nFakeArgc + 2];
-            pFakeArgvFreeable[nFakeArgc++] = strdup("-display");
-        }
-        else
-            free(pFakeArgvFreeable[nFakeArgc]);
-
-        ++nIdx;
-        osl_getCommandArg(nIdx, &aParam.pData);
-        aDisplay = OUStringToOString(aParam, osl_getThreadTextEncoding());
-        pFakeArgvFreeable[nFakeArgc] = strdup(aDisplay.getStr());
-    }
-    if (!pFakeArgvFreeable)
-        pFakeArgvFreeable = new char*[nFakeArgc];
-    else
-        nFakeArgc++;
-
-    osl_getExecutableFile(&aParam.pData);
-    osl_getSystemPathFromFileURL(aParam.pData, &aBin.pData);
-    OString aExec = OUStringToOString(aBin, osl_getThreadTextEncoding());
-    pFakeArgvFreeable[0] = strdup(aExec.getStr());
-    pFakeArgvFreeable[1] = strdup("--nocrashhandler");
-
-    char** pFakeArgv = new char*[nFakeArgc];
-    for (int i = 0; i < nFakeArgc; i++)
-        pFakeArgv[i] = pFakeArgvFreeable[i];
-
-    char* session_manager = nullptr;
-    if (getenv("SESSION_MANAGER") != nullptr)
-    {
-        session_manager = strdup(getenv("SESSION_MANAGER"));
-        unsetenv("SESSION_MANAGER");
-    }
-
-    QApplication::setAttribute(Qt::AA_DisableHighDpiScaling);
-
-    int* pFakeArgc = new int;
-    *pFakeArgc = nFakeArgc;
-    pQApplication = new QApplication(*pFakeArgc, pFakeArgv);
-
-    if (session_manager != nullptr)
-    {
-        // coverity[tainted_string] - trusted source for setenv
-        setenv("SESSION_MANAGER", session_manager, 1);
-        free(session_manager);
-    }
-
-    QApplication::setQuitOnLastWindowClosed(false);
-
-    KDE5SalInstance* pInstance = new KDE5SalInstance();
+    KDE5SalInstance* pInstance = new KDE5SalInstance(pQApp);
+    pInstance->MoveFakeCmdlineArgs(pFakeArgv, pFakeArgc, aFakeArgvFreeable);
 
     new Qt5Data(pInstance);
 
-    pInstance->m_pQApplication.reset(pQApplication);
-    pInstance->m_pFakeArgvFreeable.reset(pFakeArgvFreeable);
-    pInstance->m_pFakeArgv.reset(pFakeArgv);
-    pInstance->m_pFakeArgc.reset(pFakeArgc);
-
     return pInstance;
 }
 }

vcl/unx/kde5/KDE5SalInstance.hxx

@@ -26,14 +26,16 @@
 #include <qt5/Qt5Instance.hxx>
 #include "KDE5SalFrame.hxx"
 
+class QApplication;
 class SalYieldMutex;
 class SalFrame;
 
 class KDE5SalInstance : public Qt5Instance
 {
     Q_OBJECT
+
 public:
-    explicit KDE5SalInstance();
+    explicit KDE5SalInstance(std::unique_ptr<QApplication>& pQApp);
 
     virtual bool hasNativeFileSelection() const override { return true; }