[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[
List Home]
|
Re: [platform-swt-dev] No Custom Natives
|
Seth, you are quite correct that there are many conventions and no porting
guide but in this case, there is a document that describes exactly what we
are doing and why. See
http://eclipse.org/articles/Article-SWT-Design-1/SWT-Design-1.html.
I'm not sure what to say other than there is plenty of evidence and
experience inside OTI to show that this approach works and produces high
quality, efficient, maintainable code. You might not know this but we've
been doing portable widget toolkits for a long time, first in Smalltalk
then Java. The languages may change, the operating systems change, the
people doing the work change but the approach and quality of the end result
does not.
Hopefully, we'll be able to address the porting guide issue but until then,
the best thing to do is to understand the feedback and try to become one of
the team. If you submit a patch, see if the patch goes in directly or if
the code that goes in gets modified or rewritten. Understand why the
modifications have been made. SWT is a large, complex multi-platform
multi-language system, difficult to understand, not because it is badly
written.
Finally, we are not "pedantic". If there is a real world performance gain
to be had in a custom native, we'll take it, but never without a benchmark.
Steve
Seth Nickell
<snickell@xxxxxxxxxxxx> To: platform-swt-dev@xxxxxxx
Sent by: cc:
platform-swt-dev-admin@ Subject: Re: [platform-swt-dev] GTK2 Tree: build, patch, and one smoking shot
eclipse.org
09/09/02 03:15 PM
Please respond to
platform-swt-dev
(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
_______________________________________________
platform-swt-dev mailing list
platform-swt-dev@xxxxxxxxxxx
http://dev.eclipse.org/mailman/listinfo/platform-swt-dev
