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 <[email protected]>
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.
