A radical suggestion for QtProtobuf (please sit down before reading)

[gmabey]

I'm using qtprotobuf not only over the wire, but also as internally-used data structures in my application.
I often find the appearance of QSharedPointer to be awkward, such as when one part of my application generates a list of structures that I am going to assign to a repeated field in a protobuf message to then send over the wire.

For example, suppose I have a .proto file like this:

syntax="proto3";

package MessSet;

message ThingAttributes {
    int importance = 1;
    float value = 2;
}

message Thing {
    enum THING_STATUS {NEW_THING=0; UPDATED_THING=1;};
    THING_STATUS status = 1;
    string name = 2;
    repeated ThingAttributes attrs = 3;
}

Now suppose that there's a slot in one of my processing classes that looks like this:

void ProcessingClass::instantiateNewThing(QString new_name, QList<MessSet::ThingAttributes> new_attrs)
{
    MessSet::Thing new_thing(MessSet::::Thing::NEW_THING, new_name);
    MessSet::ThingAttributesRepeated attr_list;
    for (auto a: new_attrs) {
        attr_list.append(QSharedPointer<MessSet::ThingAttributes>(new MessSet::ThingAttributes(a)));
    }
    new_thing.setAttrs(new_attrs);
    QByteArray new_qba = new_thing.serialize(m_serializer);
    // now write new_qba to a file or whatever
}

I just find myself wishing that I could use the `` directly in assigning the attrs field, instead of having to created a whole bunch of new QSharedPointer's. Like, the use of QSharedPointer is a detail that it seems like the user shouldn't have to be aware of.

Now, I don't think I know all of the reasons for the appearance of QSharedPointer in the generated code, but I would like to suggest an alternative that I think will make using qtprotobuf much more pleasurable (in the long term) even though it would break current interfaces (you haven't reached release 1.0 yet, right??).

The alternative that I would like to propose makes the generated classes look and feel a lot more like standard Qt containers, preserving the "copy-on-write" behavior they exhibit, and reducing the verbosity of qtprotobuf usage.
Basically, the idea is to use QSharedDataPointer https://doc.qt.io/qt-5/qshareddatapointer.html as the only data member of a "methods class", and keep all of the fields in a "data class".

I have used this technique many times in the past to fashion efficient data container classes.

I suspect that (as a Qt user) you are already familiar with this programming pattern and I also suspect that the classes generated from proto messages inherit from QObject only in order to support NOTIFY in the properties.

If you're unwilling to consider removing the NOTIFY then you can probably stop reading now. However, I would like to point out that, as data containers, these messages could be considered to be more like basic Qt (and QML) types than QObjects or QQuickItems.

That is, QString doesn't have signals, like valueChanged(), nor does a qml string.

In order to illustrate how this might be done, I've hacked the generated files produced by the addressbook.proto example and gotten it to compile. I suggest you diff them against those generated by the code in the master branch today.

new_addressbook.zip

There are surely a lot of details involved in the inner workings of serialization and deserialization that I don't understand or appreciate, so I'm not going to stop using QtProtobuf if you reject this idea. However, it seems to me that transitioning to this syntax would result in a more seamless experience as a user.

[semlanik]

Hi @gmabey ,

I used QSharedPointer because there are couple issues related to serializatoin/deserialization. Some issues related to performance, since copy-on-write is not applicable in case of serialization/deserialzation - containers change each time new list portion found in proto stream.
Major impact is QML, that's the point, and it might be worse after Qt6 release. I didn't check how it works so far.

There might be raw pointers as well, but it's 2020 already, it's better to relay on modern memory management. I understand concerns about usage, and probably I could try to implement extra helper functions in generated code, that will produce required containers. I need to think how to improve your approach in any case.

[gmabey]

Hmm -- so, if you're deserializing (writing into) a new instance, and there haven't been any copies made of that instance yet, then it wouldn't get copied when non-const members are invoked, right? (because the reference count is still just 1) Maybe I'm missing your point, but it seems to me that there wouldn't be issues deserializing into COW classes. And, if only const methods are invoked in serialization, that wouldn't incur any copies either, I think.

Where I really have never worked is in the QML interface issues, nor in the grpc domain ...

