[Gtkradiant] [Bug 856] Surface Inspector module
Fri, 15 Aug 2003 09:06:10 -0500
------- Additional Comments From email@example.com 2003-08-15 09:06 -------
I had a look at the patch, butI haven't compiled it yet.
Here's some feedback purely for the code. Feedback on the new SI will follow
- #include <gtk/gtk.h> - don't #include stuff in headers - use forward
declaration: typedef struct _GtkWidget GtkWidget; (copied from gtkwidget.h)
- I suggest changing the savedInfo function to get only m_SIIncrement instead
of the entire savedinfo.
- AppSurfacePlugin api is exposing rather a lot of stuff for the SI module to
depend on. At least the dependencies are explicit =). The App* APIs are not a
good way to do things - they are no better than dumping the extra functions in
_QERFuncTable. APIs should really be specific to a given module - but App* APIs
are an acceptable short-term solution.
- why disable DO_TEXDEF_ALLOC ?
- could texdef_to_face_t be defined in isurfaceplugin.h instead?
- shouldn't those static functions be declared in the object file they're used
in, not the header? Functions are declared static because they cannot be
externally linked, putting them in a header is self-contradictory.
- looks like some evil tabs have crept in here... we can't have that =).
Don't leave code commented out, either remove it entirely (preferable) or use
#if 0 to comment a block, or #if PREPROCESSOR_SYMBOL and so on.
Don't put empty comments like "// Nurail" in.. I know everyone else does, but
they're not helpful =). Either put in a more informative comment, or nothing at
WINAPI calling convention is not required for plugin api - it will be removed
as soon as possible - don't use it in new apis.