Re: [PATCH 1/9] stkutil: display text attributes as html

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Kristen,

> +/* Defined in ETSI 123 40 9.2.3.24.10.1.1 */
> +enum stk_text_format_code {
> +	STK_TEXT_FORMAT_LEFT_ALIGN = 0x00,
> +	STK_TEXT_FORMAT_CENTER_ALIGN = 0x01,
> +	STK_TEXT_FORMAT_RIGHT_ALIGN = 0x02,
> +	STK_TEXT_FORMAT_NO_ALIGN = 0x03,
> +	STK_TEXT_FORMAT_FONT_SIZE_LARGE = 0x04,
> +	STK_TEXT_FORMAT_FONT_SIZE_SMALL = 0x08,
> +	STK_TEXT_FORMAT_FONT_SIZE_RESERVED = 0x0c,
> +	STK_TEXT_FORMAT_STYLE_BOLD = 0x10,
> +	STK_TEXT_FORMAT_STYLE_ITALIC = 0x20,
> +	STK_TEXT_FORMAT_STYLE_UNDERLINED = 0x40,
> +	STK_TEXT_FORMAT_STYLE_STRIKETHROUGH = 0x80,
> +};
> +
> +

Why the extra line?

> +static void end_format(GString *string, guint8 code, guint8 color)
> +{
> +	if ((code & ~STK_TEXT_FORMAT_ALIGN_MASK) || color)
> +		g_string_append(string, "</span>");
> +
> +	if ((code & STK_TEXT_FORMAT_ALIGN_MASK) != STK_TEXT_FORMAT_NO_ALIGN)
> +		g_string_append(string, "</div>");
> +}
> +
> +static void start_format(GString *string, guint8 code, guint8 color)
> +{
> +	guint8 align = code & STK_TEXT_FORMAT_ALIGN_MASK;
> +	guint8 font = code & STK_TEXT_FORMAT_FONT_MASK;
> +	guint8 style = code & STK_TEXT_FORMAT_STYLE_MASK;
> +	int fg = color & 0x0f;
> +	int bg = (color >> 4) & 0x0f;
> +
> +	/* align formatting applies to a block of test */

Did you mean 'block of text' here?

> +	if (align != STK_TEXT_FORMAT_NO_ALIGN)
> +		g_string_append(string, "<div style=\"");
> +
> +	switch (align) {
> +	case STK_TEXT_FORMAT_RIGHT_ALIGN:
> +		g_string_append(string, "text-align: right;\">");
> +		break;
> +	case STK_TEXT_FORMAT_CENTER_ALIGN:
> +		g_string_append(string, "text-align: center;\">");
> +		break;
> +	case STK_TEXT_FORMAT_LEFT_ALIGN:
> +		g_string_append(string, "text-align: left;\">");
> +		break;
> +	}
> +
> +	if (((code & ~STK_TEXT_FORMAT_ALIGN_MASK) == 0) && (color == 0))
> +		return;
> +

I really prefer font == 0 && style == 0 && color == 0 here.

> +	/* font, style, and color are inline */
> +	g_string_append(string, "<span style=\"");
> +
> +	switch (font) {
> +	case STK_TEXT_FORMAT_FONT_SIZE_LARGE:
> +		g_string_append(string, "font-size: big;");
> +		break;
> +	case STK_TEXT_FORMAT_FONT_SIZE_SMALL:
> +		g_string_append(string, "font-size: small;");
> +		break;
> +	}
> +
> +	switch (style) {
> +	case STK_TEXT_FORMAT_STYLE_BOLD:
> +		g_string_append(string, "font-weight: bold;");
> +		break;
> +	case STK_TEXT_FORMAT_STYLE_ITALIC:
> +		g_string_append(string, "font-style: italic;");
> +		break;
> +	case STK_TEXT_FORMAT_STYLE_UNDERLINED:
> +		g_string_append(string, "text-decoration: underline;");
> +		break;
> +	case STK_TEXT_FORMAT_STYLE_STRIKETHROUGH:
> +		g_string_append(string, "text-decoration: line-through;");
> +		break;
> +	}

Please note that text format is actually a bitmask, not a single
selector.  So switch/case here is inappropriate.  Maybe you got confused
by some earlier feedback... :)