[semlanik]

I made short code revision at code today and remember one more issue that related to objects storing by value in protobuf classes. In protobuf it's possible to make circular dependency between messages. So in case if we would define interfaces that return non-pointer types, that would not be possible. So this idea couldn't be accepted, sorry.

[gmabey]

Hmm -- how about QExplicitlySharedDataPointer? Glen

…

On Sun, Dec 13, 2020 at 4:51 AM Alexey Edelev ***@***.***> wrote: I made short code revision at code today and remember one more issue that related to objects storing by value in protobuf classes. In protobuf it's possible to make circular dependency between messages. So in case if we would define interfaces that return non-pointer types, that would not be possible. So this idea couldn't be accepted, sorry. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#180 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVTOUA5PFIT7W72NQT62M3SUSTEPANCNFSM4UBV5GCA> .

[semlanik]

I don't get idea what it should solve?

If got the idea of changes you suggest, you want to have:

class B{
};

class A  {
   B getB() const;
   QList<B> getBList() const;
}

But for protobuf it's acceptable to have following message structure:

message A {
B b = 1;
}
message B {
A a = 1;
}

I that case it's not feasible to solve value-based getters using forward declaration, like it's done currently using pointers.

[semlanik]

Ahhh, ok I didn't understand the suggestion. Then I need to think a little bit more.

[semlanik]

Ok, it's more about pimpl, but not about interfaces change. Yep, that what I supposed to add at some point, and shared data could be used, the only thing I would need to understand is how to manage pointers to other messages in any case.

[gmabey]

I'm very glad to see that this feature has been added to a specific milestone!

However, it's continuing to haunt me in my usage of qtprotobuf ... is there anything I could do to help move this feature along?

I think that the approach I provided in the new_addressbook.zip example should work ... I realize that there are competing priorities here.

[semlanik]

@gmabey I'm open to accepting contributions, so feel free to start the implementation.

[gmabey]

Ok, good -- so do you (at least initially) agree with these points?

• Remove QObject inheritance from all objects, and "changed" signals
• Separate "data" and "methods" classes
• Use QSharedDataPointer for copy-on-write reference counted objects

[semlanik]

Almost:

1. Inheritance will still be required to support all qt metaobject system advantages. So here I would say 'no'
2. Yep think we may assume this as a PIMPL pattern implementation.
3. Correct
4. Ensure that pointers are copied correctly for CoW paradigm

You could start with the adaptation of the generator to support the second point in the list. Perhaps for the simple data only as a starting point.

[gmabey]

So, none of the basic "data" types are derived from QObject, so why should the protobuf classes? https://doc.qt.io/qt-5/qtqml-cppintegration-data.html

Now, I do agree that Q_ENUM's and Q_PROPERTY's need to be supported, but I think that this can be accomplished through having the generated classes use Q_GADGET. Are there other metaobject features that you see as being needed? I think that the only thing that wouldn't remain is the "changed" signals.

Here is an example of how this worked out for the QtPositioning classes: https://doc.qt.io/qt-5/positioning-cpp-qml.html

I'm not claiming that I fully understand all of the implications here, but think that many things would be greatly simplified if a direct copy constructor were provided by the generated classes, and I don't think that's really going to happen as long as QObject is involved.

[semlanik]

You suggest taking the complexity of the QObject implementation to the QtProtobuf generated code.
signals still have to be supported, so the link you send is a new bunch of interface classes required specifically for the qml. Also, be aware that the basic "data" types have a lot of code inside the QML engine to support them out of the box. And we have no possibility to highjack QtProtobuf to behave the same way.

[gmabey]

Hmm -- actually, I was trying to ask you to reconsider whether the signals are necessary or appropriate for a data class.

The reason why I sent the link was to help establish that none of the basic qml types (bool, int, string, url, color, font) nor their C++ counterparts (bool, int, QString, QUrl, QColor, QFont) inherit from QObject (and hence, do not have any "changed" signals).

The second link was an attempt to demonstrate that even in the Qt codebase, there are complex classes (with no direct qml counterpart) that integrate well with qml and do it through the use of Q_GADGET instead of Q_OBJECT. Do you agree?

