support V2/GET_CLIENT_ICON in socketapi, add icon to dolphin right click menu #8638

gabi18 · 2021-05-18T08:21:09Z

…ick menu

CLAassistant · 2021-05-18T08:21:18Z

All committers have signed the CLA.

TheOneRing · 2021-05-18T08:54:49Z

shell_integration/dolphin/ownclouddolphinpluginhelper.cpp

+    QByteArray line_end("\n");
+    QJsonObject args { { "size", size } };
+    QJsonObject obj { { QStringLiteral("id"), "1" }, { QStringLiteral("arguments"), args } };
+    std::string command = "V2/GET_CLIENT_ICON:" + QJsonDocument(obj).toJson(QJsonDocument::Compact).toStdString();


QByteArrayLiteral("V2/GET_CLIENT_ICON:")

shell_integration/dolphin/ownclouddolphinpluginhelper.cpp

TheOneRing · 2021-05-18T08:56:34Z

shell_integration/dolphin/ownclouddolphinpluginhelper.cpp

 void OwncloudDolphinPluginHelper::slotConnected()
 {
    sendCommand("VERSION:\n");
    sendCommand("GET_STRINGS:\n");
+    sendCommand("V2/GET_CLIENT_ICON:\n");


Probably not needed.

you are right - it isn't needed

TheOneRing · 2021-05-18T08:56:50Z

shell_integration/dolphin/ownclouddolphinpluginhelper.cpp

+void OwncloudDolphinPluginHelper::sendGetClientIconCommand(int size)
+{
+    QByteArray line_end("\n");
+    QJsonObject args { { "size", size } };


QStringLietral()

TheOneRing · 2021-05-18T08:59:38Z

shell_integration/dolphin/ownclouddolphinpluginhelper.cpp

@@ -112,7 +124,26 @@ void OwncloudDolphinPluginHelper::slotReadyRead()
                _socket.disconnectFromServer();
                return;
            }
+        } else if (line.startsWith("V2/GET_CLIENT_ICON_RESULT:")) {
+            line.remove(0, QString("V2/GET_CLIENT_ICON_RESULT:").size());


QStringLiteral

TheOneRing · 2021-05-18T09:00:15Z

shell_integration/dolphin/ownclouddolphinpluginhelper.cpp

+            line.remove(0, QString("V2/GET_CLIENT_ICON_RESULT:").size());
+            QJsonParseError error;
+            auto json = QJsonDocument::fromJson(line, &error).object();
+            if (error.error != QJsonParseError::NoError)


TheOneRing · 2021-05-18T09:04:02Z

shell_integration/dolphin/ownclouddolphinpluginhelper.cpp

+            if (jsonArgs.isEmpty())
+                continue;
+
+            QByteArray pngBase64 = jsonArgs.value("png").toString().toLatin1();


const QByteArray pngBase64
toUtf8

src/gui/socketapi/socketapi.cpp

TheOneRing · 2021-05-19T08:30:45Z

shell_integration/dolphin/ownclouddolphinpluginhelper.cpp

-    QJsonObject obj { { QStringLiteral("id"), "1" }, { QStringLiteral("arguments"), args } };
-    std::string command = "V2/GET_CLIENT_ICON:" + QJsonDocument(obj).toJson(QJsonDocument::Compact).toStdString();
-    sendCommand(QByteArray(command.c_str() + line_end));
+    int msgId = 1;


Here I meant a member that counts upwards.
the v2 response will have the same id, so when you (in theory) start multiple requests you can map them

TheOneRing · 2021-05-19T08:31:09Z

shell_integration/dolphin/ownclouddolphinpluginhelper.cpp

-    sendCommand(QByteArray(command.c_str() + line_end));
+    int msgId = 1;
+    QJsonObject args { { QStringLiteral("size"), size } };
+    QJsonObject obj { { QStringLiteral("id"), QString(msgId) }, { QStringLiteral("arguments"), args } };


QString::number
and please const for the objects.

TheOneRing · 2021-05-19T08:38:15Z

shell_integration/dolphin/ownclouddolphinpluginhelper.cpp

@@ -125,17 +124,19 @@ void OwncloudDolphinPluginHelper::slotReadyRead()
                return;
            }
        } else if (line.startsWith("V2/GET_CLIENT_ICON_RESULT:")) {
-            line.remove(0, QString("V2/GET_CLIENT_ICON_RESULT:").size());
+            line.remove(0, QStringLiteral("V2/GET_CLIENT_ICON_RESULT:").size());


Hm maybe we could split at the : before the else if and only compare the command (command:argumen)
https://github.com/owncloud/client/blob/master/src/gui/socketapi/socketapi.cpp#L354

shell_integration/dolphin/ownclouddolphinpluginhelper.cpp

TheOneRing · 2021-05-19T08:40:41Z

shell_integration/dolphin/ownclouddolphinpluginhelper.cpp

+    QJsonObject args { { QStringLiteral("size"), size } };
+    QJsonObject obj { { QStringLiteral("id"), QString(msgId) }, { QStringLiteral("arguments"), args } };
+    auto json = QJsonDocument(obj).toJson(QJsonDocument::Compact);
+    sendCommand(QByteArray("V2/GET_CLIENT_ICON:" + json + "\n"));


For all the mess with the strings, we build our core library with a stricter string handling, so for example QString("a") would not compile there. That's why I'm a bit pedantic here :)

