Replaces the QtScript depedency with QtQml.
Details
Details
- Reviewers
cullmann dhaumann - Group Reviewers
Frameworks - Commits
- R39:878797830dbd: Switch from QtScript to QtQml
Diff Detail
Diff Detail
- Repository
- R39 KTextEditor
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
First: As already said in person: COOL ;=)
I failed to port some years ago and just dumped my patch on the attic.
For the changes: Do we need the helper functions in the public headers? I see that we need to make the things Q_GADGETs, thats fine, but do we need to have the Qml includes + helpers there?
Comment Actions
Actually the Q_GADGET part is not needed. I have commented out the code that registers the qmetaobject, so at this point those changes are no longer needed. I needed to do manual conversion anyway. The conversion functions doesn't need to be in public headers. But they are needed in several places, so at least in a private header.
Comment Actions
A private not-installed header is fine for that, sure.
I just would minimize the public API changes.
Comment Actions
Note all of the helper functions are now single argument, but no script we have use more. It does however remove templating in the i18n functions.
Comment Actions
First of all, I also think this is a good patch.
There are some parts with debug output, some parts with warnings (e.g. Q_FUNC_INFO), I guess this is temporary? :-)
Besides that, there are several parts with commented out code - is that work in progress, or is the commented out code not required at all anymore?
| src/script/katescript.cpp | ||
|---|---|---|
| 77 | If there is no equivalent to get the backtrace, just delete this line. | |
| 96 | If such a call is not required, please delete. | |
| 150 | If I remember correctly, Christoph once said it as quite a hassle to get this include guard working correctly. Are you sure it works or are there possibly issues due to some other behavior in QJSEngine? | |
| src/script/katescriptdocument.h | ||
| 63–65 | If I understand correctly, you are adding an overload for all functions that have KTextEditor::Cursor or Range in the parameter. If so, I would be in favor of doing this, just to make this explicit. | |
| src/script/katescripthelpers.cpp | ||
| 178 | Is this all not required anymore? | |
Comment Actions
As I said it has some instrumentation and dead-code. I will clean it up a bit. I was just hoping for a bit of help on why the ruby indent tests are being weird.
| src/script/katescripthelpers.cpp | ||
|---|---|---|
| 178 | It is not used at all in our code, not before this patch or after. I don't know if it could be used in other scripts. | |
Comment Actions
I think we should merge that after the next frameworks release it out and clean the final bits up afterwards.
Thanks a lot for the contribution!
Comment Actions
David just did his tagging work yesterday for KDE Frameworks 5.37: https://mail.kde.org/pipermail/release-team/2017-August/010528.html
That means the git shas are fixed for the next release, except if some show-stopper bug in a module forces us to pick another git sha.
This is rather unlikely in the KTextEditor.git, so it *should* be fine to commit this now, but if you want to be absolutely sure wait 3-4 more days.
Comment Actions
I cherry-pick fixes into a local branch, when needed. So there's no reason to wait. It's always summer in master.
Comment Actions
As you wish, but we'd also be fine with this commit now, and additional commits for fine tuning. The main point is to get this in as early as possible to have a longer testing period.