A separate issue is whether you feel that the qtprotobuf-generated classes need signals. I can tell you that I have not used them, and I'm very curious to know how you have used them.

I do not intent to suggest that complexity should be added to the qtprotobuf-generated classes -- to the contrary I think that by removing the QObject inheritance, the entire process will be much simpler and easier to maintain.

But it all hinges on whether you need the "changed" signals ...

[gmabey]

Oh -- wait -- do you anticipate using the qtprotobuf-derived classes as a QML Component?? I only ever envisioned using them as a property within a Component ...

[semlanik]

Oh -- wait -- do you anticipate using the qtprotobuf-derived classes as a QML Component?? I only ever envisioned using them as a property within a Component ...

Yeah, then you may change the data model field only and send an update without recreating/copying the whole data model.

Hmm -- actually, I was trying to ask you to reconsider whether the signals are necessary or appropriate for a data class.

And I'm probably lost in terms already, what do you mean under the "data" class. I could suggest starting with establishing the updated structure of the qtprotobuf-generated classes. How I see it:

class SomeMessagePrivate;//<-- this is the "data" class for me, that is not accessible from outside, only stores the data and 
                                             // implements CoW feature
class SomeMessage : public QObject { //<-- this is the "functional" or "method" class for me
//properties
//enums
//getters
//setters
//signals
private:
    SomeMessagePrivate d;
}

class SomeMessagePrivate {
// data members
}

The reason why I sent the link was to help establish that none of the basic qml types (bool, int, string, url, color, font) nor their C++ counterparts (bool, int, QString, QUrl, QColor, QFont) inherit from QObject (and hence, do not have any "changed" signals).

Because there is a lot of code that provides their support to QML

The second link was an attempt to demonstrate that even in the Qt codebase, there are complex classes (with no direct qml
counterpart) that integrate well with qml and do it through the use of Q_GADGET instead of Q_OBJECT. Do you agree?

Yep, but this requires either QtProtobuf user to write a wrapper or generate an additional code for QML

A separate issue is whether you feel that the qtprotobuf-generated classes need signals. I can tell you that I have not used them, and I'm very curious to know how you have used them.

I use it since I do not always expect that all message fields will be updated and connect to only the field that is required for the particular handler.

I do not intent to suggest that complexity should be added to the qtprotobuf-generated classes -- to the contrary I think that by removing the QObject inheritance, the entire process will be much simpler and easier to maintain.

Not sure that it's easier to maintain in this case. It's extra code that is on QtProtobuf side. Blaming Qt looks easier for me :)

[gmabey]

Preliminary side note: I think it's a lot easier to read these "reply" comments directly in the GitHub issue, instead of in the notification email ...

Oh -- wait -- do you anticipate using the qtprotobuf-derived classes as a QML Component?? I only ever envisioned using them as a property within a Component ...

Yeah, then you may change the data model field only and send an update without recreating/copying the whole data model.

Incidentally, it was "updating" a qtprotobuf-generated class instance that led me to revisit this today ...

Hmm -- do you envision a scenario in which a qtprotobuf-generated class instance would be modified in QML??

Hmm -- actually, I was trying to ask you to reconsider whether the signals are necessary or appropriate for a data class.

And I'm probably lost in terms already, what do you mean under the "data" class.

Just like you hinted above, I'm thinking of the use case (and, I've only ever used qtprotobuf-generated classes in this scenario) in which an entire message (a qtprotobuf-generated class) is updated (overwritten) when (for example) a new message is received over the network and the new values need to be displayed on the screen. I'm very surprised to think that it could be significantly faster to bind properties directly to members of a message, instead (up until now) I was only thinking of the use case that the "whole message" gets updated (which works for me) and then properties of other QML components get updated via assignment (not binding). At the time of assignment, that's when QML would determine whether the actual value has changed, and a drawing action scheduled.

I don't think that the "assignment" option involves much more code than the "on whole item changed, update other properties via assignment" approach, though -- it's either one line for binding or one line for assignment, no?

So, when I say "data" class in the context of copy-on-write, I mean an indivisible set of data members (that would change all at once). An imperfect analogy is that a qml string is certainly composed of an array of characters, but there are no NOTIFY signals on individual characters -- just on the whole string.

