D13503: Enable running commands from app

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> remotecommandsmodel.cpp:122
> +
> +    beginInsertRows(QModelIndex(), 0, commandIds.size() - 1);
> +    for (const QString& commandId : commandIds) {

Should it maybe be beginResetModel? Looks like we're setting it up from scratch. Then we'd need to m_commandList.clear()

> remotecommandsmodel.cpp:167
> +
> +    int row = index.row();
> +    if (row < 0 || row >= m_commandList.size()) {

const

> remotecommandsmodel.cpp:168
> +    int row = index.row();
> +    if (row < 0 || row >= m_commandList.size()) {
> +        return nullptr;

No need to check bounds if we call m_commendList.value(row);

> remotecommandsmodel.h:31
> +
> +class KDECONNECTINTERFACES_EXPORT RemotecommandsModel
> +    : public QAbstractListModel

Should be `RemoteCommandsModel`.

> remotecommandsmodel.h:70
> +    DeviceRemotecommandsDbusInterface* m_dbusInterface;
> +    QList<RemotecommandDbusInterface*> m_commandList;
> +    QString m_deviceId;

QVector

> remotecommand.h:21
>  
> -#ifndef REMOTECOMMANDSPLUGIN_H
> -#define REMOTECOMMANDSPLUGIN_H
> +#pragma once
>  

Don't do that just here once

> remotecommand.h:30
> +    Q_CLASSINFO("D-Bus Interface", "org.kde.kdeconnect.device.remotecommands.remotecommand")
> +    Q_PROPERTY(QString key READ key CONSTANT)
> +    Q_PROPERTY(QString name READ name CONSTANT)

Wouldn't it make sense to just return a QVariantMap or QJsonObject? Creating a dbus interface just to make it constant doesn't make a lot of sense.

> remotecommandsdbusinterface.h:48
> +
> +    bool receivePacket(const NetworkPacket& np);
>  

Why does a dbus interface have a `receivePacket`?
I'd say it's better to leave it as it was with setCommands and commands.

REPOSITORY
  R224 KDE Connect

REVISION DETAIL
  https://phabricator.kde.org/D13503

To: nicolasfella, #kde_connect
Cc: apol, kdeconnect, #kde_connect, jdvr, yannux, Danial0_0, johnq, Pitel, adeen-s, SemperPeritus, ndavis, daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, ach
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20180613/2ced1c77/attachment-0001.html>