> +
> +	/* add any color */
> +	if (fg)
> +		g_string_append_printf(string, "color: %s;", html_colors[fg]);
> +	if (bg)
> +		g_string_append_printf(string, "background-color: %s;",
> +						html_colors[bg]);
> +	g_string_append(string, "\">");
> +}
> +
> +char *stk_text_to_html(const char *utf8,
> +				const unsigned short *attrs, int num_attrs)
> +{
> +	long text_len = g_utf8_strlen(utf8, -1);
> +	GString *string = g_string_sized_new(strlen(utf8) + 1);
> +	short *formats;
> +	int pos, i, j;
> +	guint16 start, end, len, attr, prev_attr;
> +	guint8 code, color, align;
> +	const char *text = utf8;
> +	int attrs_len = num_attrs * 4;
> +
> +	formats = g_try_malloc0(sizeof(gint16) * (text_len + 1));

Please use g_try_new0 here, it is a bit more readable.

> +	if (formats == NULL)
> +		return NULL;

You're leaking the GString here.

> +
> +	/* 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 < attrs_len; i += 4) {
> +		start = attrs[i];
> +		len = attrs[i + 1];
> +		code = attrs[i + 2] & 0xFF;
> +		color = attrs[i + 3] & 0xFF;
> +
> +		if (len == 0)
> +			end = text_len;
> +		else
> +			end = start + len;
> +
> +		/* sanity check values */
> +		if (start > end || end > text_len)
> +			continue;
> +
> +		/*
> +		 * if the alignment is the same as either the default
> +		 * or the last alignment used, don't set any alignment
> +		 * value.
> +		 */
> +		if (start == 0)
> +			align = STK_DEFAULT_TEXT_ALIGNMENT;
> +		else {
> +			align = (formats[start - 1] & 0xFF) &
> +					STK_TEXT_FORMAT_ALIGN_MASK;
> +			if (align == STK_TEXT_FORMAT_NO_ALIGN)
> +				align = STK_DEFAULT_TEXT_ALIGNMENT;
> +		}
> +
> +		if ((code & STK_TEXT_FORMAT_ALIGN_MASK) == align)
> +			code |= STK_TEXT_FORMAT_NO_ALIGN;
> +

So I think the above code is actually incorrect.  The spec talks about
Language dependent (default) alignment being equal to
STK_DEFAULT_TEXT_ALIGNMENT.  So if the SIM sends us a Left aligned
attribute, it really means Left aligned.

Maybe something like this is enough?

if (start == 0)
	align = STK_TEXT_FORMAT_NO_ALIGN;
else
	align = formats[start - 1] & STK_TEXT_FORMAT_ALIGN_MASK;

Also, it would seem STK_TEXT_FORMAT_INIT should be set to 0x9003.  Black
foreground, white background, unaligned, unformatted.

> +		attr = code | (color << 8);
> +
> +		for (j = start; j < end; j++)
> +			formats[j] = attr;
> +	}
> +
> +	prev_attr = STK_TEXT_FORMAT_INIT;
> +
> +	for (pos = 0; pos <= text_len; pos++) {
> +		attr = formats[pos];
> +		if (attr != prev_attr) {
> +			if (prev_attr != STK_TEXT_FORMAT_INIT)
> +				end_format(string, prev_attr & 0xFF,
> +							(attr >> 8) & 0xFF);

Is the 3rd argument above really supposed to be attr >> 8?

Probably be more easier if end_format & start_format took the short
value instead of bitshifting everywhere.

> +
> +			if (attr != STK_TEXT_FORMAT_INIT)
> +				start_format(string, attr & 0xFF,
> +							(attr >> 8) & 0xFF);
> +
> +			prev_attr = attr;
> +		}
> +
> +		if (pos == text_len)
> +			break;
> +
> +		switch (g_utf8_get_char(text)) {
> +		case '\n':
> +			g_string_append(string, "<br/>");
> +			break;
> +		case '\r':
> +		{
> +			char *next = g_utf8_next_char(text);
> +			gunichar c = g_utf8_get_char(next);
> +
> +			g_string_append(string, "<br/>");
> +
> +			if ((pos + 1 < text_len) && (c == '\n')) {
> +				text = g_utf8_next_char(text);
> +				pos++;
> +			}
> +			break;
> +		}
> +		case '<':
> +			g_string_append(string, "&lt;");
> +			break;
> +		case '>':
> +			g_string_append(string, "&gt;");
> +			break;
> +		case '&':
> +			g_string_append(string, "&amp;");
> +			break;
> +		default:
> +			g_string_append_unichar(string, g_utf8_get_char(text));
> +		}
> +
> +		text = g_utf8_next_char(text);
> +	}
> +
> +	g_free(formats);
> +
> +	/* return characters from string. Caller must free char data */
> +	return g_string_free(string, FALSE);
> +}

Regards,
-Denis