Review: Paul Blokus -- Core text area

John-Mark Bell jmb at netsurf-browser.org
Tue Jun 23 13:38:58 BST 2009


On Tue, 2009-06-23 at 12:13 +0100, John-Mark Bell wrote:

> Index: desktop/textarea.c
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ desktop/textarea.c	2009-06-23 11:58:18.000000000 +0100
> @@ -0,0 +1,1258 @@
> +/*
> + * Copyright 2006 John-Mark Bell <jmb202 at ecs.soton.ac.uk>

You should add your details here. Also, change my email address to this
one.

> +#include <stdint.h>
> +#include <string.h>
> +#include "css/css.h"
> +#include "desktop/textarea.h"
> +#include "desktop/textinput.h"
> +#include "desktop/plotters.h"
> +#include "render/font.h"
> +#include "utils/log.h"
> +#include "utils/utf8.h"
> +#include "utils/utils.h"
> +
> +#define MARGIN_LEFT 2
> +#define MARGIN_RIGHT 2
> +
> +struct line_info {
> +	int b_start;		/**< Byte offset of line start */
> +	int b_length;		/**< Byte length of line */
> +};
> +
> +struct text_area {
> +
> +	int x, y;			/**< Coordinates of the widget
> +					     (top left corner) */

What are these with respect to?
					
> +	int scroll_x, scroll_y;
> +	
> +	unsigned int flags;		/**< Textarea flags */
> +	int vis_width;		/**< Visible width, in pixels */
> +	int vis_height;	/**< Visible height, in pixels */

Please fix the comment alignment here.

> +	char *text;			/**< UTF-8 text */
> +	unsigned int text_alloc;	/**< Size of allocated text */
> +	unsigned int text_len;		/**< Length of text, in bytes */

If these are unsigned, all the indices into the text should be, too.

> +	struct {
> +		int line;	/**< Line caret is on */
> +		int char_off;	/**< Character index of caret */
> +	} caret_pos;
> +	
> +	int selection_start;	/**< Character index of sel start(inclusive) */
> +	int selection_end;	/**< Character index of sel end(exclusive) */
> +
> +	struct css_style *style;	/**<  Text style */	
> +	
> +	int line_count;	/**< Count of lines */
> +#define LINE_CHUNK_SIZE 256

This seems rather large.

> +	struct line_info *lines;	/**< Line info array */
> +	
> +	/** Callback functions for a redraw request*/
> +	textarea_start_radraw_callback redraw_start_callback;
> +	textarea_start_radraw_callback redraw_end_callback;

s/radraw/redraw/
	 
> +	intptr_t data; /** < Callback data for both callback functions*/

Is there a good reason for this to be an intptr_t? What's wrong with a
plain void *?
	
> +	int drag_start_char; /**< Character index at which the drag was started*/
> +};
> +
> +
> +static void textarea_insert_text(struct text_area *ta, unsigned int index,
> +			  const char *text);
> +static void textarea_replace_text(struct text_area *ta, unsigned int start,
> +			   unsigned int end, const char *text);

Style: continuation lines should be indented 2 tabs.

> +static void textarea_reflow(struct text_area *ta, unsigned int line);
> +static int textarea_get_xy_offset(struct text_area *ta, int x, int y);
> +static void textarea_set_caret_xy(struct text_area *ta, int x, int y);
> +static bool textarea_scroll_visible(struct text_area *ta);
> +static void textarea_select(struct text_area *ta, int c_start, int c_end);
> +static void textarea_normalise_text(struct text_area *ta,
> +		unsigned int b_start, unsigned int b_len);
> +// static bool textarea_mouse_click(wimp_pointer *pointer);

Is this redundant? If so, remove it.

> +
> +/**
> + * Create a text area
> + *
> + * \param x X coordinate of left border
> + * \param y Y coordinate of top border
> + * \param width width of the text area
> + * \param height width of the text area
> + * \param flags Text area flags
> + * \param font_style Font style to use, or 0 for default
> + * \return Opaque handle for textarea or 0 on error
> + */

Please synchronise this comment with the implementation.

