Re: [evolution-patches] seek review for patch for bug 47033 and bug 47034: a11y impl for e-text

[Mike Kestner <mkestner ximian com>]

On Wed, 2003-09-03 at 10:50, Tianyu Tim-Wo wrote:

>    Since we are going to sync the a11y-branch with HEAD, could you
> please take look at this patch. We hope it can be checked into HEAD
> before the sync process begins (This weekend)

Thanks for the patch.  

What do you mean by the sync process?  And why is it a rush to get this
into HEAD before the sync?  Being in a hurry to commit a patch of this
size make the maintainer in me pretty nervous.  :)

I'm assuming that you have already had an accessibility expert review
this patch.  If not, please do so before committing. 

I've looked at the patch.  There are numerous places where the coding
standard is not followed.  A common problem is macro and function calls
of the form:

      foo( arg1, arg2, argn );

which should be:

      foo (arg1, arg2, argn);

Also you should use:

	while (foo) {
		...
	}

not:
	while (foo)
	{
		...
	}

also:
	if (foo) 
		do something;

not
	if (foo) do something;

All operators should be surrounded by spaces (x + 1  instead of x+1).

Please review the patch standards mail that Michael recently posted to
the list and rework the patch to incorporate all the coding standard
elements.

There are several useless comments in the patch.  

if (obj == null)
    /* object is defunct */ 
    return;

If the comments don't add any more readability than the code, kill them.

There is a lot of additional guarding code added that seems extraneous
to me.  Lots of g_return_if_fail checks like this:

+       g_return_if_fail (ATK_IS_GOBJECT_ACCESSIBLE (text));
+       obj = atk_gobject_accessible_get_object (ATK_GOBJECT_ACCESSIBLE
(text));

I'd be surprised if the function call above doesn't do an
IS_GOBJECT_ACCESSIBLE check itself.  Just call the function.

Please do not submit code with FIXMEs.  If ispunct really doesn't handle
chinese, a proper solution needs to be found.

-- 
Mike Kestner <mkestner ximian com>