[Gtkradiant] [Bug 856] Surface Inspector module

[gtkradiant@zerowing.idsoftware.com]

http://zerowing.idsoftware.com/bugzilla/show_bug.cgi?id=856





------- Additional Comments From wjoseph@europe.ea.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 
soon.

include/isurfaceplugin.h
- #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.

include/qertypes.h
- why disable DO_TEXDEF_ALLOC ?
- could texdef_to_face_t be defined in isurfaceplugin.h instead?

plugins/surfacedialog.h
- 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.

radiant/select.cpp
- looks like some evil tabs have crept in here... we can't have that =).

notes:
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 
all.
WINAPI calling convention is not required for plugin api - it will be removed 
as soon as possible - don't use it in new apis.