Extend Scripting API to allow executing commands
ClosedPublic

Authored by dhaumann on Dec 8 2017, 10:26 AM.

Details

Summary

This patch adds the following JavaScript API:

object view.executeCommand(String command,
                           String args = String(),
                           Range range = Rante::invalid())

The returned object indicates whether execution of the command
was successful:

  • object.ok [bool] is true on success, otherwise false.
  • object.status [String] contains the result of the command (e.g. "31 replacements made", or an error message)

Examples:

view.executeCommand("sort"); // all, or seletcion
view.executeCommand("sort", "", new Range(1, 0, 4, 0); // sort lines 1-4
view.executeCommand("set-indent-width", "4");

Unfortunately, we do not have any getters, e.g. "indent-width".
This is a shortcoming of the KTextEditor::Command API. We should
consider to change KTextEditor::Command as follows:

// old:
virtual bool exec(KTextEditor::View *view, const QString &cmd, QString &msg,
                  const KTextEditor::Range &range = KTextEditor::Range::invalid()) = 0;

// new:
virtual QVariant exec(const QVariant & args,
                  const KTextEditor::Range &range = KTextEditor::Range::invalid(),
                  KTextEditor::View * view = nullptr) = 0;

or similar. This way, Commands would be much more usabe in general
(and also better chainable, which could result in: command 1 | command 2 | ...).

What is missing in this patch:

  • infinite recursion is very easily possible
  • guard against some special commands: reload-scripts, anything else?
  • should the API be more smart? E.g. we could support both:
    • view.executeCommand("sort", "", new Range(1,0, 3, 0))
    • view.executeCommand("sort", new Range(1,0, 3, 0)) -> we'd have to lookup the type of the passed QJSValues (would work)...
  • Maybe only return a bool instead of object with "ok" and "status" ? -> The status could be helpful as a return value, but to be honest, this is a hack (see shortcoming of KTextEditor::Command above). We could also add KTextEditor::CommandExtension, provide an improved interface, and then do this properly here... Comments?
Test Plan

make test, manual testing

Diff Detail

Repository
R39 KTextEditor
Branch
CommandScript (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1760
Build 1778: arc lint + arc unit
dhaumann created this revision.Dec 8 2017, 10:26 AM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptDec 8 2017, 10:26 AM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
dhaumann requested review of this revision.Dec 8 2017, 10:26 AM
dhaumann retitled this revision from Extend Scripting API to allow executong commands to Extend Scripting API to allow executing commands.Dec 8 2017, 10:49 AM
dhaumann edited the summary of this revision. (Show Details)

How can we have infinite recursion? By calling a command that triggers again the script function we did call the command in? If that is the only problem, I see no issue, that can happen with normal function calls already, perhaps we could guard chained command calls inside executeCommand, thought (aka guard that not twice the same "command" string arrives via set)

Well, the scripting commands are also available on the command line as commands, aren't they? So you could essentially call yourself.

Well, the scripting commands are also available on the command line as commands, aren't they? So you could essentially call yourself.

Yeah, that is true, but that is no difference to you are calling yourself indirectly in javascript itself. Perhaps a bit more hidden.

mwolff requested changes to this revision.Jan 5 2018, 1:03 PM

in general this sounds like a good idea. Also adding getters to the API would be good.

there are two typos in your commit message, but stupid phabricator does not let me review that:

Rante::invalid
seletcion

src/script/katescriptview.cpp
124 ↗(On Diff #23628)

here and below: wrong placement of &

135 ↗(On Diff #23628)

i18n, prevent string puzzle

This revision now requires changes to proceed.Jan 5 2018, 1:03 PM
dhaumann updated this revision to Diff 39575.Aug 13 2018, 10:15 AM
  • Apply proposed fixes
  • Merge branch 'master' into CommandScript
Restricted Application edited subscribers, added: kde-frameworks-devel, kwrite-devel; removed: Frameworks. · View Herald TranscriptAug 13 2018, 10:15 AM
dhaumann marked 2 inline comments as done.Aug 13 2018, 10:15 AM
cullmann accepted this revision.Aug 14 2018, 3:37 PM
This revision was not accepted when it landed; it landed in state Needs Review.Aug 14 2018, 9:55 PM
This revision was automatically updated to reflect the committed changes.