Re: [platform-swt-dev] GTK2 Tree: build, patch, and one smoking shot

[Seth Nickell <snickell@xxxxxxxxxxxx>]

(sorry, this isn't in the thread, my computer died so I'm pasting your
comments from a webmail client) 

"I looked through the patch, and it certainly makes a lot 
of sense :-) I didn't actually *run* it yet, so I didn't 
verify what happens with the signals (which is the trickiest 
part)," 

Regarding both this (documenting proper behavior) and the various quirky
policies / restrictions placed on SWT (custom C code, java.util, that
sort of thing)... These need to be documented if there's any hope of
open source involvement. Otherwise all such information is locked in the
heads of members of the SWT team which leaves little or no possibility
for contribution other than minor bugfixes. 

"but there are several comments I have based on what 
I saw in the source." 

Thanks for reading through the patch :-) 

"The only *essential* thing I don't understand in your 
approach is the replacement of pixmaps with pixbufs.  In 
SWT, Images are drawables.  When we made the decision 
to stick with pixmaps, we couldn't find a way to obtain 
a GC on a pixbuf; my understanding is that, this is not 
possible.  Did we miss the elephant?" 

AFAIK, Pixbufs aren't allocated on the server, so they won't have a GC.
However, many GTK APIs take naught but pixbufs at this point, including
the tree view cell renderer of interest, which means that Image is going
to increasingly need to deal with this. Whereas GTK accommodates
conversion from pixbuf to pixmap, it does not make the other direction
trivial; particularly not maintaining a mask. Furthermore, pixbufs are a
superset of Pixmap functionality (containing extra information like
alpha values, etc). 

That pixbufs are easier to manipulate is even reflected in Image which
currently uses Pixbufs for all manipulation involved in transferring
ImageData to a GTK readable form, and then later renders that pixbuf to
a pixmap. 

What sort of drawable behavior is being relied upon here? From an
(admitadly cursory) glance through uses of Image it seemed to me that
and complete conversion from pixbuf to pixmap would present no problems.

"Looking at how your code gets the pixbuf from the pixmap, 
I realize we were dumb and the "preparatory code" in 
Image.getImageData() (that is, the six lines before the 
gdk_pixbuf_get_from_drawable()) is not needed - right?" 

Strictly speaking, the pixmap was "gotten" from a pixbuf anyway (in the
original code). My changes to Image essentially maintain the pixbuf (and
add code to do the proper setting of alpha values) rather than throwing
it away. This code *could* be moved to something that's done on every
call to add an image to a GtkTree, but this will involve converting from
a pixmap to a pixbuf for every item (which is wasteful since most icons,
say the folder icon, are repeated dozens of times). 

The extra memory load (fairly minimal really) seems better than the
extra runtime conversion. One option would be to lazily convert items,
so items that never get used in a pixbuf context would never incur the
extra memory load; though this will be sub-optimal compared to storing
the pixbuf at instantiation time (since, as I've mentioned, the code
already creates this pixbuf). 

"There are several minor things I would modify in your 
implementation - they are mainly deviations from the coding 
conventions we follow in SWT." 

"The rule of *No Custom Natives* is strict, and in many places" 
we sacrifice simplicity/straightforwardness to be able to 
keep it." 

"There are a few places in your Tree where the rule 
causes problems, and it feels completely natural to break it. 
Most obvious: gtk_tree_model_get_value_boolean().  This one is really 
evil because the name looks like an actual gtk function, and it's 
impossible to tell by just looking at the java code that there are 
in fact several OS calls going on in that native." 

"I understand this convention leaves one wondering, what to do 
with "polymorphic" methods like gtk_tree_store_set().  Answer: 
define polymorphic methods in OS.java, and use the "full signature" 
form for the function name in swt.c.  Look at the various forms of 
OS.memmove(...) and you'll see what I mean." 

*NONE* of the SWT calls are GTK functions anyway. The arguments are all
untyped, the marshalling (how do you pass in a C string? not as a
pointer to a C string.... as a byte array or a string object which gets
converted, etc etc etc) etc is all custom anyway. i.e., the whole idea
of no custom C code is sort of a sham because it simply doesn't exist. 

Each one of those functions has *ONE* C call; that's hardly custom C
code. If you want to measure line count I think that's less than your
suggestion, and certainely a lot simpler code (and hence easier to
maintain when everyone who knows C is dead/retired). What I'm doing with
the typed values is a common thing for GTK language bindings: in other
words its a totally standard technique for dealing with non-C languages
interfacing with this sort of GTK function. This idea of "no custom C
code" would be one thing if JNI didn't require a C layer at all; but
there's all ready a heck of a lot of C code there that's WAY weirder and
more confusing than providing the most straightforward wrapping of
varargs (which is easier to write, easier to use, and easier to
understand when using, and easier to maintain because there's actually
less total C code this way, and its MUCH simpler code). 

How does anyone win with this other than pedantic conformance to a
notion (inconsistently) taken to an extreme? 

"The other thing you certainly are wondering about, is the allocation 
of GtkIter (OS.allocateGtkTreeIter).  Whatever is done, should be 
done in Java, not in a custom native.  On the other hand, the normal 
SWT way of having a struct and using memmoves, sort of makes no sense 
here, because GtkIter is 100%-opaque, and all we need is the address. 
In the new Table, I simply call malloc(), but we don't know the real 
answer yet." 

Oops, meant to convert that to a call to g_malloc and sizeof. You're
right, there's no particular reason to have a custom function here. This
just slipped through the cricks. 

"Another convention: SWT should not use java.util.*" 

I'm guessing this is for running on embedded VMs and such, right? So...
should I implement my own Hashtable or is there already one for such
purposes, or.... ? 

"Some VMs try to resolve all natives at class load time - in the case 
of OS, this means native declarations on the java side must correspond 
exactly to swt.c, even if a method is never touched.  In other words: 
make_gtk_warnings_stop_in_c_debugger() will cause Eclipse to not 
come up on *some* installations (although it is *usually* not the case).
This is really minor cleanup, but it is needed before code can go 
into CVS. " 

Oops, make_gtk_warnings* shouldn't have made it into the final patch. I
was just using that stuff to make it easier to debug the rather
convoluted GtkTreeView call paths. I'll get rid of that. 

"Yet another convention: keep as much state in the OS, not in the 
object.  This means, for example, that treeModel should not be an 
instance variable in Tree; it is already kept in the OS." 

This seems sketchy given that we actually *allocate* the model. The
model is not grabbed as a pointer but is made totally independently of
the tree view itself (done since multiple tree views can use the same
model). Thus conceptually GTK clients never hand management/ownership of
the model over to the treeview, they retain it themselves (and are
responsible for, e.g., unreffing it). Trying to foist this state onto
the TreeView seems .... wrong, and prone to problems (avoidance of which
I assume is the whole point of keeping minimal state on the java side).
The same is true for many other GtkTreeView related types. 

"An open question still pending careful consideration is when to 
do macro conversions vs. casts:" 
<snip> 
"This is minor and non-essential, but it caught my attention because 
we (arbitrarily) follow the latter convention everywhere, and your 
use of the macro stands out as "different"." 

Hrm, I would claim not really arbitrary. Using the type conversion
macros provides much better error messages much earlier when something
goes wrong and can potentially avoid nasty segfaults. Much better to
have a GtkWarning than a segfault. Its easy enough to convert to casts
if need be, but it would be much better to use type conversion macros,
and gradually have all SWT do this. 

"I'll actually apply the patch and see how the thing runs tomorrow, 
this will certainly lead to more observations." 

Cool, I'll try to kick out a patch with your suggested changes. 

-Seth