Hey Steve, thanks for the feedback!
On Monday 08 Jun 2015 01:11:23 Stephen Kelly wrote: > 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. Right, this actually needs removing and something temporary putting in place in the examples for now. The window class doesn't do much at all but there is a need for more full featured integration points. My current thoughts on what we should aim for are: 1) A convenience window to be used with Qt3D that has a standard camera and handles aspect ratio changes etc and also uses a default framegraph (that can be changed by the user). QML and C++ versions. 2) Qt3D Widget to embed a Qt3D scene into a QWidget hierarchy. Likely analogous to QOpenGLWidget but with the above Qt3D plumbing. 3) Embed Qt3D into a Qt Quick 2 scene. Already available via the Scene3D Qt Quick 2 element (provided by Qt3D module). 4) Use Qt Quick within Qt3D as e.g. a texture provider so that it is possible to use a Qt Quick 2 render within the 3D world. I'll try to tidy up and move the existing Window thing to the examples later today or tomorrow. > 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? I can't find that. Is that from the 5.5 branch? > 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? Hmm I thought they were, but obviously not. I'm looking at making the test work now. > 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? Right, https://codereview.qt-project.org/#/c/113899/ > 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. It's precisely because of these kinds of issues that we decided to use namespaces in Qt3D rather than the poor-man's prefix name spacing. If it's required to not use namespaces to be part of the Qt project then we can of course change it. However, I would argue against doing so, especially in the light of being able to use some more modern C++ features in upcoming QT versions. Name spaces are supported everywhere these days so why not just use them, especially in a new add-on module? > 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. Thanks, https://codereview.qt-project.org/#/c/113906/ > 8) Docs in headers. > > I saw one method documented in qparameter.h instead of in the cpp and I > didn't look for more. The docs need a lot of work. I have some time set aside for doc writing this week and will do a clean up pass for such issues. Thanks again, Sean -- Dr Sean Harmer | [email protected] | Managing Director UK KDAB (UK) Ltd, a KDAB Group company Tel. +44 (0)1625 809908; Sweden (HQ) +46-563-540090 Mobile: +44 (0)7545 140604 KDAB - Qt Experts _______________________________________________ Development mailing list [email protected] http://lists.qt-project.org/mailman/listinfo/development
