Re: [PATCH 01/10] stkutil: display text attributes as html

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1988 bytes --]

Hi Kristen,

> +#define STK_TEXT_FORMAT_INIT -1

I'd really prefer 0xffff here

> +char *stk_text_to_html(const char *text, int text_len,
> +				const unsigned char *attrs, int attrs_len)
> +{

I think that text_len is not required, this function can figure it out
itself by using strlen or friends.

> +	GString *string = g_string_sized_new(text_len + 1);
> +	int *formats;
> +	int pos, i, j, attr, prev_attr;
> +	guint8 start, end, code, color, len, align;
> +
> +	formats = g_try_malloc0(sizeof(int) * (text_len + 1));

Why are you allocating ints instead of shorts?

Also, you have to be really careful here.  Text inside oFono is
understood to be UTF8 always.  The formats array should be allocated
according to how many UTF8 characters there are, not how many bytes
there are.  GSM characters can be represented by up to 3 byte UTF8
chars.  UCS2 characters can be represented by up to 4 byte UTF8 chars.

Also please realize that is is not required unless you modify the
signature of attrs to be something other than unsigned char (limited to
max=255)

> +	if (formats == NULL)
> +		return NULL;
> +
> +	/* we will need formatting at the position beyond the last char */
> +	for (i = 0; i <= text_len; i++)
> +		formats[i] = STK_TEXT_FORMAT_INIT;
> +
> +	for (i = 0; i+3 < attrs_len; i += 4) {
> +		start = attrs[i];
> +		len = attrs[i + 1];
> +		code = attrs[i + 2];
> +
> +		if (i + 3 < attrs_len)
> +			color = attrs[i + 3];
> +		else
> +			color = 0;

My suggestion is to generalize this function to support both EMS
messages and STK strings.  We can treat attrs as:

unsigned short *attrs, unsigned short len where len = number of
quadruples.  See the earlier thread for details.

The reason for using shorts is to support larger EMS message sizes.

> +
> +	for (pos = 0; pos <= text_len; pos++) {

And again here you have to be careful and use UTF8 character lengths,
not simple ASCII lengths.

Regards,
-Denis