Add documentation, especially ownership & const information to public interfaces

[florianjacob]

This is a work in progress, only application.h is documented for now.

Please review if everything is correct & comment on the style. I'll continue to add commits with comments for other classes to this PR soon.

[seanchas116]

Thanks for cool documentation ✨
Looks perfect to me.

[florianjacob]

Thanks! 😄

You might be interested in publishing the documentation of the master branch via gh-pages, I just managed to enable that for alaCarte. See https://alacarte-maps.github.io/alacarte/ for the result and
https://github.com/alacarte-maps/alacarte/blob/master/.travis.yml for how I have done it. You'd need to add a ${GH_TOKEN} as hidden environment variable on travis.

[florianjacob]

In other news: I'll need to remove the pointer part of the opaqua struct typedefs and instead add it in all signatures, because otherwise one can't define arguments taking them as pointers to const data. See http://stackoverflow.com/questions/8504411/typedef-pointer-const-weirdnesst for more reasoning.

Like it is said there, I also think it's a good idea not to hide the fact that those are pointers from the parameter definitions of libqmlbind's functions, but my main argument is, if we start to add const, it should be used consistently. 😉

[florianjacob]

@seanchas116 I just tried to think of good coments for qmlbind_interface, but I don't understand why it exists. For me, it's just a wrapper around qmlbind_interface_handlers, used in a single place, when creating exporters. Couldn't the same things be achieved by directly giving new exporters a pointer to qmlbind_interface_handlers?

[florianjacob]

Because a PR says more than thousend words: What I mean is this: #11.

[seanchas116]

Sorry for confusing naming, qmlbind_interface exists to manage interface from QML to binding target language (e.g. calling methods, setting or getting properties, releasing wrapped objects) and I created it as a separate public class because of #11 (comment).

[florianjacob]

Now that I understood that, I have two remaining questions:

1. When should the caller delete the generic qmlbind_interface it created? Is it ok to call qmlbind_interface_release when all metaobjects are registered?

   I'd think so, because of the QSharedPointer that is used. Or does the qmlbind_interface need to stay valid for the application's lifetime and _release should only be called on teardown?

2. The intention is to have a generic qmlbind_interface which can implement a generic new_object using the classRef backref. I'm writing bindings for rust, though, where there is no such thing as generic class objects which cann call the class' constructor, but stuff like this is solved via template parameters, similar to C++. But of course, the template expansion results in multiple qmlbind_interfaces, one for each resulting metaobject.

   Would that also be ok, create a new qmlbind_interface instance per qmlbind_exporter instance? I'd think so, or did I miss something?

[seanchas116]

I haven't tested but I think both are possible.
If possible, forcing single qmlbind_interface instance per qmlbind_exporter and removing qmlbind_interfacefrom public API would be a nice design change...

[florianjacob]

I re-checked again, I really think both variants should work currently. I just pushed documentation for interface and exporter including my new understanding of how things work.

Regarding the possible API change, I really think merging qmlbind_interface and qmlbind_exporter in a single “MetaObjectBuilder” thing which could then be used in a

new MetaObjectBuilder(interface_handlers).add_signal(foo).add_property(bar).into_metaobject();

kind of fashion is possible from the public API standpoint, but let's discuss this later as soon as I finnished my journey through the code and unterstand what's going on. 😉

New questions:

1. What's the qmlbind_property_option enum in https://github.com/seanchas116/libqmlbind/blob/master/qmlbind/include/qmlbind/value.h for? On my first read, I thought those could be used to specify property options in qmlbind_exporter's add_property(…) function or similar, but it seems like it is currently not used at all.
2. All qmlbind_exporter_add_*-methods in https://github.com/seanchas116/libqmlbind/blob/master/qmlbind/include/qmlbind/exporter.h specify an int return value, which is the method index in the metaobject, as far as I understand. But I don't see any point in the API where method indexes are used again, so what use do I have as a libqmlbind user from those ints?
3. What happens when qmlbind_exporter_add_property's notifySignal is an empty string or a nullptr? I'm not sure whether it will just work or horribly explode and should not be done.

[seanchas116]

Thanks for advancing documentation!

merging qmlbind_interface and qmlbind_exporter in a single “MetaObjectBuilder” thing

I think it's nice change too 👍

To answer questions:

1. Oh I have forgotten removing qmlbind_property_option...
   qmlbind_property_option represents JavaScript property options used in Object.defineProperty. I was going to add equivalent method in libqmlbind but I found you can just evaluate Object.defineProperty in qmlbind_engine_eval alternatively.
2. Yes, method indexes are not used again (I checked they are not used in ruby-qml), so we can remove them from API.
3. libqmlbind does not validate it and probably it blows up (

   libqmlbind/qmlbind/src/exporter.cpp

   Line 60 in 7bba3a6

   QMetaPropertyBuilder property = mBuilder.addProperty(name, "QJSValue", mSignalIndexMap[notifier]);

   )
   I think it's not good not to provide notifying signals because they are important in QML property binding but we have to validate it.

[florianjacob]

@seanchas116 I'm not quite sure on ownership handling of argv in qmlbind_signal_emitter_emit and in call_method from qmlbind_interface_handlers, also get_property's return value and set_property's value.

Could you comment on that? Is ownership transfered in all mentioned places, or is there some copying involved? e.g. what happens if a signal is connected to multiple slots?

[seanchas116]

Is ownership transfered in all mentioned places, or is there some copying involved?

