D14536: [scripting] Port ScriptedEffects to QJSEngine

zzag added inline comments.

INLINE COMMENTS

> scriptedeffect.cpp:38
>  
>  typedef KWin::EffectWindow* KEffectWindowRef;
>  

I suppose we don't need it anymore.

> scriptedeffect.cpp:49
>      QEasingCurve::Type curve;
> -    FPx2 from;
> -    FPx2 to;
> +    QJSValue from; //should be a KFx2
> +    QJSValue to;

Is it a hint? KFx2 => FPx2?

> scriptedeffect.cpp:194
> +    effectsObject.setProperty("desktopChanged(int,int)", effectsObject.property("desktopChangedCompat"));
> +    effectsObject.setProperty("desktopChanged", effectsObject.property("desktopChangedCompat"));
> +

Is it correct? The third argument has to be a window.

> scriptedeffect.cpp:227
>      if (ret.isError()) {
> -        signalHandlerException(ret);
> +        qCDebug(KWIN_SCRIPTING) << "KWin Effect script encountered an error at [Line " << ret.property("lineNumber").toString() << "]";
> +        qCDebug(KWIN_SCRIPTING) << "Message: " << ret.property("message").toString();

Should it really be a qCDebug? Maybe qCCritical?

> scriptedeffect.cpp:285
> +    bool ok = true;
> +    for (int animationId: qAsConst(animationIds)) {
> +        ok |= retarget(animationId, newTarget, newRemainingTime);

Don't need qAsConst, animationIds is already const. Also, maybe `const int &animationId`.

> scriptedeffect.cpp:302
> +QJSValue ScriptedEffect::createError(const QString &errorMessage) {
> +    return m_engine->evaluate(QString("new Error('%1');").arg(errorMessage));
> +}

QStringLiteral

> scriptedeffect.cpp:305
> +
> +QJSValue ScriptedEffect::startAnimation(const QJSValue &object, bool settingPersists)
> +{

Can we use an enum instead of the boolean trap?

> scriptedeffect.cpp:425
> +    bool ok = true;
> +    for (int animationId: qAsConst(animationIds)) {
> +        ok |= cancel(animationId);

Same here.

> scriptedeffect.cpp:473
> +    if (it != screenEdgeCallbacks().constEnd()) {
> +        for(const QJSValue &value: qAsConst(it.value())) {
> +            QJSValue callback(value);

The value() will return a reference to a const list, thus we don't need qAsConst. Also, whitespace between for keyword and `(`, and before `:`.

> scriptedeffect.cpp:495
> +        ScreenEdges::self()->reserve(static_cast<KWin::ElectricBorder>(edge), this, "borderActivated");
> +        screenEdgeCallbacks().insert(edge, QList<QJSValue>() << callback);
> +    } else {

(edge, {callback})

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D14536

To: davidedmundson, #kwin, mart, fvogt
Cc: fvogt, zzag, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart