Moves away from the deprecated QtScript module
Details
Diff Detail
- Repository
- R249 KI18n
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Just wanted to add this: Really great work! -- I wanted to raise this issue earlier on the KF5 mailing list: ki18n was the last major framework that depended on the deprecated QtScript. Nice to see it being ported! <3
Can't really review this code either, though. I don't now enough about the QtScript/QtQML differences.
The changes to from QScriptValue to QJSValue look quite good (similar to the ones done in KTextEditor). I cannot really comment on the FIXMEs you have added, though.
Could someone fluent in QtScript/QtQML review this so we can merge it?
src/ktranscript.cpp | ||
---|---|---|
96 | Consistency: Use const QString & |
I can't say about QtScript to QtQML conversion, but wrt. those FIXMEs, removing variable argument lists obviously breaks compatibility, and especially the acall function makes no sense without it.
@carewolf, have you had a chance to work on this? It would be a shame for such a lovely patch to fall by the wayside. :)
No, but I will see if I can find some. Otherwise feel free to take over an fix the remaining issues.
Note the latest patch still has a maximum number of arguments for acall. I checked all the usages, and the only place that uses acall with a variable set of arguments is the sr (Serbian) translation. Since supporting a variable number of arguments would require changes to QtDeclarative and thus mean we would need to support both models until KDE depends on Qt 5.13, perhaps it would be easier to just rewrite the serbian translations apply_to_word function?
I solved the Ts.acall. Unfortunately I forgot how arc works, so it became a new entry https://phabricator.kde.org/D14769
I guess it's better to accept this now than later to have a longer testing period at Akademy and until the frameworks release.