[Development] Some Qt3D feedback

Stephen Kelly steveire at gmail.com
Mon Jun 8 01:11:23 CEST 2015


Hello,

Congrats to Paul, Sean and others working on getting this module in a 
releasable state for Qt 5.5!

I have not reviewed the code, but I found some items to raise:

1) The include/Qt3DCore/Window file doesn't have a Q prefix.

as every other header does. Should probably be Qt3DWindow.

2) A private header is included in a public header:

 include/Qt3DCore$ grep private/ *.h
 qaspectjobmanager.h:#include <Qt3DCore/private/qt3dcore_global_p.h>

This is concerning - Don't we have a unit test preventing that?

3) The cmake unit tests don't pass.

It is easily fixable, but does this mean that the cmake tests are not run 
for this module? That is concerning.

Are unit tests run for this module in CI at all?

4) Private dependencies 

 git grep -w -e QT --and -e private

shows a bunch of content. Shouldn't they be added to QT_PRIVATE instead?

5) Qt3D namespace

This is the first time that all classes in a library are in a namespace. 
Previously only enums (in various modules) and free functions (in 
QtConcurrent) have been put in namespaces. 

In QtConcurrent, the module name also appears in the header file, but that 
is not followed by Qt3D libraries.

Given that Qt has never put classes in a namespace like this, is there 
something to be consistent about here?

6) QParameter is a very generic name

I realise it is in a namespace, but still...

Qt3DParamter might be better *and* more consistent. Similar applies to other 
classes.

7) Unneeded Q_DECLARE_METATYPE

Using the macro for QObject derived types is not needed. I saw

 Q_DECLARE_METATYPE(Qt3D::QNode *)

and similar for QParameter and then stopped searching.

8) Docs in headers. 

I saw one method documented in qparameter.h instead of in the cpp and I 
didn't look for more.

Thanks,

Steve.





More information about the Development mailing list