Accessing Messages Qproperties from Qml cause GUI app to crash #272

[semlanik]

Thanks for the patchset, but I already mentioned that all works in this repo are on hold for now.

Nevertheless, I still don't see the usecase when the issue occurs. Users that return 'messages' from invokable functions must to take care about pointer ownership by their own. Also this adds the ambiguity in case if the message was created in QML context. And the last point all QML-related code need to be protected, since messages can be generated without QML feature.

[RBQt]

Hi,

"Users that return 'messages' from invokable functions must to take care about pointer ownership by their own."

Actually this function is a missing feature of this project generator option QmlPluginOption("QML"). A message with a 'repeated Message' field is unusable in the QML. It's not that the user want to return 'messages' through invokable functions for fun, it's the only way to access a message's 'repeated Message' field from the QML (doing something like mymessage.myRepeatedMessageField[i] in the Qml won't work since this call would return a QSharedPointer, unusable in the QML). So as i mentioned before the project should either have a custom QML compatible QSharedPointer to make things work or implement a missing Q_INVOKABLE getter (i say missing because it's actually part of the getters generated by the google protocol buffer generator, u can access a repeated field items by index where in this project's generator u can only retreive the repeated field itself, the QList<QSharedPointer>)

"Users that return 'messages' from invokable functions must to take care about pointer ownership by their own" Well, in this senario, the user would have to workaround the issue mentioned in the first point by setting the cpp ownership a the repeated message field items creation which is actually wot this commit does but in the generated code instead which is the purpose of this project and factoring into acount that the ownership for any message's repeated message field is by definition forced to CPP by the QSharedPointer.

"And the last point all QML-related code need to be protected, since messages can be generated without QML feature." Agree, but the way the generated code is structured make it impossible to set the ownership at any point further after the field construction. One can set the owner in the "by index Q_INVOKABLE getter" of a repeated Message field but this getter is actually missing as mentioned in the first point. To maintain the QML compatibility optional, the project's would required to add this getter in the code generation or implement a custom QML friendly QSharedPointer.

[semlanik]

Hi :) Hm would it be enough to fix qmllistpropertyAt then? I was surprised that Qml takes ownership of pointer returned there.

[semlanik]

Oh, I actually shouldn't be surprised, my memory betrayed me: https://github.com/semlanik/qtprotobuf/blob/master/src/protobuf/qqmllistpropertyconstructor.h#L35

[semlanik]

ooops, not, still surprised, since it fixes another issue :)

[RBQt]

First of all, didn't know about this header, why are the methods tagged private ? Also ur login is weird, what does it mean ? :)

Regarding the header content, i gave it a fair amount of tought and concluded the following:

Using exclusively ### qmllistpropertyAppend to populate message would prevent ownership issue on cpp side for most cases but would prevent instantiating messages from the qml since it does not allow template function call which would be quite annoying since most repeated messages fields in a IHM application are created by a graphical qml interface such as the adressbook from the project's example. Issue would still remain as well for copy constructed messages. There are actually 4 solutions listed from best to worst to have a perfect qml compatibility feature regarding message construction, if time is a constrain for you, i'll simply keep the merge request i provided you with and add a if(hasQml()){} on top of the additions.

• have a by index append method in each message similar to what's done in the reference generator.

• geneate a hash of functors that construct and set the ownership of messages, the qml would be able to create messages by name (this solution is not bad actually since it provide features such as json or RPC based message generation).

• keep ### qmllistpropertyAppend have the messages inherit QQmlParserStatus and set the ownership of all the message field in the ::complete() hook, Implement a intermediate Q_INVOKABLE that SFINAE-deduce the type from the message name passed by the QML to pass it as a template parameter to ### qmllistpropertyAppend (u'll need to modify Q_DECLARE_METATYPE to generate SFINAE structures to deduce the message type from the QVariant returned by the QObject::property(const char*) call performer by the Q_INVOKABLE to retreive the repeated message field).

Same goes for qmllistpropertyAt.

is it possible to create a developpement branche for me to implement the configurable version of the message generation i was talking about and see where it goes? Don't feel to implement the reference generator plugin from scratch on my own hub !

[RBQt]

Actually, QMetaType::convert implementation is still stupid enough in Qt 6.# to prevent to SFINAE out the message repeated field type for a dynamic call usage. so the 3rd option i mentioned in the previous comment is not an option actually in qt 6. Qml QProtobuf module is still unusable without the 1st or 2nd option.

I proposed a better implementation and they promised to integrate for Qt 7.0. https://bugreports.qt.io/browse/QTBUG-107052

Then, u'll be able to do some rubish stuff like the following to go on with ur stuberness of not implementing a ::append method in each message to populate repeated fields :)

//A type trait, ignore this code `template using type = typename TemplateParameter::type;

//A single template converter for all repeated messages field template <typename From, typename To>To fromSharedVariantList(const From &from){ To to;

for(const auto& it : from){
    to << *reinterpret_cast<type<To>*>(it.data());
}

return to;

} //! Make the qmllistproperty constructible by name (from the QML or through a RPC call) static void qmllistpropertyAppend(QString name, QObject message, QObject repeatedFieldMessage) { QQmlEngine::setObjectOwnership(repeatedFieldMessage, QQmlEngine::CppOwnership); auto* mo = message->metaObject();

auto list =  message->property(name.toUtf8()).toList();
auto shared = QSharedPointer<QObject>(repeatedFieldMessage);

list << qVariantFromValue(shared);

QMetaType::convert(&list, QMetaType::fromType<QVariantList>().id(), message->property(name.toUtf8()).data(),
                   mo->property(mo->indexOfProperty(name.toUtf8())).userType());
shared.clear(); //prevent destruction

}`