> +struct text_area *textarea_create(int x, int y, int width, int height, 
> +		unsigned int flags, const struct css_style *style,
> +  		textarea_start_radraw_callback redraw_start_callback,
> +    		textarea_end_radraw_callback redraw_end_callback, intptr_t data)
> +{
> +	struct text_area *ret;
> +
> +	if (!redraw_start_callback || !redraw_end_callback) {
> +		LOG(("no callback provided"));
> +		return 0;
> +	}

I'd prefer to see if (foo == NULL), rather than if (!foo), please.
Also, return NULL, not 0.

	
> +	ret = malloc(sizeof(struct text_area));
> +	if (!ret) {
> +		LOG(("malloc failed"));
> +		return 0;

NULL.

> +	}
> +
> +	ret->redraw_start_callback = redraw_start_callback;
> +	ret->redraw_end_callback = redraw_end_callback;
> +	ret->data = data;
> +	ret->x = x;
> +	ret->y = y;
> +	ret->vis_width = width;
> +	ret->vis_height = height;
> +	ret->scroll_x = 0;
> +	ret->scroll_y = 0;
> +	ret->drag_start_char = 0;
> +	
> +	
> +	ret->flags = flags;
> +	ret->text = malloc(64);
> +	if (!ret->text) {
> +		LOG(("malloc failed"));
> +		free(ret);
> +		return 0;

NULL.

> +	}
> +	ret->text[0] = '\0';
> +	ret->text_alloc = 64;
> +	ret->text_len = 1;
> +	
> +	ret->style = malloc(sizeof(struct css_style));
> +	if (!ret->style) {
> +		LOG(("malloc failed"));
> +		free(ret->text);
> +		free(ret);
> +		return 0;
> +	}
> +	memcpy(ret->style, style, sizeof(struct css_style));
> +	
> +	ret->caret_pos.line = ret->caret_pos.char_off = 0;
> +	ret->selection_start = -1;
> +	ret->selection_end = -1;
> +	
> +	ret->line_count = 0;
> +	ret->lines = 0;
> +
> +	return (struct text_area *)ret;

This cast is redundant.

> +}
> +
> +
> +/**
> + * Destroy a text area
> + *
> + * \param ta Text area to destroy
> + */
> +void textarea_destroy(struct text_area *ta)
> +{
> +	free(ta->text);
> +	free(ta->style);
> +	free(ta);
> +}

What about freeing the lines array?

> +/**
> + * Insert text into the text area
> + *
> + * \param ta Text area
> + * \param index 0-based character index to insert at
> + * \param text UTF-8 text to insert
> + */
> +void textarea_insert_text(struct text_area *ta, unsigned int index,
> +		const char *text)

This should report memory exhaustion.