My QML code looks like this:

Item {
    property Event currentEvent
...
    onCurrentEventChanged: {
        eventIdText.text = currentEvent.eventId
...
    }
}

I suspect that for QGeoCircle (as a property of a Component) it'd be very similar, although I haven't used it. That is, I believe that if there were a Q_PROPERTY of type QGeoCircle it would work the same way; the component could certainly emit a "changed" signal without individual signals on the center and radius members.

The reason why I sent the link was to help establish that none of the basic qml types (bool, int, string, url, color, font) nor their C++ counterparts (bool, int, QString, QUrl, QColor, QFont) inherit from QObject (and hence, do not have any "changed" signals).

Because there is a lot of code that provides their support to QML

Yeah I guess it comes down to that -- I don't think that more QML code is needed in order to handle a QGeoCircle besides handling center and radius separately.

The second link was an attempt to demonstrate that even in the Qt codebase, there are complex classes (with no direct qml
counterpart) that integrate well with qml and do it through the use of Q_GADGET instead of Q_OBJECT. Do you agree?

Yep, but this requires either QtProtobuf user to write a wrapper or generate an additional code for QML

Same discussion as above ...

A separate issue is whether you feel that the qtprotobuf-generated classes need signals. I can tell you that I have not used them, and I'm very curious to know how you have used them.

I use it since I do not always expect that all message fields will be updated and connect to only the field that is required for the particular handler.

Ok, now at least I understand the motivation here, and I now agree it is a reasonable use model.

I do not intent to suggest that complexity should be added to the qtprotobuf-generated classes -- to the contrary I think that by removing the QObject inheritance, the entire process will be much simpler and easier to maintain.

Not sure that it's easier to maintain in this case. It's extra code that is on QtProtobuf side. Blaming Qt looks easier for me :)

I still think there's a disconnect on the copy-on-write behavior and the use of QObject -- if the generated classes looks like this

class SomeMessage : public QObject

then I think that copy-on-write can't be implemented, mostly because there is no copy constructor for QObject. What do you think?

That is, the primary obstacle to this working is the question "what happens to existing signal and slot connections when copy-on-write occurs?"

I think if you review the Qt provided classes I think you'll agree that copy-on-write and QObject are mutually exclusive -- they never overlap, and for good reasons.

However, I agree that in order for qtprotobuf to use Q_GADGET, you'd have to give up not only the NOTIFY aspect of the property, but also WRITE -- and that would make it kind of a dumb data class -- in order to not mislead the user into thinking that assignment or binding in QML (both of which would initiate a copy, on write) will affect the original instance. However, there certainly should be set___() methods on the class that could be invoked from C++. And so, in order to modify a member of a qtprotobuf-generated class instance, the QML code would have to invoke a slot on the object that produced the instance, which would then trigger its "changed" signal ...

I hope these ramblings are helpful in the end!

[gmabey]

I'm extremely curious to know what your thoughts and plans are for this suggestion. Like, if you don't agree, that's fine, but it affects some decisions that I need to make soon.

I'm also curious to hear perspectives from others who might be following this project (independent of my opinion) while still recognizing all of the leadership and hard work that you (Alexey) have put into this project.

[gmabey]

I've taken a whack at this approach, but I got stuck trying to understand what was going on with the nested messages :-/

https://github.com/gmabey/qtprotobuf/tree/shareddata

This is definitely a WIP, but please take a look at what I've got so far. The generated source for all of the WellKnownTypes compiles (they are all pretty simple messages) -- it's the tricky tests that I get tangled up in.

And just to review -- these changes propose the following behavior changes:

1. No NOTIFY signal is present for the elements of messages. Instead, a class that has a generated-message-class as a property would need to NOTIFY that the [whole] message-typed property has changed.
2. No QML-specific registration/use is needed/appropriate. The message class properties would be accessible within QML, just not notifications that those "interior" properties had changed.

Also, please be aware that I have not yet tested these changes within my own project; for now I'm just asking for help in understanding the nested messages. In addition, the move() semantics is currently disabled ... need to look deeper into how that is best implemented in the context of QSharedDataPointer.

