Concerning the test script: we can also turn that into an autotest. So far we don't have any autotests for declarative scripts yet - only for Qtscript.

scripting/workspace_wrapper.h
38	I'm wondering whether it makes that much sense to rename the class. As we have two dedicated subclasses, it looks like a lot of name changes all over the place for not much gain. Also the name is actually wrong. The class is not abstract.
202	I would make the ctor protected.
365	just wondering: why doesn't it need to be: QQmlListProperty<KWin::Client*>

jpike added inline comments.Oct 28 2016, 1:58 PM

scripting/workspace_wrapper.h
38	Cool, I'll change my patch to stick with `WorkspaceWrapper` even though it will be made abstract?
202	Good catch.
365	Yeah it's weird isn't it, but that's just how they designed QQmlListProperty. WIth the pointer template argument it doesn't work.

Responded to comments in review: Revert rename of WorkspaceWrapper from AbstractWorkspaceWrapper and make its constructor protected.

jpike marked 4 inline comments as done.Oct 29 2016, 12:33 AM

I think I've covered all of the points in the code review, I've been testing it for a few days now and it's working well, let me know if there's anything else you need or if I can push?

graesslin accepted this revision.Nov 2 2016, 7:55 AM

graesslin edited edge metadata.

This revision is now accepted and ready to land.Nov 2 2016, 7:55 AM

jpike mentioned this in R108:4730be084c1b: Support for workspace.clientList() in declarative script.Nov 3 2016, 12:32 AM

aacid closed this revision.Feb 27 2017, 12:14 AM

Revision Contents
Changeset List

			Path	Packages
M			scripting/scriptedeffect.cpp (2 lines)
M			scripting/scripting.h (23 lines)
M			scripting/scripting.cpp (13 lines)
M			scripting/workspace_wrapper.h (33 lines)
M			scripting/workspace_wrapper.cpp (30 lines)