Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Qt JSON types support #6

Merged
merged 3 commits into from
May 21, 2020
Merged

Qt JSON types support #6

merged 3 commits into from
May 21, 2020

Conversation

buschmann23
Copy link
Collaborator

This adds support for Qt’s JSON types (QJsonDocument, QJsonObject, QJsonArray, QJsonValue) as proposed in the comment of PR #5.

The difficulty in using Qt’s JSON data types was that they can be casted to QVariantList/QVariantHash if they are inside a QVariant and e.g. QVariant::canConvert<QVariantList> returns true for a QJsonArray, but the next step by Cutelee is to cast them into QAssociativeIterable or QSequentialIterable. That only works for template based containers and ends with a segfault for the JSON types.

So, my solution is to “intercept“ this types before the QVariant cast is tried and convert them directly into QVariantList or QVariantHash. If the JSON type is direct part of the context, this happens in Cutelee::Variable::resolve(), if the JSON type is stored in a context variable’s property or further down the road, the lookup will be done in Cutelee::MetaType::lookup().

Unit tests are currently written to test “normal“ usage as variable and to be used in a {% for %} loop tag. Maybe we should add some more tests to see if they play nicely with filters and the rest, even though I think that this implementations should make this possible without further changes.

This adds support for Qt’s JSON types as proposed in the comment of PR cutelyst#5.

The difficulty in using Qt’s JSON data types was that they can be
casted to QVariantList/QVariantHash if they are inside a QVariant and
e.g. `QVariant::canConvert<QVariantList>` returns true for a QJsonArray,
but the next step by Cutelee is to cast them into QAssociativeIterable
or QSequentialIterable. That only works for template based containers.

So, my solution is to “intercept“ this types before the QVariant cast is
tried and convert them directly into QVariantList or QVariantHash. If
the JSON type is direct part of the context, this happens in
`Cutelee::Variable::resolve()`, if the JSON type is stored in a context
variable’s property or further down the road, the lookup will be done in
`Cutelee::MetaType::lookup()`.

Unit tests are currently written to test “normal“ usage as variable and
to be used in a `{% for %}` loop tag. Maybe we should add some more tests
to see if they play nicely with filters and the rest, even though I
think that this implementations should make this possible without
further changes.
@buschmann23 buschmann23 requested a review from dantti May 20, 2020 10:21
@dantti
Copy link
Member

dantti commented May 20, 2020

Cool thanks, I think it would be best to remove the calls to make it a QVariantList/Hash, this would avoid creating a container each time a propriety is requested, you can use QJsonValue::toVariant() to return the result.

Make the switch a bit simpler and convert directly to QVariant.
@buschmann23
Copy link
Collaborator Author

I now simplified the switch in Variable::resolve() and use QJsonValue::toVariant() there. For MetaType::lookup() it is imho currently not possible, as I rely on the exact type and would have to convert in the specialized lookup functions anyway.

@@ -106,12 +110,89 @@ static QVariant doQobjectLookUp(const QObject *const object,
return object->property(property.toUtf8().constData());
}

static QVariant doVariantListLookUp(const QVariantList &list,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant was here (and on the hash lookup), change the type to a QJsonArray and return the QJsonValue::toVariant() of it

@dantti
Copy link
Member

dantti commented May 21, 2020

great thanks!

@dantti dantti merged commit 2e7317d into cutelyst:master May 21, 2020
@buschmann23 buschmann23 deleted the jsontypes branch May 22, 2020 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants