Add QML interface to Task<T> by jbruechert · Pull Request #110 · qcoro/qcoro

jbruechert · 2022-08-14T01:50:26Z

Contributes to #93

At this point the code allows to write the following:

    Q_INVOKABLE QCoro::QmlTask test() {
        return []() -> QCoro::Task<QString> {
             co_return co_await QtConcurrent::run([]() {
                return QStringLiteral("Hello World");
             });
         }();
    }

let future = CustomQmlType.test()
future.then((val) => console.log("future: " + val))

While not as nice as proper support for the JavaScript async / await syntax, this allows to move forward on this.

However there are a number of problems with my implementation:

Needs private APIs starting from Qt6, since QJSValue::engine doesn't exist anymore. Maybe there's a new API for this, but I wasn't able to find it yet.
the use of const_cast, maybe it's be possible to get rid of it

Although this is still a draft for the reasons mentioned above, I already want to gather some feedback.

danvratil

Hi Jonah,

thanks a lot for digging into QML support and for submitting a patch! I like the general direction, this is something I've been thinking about myself, but the major drawback I see for this solution is the fact that you need to create a wrapper class every class that uses QCoro::Task<T> that you want to expose to QML.

The idea I had, and one which would also solve the problem with access to QJSEngine, is to make use of QQmlEngineExtensionPlugin. By overriding QQmlEngineExtensionPlugin::initializeEngine(QQmlEngine*) we would 1) get access to the engine pointer and 2) would register a QCoro QObject with awaitTask() method, which would be able to receive QCoro::Task<T> (inside a QVariant) and we would be able to wrap it into your QmlTask, which then allows attaching a then() continuation from JavaScript.

class QCoroQml : public QObject {
    Q_OBJECT
public:
    Q_INVOKABLE QCoro::QmlTask awaitTask(const QVariant &task) {
        // 1) validate task actually holds a QCoro::Task<??>
        // 2) get `QCoro::Task<??>` from the variant
        // 3) ???
        // 4) return QmlTask wrapping the original coroutine
    }
};

The problem here is that you cannot extract QCoro::Task<T> from the QVariant to just wrap it into QQmlTask, since you don't know the T at runtime. The solution here would be to refactor QCoro::Task<T> to have a non-templated QCoro::TaskBase base-class (pointer to which we can get by reinterpret_cast<QCoro::TaskBase*>(variant.data())) and somehow (this is the part I haven't thought through yet) "bridge"/"wrap"/"convert"/??? it to the QmlTask.

What do you think of such approach?

Usage:

C++

class MyCppClass : public QObject {
    Q_OBJECT
public:
    ...
    Q_INVOKABLE QCoro::Task<int> myAsyncOperation();
};

QML:

QCoro.awaitTask(myCppObject.myAsyncOperation()).then((result) => console.log(result))

qcoro/qml/qcoroqmltask.cpp

jbruechert · 2022-08-14T13:32:17Z

What do you think of such approach?

I quite like your proposed API, but I see a number of problems with implementing it.

I can't really think of a way to map a Task<T> to Task<QVariant> once it has moved through QVariant::fromValue(), since I think that would require special-casing the conversion for each T by using QVariant::canConvert numerous times.

Doing this conversion before going through QVariants allows to use templates and do everything automatically.
Allowing to pass any QCoro::Task<T> to QML would also require to register a metatype for each template instantiation, which is quite error prone and hard to work with.

For my usecase (use in applications), which I think is currently the one that most people share, it is not required to have functions that can be called from C++ and from QML.
When using C++20 and QCoro in libraries becomes more popular, this might change. For applications, most of the time you define a specific QML API for your backend anyway, and writing QmlTask instead of QCoro::Task<T> in that class shouldn't be much of an issue, and it solves getting rid of the type parameter.

The QML engine extension is probably nevertheless a good idea, to

Register the metatype automatically removing the need to call QCoro::Qml::registerTypes()
get a pointer to the QJSEngine. Passing the pointer to QmlTask would need some kind of global instance though as far as I can see. A function like https://doc.qt.io/qt-5/qjsengine.html#qjsEnginex would be awesome, but it only seems to exist for QObject * and not for Q_GADGETs

danvratil · 2022-08-14T15:39:48Z