QByteArray("V2/GET_CLIENT_ICON:" + json + "\n")

improved - hopefully correct now

src/gui/socketapi/socketapi.cpp

TheOneRing · 2021-05-25T13:43:29Z

shell_integration/dolphin/ownclouddolphinpluginhelper.cpp

+        // get the command (at begin of line, before first ':')
+        const QByteArray command = line.left(firstColon);
+        // rest of line contains the information
+        QByteArray info = line.right(line.size() - firstColon - 1);


Might be me but I find line.mid(firstColon + 1) more intuitive.
Also const.
If you use midRef it event removes the need to create a copy of the string.

Yes, line.mid() IS more intuitive

TheOneRing · 2021-05-25T13:45:46Z

shell_integration/dolphin/ownclouddolphinpluginhelper.cpp

        QByteArray line;
        qSwap(line, _line);
        line.chop(1);
        if (line.isEmpty())
            continue;
+        int firstColon = line.indexOf(':');


sonarcloud · 2021-05-26T12:45:12Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

0.0% Coverage
0.0% Duplication

support V2/GET_CLIENT_ICON in socketapi, add icon to dolphin right cl…

488c8c6

…ick menu

TheOneRing requested changes May 18, 2021

View reviewed changes

apply review improvements

a59359a

TheOneRing requested changes May 19, 2021

View reviewed changes

TheOneRing reviewed May 19, 2021

View reviewed changes

src/gui/socketapi/socketapi.cpp Outdated Show resolved Hide resolved

further improvements

f4f4a6a

gabi18 force-pushed the work/dolphin_icon branch from f32c507 to 4d841db Compare May 20, 2021 11:55

logging added, improvements

db08f51

TheOneRing approved these changes May 25, 2021

View reviewed changes

gabi18 force-pushed the work/dolphin_icon branch from 4d841db to db08f51 Compare May 25, 2021 13:29

improve OwncloudDolphinPluginHelper::slotReadyRead

313afd8

TheOneRing requested changes May 25, 2021

View reviewed changes

TheOneRing reviewed May 25, 2021

View reviewed changes

use line.mid(), add const

6ef95a4

gabi18 force-pushed the work/dolphin_icon branch from 22116ef to 6ef95a4 Compare May 25, 2021 14:50

TheOneRing approved these changes May 26, 2021

View reviewed changes

changlog for #8464

19deb49

TheOneRing approved these changes May 26, 2021

View reviewed changes

gabi18 merged commit 066ed56 into master May 26, 2021

delete-merged-branch bot deleted the work/dolphin_icon branch May 26, 2021 13:03

TheOneRing mentioned this pull request May 26, 2021

Provide the client icon in the file browser context menu #8464

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support V2/GET_CLIENT_ICON in socketapi, add icon to dolphin right click menu #8638

support V2/GET_CLIENT_ICON in socketapi, add icon to dolphin right click menu #8638

gabi18 commented May 18, 2021

CLAassistant commented May 18, 2021 •

edited

Loading

TheOneRing May 18, 2021

TheOneRing May 18, 2021

gabi18 May 19, 2021

TheOneRing May 18, 2021

TheOneRing May 18, 2021

TheOneRing May 18, 2021

TheOneRing May 18, 2021

TheOneRing May 19, 2021

gabi18 May 19, 2021

TheOneRing May 19, 2021

TheOneRing May 19, 2021

TheOneRing May 19, 2021

gabi18 May 19, 2021

TheOneRing May 25, 2021

gabi18 May 25, 2021

TheOneRing May 25, 2021

sonarcloud bot commented May 26, 2021

support V2/GET_CLIENT_ICON in socketapi, add icon to dolphin right click menu #8638

support V2/GET_CLIENT_ICON in socketapi, add icon to dolphin right click menu #8638

Conversation

gabi18 commented May 18, 2021

CLAassistant commented May 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented May 26, 2021

CLAassistant commented May 18, 2021 •

edited

Loading