Add Variable interface to KTextEditor::Editor
ClosedPublic

Authored by dhaumann on Mar 15 2019, 9:00 PM.

Details

Summary

The Variable interface allows to register either exact matches
of the form %{Document:Text} or prefix matches of the form
%{Date:} where the text after the colon (separator) is passed
to the evaluation of the variable.

This functionality is required for the External Tools plugin,
that will use this interface to do its macro expansion.

A unit test demonstrates a bit how it works.

Some possible/fictive examples:

  • %{CurrentDocument:Text} evaluates to documents contents
  • %{UUID} evaluates to a random uuid
  • %{Time:hh-mm} evaluates to 17-35

Currently, none of these variables are added, it's just the
interface.

Test Plan

make && make test

Diff Detail

Repository
R39 KTextEditor
Branch
variable-interface
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9809
Build 9827: arc lint + arc unit
dhaumann created this revision.Mar 15 2019, 9:00 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptMar 15 2019, 9:00 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
dhaumann requested review of this revision.Mar 15 2019, 9:00 PM

What I wonder is whether we really need the "Variable" class. Maybe it would be better to just have

  • registerExactMatch(QString name, QString description, function)
  • registerPrefixMatch(QString prefix, QString description, function)

If such a change would be preferred, then I can easily change the patch.

...and as the unit test currently shows, recursive macro expansion is not supported, but I think it's needed for e.g. %{JS: bla %{...} : %[...}}.

I think without the Variable class it would be nicer.
One can extend it later easier, too, by just adding a new overload with a different function type (if needed).

dhaumann updated this revision to Diff 54069.Mar 17 2019, 9:27 AM
  • Hide class Variable

Unfortunately, I still see one issue: In the unit test, expanding the variable to "World" works, but expanding it to "Smart World" leads to single-quoted text "'Smart World'" because of the space. That of course makes sense, but is not always what you want.

Therefore, I suggest we expand the API as follows:

bool expandVariable(const QString& variable, KTextEditor::View* view, QString& output, KTextEditor::ExpansionMode mode = QuoteWhenNeeded) const;
bool expandText(const QString& text, KTextEditor::View* view, QString& output, KTextEditor::ExpansionMode mode = QuoteWhenNeeded) const;

with

enum class KTextEditor::ExpansionMode { Never, QuoteWhenNeeded }

Any comments?

Hmm, I am actually not sure we want the quoting per default at all.
You use expandMacrosShellQuote, I am not sure that that is that often wanted (at least not if not passing to commandline/shell).
Perhaps the Enum should more state "NoQuoting, ShellQuoting, ..."

dhaumann updated this revision to Diff 54078.Mar 17 2019, 11:45 AM
  • Support optional quoting, default is Never

Hmm. the Variable class and files is just an artifact now or?

Hmm. the Variable class and files is just an artifact now or?

Yes, it's just used internally. Without it, I'd need to add ugly std::tuple into the QHash. It's still in the KTextEditor namespace though. I can change to KateVariable if you want.

Ok, then I am fine with this beside that I think the Quoting stuff should be more explicit that it is "Shell" quoting.
Isn't it even different for Windows vs. unices?
I still think QuotationMode NoQuoting + ShellQuoting would be more easy to understand. WhenNeeded is a bit unclear for the "when"?

dhaumann updated this revision to Diff 54089.Mar 17 2019, 2:14 PM
  • Enum: Use NoQuoting and ShellQuoting
cullmann accepted this revision.Mar 17 2019, 2:41 PM

Looks reasonable to me and the interface is nicely minimal ;=)

This revision is now accepted and ready to land.Mar 17 2019, 2:41 PM
dhaumann planned changes to this revision.Mar 17 2019, 10:30 PM

Will do another change: since recursive expansion of macros is not supported by KMacroExpander (e.g. %{JS:if(true) %{Document:Selection}}), I'll provide an implementation that does not use KMacroExpander at all. Have that working already locally here. Will post an updated patch tomorrow.

dhaumann updated this revision to Diff 54264.Mar 18 2019, 8:53 PM
  • Drop KWordMacroExpander in favor of own simple implementation (50 slocs)
This revision is now accepted and ready to land.Mar 18 2019, 8:53 PM
This revision was automatically updated to reflect the committed changes.

It's committed now:

  • quoting is now not supported at all anymore
  • expandText() now returns void