[java-gnome-hackers] Reviewing "patch for RGBA windows and misc. Cairo functionality"

[Andrew C. <an...@op...>]

On Tue, 2008-08-12 at 17:09 +1000, Zak Fenton wrote:
> [patch]

Welcome Zak!

> @authors

Unfortunately Ken misguided you slightly; @author tags are welcome, but
they don't fit into the "schema" (sic) of JavaDoc; they're not valid on
methods, only classes.

As

        $ make doc

would have told you this, I have a fairly strong indication that you
didn't run it. Ahem. Doing so would help you fix another doc bug you
introduced as well. Please preview your API documentation work before
submitting contributions which expose new public API.

Also, we tend to use @link tags in proper sentences in preference to
@see tags. It's more conversational, and there's rarely a need to
distract someone with the full signature of a method when linking to it.
Most importantly, though is the fact that you can then write:

        "see also Thermostat's {@link Thermostat#setTemperature(int)
        setTemperature()} because that method will really warm things
        up, as opposed to this one which only makes you <i>think</i> it
        is warmer."

as opposed to:

        @see Thermostat.setTemperature(int)

which really doesn't help a developer very much.

> === AUTHORS

[Just FYI, traditionally in Open Source projects it is the maintainer(s)
who arbitrate people being added to the AUTHORS file. As it happens I
have been pretty generous about this, and your contribution is (assuming
it is polished up to the point of being accepted, which I'm sure you're
going to do) almost certainly would have moved me to add you there. But
beware that in a different Open Source community you could potentially
annoy people who might perceive it as presumptuous. Just a tinsy
etiquette point, but some people get rabid about this sort of thing]

> [Formatting]

It would probably be a good idea if you ran:

        $ make format

before committing - or at least before submitting a bundle - assuming
you are not working in Eclipse.

I blogged about this the other day, see
http://research.operationaldynamics.com/blogs/andrew/software/java-gnome/eclipse-code-format-from-command-line.html ]
and if you *are* hacking in Eclipse, then run the formatter already! :)


> // Thread that updates count 20 times per second and redraws the window

Comments that describe the next step of an algorithm or procedures
should be multi line, ie

/*
 * Thread that updates count 20 times per second and
 * redraws the window.
 */

It encourages us to be a bit more verbose and not to fight to be so
terse as to fit onto a single line at the cost of not actually
explaining what we're up to. Especially in our Examples, full
explanations are to be sought after.

(ie, All the descriptive comments in your ExposeEvent handler should be
multi-line, thanks)

Comments that are there to *quickly* explain something weird or
not-obvious about the vagaries of a particular call or usage then yes,
an // immediately before that call is appropriate.


> final Thread anim = ...

I'd prefer it if you would pre-declare variables.


> +       // Animation frame counter
> +       static long count; 

If it's important enough to explain what a field is, then it should be
JavaDoc. [in this case I'm not really sure that it is, but that's up to
you. Examples have the burden that most people are going to read them
top to bottom. That, of course, is not the best way to study real Java
code, but examples that are online kinda have to be pushed into a
slightly different mold.


> +                               System.exit(0);

You should not call System.exit() after Gtk.mainQuit(). Let the signal
handler return.

[call it after Gtk.main() if you really need to, which, generally, you
don't]


> /**
>       * Get the default Colormap associated with this Screen.
>       */
> -    Colormap getDefaultColormap() {
> +    public Colormap getDefaultColormap() {
>          return GdkScreen.getDefaultColormap(this);
>      }
> +    
> +    /**
> +     * Get the RGBA Colormap associated with this Screen. This is useful for
> +     * per-pixel translucency in top level windows.
> +     *

So now we're getting on to the heart of your patch.

It's not clear to me what the difference between the "default" colormap
and the "rgba" one is. Why would you call one, or the other?

        [incidentally, does the answer to this question (whatever it is)
        explain why ExampleDrawingOffscreen never worked right?]

What *is* Colormap, anyway?

These are the kinds of thing that needs explaining - the fact that I
wasn't able to answer such questions on my own is why that particular
method isn't yet public. 

Actually, the overarching question is always "do we actually need to
expose this?", and I ask it here. In this case, I can't help but wonder
(despite your otherwise excellent writing for Widget's setColormap()]
whether this isn't in the category of "[should] Just Work"

        [that's partly a GTK/GDK/Cairo question, and partly a
        if-there's-
        -really-only-one-way-this-should-be-used-then-really-it-doesn't-
        need-exposing-in-java-gnome type question.

        Quite frequently there have been things that "really, there are
        just one of" at which point we can honestly consider whether we
        need a setter / whether we need that parameter to a signature /
        etc. [An similar example would be the numerous places where we
        have used Java method overloads rather than arbitrarily
        different method names - necessary in C name space but not
        necessary in Java name space where overloads are possible]

        ... but when the "default" of something is the *only* of
        something, and the only reason you need to get the something is
        to immediately pass it to the next method call that you always
        call next... well, you can see that at that point there is room
        to put our on our thinking caps. There has been a fair bit of
        this going on in the 'textview' branch, incidentally]


> +     * @since 4.0.8

As it happens we're *right* on the boundary of closing 4.0.8, but if we
can clean this patch up quickly then I think we can get it in. But yes,
I do encourage people to put @since tags in with the assumption that
their work is going to be in the next release, so these are great,
thanks.

> +    public Colormap getRgbaColormap() {

Hmm. Looking around at other places where "r g b [a]" and other such
acronyms have shown up, we have gone with all caps [Context's
setSourceRGBA() and FileChooser's getURI() come immediately to mind].

So the right signature (assuming we expose this property) for the
accessor and mutator would seem to be:

        getRGBAColormap()

*BUT* the one place where we dramatically fiddle underlying signatures
is when we can improve Java completion space. So it may well be that:

        getColormapRGBA() and
        getColormapDefault()

will be best - assuming (referring to the previous discussion) we
actually need both.

> [Cairo methods]

Srichand, you were doing some work close to here. Can you comment on
these methods and/or their descriptions?


> public static final Operator DEFAULT = OVER;

Hm.

So is such an alias the right thing, or should we just help teach people
that OVER is the default mode?


++

> I'll probably be experimenting with some SVG stuff in the coming
> weeks, so I'll probably fill in more gaps in the Cairo bindings as I
> go.

Super.

AfC
Sydney