Re: [evolution-patches] patch to imlement a11y support for minicard of AB

[Chris Toshok <toshok ximian com>]

Just a few comments at the moment:

there are lots of strings used without _()'s throughout the code.  does
a11y code not need to worry about localization?

> + *  we access the main content of current minicard, including 
> + *  header text, label(field, field name)
> + */
> +static G_CONST_RETURN gchar*
> +ea_minicard_get_name (AtkObject *accessible)
> +{
> +#define BUFFSIZE 500
> +	static gchar name[BUFFSIZE];
> +	char *string;
> +	char *p = name;
> +	int len, count, r;
> +	EMinicard *card;
> +	GList *list;
> +
> +	g_return_val_if_fail (EA_IS_MINICARD(accessible), NULL);
> +	memset(name, '\0', BUFFSIZE);
> +
> +	strcat(p, "contact's header: ");

this needs i18n work, no?  also using GString would make this code a lot
simpler.

> +	card = E_MINICARD(atk_gobject_accessible_get_object 
> +			 (ATK_GOBJECT_ACCESSIBLE(accessible)));
> +	g_object_get (card->header_text, "text", &string, NULL);
> +
> +	count = strlen(p);
> +	p += count;
> +	r = BUFFSIZE - count;
> +
> +	/* we copy header to current buffer */
> +	len = strlen(string);
> +	count = len < r ? len : r;
> +	strncpy(p, string, count);
> +	g_free(string);
> +
> +	p += count;
> +	r -= count;
> +	/* if there exist no enough space for remain info, return */
> +	if (r <= 1 ) {
> +		return name;
> +	}
> +
> +	*p = ' ';
> +	p++;
> +	r--;
> +
> +	for ( list = card->fields; list; list = g_list_next( list ) ) {
> +		char *f, *fn;
> +        	int f_len, fn_len;
> +		EMinicardLabel *label;
> +
> +		label = E_MINICARD_LABEL(get_label_from_list(list));

see below.

> +
> +		g_object_get (label->fieldname, "text", &fn, NULL);
> +		fn_len = strlen(fn);
> +
> +		/* copy field name to current buffer */
> +		count = fn_len < r? fn_len : r;
> +		strncpy(p, fn, count);
> +		g_free(fn);
> +
> +		p += count;
> +		r -= count;
> +		if (r <= 1 ) {
> +			break;
> +		}
> +
> +		*p = ' ';
> +		p++;
> +		r--;
> +
> +		g_object_get (label->field, "text", &f, NULL);
> +		f_len = strlen(f);
> +
> +		/* copy field to current buffer */
> +		count = f_len < r ? f_len : r;
> +		strncpy(p, f, count);
> +		g_free(f);
> +
> +		p += count;
> +		r -= count;
> +		if (r <= 1 ) {
> +			break;
> +		}
> +
> +		*p = ' ';
> +		p++;
> +		r--;
> +	}
> +
> +	return name;
> +}

really, rewrite that function using GString (and g_string_append, etc.) 
the extra price in allocation costs will simplify things immensely.

> +
> +static G_CONST_RETURN gchar*
> +ea_minicard_get_description (AtkObject *accessible)
> +{
> +	if (accessible->description)
> +		return accessible->description;
> +																
> +	return "evolution minicard";
> +}

i18n?

> Index: addressbook/gui/widgets/e-minicard.c
> ===================================================================
> RCS file: /cvs/gnome/evolution/addressbook/gui/widgets/e-minicard.c,v
> retrieving revision 1.110
> diff -u -r1.110 e-minicard.c
> --- addressbook/gui/widgets/e-minicard.c	14 Nov 2003 00:38:47 -0000	1.110
> +++ addressbook/gui/widgets/e-minicard.c	10 Dec 2003 06:39:07 -0000
> @@ -225,6 +225,9 @@
>  	item_class->event      = e_minicard_event;
>  
>  	klass->selected        = NULL;
> +
> +	/* init the accessibility support for e_minicard */
> +	e_minicard_a11y_init();
>  }
>  
>  static void
> @@ -966,3 +969,11 @@
>  	}
>  	return ret_val;
>  }
> +
> +GnomeCanvasItem *
> +get_label_from_list(GList *list)
> +{
> +	g_return_val_if_fail (list != NULL, 0);
> +
> +	return GNOME_CANVAS_ITEM(E_MINICARD_FIELD(list->data)->label);
> +}

I dislike this function for a couple of reasons.  it's a public
entrypoint with no namespace.  Also, I'd rather see the casting just
used in place of the function call, from above:

+		label = E_MINICARD_LABEL(get_label_from_list(list));

I'd rather see the get_label_from_list function removed entirely and
have this line replaced with (this will require making the
EMinicardField structure/cast macros public, but I really don't see that
as a problem) the following:

label = E_MINICARD_LABEL (E_MINICARD_FIELD (list->data)->label);

Chris