Reviewing merge request #1399: New DirectFB backend featuring DirectFBGL support
This is a new DirectFB backend from Orange (Cedric Chedaleux).
It has all features the old DirectFB has plus support for DirectFBGL, i.e. Qt GLWidget is working.
Commits that would be merged:
Comments
Pushed new version 1
there is absolutely no way we'd add this to qt 4.7, and even for 4.8 it is “a bit” late. consequently, this needs to be rebased against qt5 (and possibly rewritten as a qpa plugin, but i wouldn’t know). but discuss it with the graphics subsystem maintainers.
without checking the context, it looks to me as if the changes to qegl.cpp might belong into a separate commit.
what does “from Orange (Cedric Chedaleux)” mean? did you sign the contribution agreement on behalf of that company?
target repository should be 4.8; to be discussed with subsystem maintainers about split
Why the commented out “typedef GLfloat GLdouble;”, is there a better way to do that?
In qegl.cpp the removing of “contextProps = *properties;” worries me, doesn’t that mean tat the supplied QEglProperties are ignored? Seems like it might break other platforms. Why are the qegl.cpp changes required?
In qgl.h #ifndef QT_NO_EGL is used to forward declare QEglContext but then the QEglContext name is used inside #ifdef QT_QWS_DIRECTFBGL. Maybe it should be #if !defined(QT_NO_EGL) && defined(QT_QWS_DIRECTFBGL) instead to prevent compile errors when both QT_NO_EGL and QT_QWS_DIRECTFBGL are defined.
Can’t really comment on src/plugins/gfxdrivers since I don’t know QWS nor DirectFB, but at least it shouldn’t break any other code so from my point of view it’s safe (not for 4.7, but potentially for 4.8? what’s the policy on adding gfxdrivers?).
The commented out line (i.e. typedef Glfloat GLDouble) is not necessary. I have no idea what it’s like this (maybe some remains during testing phase).
In QEgl.cpp, the supplied QEglProperties has previously being used in the “cfg” attributes. The forth argument of eglCreateContext is only an EGL context attribute that must specify the EGL_CONTEXT_CLIENT_VERSION. There is no need to fill this attribute with all the supplied QEglProperties.
But you’re right, the qegl.cpp are not required for this backend. It could be merge in another request. That’s just a patch I added to make EGL context creation on our chip.
You’re absoluty right for qgl.h, that would be safer with this define.
Concerning your last point, what do you wanna comment?
fwiw, the code is full of coding style violations. please see http://developer.qt.nokia.com/wiki/Qt_Coding_Style
in qegl.cpp:396 you add a space for no good reason.
i don’t know whether we have any policy regarding adding features during the freeze which are provably guaranteed not to affect anyone who doesn’t explicitly use them. i wouldn’t mind just shoving in the plugin, but i'm known to be liberal in that regard.
superseded by MR 1455


Add a new comment:
Login or create an account to post a comment