Re: [java-gnome-hackers] Patch bundle for java-gnome, CellRendererToggle

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

On Wed, 2008-06-25 at 17:47 +0200, Andreas Kühntopf submitted a patch in
an email where he wrote:

> I have not written a test case for it, but i use it in a small app and
> it seems to work fine.

No doubt. However, if *I* don't have an app using it, it is a bit harder
for me to establish the same level of confidence. Nothing personal.

[This is where snapshots have proved so useful; not only do they improve
our documentation but they also act as a form of test coverage and a
demo (for hackers' review purposes) as well]

> Please let me know if this patch doesn't meet your quality standards
> or
> if you need something else.
> Any feedback is appreciated.

Sure. It looks pretty close. Let's see.


> === modified file 'src/bindings/org/gnome/gtk/CellRendererToggle.java'
> --- src/bindings/org/gnome/gtk/CellRendererToggle.java	2008-04-03
> 06:13:03 +0000
> +++ src/bindings/org/gnome/gtk/CellRendererToggle.java	2008-06-25
> 15:30:42 +0000
> +    /**
> +     * Indicate the appearance of this CellRenderer. If
> +     * this is set to true a radio button is used instead of
> +     * the default check box.
> +     */
> +    public void setRadio(boolean radio) {
> +        GtkCellRendererToggle.setRadio(this, true);                
> +    }

You noted that you had not included any unit tests. While I am not
compulsive at insisting that there must be a unit test for every last
setter or getter (finding that a bit pedantic) I cannot help but notice
that having a test of this one would have helped you notice something
wrong here...


> +
> +    /**
> +     * Indicate if the CellRenderer's toggle button
> +     * should be active.
> +     * 
> +     * <p>
> +     * If you want to bind the state of the toggle button
> +     * to a DataColumn you might want to use the overloaded method
> +     * {@link #setActive(DataColumnBoolean) setActive()}
> +     */
> +    public void setActive(boolean active) {
> +        GtkCellRendererToggle.setActive(this, active);
> +    }
> +    
> +    /**
> +     * Get the current state of the toggle button
> +     */
> +    public boolean getActive() {
> +        return GtkCellRendererToggle.getActive(this);
> +    }
> +    
> +    /**
> +     * Indicate the DataColumn the state of the CellRendererBoolean
> is
> +     * bound to. This is the mapping method you should use if you
> want to bind
> +     * the toggle button's state to a DataColumn's value.      
> +     */
> +    public void setActive(DataColumnBoolean column) {
> +        GtkCellLayout.addAttribute(vertical, this, "active",
> column.getOrdinal());       
> +    }
> +       

 
> +    /**
> +     * Event generated after user toggles the renderered 
> +     * toggle button in a Cell.

"Event" has particular meaning in GTK, so we have to be kind of careful
about using the word. An "event signal" is a signal that is emitted as a
result of an event coming out of GDK [which, as far as I understand it,
is more or less a 1:1 mapping of events coming out of the X server].

So "The signal emitted..." tends to be the prose we have been using,
though we have been sloppy in a few places, especially on Widget where
the signals are events :)

> +     * Note that the act of toggling the cell does not cause a 
> +     * change in the underlying model.

That is EXACTLY the sort of knowledge that needs to be incorporated into
our API documentation. Well done.


> +     * 
> +     * @author Andreas Kuehntopf
> +     */

When proposing a patch for merging to mainline, please include @since
tags on all classes and methods.

Actually, unless it's just a diff fragment for conceptual review, always
put @since tags in; you can assume that if accepted your code will be in
the next release. That's the whole idea, after all.

[If a branch we're collaborating on doesn't get merged before a release
we just bump the tags when we next work on it]


> +     * renderer.setActive(column);
> +     * renderer.setEditable(true);

From the example snippet you wrote. You have to set a CellRendererToggle
editable to make it toggleable? I suppose that is understandable, but I
wouldn't have thought of that as the fix if wondering why my
CellRendererToggle wasn't working. It would be good to add a paragraph
about that to the class comment.


> +    /*
> +     * Helper class to convert second parameter from String to
> TreePath
> +     */

Great.

Code formatting applied;*
@since tags added;
Test cases added and bug fixed;
More test cases;
Spelling;
Documentation touch ups.

Merged to 'mainline'.

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. We actively
carry out research and development in these areas on behalf of our
clients, and enable successful use of open source in their mission
critical enterprises, worldwide.

http://www.operationaldynamics.com/

Sydney   New York   Toronto   London