Details

Reviewers

mart
carewolf
fvogt

Group Reviewers

KWin

Summary

QScriptEngine is deprecated for years and suffers bitrot.
Plasma hit one super major bug with it in 5.11.0 and has now ported
away.

Main porting notes:

creating low level functions no longer exists The old global functions are exposed on the ScriptedEffect instance

and then the QJSValue wrappers of the globalObject are modified to
trampoline the methods at a wrapper level.

We can then use QJSEngine to automatically do argument error checking

rather than unmarshalling a QJSValue manually which significantly
reduces a lot of code.

We can't make FPX2 a native type, so these are QJSValue args and

unboxed there.

Long term I want overloads for animate that take int/QSize/QPoint which
are native JS types, but that might be an API break.

One can no longer throw errors from C++, instead you can return an

error object which will then be thrown when we're back in JS space

QList<QObjectDerived*> is not understood by QJSEngine. Converting to

QList<QObject*> was the only sane solution I found.

Test Plan

Hopefully comprehensive unit test which passes
Tested fade/fadeDesktop manually.

It's a very invasive change, so I expect some things will be broke
please help test any JS effects.

Diff Detail

Repository

R108 KWin

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 1428
Build 1446: arc lint + arc unit

davidedmundson created this revision.Aug 1 2018, 12:22 PM

Restricted Application added a project: KWin. · View Herald TranscriptAug 1 2018, 12:22 PM

Restricted Application added a subscriber: kwin. · View Herald Transcript

davidedmundson requested review of this revision.Aug 1 2018, 12:22 PM

Harbormaster completed remote builds in B1428: Diff 38900.Aug 1 2018, 12:22 PM

davidedmundson added a reviewer: mart.Aug 1 2018, 12:27 PM

Typo--

Harbormaster completed remote builds in B1429: Diff 38902.Aug 1 2018, 12:41 PM

Interesting(or not?) discovery: if I run the following code

print('random stuff');
dragon.game.of.thrones.lu.lu(); // dragon is not declared

I get only

"/home/vlad/KDE/usr/share/kwin/effects/kwin4_effect_fade/contents/code/main.js" : "random stuff"

no errors, etc. With QScriptEngine, I get the following error

kwin_scripting: KWin Effect script encountered an error at [Line  93 ]
kwin_scripting: Message:  "ReferenceError: Can't find variable: dragon"
kwin_scripting:   "message" :  "Can't find variable: dragon"
kwin_scripting:   "lineNumber" :  "93"
kwin_scripting:   "sourceId" :  "139731997129776"
kwin_scripting:   "fileName" :  ""
kwin_scripting:   "expressionBeginOffset" :  "3600"
kwin_scripting:   "expressionCaretOffset" :  "3606"
kwin_scripting:   "expressionEndOffset" :  "3606"

Yeah, error handling is different. I had filed https://bugreports.qt.io/browse/QTBUG-69764 hopefully I'll hear soon.

There's another bug that's hard to fix.
EffectWindow::mainWindows has the same issue that EfffectHandler::stacking order has; only it's not an invokable method not a property so doing a change that doesn't break C++ API is harder.

easing curve changes

Harbormaster completed remote builds in B1586: Diff 39235.Aug 6 2018, 11:13 PM

In D14536#303767, @davidedmundson wrote:

There's another bug that's hard to fix.
EffectWindow::mainWindows has the same issue that EfffectHandler::stacking order has; only it's not an invokable method not a property so doing a change that doesn't break C++ API is harder.

Do you know why EffectWindowList can't be understood by QJSEngine? (sorry, if a stupid question, still learning this thing)