> +{
> +	unsigned int b_len = strlen(text);
> +	size_t b_off, c_len;
> +
> +	if (ta-> flags & TEXTAREA_READONLY)

Lose the space before flags.

> +/**
> + * Replace text in a text area
> + *
> + * \param ta Text area
> + * \param start Start character index of replaced section (inclusive)
> + * \param end End character index of replaced section (exclusive)
> + * \param text UTF-8 text to insert
> + */
> +void textarea_replace_text(struct text_area *ta, unsigned int start,
> +		unsigned int end, const char *text)

Again, should report memory exhaustion.

> +{
> +	int b_len = strlen(text);
> +	size_t b_start, b_end, c_len, diff;
> +
> +	if (ta-> flags & TEXTAREA_READONLY)
> +		return;
> +	
> +	c_len = utf8_length(ta->text);
> +
> +	if (start > c_len)
> +		start = c_len;
> +	if (end > c_len)
> +		end = c_len;
> +
> +	if (start == end)
> +		return textarea_insert_text(ta, start, text);
> +
> +	if (start > end) {
> +		int temp = end;
> +		end = start;
> +		start = temp;
> +	}

Might it be better to just give up here and tell the client they've got
their parameters the wrong way around?

> +
> +	diff = end - start;
> +
> +	for (b_start = 0; start-- > 0;
> +			b_start = utf8_next(ta->text, ta->text_len, b_start))
> +		; /* do nothing */
> +
> +	for (b_end = b_start; diff-- > 0;
> +			b_end = utf8_next(ta->text, ta->text_len, b_end))
> +		; /* do nothing */

These loops want documenting.

> +/**
> + * Set the caret's position
> + *
> + * \param ta Text area
> + * \param caret 0-based character index to place caret at
> + */
> +void textarea_set_caret(struct text_area *ta, int caret)
> +{
> +	int c_len;
> +	int b_off;
> +	int i;
> +	int index;
> +	int x, y;
> +	int x0, y0, x1, y1;
> +	int height;
> +	
> +		
> +	if (ta-> flags & TEXTAREA_READONLY)
> +		return;
> +	
> +	ta->redraw_start_callback(ta->data);
> +	
> +	c_len = utf8_length(ta->text);

We seem to be calculating the utf8 length of the text almost everywhere.
Would it be better to cache this?

> +/**
> + * get character offset from the beginning of the text for some coordinates
> + *
> + * \param ta	Text area
> + * \param x	X coordinate
> + * \param y	Y coordinate
> + * \return	character offset
> + */
> +static int textarea_get_xy_offset(struct text_area *ta, int x, int y)
> +{
> +	size_t b_off, c_off, temp;
> +	int line_height;
> +	int line;
> +
> +	if (!ta->line_count)
> +		return 0;
> +	
> +	line_height = css_len2px(&(ta->style->line_height.value.length),
> +				   ta->style);
> +
> +	x = x - ta->x - MARGIN_LEFT + ta->scroll_x;
> +	y = y - ta->y + ta->scroll_y;
> +
> +	if (x < 0)
> +		x = 0;
> +	
> +	line = y / line_height;
> +
> +	if (line < 0)
> +		line = 0;
> +	if (ta->line_count - 1 < line)
> +		line = ta->line_count - 1;
> +
> +	nsfont.font_position_in_string(ta->style,
> +			ta->text + ta->lines[line].b_start, 
> +	   		ta->lines[line].b_length, x, &b_off, &x);
> +
> +
> +	for (temp = 0, c_off = 0; temp < b_off + ta->lines[line].b_start;
> +			temp = utf8_next(ta->text, ta->text_len, temp))
> +		c_off++;
> +
> +	/* if the offset is a space at the end of the line set the caret before
> +	   it otherwise the caret will go on the beginning of the next line
> +	*/
> +	if (b_off == (unsigned)ta->lines[line].b_length &&
> +			ta->text[ta->lines[line].b_start +
> +		   	ta->lines[line].b_length - 1] == ' ')
> +		c_off--;

This seems odd. Can you explain why it's needed?

> +/**
> + * Set the caret's position
> + *
> + * \param ta Text area
> + * \param x X position of caret in a window
> + * \param y Y position of caret in a window

Presumably, these are relative to the top-left of the text area?

> + */
> +void textarea_set_caret_xy(struct text_area *ta, int x, int y)
> +{
> +	int c_off;
> +	
> +	if (ta->flags & TEXTAREA_READONLY)
> +		return;
> +	
> +	c_off = textarea_get_xy_offset(ta, x, y);
> +	textarea_set_caret(ta, c_off);
> +}
> +
> +
> +/**
> + * Get the caret's position
> + *
> + * \param ta Text area
> + * \return 0-based character index of caret location, or -1 on error
> + */
> +int textarea_get_caret(struct text_area *ta)
> +{
> +	int c_off = 0, b_off;
> +
> +	if (ta->text_len == 1)
> +		return 0;

Please document why this is needed.

> +/**
> + * Reflow a text area from the given line onwards
> + *
> + * \param ta Text area to reflow
> + * \param line Line number to begin reflow on
> + */
> +void textarea_reflow(struct text_area *ta, unsigned int line)

This should report memory exhaustion. Anything that calls this should
check for that. Please ensure the code in this function is wrapped at 80
columns.

> +/**
> + * Handle redraw requests for text areas
> + *
> + * \param redraw Redraw request block
> + */
> +void textarea_redraw(struct text_area *ta, int x0, int y0, int x1, int y1)
> +{
	
> +	plot.clip(x0, y0, x1, y1);
> +	plot.fill(x0, y0, x1, y1, (ta->flags & TEXTAREA_READONLY) ?
> +			0xD9D9D9 : 0xFFFFFF);

What are these magic numbers? Please #define them somewhere.

> +	plot.rectangle(ta->x, ta->y, ta->vis_width - 1, ta->vis_height - 1, 1,
> +			0x000000, false, false);

Similarly.

> +			plot.fill(x0 - ta->scroll_x, ta->y + line * line_height + 1 - ta->scroll_y,
> +					x1 - ta->scroll_x, ta->y + (line + 1) * line_height - 1 - ta->scroll_y,
> +				 	0xFFDDDD);

Again, magic number.
		
> +		plot.text(ta->x + MARGIN_LEFT - ta->scroll_x, y0 - ta->scroll_y, ta->style,
> +			  	ta->text + ta->lines[line].b_start,
> +			   	ta->lines[line].b_length,
> +			   	(ta->flags & TEXTAREA_READONLY) ?
> +					0xD9D9D9 : 0xFFFFFF,
> +			   	0x000000);

Similarly.

> +	}
> +}
> +
> +/**
> + * Key press handling for text areas.
> + *
> + * \param ta	The text area which got the keypress
> + * \param key	The ucs4 character codepoint
> + * \return     	true if the keypress is dealt with, false otherwise.
> + */
> +bool textarea_keypress(struct text_area *ta, uint32_t key)

Ensure code in this function is wrapped at 80 columns.

> +		case KEY_COPY_SELECTION:
> +		case KEY_TAB:
> +		case KEY_SHIFT_TAB:
> +		case KEY_CR:
> +		case KEY_CUT_LINE:
> +		case KEY_PASTE:
> +		case KEY_CUT_SELECTION:
> +		case KEY_ESCAPE:
> +		case KEY_WORD_LEFT:
> +		case KEY_WORD_RIGHT:

Presumably, these need implementing? At the very least, should they not
just be handled by the default case?

> +/**
> + * Removes any CR characters and changes newlines into spaces if it is a single
> + * line textarea.
> + *
> + * \param ta		Text area
> + * \param b_start	Byte offset to start at
> + * \param b_len		Byte length to check
> + */
> +void textarea_normalise_text(struct text_area *ta,
> +		unsigned int b_start, unsigned int b_len)
> +{
> +	bool multi = (ta->flags & TEXTAREA_MULTILINE) ? true:false;
> +	unsigned int index;
> +	
> +	/*remove CR characters*/
> +	for (index = 0; index < b_len; index++) {
> +		if (ta->text[b_start + index] == '\r') {
> +			if (b_start + index + 1 <= ta->text_len &&
> +					ta->text[b_start + index + 1] == '\n') {
> +				ta->text_len--;
> +				memmove(ta->text + b_start + index,
> +						ta->text + b_start + index + 1,
> +						ta->text_len - b_start - index);

This is CRLF -> LF, right?

> +			}
> +			else
> +				ta->text[b_start + index] = '\n';

CR -> LF.

> Index: desktop/textarea.h
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ desktop/textarea.h	2009-06-23 11:58:18.000000000 +0100
> @@ -0,0 +1,61 @@
> +/*
> + *

Who wrote this file?

> +/** \file
> + * Single/Multi-line UTF-8 text area (interface)
> + */
> +
> +#ifndef _NETSURF_DESKTOP_TEXTAREA_H_
> +#define _NETSURF_DESKTOP_TEXTAREA_H_
> +
> +#include <stdint.h>
> +#include <stdbool.h>
> +#include "css/css.h"
> +#include "desktop/browser.h"
> +
> +/* Text area flags */
> +#define TEXTAREA_MULTILINE	0x01	/**< Text area is multiline */
> +#define TEXTAREA_READONLY	0x02	/**< Text area is read only */
> +
> +struct text_area;
> +
> +/* this is temporary only*/
> +browser_mouse_state textarea_mouse_state;
> +int textarea_pressed_x;
> +int textarea_pressed_y;

If this is temporary, it should go.

> +typedef void(*textarea_start_radraw_callback)(intptr_t data);
> +typedef void(*textarea_end_radraw_callback)(intptr_t data);

s/radraw/redraw/

> Index: gtk/gtk_window.h
> ===================================================================
> --- gtk/gtk_window.h	(revision 7935)
> +++ gtk/gtk_window.h	(working copy)
> @@ -76,6 +76,9 @@
>  int nsgtk_gui_window_update_targets(struct gui_window *g);
>  void nsgtk_window_destroy_browser(struct gui_window *g);
>  
> +/*TODO: find a better place for this?*/
> +uint32_t gdkkey_to_nskey(GdkEventKey *);

Yes, probably. If not, at least namespace it correctly.

> Index: gtk/gtk_plotters.c
> ===================================================================
> --- gtk/gtk_plotters.c	(revision 7935)
> +++ gtk/gtk_plotters.c	(working copy)
> @@ -119,7 +119,7 @@
>  		line_width = 1;
>  
>  	cairo_set_line_width(current_cr, line_width);
> -	cairo_rectangle(current_cr, x0, y0, width, height);
> +	cairo_rectangle(current_cr, x0 + 0.5, y0 + 0.5, width, height);

This seems orthogonal to the textarea changes.

> Index: gtk/gtk_scaffolding.c
> ===================================================================
> --- gtk/gtk_scaffolding.c	(revision 7935)
> +++ gtk/gtk_scaffolding.c	(working copy)

The changes in this file look like test code for the textarea. They
should probably disappear.

> Index: desktop/textinput.h
> ===================================================================
> --- desktop/textinput.h	(revision 7935)
> +++ desktop/textinput.h	(working copy)
> @@ -28,7 +28,8 @@
>  
>  #include <stdbool.h>
>  
> -struct browser_window;
> +#include "desktop/browser.h"
> +

Why this change?


J.




More information about the netsurf-dev mailing list