dotherside icon indicating copy to clipboard operation
dotherside copied to clipboard

RFC: rootObject is unsafe

Open filcuc opened this issue 8 years ago • 6 comments

This is due to returning references to temporary objects to the binded language. This is totaly usafe because the QObject wrappers in the binded language don't have the ownership of the QObject pointers returned by this function. To solve this we have different solutions:

  1. Remove this API function
  2. Provide a semantic for unowned QObject in the binded language

Solution (2) is very cumbersome to implement basically we should keep QPointer somewhere in order to track the deletion of the QObjects. Furthermore this is safe only if everything works in the UI Thread.

filcuc avatar Jan 28 '16 20:01 filcuc

@Calrama since you're the one who needed this change can you give me your opinion on this. Is it really necessary?

filcuc avatar Jan 28 '16 20:01 filcuc

@filcuc This was one of the functions, that I used purely for debugging purposes while binding other API functions. I do not need it in particular, I included it in the PR in case someone may need it. If you think it ought to be removed, I have nothing against that.

MoritzMaxeiner avatar Jan 30 '16 13:01 MoritzMaxeiner

OK the same goes for find child. I think it should be implemented purely in the binding side. This gives more safety since the QObject on the binding language are managed by the GC. Returning a a reference to a child QObject not managed by the bindings it totaly unsafe.

_offtopic_ Btw if you are interested i reworked most of the implementation on both the DOtherSide and DQml. On the DQml side i slighly improved the syntax and now it's possible to create QObject that gets instantiated inside QML. Cheers

filcuc avatar Jan 30 '16 13:01 filcuc

While I currently do not use findChild, there must be a way to access children of objects managed in the bindings, which themself are not managed by the bindings. Otherwise you severely limit the API's functionality in comparison with the original C++. It is - obviously - not safe - for that matter IIRC it is not safe in C++ either - and should be documented as unsafe, but being unsafe does not mean it should not be supported.

offtopic Thank you for the information, I will look at it when I continue my work on Beacon (hopefully soon).

MoritzMaxeiner avatar Jan 30 '16 13:01 MoritzMaxeiner

Well, findChild can be implemented in the bindings, basically it should work by exploring the hierarchy of QObjects (created in the bindings). Obviously this solution doesn't allow to access QObjects created by someone else in the C++ side. This is safer. Keep in mind that this projects doesn't aim to create Qt full bindings, just the minimal set for creating databound applications in QML (and with databound i mean that basically the view should just received/push values to QObjects). In practice it shouldn't be necessary to explore the QObjects hierarchy in the C++ side. If you're doing such a thing than there's something wrong. Given that, another solution could be the addition of new class/object in the bindings that maps the concept of unsafe QObject. In C++ you can have some sort of safety by using a QPointer. Basically a QPointer track the lifetime of a QObject by connecting to its destroy signal.

filcuc avatar Jan 30 '16 14:01 filcuc

Hm, well if you think that you truly just want to support databound applications, then I guess just remove the findChild-function and be done with it. Personally, I would prefer introducing the QPointer type to the bindings (where QPointers always track the destroy signal, as you said) and then have findChild return a QPointer. Should I ever need it, I guess I will implement that way and send a PR (unless you are opposed to that).

MoritzMaxeiner avatar Jan 30 '16 15:01 MoritzMaxeiner