Review: Paul Blokus -- core text widget (round 2)

John-Mark Bell jmb at netsurf-browser.org
Wed Jun 24 18:40:38 BST 2009


On Wed, 2009-06-24 at 17:33 +0100, John-Mark Bell wrote:

> Index: desktop/textarea.c
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ desktop/textarea.c	2009-06-24 17:28:49.000000000 +0100

> +#define MARGIN_LEFT 2
> +#define MARGIN_RIGHT 2
> +#define CARET_COLOR 0x000000
> +/* background color for readonly textarea*/

Style: Space before */ (This applies in multiple places)

> +struct text_area {
> +
> +	int x, y;			/**< Coordinates of the widget
> +					     (top left corner) with respect to
> +				 	     canvas origin(these don't change
> +					     it is the canvas which gets
> +					     scrolled) */

Does the widget need to know about this? Can the client not present
coordinates relative to the top left of the text area itself?
					
> +	int scroll_x, scroll_y;		/**< scroll offsets of the textarea
> +					     content */
> +	
> +	unsigned int flags;		/**< Textarea flags */
> +	int vis_width;			/**< Visible width, in pixels */
> +	int vis_height;			/**< Visible height, in pixels */
> +
> +	char *text;			/**< UTF-8 text */
> +	unsigned int text_alloc;	/**< Size of allocated text */
> +	unsigned int text_len;		/**< Length of text, in bytes */
> +	unsigned int text_utf8_len;	/**< Length of text, in characters
> +					     without the /0 character*/

s#/0 character#trailing NUL#

> +	struct {
> +		int line;		/**< Line caret is on */
> +		int char_off;		/**< Character index of caret */

Make it clear that this index is *within* line and is not the global
caret index.

> +	} 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 16
> +	struct line_info *lines;	/**< Line info array */
> +	int line_height;		/**< Line height obtained from style */
> +	
> +	/** Callback functions for a redraw request*/
> +	textarea_start_radraw_callback redraw_start_callback;
> +	textarea_start_radraw_callback redraw_end_callback;

s/radraw/redraw/
	 
> +	void *data; /** < Callback data for both callback functions*/
> +	
> +	int drag_start_char;/**< Character index at which the drag was started*/
> +};
> +
> +
> +static bool textarea_insert_text(struct text_area *ta, unsigned int index,
> +			  const char *text);
> +static bool textarea_replace_text(struct text_area *ta, unsigned int start,
> +			   unsigned int end, const char *text);

Style: continuation line indentation should be 2 tabs.

