D6914: Switch from QtScript to QtQml

[Dominik Haumann]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170726/ec321474/attachment-0001.html>