Re: [java-gnome-hackers] Cairo Operators

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

Hello Kamil,

Thank you for your contribution! I took a bit of time to review your
patch and have a few stylistic and organizational points for you to
consider.



On Mon, 2009-02-23 at 11:10 +0100, Kamil Szymala wrote:
> === modified file '.classpath'
> --- .classpath	2008-09-15 14:27:46 +0000
> +++ .classpath	2009-02-23 06:55:19 +0000
> @@ -10,5 +10,6 @@
> +	<classpathentry kind="lib" path="/usr/share/java/gtk.jar"/>


java-gnome doesn't need _your_ installed .jar location on its Eclipse
build path. I have no idea why you did this, but it is wrong, sorry.

You'll need to revert this change.



> === added file 'doc/examples/cairo/CairoOperatorTest.java'

Neither of the two demos you have submitted are examples; in out project
the things we put in the doc/examples/ tree are intended to teach other
people how to use a feature or system. The programs linked from
http://java-gnome.sourceforge.net/4.0/doc/examples/START.html are at the
standard we aim for.

If that's what you have in mind, then you'll a) have to put some
explanation in, b) meet the coding standards used in the other examples,
and c) follow the naming conventions in use. All the examples start with
a prefix of "Example" to keep the type completion space clear.

I was going to suggest these can move to tests/bindings/ with a prefix
of "Demo", but actually this on its way to what we'd need for a *really*
great set of Snapshots. Perhaps you and Kenneth Prugh can get together
on this....


> + * Copyright (c) 2007-2008 ...

Just so you know, it is 2009 now. :)

If you used an an existing source file which had "2007-2008" then
"2007-2009" would be appropriate. If you created something from scratch
then "2009" would be fine.




> +    @SuppressWarnings("empty-statement")

I'd appreciate it if you did up your code so that there aren't any
warnings to suppress.



> === modified file 'src/bindings/org/freedesktop/cairo/Operator.java'
> --- src/bindings/org/freedesktop/cairo/Operator.java	2009-02-20 14:23:13 +0000
> +++ src/bindings/org/freedesktop/cairo/Operator.java	2009-02-23 06:55:19 +0000
> @@ -30,16 +30,104 @@
>      }
>  
>      /**
> -     * Clear a surface to all transparent.
> +     * Where the second object is drawn, the first is completely removed. 
> +     * Anywhere else it is left intact. The second object itself is not drawn.

Did you write this yourself, or copy it from somewhere? It looks
suspiciously like what is written at http://cairographics.org/operators/

We try *very* hard not to blindly take raw text from other sources. [if
you do, you MUST add a comment to the top of the file indicating the
origin of the material, its copyright, and the licence it was made
available under; see
http://research.operationaldynamics.com/bzr/java-gnome/mainline/src/bindings/org/gnome/gdk/InterpType.java for an example]

The key is to try and think of what someone is trying to do when they
are looking at the docs for particular method or constant, and to then
try and explain what something is for and when [how] to use it (when not
to use it!).

Taking the above example, given that "clearing to all transparent" is a
common idiom in Cairo, and a critical part of doing the cairo-clock like
transparent background non-rectangular shape work we were doing together
the other day, I would encourage you to add a paragraph about this
Operator's role in that scenario.

In the end the test is simple. Assume the person merging your patch does
not know GTK / Cairo / libwhatever, and in the JavaDoc you are
explaining to them what it is, and how to use it.

Send the maintainer a patch that teaches him what something is for and
how to use it, and you're going to be find it very easy to get that
patch accepted. :)

AfC
Sydney


-- 
Andrew Frederick Cowie

Operational Dynamics is an operations and engineering consultancy
focusing on IT strategy, organizational architecture, systems
review, and effective procedures for change management: enabling
successful deployment of mission critical information technology in
enterprises, worldwide.

http://www.operationaldynamics.com/

Sydney   New York   Toronto   London