Review: Paul Blokus -- Core text area
jmb at netsurf-browser.org
Wed Jun 24 11:43:16 BST 2009
On Wed, 2009-06-24 at 10:14 +0200, Paweł Blokus wrote:
> I have changed the code according to your suggestions(I will remove
> the test code in the last commit). At some places I had doubts which I
> explain below.
Thanks. Once you've done that, please say so, and I'll re-review the
> > > + /* 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?
> The caret position is the character offset before which the caret is
> positioned. Only a space or newline can be at the end of a line. A
> newline doesn't belong to any line and the caret doesn't go to the
> next line this way. A space however is counted as belonging to the
> line which is higher so a caret after it would float to the beginning
> of the next line.
Ok, so this is to cater for lines which are soft-wrapped, yes?
Perhaps something like this would help:
/* If the calculated byte offset corresponds with the number of bytes
* in the line, and the line has been soft-wrapped, then ensure the
* caret offset is 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.
That said, however, perhaps we should give some thought to how to
resolve the ambiguity of character indices when soft-wrapping occurs.
> What should lines, that are already indented so much that it's hard to
> wrap them, look like? The style guide didn't give me an answer to that
It's a sure sign the code is too complex and needs refactoring.
> > > 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.
> I've changed this in treeview too as it was drawing blurry rectangles.
Yeah. It's unrelated to either the treeview or textarea changes, however
(which was mostly my point).
> Also, most of the unimplemented key presses deal with clipboard
> handling. I was wondering if I should integrate it with what is
> currently available in selection and create an unified API which would
> have be declared in desktop/gui.h, create a local clipboard or not
> implement it at all.
Good question. I don't have an answer right now. Perhaps someone else
has some thoughts.
More information about the netsurf-dev