If my memory is correct, ownership of all qmlbind_values retuned from functions are transferred but ownership of qmlbind_valuess passed to functions are not transferred.
So you have to release returned qmlbind_values but do not have to copy qmlbind_values to pass them to functions (copying is done in called functions).

what happens if a signal is connected to multiple slots?

I took a look how Qt metacall are done in slots by seeing moc output:

// slot signature
QJSValue foo(const QJSValue &value) const;

// in qt_static_metacall
switch (_id) {
case 0: { QJSValue _r = _t->foo((*reinterpret_cast< const QJSValue(*)>(_a[1])));
    if (_a[0]) *reinterpret_cast< QJSValue*>(_a[0]) = _r; }  break;
default: ;
}

Values are copied so we don't have to pass ownership.

[florianjacob]

Thanks for the explanation, just integrated that.

Following what you said, I now think that

qmlbind_value* (*call_method)(qmlbind_engine *engine, qmlbind_backref *objRef,
                                  const char *name, int argc, qmlbind_value **argv);

is the correct signature, meaning that the call_method function takes ownership of the argv arguments, as it gets a copy of the original argv array which is made behind the scenes of the Qt metacall.

Do you think that's right, or does the implementation of call_method need to do a the copy itself if it needs one?

[seanchas116]

call_method function does not take ownership of the argv arguments (in the above code foo method does not receive ownership because it receives a value as a reference).
So call_method implementation need to copy or just refer and not delete values.

[florianjacob]

Now I see. Could you please re-check qmlbind_interface_handlers at https://github.com/florianjacob/libqmlbind/blob/doc_ownership_const/qmlbind/include/qmlbind/qmlbind_global.h#L82 as a whole? I found it especially tricky to understand what's happening with those callbacks, everything else is quite straightforward.

Another thing I stumbled over: I don't understand what you're doing in the signal emitter at https://github.com/florianjacob/libqmlbind/blob/doc_ownership_const/qmlbind/src/signalemitter.cpp#L31. I added a comment above it with what I thought what's going on there yesterday, but today I think this is not what's happening there anymore. 😆 I don't get why this works, why don't you need to reserve a new array with an additional element at the front for the return value?

[seanchas116]

As talked in comments, this method does not take ownership of argv values.

[florianjacob]

My fault, I updated the code but forgot to update the comments. 😆 Good that you found it.

[seanchas116]

Thanks 😄

[seanchas116]

why don't you need to reserve a new array with an additional element at the front for the return value?

I thought signals are methods with no return value so the front element (used for return values) is not needed...
Actually I'm not sure the first element is definitely not used in anywhere.

[florianjacob]

I thought signals are methods with no return value so front element (used for return values) is not needed...
Actually I'm not sure the first element is definitely not used in anywhere.

Now I understand. 😄 It would be a nice solution if the front element really was not used, but I think this is not the case. Signals actually can have return values. It's undocumented behaviour, but they have the return value of the slot that was called last. Obviously, no sane person uses that behaviour, but this will result in an arbitrary write to a few bytes before the array if the signal is connected to a slot or method with a return value. Especially if a signal is connected to a method, the method could have a return value that we like to ignore if it's called via the signal, but still gets written to the front element.

A source I found to back this up: http://stackoverflow.com/questions/5842124/can-qt-signals-return-a-value/5903082

I'll prepare a PR for fixing this in a moment. ⌚

[florianjacob]

Just finished the last bits & bobs and then rewrote history (I love to say that ⏳ ) to make it more understandable.

Notable changes since my last comment: I added a hello world example for window creation, added code to automatically publish the documentation via travis to github pages as https://seanchas116.github.io/libqmlbind, and extended license.txt to set the documentation under GNU FDL. As big portions are mainly copied from Qt's documentation (which is under FDL), I think we have to do that somehow (as FDL is a copyleft license), I hope I did that correctly.

@seanchas116 The only thing to do for you to activate documentation publishing would be to add a GH_TOKEN of yours as hidden travis environment variable, as explained in the publish script.

The PR is now in the state I want it to be and ready for final proof-reading and polishing. ✨

@rainbyte, as an outsider and initiator of #3, could you please fire up doxygen and see whether everything is understandable and fullfills your needs?

@tpickett66, as you already had a look at libqmlbind, maybe you can review the docs, too, in case you can find the time?

[rainbyte]

@florianjacob, first, thanks for your work in this again

New documentation is great, my only remarks are already known (issue #15).

In order to close issue #3, I think some usage examples are needed.
Or maybe a new issue should be reported?

[rainbyte]

@florianjacob, I've updated the issue accordingly

I'll try to write the examples using this docs.

[seanchas116]

@florianjacob Thanks a lot for your work again! I'll merge this pull request and start publishing documentation.

[seanchas116]

Documentations is successfully published! http://seanchas116.github.io/libqmlbind/

[suy]

Awesome! Kudos for the project! :)

[florianjacob]

@rainbyte @suy thank you. 😄

@seanchas116 There's an http:/ link left in the project description which could be changed to https://, too. 🔒

[florianjacob]

@seanchas116 and excuse all the fiddling that was still necessary to get travis to work. 😆

[seanchas116]

@seanchas116 There's an http:/ link left in the project description which could be changed to https://, too.

Thanks! Fixed now.

[seanchas116]

I just updated libqmlbind to latest in ruby-qml. The const information helps finding some leaks and I was able to fix them 😄

[florianjacob]

That's nice to hear! 🔩 😄