Created attachment 89252 [details]
Testcase

> I can't actually find anything wrong with scripting.{cpp,h} in KWin (though
> I'm certainly no C++ master), so this could also be a Qt upstream bug.

no it's a bug in scripting. The methods are only installed for QScriptEngine 
which doesn't exist for QML any more.

With 5.2 (not sure about 5.1), I can get the following:

> qml: TypeError: Property 'registerShortcut' of object KWin::JSEngineGlobalMethodsWrapper(0x29efb10) is not a function

by calling "KWin.registerShortcut" instead of plain "registerShortcut".

Same thing for "registerUserActionsMenu".

(In reply to Martin Gräßlin from comment #2)
> 
> no it's a bug in scripting. The methods are only installed for QScriptEngine 
> which doesn't exist for QML any more.

Any news?

sorry no, but I just added it on my todo list.

(In reply to Martin Gräßlin from comment #5)
> sorry no, but I just added it on my todo list.

Is it still on the TODO list or already on "Done"? ; )

(In reply to paul from comment #6)
> (In reply to Martin Gräßlin from comment #5)
> > sorry no, but I just added it on my todo list.
> 
> Is it still on the TODO list or already on "Done"? ; )

still on TODO list and that one is long, sorry

It is pretty unfair. I wanted to go to KWin as everyone going to Wayland, but that bug means that tiling script is broken, so I guess I have to stick either to Sway, or to search whether gnome has tiling. It is very sad, everytime I try to work with KDE, something wrong with it.

I really want to fix this myself, if a KDE dev could provide some brief information on how to approach the solution for this (i.e. relevant source files or changelog entries) that would be great.

A bunch of my code is already in KDE (from 7 years ago), pretty sure I could handle this one.

the relevant code is in kwin/scripting/scripting.cpp

any progress on this ? we need this for kwin-tiling

I'd like to be able to but I need a bit more info about why the existing binding code doesn't work and a sentence or two describing what's needed. I assumed from Martin's terse response that he's too busy to be able to help me so don't get your hopes up. I do have the skills to fix this though, just need a bit more knowledge.

if you have any specific questions please ask. I thought that this would be sufficient as a starter.

> we need this for kwin-tiling

why does kwin-tiling need the declarative script? Does it do any user interface? If not it should use the qscript based scripting where this is still available.

>why does kwin-tiling need the declarative script? Does it do any user interface? If not it should use the qscript based scripting where this is still available.

Currently for Qt.include - the script is about 3000 lines total, and that's a bit much for one file.

Adding a tiling-layout switcher is on my (distant) TODO, however.

I've written the code for it but the code necessary to handle calling a declarative js function from C++ seems a bit complicated. I'm trying to find another example in the KDE code so I can simplify this.

Am 23. Oktober 2016 18:14:34 MESZ, schrieb via KDE Bugzilla <bugzilla_noreply@kde.org>:
>https://bugs.kde.org/show_bug.cgi?id=340225
>
>--- Comment #17 from kdebugs@chilon.net ---
>I've written the code for it but the code necessary to handle calling a
>declarative js function from C++ seems a bit complicated. I'm trying to
>find
>another example in the KDE code so I can simplify this.

Have a look at JSEngineGlobalMethodsWrapper. Any slot in this class is exported in the object KWin in declarative scripts. That's different to the normal scripts, but iirc I introduced this class as a solution to the problem that we cannot export method globally

Yep, I added the shortcut registration to this class as a slot. I use the QJSValue class to accept the callback and then use setObjectOwnership to keep it alive in C++ world. I moved the stuff relating to shortcuts out of AbstractScript and into Script. Then I wrote similar versions that use QJSValue objects (instead of QScriptValue) objects in DeclarativeScript. Just testing it now.

Finished the code but trying to test the KDE I built but keep getting "All shell packages missing. This is an installation issue, please contact your distribution." I built plasmashell but maybe this isn't the "shell package" it is looking for.

Got it running but can't find the `console.log` messages from my test script anywhere so not sure if it is working. Guide online seems to be out of date, found info on reddit on how to bring up the interactive console but don't see anything logged.

Okay managed to get it working eventually... needed to manually remove a plasmoid directory to get the script to refresh so I could see my logs. The review is here: https://git.reviewboard.kde.org/r/129250/

An idea what could be done in addition. We could provide a declarative way to export the shortcuts:
GlobalShortcut {
    shortcut: "Ctrl+Alt+1"
    onTriggered: console.log("triggered")
}

Thanks for reviewing so quickly. Yes I'd considered adding a declarative method but I'd like to get this bug sorted first so that the kwin-tiling guys can use my work. I'll submit a future patch for a declarative version... if that would be okay? First I want to work on plasmapkg2 removal of kwinscripts as without this developing scripts is quite painful.

> if that would be okay?

yes of course. I also want to have the script API fixed. It was just an idea I got for in addition.

Created attachment 101782 [details]
Can be used to test registerShortcut in declarativescript, works with my latest patcth which is currently in review

Git commit 0c4c529d6884c101cfe7aee05557f18dc86802e0 by James Pike.
Committed on 30/10/2016 at 00:15.
Pushed by pikejames into branch 'Plasma/5.8'.

Support for KWin.registerShortcut() in declarative script

Summary:
registerShortcut is available to javascript KWin scripts but was not
available to those written in declarative script.
FIXED-IN: 5.8.4
REVIEW: 129250

M  +22   -0    scripting/scripting.cpp
M  +2    -0    scripting/scripting.h

http://commits.kde.org/kwin/0c4c529d6884c101cfe7aee05557f18dc86802e0