Re: [java-gnome-hackers] added getInverted and setInverted to Range

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

On Sat, 2009-05-09 at 12:00 +0200, Peter Mosseveld wrote:
> I have attached a bundle containing these methods.

Hey, lovely.

>  This is my first 
> patch. So I hope I have done everything right.

Just some minor details that come under "style" and "consistency"

> -     * Retreive the value currently indicated by this Range instance.
> +     * Retrieve the value currently indicated by this Range instance.

Thanks for fixing the spelling :)

> +     * Retrieve the current value set by setInverted

When you need to refer to another method, use a @link tag. In this case,

        ... yada blah yada {@link #setInverted(boolean) setInverted()}
        yada yada blah blah...


As a very high level thing, rather than saying "Get the value" try
something more specific, perhaps "Get whether or not the rate scale of
this Range is inverted; see {@link ...}."

[I have often skipped bothering to write a getter simply because, well,
you wrote the code, and you know full well whether you changed it from
the default or not. But that's just me; nothing wrong with exposing a
getter if you need it and/or are willing to put the work in to offer it]

> +    /**
> +     * Inverts the way the value changes when moving the slider
> +     *
> +     * Ranges normally move from...

You need a <p> tag in there if you are trying to start a new paragraph.
Otherwise the whitespace is swallowed (as you'd expect with HTML as the
end result).

> +     * ...higher values at the top or on the right rather than on the
> +     * bottom or left.
> +     */

Please add a @since tag. Assume your patch will be accepted and in the
next release :) For example, the latest release was 4.0.11; 'mainline'
says it is 4.0.12-dev right now; assume you'll be in 4.0.12 and put

        * @since 4.0.12

This has the wonderful effect of saving the maintainer the trouble of
adding it for you, and anything that saves the maintainer trouble is
well regarded :).

So maybe just touch up those things and then we can merge it!

AfC
Sydney

P.S. I find it a bit easier if you include a bundle as an attachment
rather than inline, but that's just me.