libkwineffects/kwineffects.cpp
770	sourceList?
scripting/scriptedeffect.cpp
208	IMHO, for the sake of consistency, we have to use either m_engine or engine().
247	What about ConsoleExtension(http://doc.qt.io/qt-5/qjsengine.html#Extension-enum)? Can it be used?
466–471	QAction *a
469	Please put whitespace around = and <.
490	Why foreach?
494	I think we don't need MetaTypeMap. Also, FWIW, in order to guarantee that metaTypes is initialized in thread safe manner, compilers add a synchronization primitive https://godbolt.org/g/YeZ3by. It won't impact performance too much in our case, but still, it would be great to do as less as possible. Also, the initializer list is too deep. It would be great to reduce indentation.
504	auto.
534	No whitespace before ';'.
555	Somewhat unrelated change.
569	Please put whitespace between for keyword and '(' and before ':'. Also, please use qAsConst.
575	Unrelated change.

Do you know why EffectWindowList can't be understood by QJSEngine? (sorry, if a stupid question, still learning this thing)

We see this all over QV4Engine:

else if (type == qMetaTypeId<QList<QObject *> >()) {
//make it an array
}

the metatype of QList<SomeOtherQObjectDerived*> is different. So it's just provided to JS space as an opaque pointer that it can't interact with.

What about ConsoleExtension

We could and maybe should moving forwards, but it'd be an API break to deprecate print. I couldn't find a way to proxy from one to the other.

scripting/scriptedeffect.cpp
490	it's a copy paste from scripting.cpp but ported to QJSValue same for the comments in startAnimation which is just moving the old ::animationSettings but I had to merge it with another method due to throwing errors being different. I may as well modernise it in the port though. Will adjust.

In D14536#304767, @davidedmundson wrote:

We could and maybe should moving forwards, but it'd be an API break to deprecate print. I couldn't find a way to proxy from one to the other.

It looks like it provides print. https://code.woboq.org/qt5/qtdeclarative/src/qml/qml/v8/qqmlbuiltinfunctions.cpp.html#1798
Also, QJSEngine documentation mentions it, so it's not private API.

There are uses of QVariant in method parameters, so this is most likely affected by https://bugs.kde.org/show_bug.cgi?id=397338

This revision now requires changes to proceed.Aug 14 2018, 10:04 AM

I don't think there's an issue in this case.

The only qvariant is readConfig's default param. If we get a QJSValue in, we get a QJSValue out and that does get unboxed correctly.

print(effect.readConfig("foobar", 4));
print(effect.readConfig("foobar", [4,2]));

gives an int and a JS array respectively

Use inbuilt print. Seems to work
Fix the QList<QObjectDerived*> problem in EffectWindow::mainWindows
Misc other changes

Harbormaster completed remote builds in B2024: Diff 40058.Aug 20 2018, 3:19 PM

zzag added inline comments.Aug 20 2018, 4:04 PM

libkwineffects/kwineffects.h
2177–2178	We're breaking third-party effects, that's not good.

Rambling comment--

Harbormaster completed remote builds in B2025: Diff 40061.Aug 20 2018, 4:19 PM

It does. It's not ideal.
I don't think there's any other neat solution.

scripting/scriptedeffect.cpp
247	good idea, seems to work.

@fvogt I addressed your specific concern in a comment above

fvogt added inline comments.Sep 4 2018, 7:56 AM

libkwineffects/kwineffects.h
2228	@davidedmundson This might be one of the broken cases - please check

As an alternative approach, we could wrap EffectsHandler and EffectWindow.

Pros:

We don't touch C++ API;

Cons:

Sometimes we would need to copy API.

PoC: https://github.com/zzag/kwin/commits/port-scriptedeffects-to-qjsengine-wrapper

zzag added inline comments.Sep 11 2018, 9:05 AM

scripting/scriptedeffect.cpp
38	I suppose we don't need it anymore.
49–50	Is it a hint? KFx2 => FPx2?
193	Is it correct? The third argument has to be a window.
228–230	Should it really be a qCDebug? Maybe qCCritical?
298	Don't need qAsConst, animationIds is already const. Also, maybe `const int &animationId`.
315	QStringLiteral
318	Can we use an enum instead of the boolean trap?
433	Same here.
490	The value() will return a reference to a const list, thus we don't need qAsConst. Also, whitespace between for keyword and `(`, and before `:`.
512	(edge, {callback})

As an alternative approach, we could wrap EffectsHandler and EffectWindow.

You can just describe your ideas in words without having to create a full working implementation.

It's definitely technically an option, personally I had ruled as we'd got so far without having to shadow anything...

My other options I've considered are:

https://paste.kde.org/pfu7nznz6
In Qt replace if (type == qMetaTypeId<QList<QObject *> >()) { with if (QMetaType::hasRegisteredConverterFunction(type, qMetaTypeIdQList<QObject*>>) in the billion places. Then merge this in 18 months.

Ultimately libkwineffects has no API or ABI promises so we don't need to create too much of a maintenance burden on ourselves if we don't need to.

fvogt added inline comments.Sep 11 2018, 9:10 AM

scripting/scriptedeffect.cpp
436	This always returns true - is that intentional? I assume this should've been `ok &= cancel(...)`.

davidedmundson marked 10 inline comments as done.Sep 11 2018, 10:51 AM

davidedmundson added inline comments.

scripting/scriptedeffect.cpp
228–230	The old code was a qcdebug. I remember trying to change it once in the past and I had that change blocked.
436	heh, old code quit on first error I'll change to that.

misc review comments

Harbormaster completed remote builds in B2712: Diff 41390.Sep 11 2018, 10:51 AM

davidedmundson added inline comments.Sep 11 2018, 11:14 AM

libkwineffects/kwineffects.h
2228	There are two types of role GrabRole which should be set to a pointer to the object For a Scripted effect: w.setData(Effect.WindowClosedGrabRole, effect); resulted in QVariant(QObject*, ScriptedEffectWithDebugSpy(0x7f7f60006190)) which looks fine The other roles (such as WindowForceBackgroundContrastRole) should be a boolean w.setData(Effect.WindowForceBackgroundContrastRole, true); which resulted in QVariant(bool, true) which also looks fine Obviously one can do something stupid like setData(["asdfasf", 2]) which results in a QJSValue but unboxing won't help.

fvogt resigned from this revision.Sep 11 2018, 11:20 AM

fvogt added inline comments.

libkwineffects/kwineffects.h
2228	Hm, so it is affected. If a new role with a QStringList gets instroduced (or even a QVariantList) it might stop working (or behave unexpectedly). Currently it shouldn't cause any issues though.

In D14536#323772, @davidedmundson wrote:

My other options I've considered are:

https://paste.kde.org/pfu7nznz6

Hmm, really tricky option. :D

In Qt replace if (type == qMetaTypeId<QList<QObject *> >()) { with if (QMetaType::hasRegisteredConverterFunction(type, qMetaTypeIdQList<QObject*>>) in the billion places. Then merge this in 18 months.

18 months because the change would be that big?

Ultimately libkwineffects has no API or ABI promises so we don't need to create too much of a maintenance burden on ourselves if we don't need to.

Still, it would be nice to keep source-code compatibility. FWIW, there are some third party effects. For example, today when I was browsing reddit, I stumbled upon a C++ effect that rounds corners.

I don't like that we're polluting C++ API just because of scripting. Is it a good idea in long term?

18 months because the change would be that big?

Because it would be Qt 5.13 at the earliest, then we have to wait for us to depend on that version which takes forever.

Ultimately libkwineffects has no API or ABI promises so we don't need to create too much of a maintenance burden on ourselves if we don't need to.

I don't like that we're polluting C++ API just because of scripting.

It don't exactly like it either, but that doesn't make the other options any better, especially long term.

Right now our options are:
Using a deprecated module, a monsterous amount of shadowing, tricking Qt's moc, a single change in the C++ API, or alternatively breaking the JS API (it arguably should have been a property anyway)

Given we're not in an urgent rush, we do have the option of deprecating something before dropping.

BTW I can fairly easily move the QList<QObject*> mainWindows() out of EffectWindow so it's not "polluting", I can put the invokable in EffectWindowImpl or in a class inbetween.
But I still need to rename that existing method there.

Prodded further into fixing Qt. One normally should be using QQmlListProperty<QObjectDerived*> instead of QList. Given they have a mechanism there's no way I'd get a change in.

Doesn't help us in our case.

Just an idea: duplicate all the code and keep the QScript variant to have backwards compatibility. Add the new qml based one and require a desktop file entry to be used there. If we need to break the API anyway, because not everything is 1:1 mapped, let's break it properly and do some cleanup in the API. And then with Qt 6 or removal of QScript we switch over. That would give every user some time to update the scripts.

Note its effect window with the problem, I don't want to duplicate that

davidedmundson mentioned this in T9228: Port KWin scripts from QtScript.Oct 15 2018, 12:00 PM

Tonnes of new API changes :/

davidedmundson added a reviewer: carewolf.Nov 22 2019, 3:21 PM

			Path	Packages
M			autotests/CMakeLists.txt (2 lines)
M			autotests/integration/effects/scripted_effects_test.cpp (39 lines)
M			autotests/test_scripted_effectloader.cpp (7 lines)
M			libkwineffects/kwineffects.h (7 lines)
M			libkwineffects/kwineffects.cpp (13 lines)
M			scripting/scriptedeffect.h (77 lines)
M			scripting/scriptedeffect.cpp (737 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	38900	5d279a0		Aug 1 2018, 12:22 PM	★	★
Diff 2	38902	5d279a0	Typo--	Aug 1 2018, 12:41 PM	★	★
Diff 3	39235	1df27f8	easing curve changes	Aug 6 2018, 11:12 PM	★	★
Diff 4	40058	cca1140	Use inbuilt print. Seems to work	Aug 20 2018, 3:19 PM	★	★
Diff 5	40061	cca1140	Rambling comment--	Aug 20 2018, 4:19 PM	★	★
Diff 6	41390	03a2a05	misc review comments	Sep 11 2018, 10:51 AM	★	★

[scripting] Port ScriptedEffects to QJSEngine
Changes PlannedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 38900

autotests/CMakeLists.txt

autotests/integration/effects/scripted_effects_test.cpp

autotests/test_scripted_effectloader.cpp

libkwineffects/kwineffects.h

libkwineffects/kwineffects.cpp

scripting/scriptedeffect.h

scripting/scriptedeffect.cpp

[scripting] Port ScriptedEffects to QJSEngineChanges PlannedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 38900

autotests/CMakeLists.txt

autotests/integration/effects/scripted_effects_test.cpp

autotests/test_scripted_effectloader.cpp

libkwineffects/kwineffects.h

libkwineffects/kwineffects.cpp

scripting/scriptedeffect.h

scripting/scriptedeffect.cpp

[scripting] Port ScriptedEffects to QJSEngine
Changes PlannedPublic
Actions

Revision Contents
Changeset List