> +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, void *data)
> +{

> +	ret->text = malloc(64);
> +	if (ret->text == NULL) {
> +		LOG(("malloc failed"));
> +		free(ret);
> +		return NULL;
> +	}
> +	ret->text[0] = '\0';
> +	ret->text_alloc = 64;
> +	ret->text_len = 1;
> +	ret->text_utf8_len = 1;

Should be 0, if the utf8 length doesn't include the trailing NUL?
	
> +	ret->style = malloc(sizeof(struct css_style));
> +	if (ret->style == NULL) {
> +		LOG(("malloc failed"));
> +		free(ret->text);
> +		free(ret);
> +		return NULL;
> +	}
> +	memcpy(ret->style, style, sizeof(struct css_style));
> +	ret->line_height = css_len2px(&(style->line_height.value.length),
> +			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;

ret->lines = NULL (it's a pointer)

> +	return ret;
> +}

> +/**
> + * 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
> + * \return true on success, false otherwise (memory exhaustion or
> + *	   readonly flag set)

I'd rather avoid the ambiguity as to what "false" means here. Do we ever
care if the textarea's readonly? If not, then make that case return
true. If we do, then it would be better to define an enum that specifies
the return codes. (This applies in multiple cases)

> + */
> +bool textarea_insert_text(struct text_area *ta, unsigned int index,
> +		const char *text)
> +{
> +	unsigned int b_len = strlen(text);
> +	size_t b_off;
> +
> +	if (ta->flags & TEXTAREA_READONLY)
> +		return false;
> +
> +	/* Find insertion point */
> +	if (index > ta->text_utf8_len)
> +		index = ta->text_utf8_len;
> +
> +	LOG(("inserting at %i\n", index));

Lose this debug, once you're satisfied that it's working.
	
> +/**
> + * Set the caret's position
> + *
> + * \param ta 		Text area
> + * \param caret 	0-based character index to place caret at

This needs to document the special behaviour of -1 (i.e. that it removes
the caret)

> + * \param line_end 	says whether caret should be positioned at the end of
> + *			the previos line if it indices on the beginning of
> + *			a line that was crated by a soft wrap (on a space)

Spelling: previous & created

> + * \return true on success false otherwise
> + */
> +bool textarea_set_caret(struct text_area *ta, int caret, bool line_end)

Does the client ever care about the caret position when soft-wrapping
occurs? If not, I suggest making this function static and making the
public function a wrapper that always sets line_end to true/false as
appropriate.

Perhaps there's a better solution entirely. We know when soft wrapping
has occurred because line[n+1].b_start == line[n].b_end. Whereas, with a
hard-wrap, line[n+1].b_start == line[n].b_end + 1. 

So, define textarea_set_caret as always placing the caret in the first
line that corresponds with a given character offset (i.e. if a line is
soft-wrapped, the caret will always be placed after the space at the end
of line n).

The only difficulty that then arises is what happens when the user
presses KEY_(DELETE_)?{LEFT,RIGHT}. These will need to check for the
soft-wrap case and deal with it appropriately.

> +/**
> + * get character offset from the beginning of the text for some coordinates
> + *
> + * \param ta		Text area
> + * \param x		X coordinate
> + * \param y		Y coordinate
> + * \param line_end	will be updated to true if the caret should be placed at
> + * 			the end of a soft wrapped line false otherwise

Similarly. Why does the client have to care about soft-wrapping?

> + * \return		character offset
> + */
> +unsigned int textarea_get_xy_offset(struct text_area *ta, int x, int y,
> +		bool *line_end)
> +{
> +	size_t b_off, temp;
> +	unsigned int c_off;
> +	int line;
> +
> +	if (!ta->line_count)
> +		return 0;
> +
> +	x = x - ta->x - MARGIN_LEFT + ta->scroll_x;
> +	y = y - ta->y + ta->scroll_y;
> +
> +	if (x < 0)
> +		x = 0;
> +	
> +	line = y / ta->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);
> +	
> +	*line_end = false;
> +	/* If the calculated byte offset corresponds with the number of bytes
> +	* in the line, and the line has been soft-wrapped, then check line_end
> +	* true to ensure the caret offset will be set before the trailing space
> +	* character, rather than after it. Otherwise, the caret will be placed
> +	* at the start of the following line, which is undesirable.
> +	*/	

Style: The *s should line up in this comment.

> +/**
> + * 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)
> +{
> +	unsigned int c_off = 0, b_off;
> +
> +	
> +	/* if the text is a \0 only*/
> +	if (ta->text_len == 1)
> +		return 0;

May be clearer to use ta->text_utf8_len == 0 here.
	

> +/**
> + * Removes any CR characters and changes newlines into spaces if it is a single
> + * line textarea.
> + *

The following would be better:

Normalises any line endings within the text, replacing CRLF or CR with
LF as necessary. If the textarea is single line, then all linebreaks are
converted into spaces.

> + * \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. If it's a CRNL pair delete it ot replace it
> +	 * with NL otherwise.
> +	 */

s/NL/LF/

> Index: desktop/textarea.h
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ desktop/textarea.h	2009-06-24 17:28:49.000000000 +0100
> @@ -0,0 +1,58 @@
> +/*
> + * Copyright 2006 John-Mark Bell <jmb at netsurf-browser.org>
> + * Copyright 2009 Paul Blokus <paul_pl at users.sourceforge.net> 
> + *
> + * This file is part of NetSurf, http://www.netsurf-browser.org/
> + *
> + * NetSurf is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * NetSurf is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/** \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;
> +
> +typedef void(*textarea_start_radraw_callback)(void *data);
> +typedef void(*textarea_end_radraw_callback)(void *data);

s/radraw/redraw/.

> Index: gtk/gtk_gui.c
> ===================================================================
> --- gtk/gtk_gui.c	(revision 7935)
> +++ gtk/gtk_gui.c	(working copy)
>  
> +uint32_t gtk_gui_gdkkey_to_nskey(GdkEventKey *key)
> +{
> +        /* this function will need to become much more complex to support
> +         * everything that the RISC OS version does.  But this will do for
> +         * now.  I hope.
> +         */
> +        switch (key->keyval)
> +        {
> +                case GDK_BackSpace:             
> +			if (key->state & GDK_SHIFT_MASK)
> +				return KEY_DELETE_LINE_START;
> +			else
> +				return KEY_DELETE_LEFT;
> +                case GDK_Delete:                
> +			if (key->state & GDK_SHIFT_MASK)
> +				return KEY_DELETE_LINE_END;
> +			else
> +				return KEY_DELETE_RIGHT;
> +                case GDK_Linefeed:              return 13;
> +                case GDK_Return:                return 10;
> +                case GDK_Left:                  return KEY_LEFT;
> +                case GDK_Right:                 return KEY_RIGHT;
> +                case GDK_Up:                    return KEY_UP;
> +                case GDK_Down:                  return KEY_DOWN;
> +		case GDK_Home:			
> +			if (key->state & GDK_CONTROL_MASK)
> +				return KEY_TEXT_START;
> +			else
> +				return KEY_LINE_START;
> +		case GDK_End:			
> +			if (key->state & GDK_CONTROL_MASK)
> +				return KEY_TEXT_END;
> +			else
> +				return KEY_LINE_END;
> +		case GDK_Page_Up:
> +			return KEY_PAGE_UP;
> +		case GDK_Page_Down:
> +			return KEY_PAGE_DOWN;
> +		case 'a':
> +			if (key->state & GDK_CONTROL_MASK)
> +				return KEY_SELECT_ALL;
> +			return gdk_keyval_to_unicode(
> +					key->keyval);
> +		case 'u':
> +			if (key->state & GDK_CONTROL_MASK)
> +				return KEY_CLEAR_SELECTION;
> +			return gdk_keyval_to_unicode(
> +					key->keyval);
> +		case GDK_Escape:
> +			return KEY_ESCAPE;
> +
> +                /* Modifiers - do nothing for now */
> +                case GDK_Shift_L:
> +                case GDK_Shift_R:
> +                case GDK_Control_L:
> +                case GDK_Control_R:
> +                case GDK_Caps_Lock:
> +                case GDK_Shift_Lock:
> +                case GDK_Meta_L:
> +                case GDK_Meta_R:
> +                case GDK_Alt_L:
> +                case GDK_Alt_R:
> +                case GDK_Super_L:
> +                case GDK_Super_R:
> +                case GDK_Hyper_L:
> +                case GDK_Hyper_R:               return 0;
> +
> +                default:                       return gdk_keyval_to_unicode(
> +								key->keyval);
> +        }

There's something odd with the indentation here. Make sure everything is
tabs, not spaces.


J.




More information about the netsurf-dev mailing list