[gmabey]

I've now resolved the functional issues I was working against the last time I pushed an update to my fork (nested messages). The following tests now work: test_extra_namespace, test_protobuf, test _qttypes, and test_wellknowntypes. I believe that test_grpc would work if I spent a bit more time trying to figure out how to run the client+server.

A word about my application ... all of the protobuf-deserializing code is threaded apart from the main thread, and there are very deep maps. Now that I've updated my application to use the syntax changes incurred by these changes, I no longer get random seg faults, even though I have not changed anything about the mutex protections.

I know what I've done is not perfect (in particular, I need to create an example of how it would interface with QML) but I'm really curious to know where we go from here. I do not want to fork your project permanently, and I admire all of the very careful work you've done. I hope that you will consider this contribution seriously or advise me that you're not interested; I'm at a point where I really need to know.

Best Regards,
Glen

[semlanik]

Hi :) Sorry, it's quite hard to review such a lot of code in one piece, but overall I like the idea to use Q_GADGET instead Q_OBJECT. Also I didn't look closely yet at the QProperty<> that was invented several months ago. So I suspect that combination of QProperty<> and Q_GADGET might give even better experience for users, but still I need to understand how it could be combined with QML and other things that might need 'signals' involved. I'll back in couple of days with some more comments, since as I already mentioned I need to read the code and understand what is impact on existing concepts.
Thanks!

Regards,
Alexey.

P.S. have not too much powers for a while, now I'm trying to recover and get back to the project.

[gmabey]

@semlanik I'm sorry to hear you haven't been doing well and I hope that you recover soon.

I just now pushed some more changes to my shareddata branch after finding some initialization issues. I have also now successfully accessed members of the message class instances in QML! The limitation is that they can't be properties (because they don't NOTIFY) but what I've done so far is to just emit a signal from the C++ class that deserialized the instance with the message as an argument and it works great! I should probably work that into the README somehow, but I'm pretty happy with it.

[gmabey]

P.S. One of the other ways (I think) to use the message contents in QML is to have the message as a property of another class that does inherit from QObject and that has been registered with qmlRegisterType<>(). This is analogous to having a QString or a QGeoRectangle as a property of a C++ class, even though those classes do not inherit from QObject.

[gmabey]

P.P.S. I'd love it if there was a simpler way to handle the initialization of nested messages (it really hit hard in the cyclic loop scenario) than using Q_GLOBAL_STATIC but it seems to work. Also, it would be nice if things could be rearranged such that the mystery nothing_placeholder data member could be removed ...

[gmabey]

@semlanik any further thoughts on this suggestion? I need to go back and merge your recent commits into my branch, but haven't yet. I am still actively using and appreciating this code!

[gmabey]

@semlanik in case it wasn't clear, one of the main snags I hit as I was trying to get the gRPC stuff to work was the use of QPointer, in that there's no longer a deleted() signal being emitted by the classes generated by QtProtobuf, since it no longer inherits from QObject. I believe that this can be overcome, and I did start to do this, but as I don't actually use the gRPC layer at all (and, I have to focus on my day job) I have not yet been able to complete that task. As such, the examples don't all compile.

[gmabey]

@semlanik I'm working today on trying to get my fork to compile for Windows, but it's getting pretty ugly. I sure wish I was better at the cmake ninja techniques you have used for the build system of qtprotobuf, but I'm still learning. Please do advise whether you intend to review/revisit/consider this suggestion.

[semlanik]

@gmabey sorry I abandoned this repo for now. I'm not sure if I will have time to look in near future, but over all idea to use Q_GADGET as the optional generating output is still relevant I guess.

[gmabey]

@semlanik Do you mean the whole qtprotobuf project? Or, just abandoned the idea of switching your repo to use Q_GADGET?

I hope that you will reconsider someday. Particularly for the sake of usage -- it makes for much simpler code for the user, in my humble opinion, and is very Qt-esque. And -- I'm able to access message members in my QML with no problems, just like other Qt data classes.

And ... the longer you wait to make the switch, the more painful the change will be for your growing user base ... just sayin'.