Review: Paul Blokus -- Core text area

John-Mark Bell 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
code.

> > > +     /* 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.


J.





More information about the netsurf-dev mailing list