dhaumann added a comment.
First of all, I also think this is a good patch. There are some parts with debug output, some parts with warnings (e.g. Q_FUNC_INFO), I guess this is temporary? :-) Besides that, there are several parts with commented out code - is that work in progress, or is the commented out code not required at all anymore? INLINE COMMENTS > katescript.cpp:77 > > - bt += m_engine->uncaughtExceptionBacktrace().join(QStringLiteral("\n")) > + QLatin1Char('\n'); > +// bt += m_engine>uncaughtExceptionBacktrace().join(QStringLiteral("\n")) > + QLatin1Char('\n'); > If there is no equivalent to get the backtrace, just delete this line. > katescript.cpp:96 > } > - m_engine->clearExceptions(); > +// m_engine->clearExceptions(); > } If such a call is not required, please delete. > katescript.cpp:150 > + m_engine->globalObject().setProperty(QStringLiteral("require"), > functions.property(QStringLiteral("require"))); > m_engine->globalObject().setProperty(QStringLiteral("require_guard"), > m_engine->newObject()); > If I remember correctly, Christoph once said it as quite a hassle to get this include guard working correctly. Are you sure it works or are there possibly issues due to some other behavior in QJSEngine? > katescriptdocument.h:63-65 > Q_INVOKABLE QString text(const KTextEditor::Cursor &from, const > KTextEditor::Cursor &to); > Q_INVOKABLE QString text(const KTextEditor::Range &range); > + Q_INVOKABLE QString text(const QJSValue &jsrange); If I understand correctly, you are adding an overload for all functions that have KTextEditor::Cursor or Range in the parameter. Does that imply we could remove the Q_INVOKABLE part of the overloads that use the complex types such as Cursor and Range? If so, I would be in favor of doing this, just to make this explicit. > katescripthelpers.cpp:178 > /// helper function to do the substitution from QtScript > QVariant > real > values > -KLocalizedString substituteArguments(const KLocalizedString &kls, const > QVariantList &arguments, int max = 99) > -{ > - KLocalizedString ls = kls; > - int cnt = qMin(arguments.count(), max); // QString supports max 99 > - for (int i = 0; i < cnt; ++i) { > - QVariant arg = arguments[i]; > - switch (arg.type()) { > - case QVariant::Int: ls = ls.subs(arg.toInt()); break; > - case QVariant::UInt: ls = ls.subs(arg.toUInt()); break; > - case QVariant::LongLong: ls = ls.subs(arg.toLongLong()); break; > - case QVariant::ULongLong: ls = ls.subs(arg.toULongLong()); break; > - case QVariant::Double: ls = ls.subs(arg.toDouble()); break; > - default: ls = ls.subs(arg.toString()); break; > - } > - } > - return ls; > -} > +//KLocalizedString substituteArguments(const KLocalizedString &kls, const > QVariantList &arguments, int max = 99) > +//{ Is this all not required anymore? REPOSITORY R252 Framework Integration REVISION DETAIL https://phabricator.kde.org/D6914 To: carewolf, cullmann, dhaumann, #frameworks Cc: cullmann, #frameworks