D14822: Use QJSValue as method parameter type for the scripting interface
This revision was automatically updated to reflect the committed changes. Closed by commit R120:6e5c9e9b1666: Use QJSValue as method parameter type for the scripting interface (authored by fvogt). REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14822?vs=39676=40220 REVISION DETAIL https://phabricator.kde.org/D14822 AFFECTED FILES shell/scripting/applet.cpp shell/scripting/applet.h shell/scripting/configgroup.cpp shell/scripting/configgroup.h shell/scripting/scriptengine_v1.cpp To: fvogt, #plasma, davidedmundson Cc: mart, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D14822: Use QJSValue as method parameter type for the scripting interface
fvogt added a comment. Ping. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D14822 To: fvogt, #plasma Cc: mart, anthonyfieroni, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D14822: Use QJSValue as method parameter type for the scripting interface
fvogt added inline comments. INLINE COMMENTS > mart wrote in applet.h:70 > maybe it needs to return a QJSValue as well then? No, that works fine. My theory is that it works because as a QVariant can still be converted to QString, int etc. KConfig accesses the meta type ID directly (which is always the user type ID of QJSValue) and doesn't check for "is_convertible" or similar, so does not know what to do with it. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D14822 To: fvogt, #plasma Cc: mart, anthonyfieroni, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D14822: Use QJSValue as method parameter type for the scripting interface
mart added inline comments. INLINE COMMENTS > applet.h:70 > public Q_SLOTS: > -virtual QVariant readConfig(const QString , const QVariant = > QString()) const; > -virtual void writeConfig(const QString , const QVariant ); > -virtual QVariant readGlobalConfig(const QString , const QVariant > = QString()) const; > -virtual void writeGlobalConfig(const QString , const QVariant > ); > +virtual QVariant readConfig(const QString , const QJSValue = > QString()) const; > +virtual void writeConfig(const QString , const QJSValue ); maybe it needs to return a QJSValue as well then? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D14822 To: fvogt, #plasma Cc: mart, anthonyfieroni, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D14822: Use QJSValue as method parameter type for the scripting interface
fvogt added a comment. A different approach of fixing this is to do something like if(value.userType() == qMetaTypeId()) value = value.value().toVariant(); in every function for every QVariant parameter. Advantages are that it's more obvious and the function's interface does not change. Disadvantages are that it has more overhead and more lines of code. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D14822 To: fvogt, #plasma Cc: anthonyfieroni, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D14822: Use QJSValue as method parameter type for the scripting interface
fvogt added inline comments. INLINE COMMENTS > anthonyfieroni wrote in applet.cpp:139 > So writeEntry gets QJSValue& now why you call again toVariant(), here > everywhere else. KConfigGroup != ConfigGroup. d->configGroup is from kconfig and does not take QJSValue. That's the core of the bugfix. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D14822 To: fvogt, #plasma Cc: anthonyfieroni, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D14822: Use QJSValue as method parameter type for the scripting interface
anthonyfieroni added inline comments. INLINE COMMENTS > applet.cpp:139 > > -d->configGroup.writeEntry(key, value); > +d->configGroup.writeEntry(key, value.toVariant()); > d->configDirty = true; So writeEntry gets QJSValue& now why you call again toVariant(), here everywhere else. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D14822 To: fvogt, #plasma Cc: anthonyfieroni, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D14822: Use QJSValue as method parameter type for the scripting interface
fvogt created this revision. fvogt added a reviewer: Plasma. Herald added a project: Plasma. fvogt requested review of this revision. REVISION SUMMARY If a slot or Q_INVOKABLE has a QVariant as parameter and gets called from a QJSEngine's script, it receives a QJSValue wrapped as QVariant. To get a QVariant with the actual value wrapped, calling QJSValue::toVariant is necessary. I'm not entirely sure whether this is intentional behaviour of QJSEngine, but even if it's a bug we'll have to workaround it. BUG: 397338 TEST PLAN I have favorites in kickoff again. REPOSITORY R120 Plasma Workspace BRANCH master REVISION DETAIL https://phabricator.kde.org/D14822 AFFECTED FILES shell/scripting/applet.cpp shell/scripting/applet.h shell/scripting/configgroup.cpp shell/scripting/configgroup.h shell/scripting/scriptengine_v1.cpp To: fvogt, #plasma Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