The approach I outlined works (see https://gist.github.com/danvratil/4636d2dc1b174275eb2d4e4f22cf4c90), but only in Qt6 - in Qt5 we would need to register QCoro::Task<> to the metatype system, which doesn't really work (QCoro::Task is not copyable).

So for Qt6 we can easily do

qCoro(myCppObject.myAsyncCall()).then((result) => console.log(result))

For Qt5, if it's really not possible to work around the issue with metatypes, I'm fine to go your way, that is forcing QmlTask in C++ API. It would work in Qt6 too, but Qt6 would additionally allow the neat qCoro() syntax.

While you might be right, that it's mostly applications using QCoro now, that's not an excuse for not designing the API properly

jbruechert · 2022-08-14T15:48:59Z

Thank you for the example code, I'll have a more in depth look later.
My knowledge on metatypes is currently still stuck in Qt5 times, so I'll need to check what changed :)

Adding the qCoro() API for Qt6 seems like a good way to go

danvratil · 2022-08-14T17:01:27Z

I'm no metatype wizard either, I knew this worked for Qt6, because I already did this PoC a while ago, but I only found out now it doesn't work in Qt5 :-)

Just to be clear, there's no need to build everything at once, it's fine to merge a smaller foundational code and work on the remaining bits later on, or leave it up to someone else (e.g. qCoro for Qt6) if some particular detail is of no interest or use to you.

jbruechert · 2022-08-15T18:26:23Z

What would be the minimum amount of changes to this to consider it mergable?
I guess it should not block the implementation of a fully qml side task watcher, since that doesn't even necessarily have to be the same type.

If needed, I can probably hack around the private API usage by storing a pointer to the QJSEngine in a global static. I would need to be documented that only one can be used at a time then.

danvratil

I think the current solution is good enough for merging as v1, even with the private API usage in Qt6. The QmlTask can be used as you intended, I think it's OK and we can improve on it later iteratively.

Could you please also add documentation for QmlTask as part of the PR?

I would like to have some tests for the feature as well, but I'm fine with merging this without tests now, and adding them later (before the next release, though).

.github/workflows/generate-matrix.py

qcoro/qml/CMakeLists.txt

qcoro/qml/qcoroqmltask.h

jbruechert · 2022-08-16T14:17:03Z

This is still missing documentation, I'll try to write that soon.

Future work is QCoro integration See also: qcoro/qcoro#110 Contributes to: https://invent.kde.org/plasma-mobile/plasma-dialer/-/issues/36

danvratil · 2022-10-09T13:57:43Z

Thanks for adding the docs. Would you be willing to look into some basic tests? Otherwise I think it's OK to mark this as Ready for Review, so we can merge it and I can add in some tests later :)

jbruechert · 2022-10-09T21:45:37Z

There is now just one test failure left, apparently specific to windows and clang. I don't understand where it comes from yet, and expect it to be a Qt or clang bug.

danvratil · 2022-10-10T13:44:08Z

Looks like a Qt bug: https://bugreports.qt.io/browse/QTBUG-91768. I'm trying to reproduce on a Windows VM and build some minimal reproducer.

danvratil

Thanks a lot, just a few small nitpicks after a detailed look, then we can merge it.

I'll try to reproduce the Qt bug in my Windows VM and see if there's any possible workaround (once I see where exactly the issue inside Qt is), but that's totally not a blocker for merging this.

tests/qcoroqmltask.cpp

docs/reference/qml/qmltask.md

qcoro/CMakeLists.txt

qcoro/qml/qcoroqml.h

qcoro/qml/qcoroqmltask.cpp

danvratil

Many thanks!

danvratil · 2022-10-12T08:21:51Z

FTR: the bug with Clang on Windows has been fixed in Qt 6.4.

a-andreyev · 2022-11-16T11:41:10Z

awesome! 🎉 is it makes sense to introduce a new release? 🤔

danvratil · 2022-11-16T20:18:35Z

Agreed, new release is long overdue.

xvitaly · 2022-11-21T14:03:39Z

Using Qt private API is a very bad idea. They have no ABI compatibility between releases and GNU/Linux maintainers will need to rebuild this library after every Qt update (even minor).

jbruechert · 2022-11-21T14:22:57Z

At this point it seems like the alternative is not having this feature. Distros can still just not build it, if nothing they want to package depends on it

xvitaly · 2022-11-21T14:31:12Z

Distros can still just not build it, if nothing they want to package depends on it

Don't build the entire QML module? I doubt this is a good idea.

jbruechert · 2022-11-21T14:34:26Z

