Enable running commands from app
ClosedPublic

Authored by nicolasfella on Jun 13 2018, 12:02 AM.

Details

Summary

The RemotecommandsPlugin lacks a graphical frontend.

Inlcudes a Dbus Interface for fetching the commands and a Model exposing them to QML. For this I oriented on the NotificatonsPlugin.

Test Plan

Open command list in app, check available commands, trigger some. Do same for CLI.
Activate edit action, check KCM opening on remote device, add command, check for new command in list

Diff Detail

Repository
R224 KDE Connect
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
nicolasfella created this revision.Jun 13 2018, 12:02 AM
Restricted Application added a project: KDE Connect. · View Herald TranscriptJun 13 2018, 12:02 AM
Restricted Application added a subscriber: kdeconnect. · View Herald Transcript
nicolasfella requested review of this revision.Jun 13 2018, 12:02 AM
apol added a subscriber: apol.Jun 13 2018, 10:45 AM
apol added inline comments.
interfaces/remotecommandsmodel.cpp
123

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

168

const

169

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

interfaces/remotecommandsmodel.h
32

Should be RemoteCommandsModel.

71

QVector

plugins/remotecommands/remotecommand.h
21 ↗(On Diff #36077)

Don't do that just here once

30 ↗(On Diff #36077)

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.

plugins/remotecommands/remotecommandsdbusinterface.h
48 ↗(On Diff #36077)

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

nicolasfella added inline comments.Jun 13 2018, 1:13 PM
plugins/remotecommands/remotecommand.h
30 ↗(On Diff #36077)

I thought about that too when it was already too late. This way we already expose the parsed data, otherwise we would need to do that in the model (which would be fine). But the current approach would be beneficial if something else would use the DBus interface (some hypothetical extra UI) so we don't need to implement the parsing twice.

apol requested changes to this revision.Jun 13 2018, 4:46 PM
apol added inline comments.
plugins/remotecommands/remotecommand.h
30 ↗(On Diff #36077)

Any UI will end up using the RemoteCommandsModel.

This revision now requires changes to proceed.Jun 13 2018, 4:46 PM

Expose JSON instead of complex interface

nicolasfella marked 5 inline comments as done.Jun 13 2018, 11:40 PM
apol added inline comments.Jun 14 2018, 12:41 AM
interfaces/remotecommandsmodel.h
69

Just QVector<Command>. It will make the code slightly simpler.

plasmoid/declarativeplugin/kdeconnectdeclarativeplugin.cpp
97

I would always write upper-case C on RemoteCommands.

  • Aleix's comments
nicolasfella marked 2 inline comments as done.Jun 18 2018, 7:04 PM
apol accepted this revision.Jun 20 2018, 12:23 AM
This revision is now accepted and ready to land.Jun 20 2018, 12:23 AM
This revision was automatically updated to reflect the committed changes.