Switch from QtScript to QtQml

Authored by carewolf on Jul 26 2017, 9:07 AM.



Replaces the QtScript depedency with QtQml.

Diff Detail

Lint Skipped
Unit Tests Skipped
carewolf created this revision.Jul 26 2017, 9:07 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 26 2017, 9:07 AM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript

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?

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.

cullmann edited edge metadata.Jul 26 2017, 9:27 AM

A private not-installed header is fine for that, sure.
I just would minimize the public API changes.

carewolf added a comment.EditedJul 26 2017, 9:28 AM

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.

dhaumann edited edge metadata.Jul 26 2017, 9:35 AM

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?


If there is no equivalent to get the backtrace, just delete this line.


If such a call is not required, please delete.


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?


If I understand correctly, you are adding an overload for all functions that have KTextEditor::Cursor or Range in the parameter.
Does that imply we could remove the Q_INVOKABLE part of the overloads that use the complex types such as Cursor and Range?

If so, I would be in favor of doing this, just to make this explicit.


Is this all not required anymore?

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.


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.

carewolf updated this revision to Diff 17223.Jul 26 2017, 10:19 AM
carewolf updated this revision to Diff 17224.Jul 26 2017, 10:25 AM
carewolf edited the summary of this revision. (Show Details)

Forgot to add the new private headers.

For me only the ruby tests fail, too.

carewolf updated this revision to Diff 17281.Jul 27 2017, 10:44 AM
carewolf edited the summary of this revision. (Show Details)

Fixed ruby tests

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!

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.

dfaure added a subscriber: dfaure.Aug 8 2017, 7:26 AM

I cherry-pick fixes into a local branch, when needed. So there's no reason to wait. It's always summer in master.

cullmann accepted this revision.Aug 8 2017, 7:26 AM

Ok, cool, then please commit this Allen ;=)

This revision is now accepted and ready to land.Aug 8 2017, 7:26 AM

@carewolf Shall we commit and push this for you?

Actually I wanted to clean a few minor things first. I will update it.

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.

carewolf updated this revision to Diff 18072.Aug 13 2017, 7:22 AM

Minor cleanups

This revision was automatically updated to reflect the committed changes.