There's nothing else in it so far.

kelteseth · 2022-11-30T07:16:45Z

Thanks for building this feature! For future reference, here is the Qt issue to make C++ returned QFuture usable with QML https://bugreports.qt.io/browse/QTBUG-101025. Maybe there can be some collaboration, so we do not have to use a private Qt api :)

danvratil reviewed Aug 14, 2022

View reviewed changes

qcoro/qml/qcoroqmltask.cpp Outdated Show resolved Hide resolved

qcoro/qml/qcoroqmltask.cpp Show resolved Hide resolved

jbruechert force-pushed the qml-task branch 2 times, most recently from df39178 to 9aadd58 Compare August 14, 2022 13:19

jbruechert force-pushed the qml-task branch from b96f248 to 41bdc6a Compare August 14, 2022 15:36

jbruechert force-pushed the qml-task branch from b6772ef to eb74300 Compare August 14, 2022 21:32

danvratil requested changes Aug 16, 2022

View reviewed changes

jbruechert force-pushed the qml-task branch 3 times, most recently from cb10c13 to 0743ceb Compare August 16, 2022 14:16

jbruechert force-pushed the qml-task branch 3 times, most recently from f141b0e to 3745def Compare October 1, 2022 21:08

Add QML interface to Task<T>

dc6009d

jbruechert force-pushed the qml-task branch 5 times, most recently from 1c8d3b5 to 3acc542 Compare October 9, 2022 21:31

jbruechert marked this pull request as ready for review October 9, 2022 21:41

jbruechert force-pushed the qml-task branch from 3acc542 to 11fc880 Compare October 9, 2022 22:04

danvratil requested changes Oct 10, 2022

View reviewed changes

tests/qcoroqmltask.cpp Outdated Show resolved Hide resolved

docs/reference/qml/qmltask.md Show resolved Hide resolved

qcoro/CMakeLists.txt Outdated Show resolved Hide resolved

qcoro/qml/qcoroqml.h Show resolved Hide resolved

qcoro/qml/qcoroqmltask.cpp Outdated Show resolved Hide resolved

Add test for QmlTask

2361701

jbruechert force-pushed the qml-task branch from 11fc880 to 2361701 Compare October 10, 2022 16:38

jbruechert added 3 commits October 10, 2022 18:40

QCoroQml: Automatically include other headers

d1b19ea

Move capture func

f86a6cd

Remove unueeded header from cmake

3cc339a

jbruechert force-pushed the qml-task branch 2 times, most recently from c0637e5 to 2c26405 Compare October 10, 2022 20:23

Fix defined check

7fc8f2c

jbruechert force-pushed the qml-task branch from 2c26405 to 7fc8f2c Compare October 10, 2022 20:34

jbruechert added 2 commits October 10, 2022 22:44

Improve docs

cb57974

Allow mutable then callbacks

e20cf68

danvratil approved these changes Oct 11, 2022

View reviewed changes

danvratil merged commit b7d510a into qcoro:main Oct 11, 2022

jbruechert deleted the qml-task branch October 12, 2022 10:30

Uh oh!

Conversation

jbruechert commented Aug 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danvratil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jbruechert commented Aug 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danvratil commented Aug 14, 2022

Uh oh!

jbruechert commented Aug 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danvratil commented Aug 14, 2022

Uh oh!

jbruechert commented Aug 15, 2022

Uh oh!

danvratil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jbruechert commented Aug 16, 2022

Uh oh!

danvratil commented Oct 9, 2022

Uh oh!

jbruechert commented Oct 9, 2022

Uh oh!

danvratil commented Oct 10, 2022

Uh oh!

danvratil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danvratil left a comment

Choose a reason for hiding this comment

Uh oh!

danvratil commented Oct 12, 2022

Uh oh!

a-andreyev commented Nov 16, 2022

Uh oh!

danvratil commented Nov 16, 2022

Uh oh!

xvitaly commented Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbruechert commented Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xvitaly commented Nov 21, 2022

Uh oh!

jbruechert commented Nov 21, 2022

Uh oh!

kelteseth commented Nov 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jbruechert commented Aug 14, 2022 •

edited

Loading

jbruechert commented Aug 14, 2022 •

edited

Loading

jbruechert commented Aug 14, 2022 •

edited

Loading

xvitaly commented Nov 21, 2022 •

edited

Loading

jbruechert commented Nov 21, 2022 •

edited

Loading