On Tue, Jul 06, 2010 at 10:14:46AM +0100, Vincent Sanders wrote:
> here is the branch diff, its not perfect but it is better than what we
> have now and I want it to be merged.
I will take this comment into account as I review then. There's lots of
indentation snafus in this diff, perhaps you need to re-indent some files?
Also, some places you have mixed
rettype
funcname(args)
{
}
with
rettype funcname(args)
{
}
for exported functions.
Please make it consistent.
> Index: framebuffer/options.h
> #define EXTRA_OPTION_TABLE \
> - { "fb_depth", OPTION_INTEGER, &option_fb_depth }, \
> + { "fb_depth", OPTION_INTEGER, &option_fb_depth }, \
Gratuitious changing of indentation?
> Index: framebuffer/fbtk/scroll.c
> + * Copyright 2008 Vincent Sanders <[email protected]>
Shiny new file, and it's 2010 now. If you're just C&Ping code to split it out
then 2008,2010.
> +/** Vertical scroll widget */
> +static int
> +vscroll_redraw(fbtk_widget_t *widget, fbtk_callback_info *cbi)
This documentation comment seems pretty poor. Either drop the ** or do it
properly.
> +static int
> +vscrollarea_click(fbtk_widget_t *widget, fbtk_callback_info *cbi)
> + fbtk_tgrab_pointer(widget);
tgrab?
> +/* Horizontal scroll widget */
Aah, this isn't doc, so drop the ** from the above.
Such a large proportion of the code appears remarkably similar between the
horiz and vert scrollbars, couldn't there be some code sharing going on?
> Index: framebuffer/fbtk/event.c
> + * Copyright 2008 Vincent Sanders <[email protected]>
Copyright 2010?
> +fbtk_click(fbtk_widget_t *widget, nsfb_event_t *event)
> +{
Indentation snafud a lot in this function.
> +/* toggle pointer grab */
> +bool fbtk_tgrab_pointer(fbtk_widget_t *widget)
> +{
More indentation snafus here.
> +void
> +fbtk_warp_pointer(fbtk_widget_t *widget, int x, int y, bool relative)
> + } else {
> + /* pointer movement has been grabebd by a widget */
grabbed
> + /* post the movement */
Indentation snafu?
> +bool fbtk_event(fbtk_widget_t *root, nsfb_event_t *event, int timeout)
> +{
> + //LOG(("Reading event with timeout %d",timeout));
No C++ comments please.
> +/*
> + * Local Variables:
> + * c-basic-offset:8
> + * End:
Given this, it's amazing that there's loads of stuff in the above diff which is
4-char indent.
> Index: framebuffer/fbtk/text.c
> + * Copyright 2008 Vincent Sanders <[email protected]>
2010?
Also, this file is also full of indentation snafu, so I'm not going to mention
them, just go tidy it up please.
> +#define TEXT_WIDGET_BORDER 3
Can this please be given some kind of comment?
> +/* Lighten a colour by taking seven eights of each channel's intensity
> + * and adding a full eighth
> + */
> +#define brighten_colour(c1) \
> + (((((7 * ((c1 >> 16) & 0xff)) >> 3) + 32) << 16) | \
> + ((((7 * ((c1 >> 8) & 0xff)) >> 3) + 32) << 8) | \
> + ((((7 * (c1 & 0xff)) >> 3) + 32) << 0))
Why is this in text.c and not some generic colour manipulation header?
> Index: framebuffer/fbtk/fbtk.c
> + * Copyright 2008 Vincent Sanders <[email protected]>
2010? Also, indentation snafus in this file too
> +static void dump_tk_tree(fbtk_widget_t *widget)
> + LOG(("%*s%p",indent,"", widget));
spaces?
> + if (widget->first_child != NULL) {
> + widget = widget->first_child;
> + indent+=6;
spaces?
> + } else if (widget->next != NULL) {
> + widget = widget->next;
> + } else {
> + while ((widget->parent != NULL) &&
> + (widget->parent->next == NULL)) {
> + widget = widget->parent;
> + indent-=6;
spaces?
> + }
> + if (widget->parent != NULL) {
> + indent-=6;
spaces?
> + widget = widget->parent->next;
> + } else {
> + widget = NULL;
> + }
> + }
> + }
> +}
> +
> +/* @todo fbtk_set_pos_and_size, fbtk_set_mapping and
> + * fbtk_request_redraw need properly updating !
> + */
This isn't an @todo unless it's in a /** block.
> +int
> +fbtk_set_mapping(fbtk_widget_t *widget, bool map)
> +{
> + LOG(("setting mapping on %p to %d",widget,map));
spaces?
> +static void
> +swap_siblings(fbtk_widget_t *lw)
> + LOG(("Swapping %p with %p",lw,rw));
Spaces? (This is getting tedious. I know they're debug statements, but
they're still first-class-citizens in our code. I won't mention any more, but
a quick grep through your code ought to allow you to spot the ones which need
fixing up.)
> +enum {
> + POINT_LEFTOF_REGION = 1,
> + POINT_RIGHTOF_REGION = 2,
> + POINT_ABOVE_REGION = 4,
> + POINT_BELOW_REGION = 8,
> +};
> +
> +#define REGION(x,y,cx1,cx2,cy1,cy2) \
> + (( (y) > (cy2) ? POINT_BELOW_REGION : 0) | \
> + ( (y) < (cy1) ? POINT_ABOVE_REGION : 0) | \
> + ( (x) > (cx2) ? POINT_RIGHTOF_REGION : 0) | \
> + ( (x) < (cx1) ? POINT_LEFTOF_REGION : 0) )
> +
Some comments would be nice for these
> +#define SWAP(a, b) do { int t; t=(a); (a)=(b); (b)=t; } while(0)
Ick, but again, comments please.
> +/* clip a rectangle to a widgets area rectangle */
> +bool fbtk_clip_rect(const bbox_t * restrict clip, bbox_t * restrict box)
Given this is clearly exported, why is it not commented in the header?
> + if (box->x1 < box->x0) SWAP(box->x0, box->x1);
> + if (box->y1 < box->y0) SWAP(box->y0, box->y1);
NetSurf style requires
if (condition)
single_statement_body();
> + if ((region1 | region2) && (region1 & region2))
> + return false;
Just like you did here.
> +/** call the widgets drawing callbacks
> +static int
> +do_redraw(nsfb_t *nsfb, fbtk_widget_t *widget)
Perform a depth-first tree-walk, calling the redraw callback of the widgets in
turn.
> Index: framebuffer/fbtk/window.c
> + * Copyright 2008 Vincent Sanders <[email protected]>
2010? Indentation...
> +static int
> +fb_redraw_window(fbtk_widget_t *widget, fbtk_callback_info *cbi)
> +{
> + nsfb_bbox_t bbox;
> + nsfb_t *nsfb;
> +
> + if ((widget->bg & 0xFF000000) == 0)
I've seen this a few times now and it's starting to grate. Do you need a
"widget_is_transparent()" call to use?
> Index: framebuffer/fbtk/widget.h
> + * Copyright 2008 Vincent Sanders <[email protected]>
2010?
> +struct fbtk_widget_s {
> + fbtk_widget_t *next; /* next lower z ordered widget in tree */
> + fbtk_widget_t *prev; /* next higher z ordered widget in tree */
This type does not exist. The header hasn't included anything to bring it in,
nor has it defined it. Bad Vincent.
> + fbtk_callback callback[FBTK_CBT_END];
Ditto.
Also you're using bool without including stdbool.h
> + nsfb_t *fb;
Where has this come from?
> + struct {
> + struct bitmap *bitmap;
And this?
> + } bitmap;
> +
> + fbtk_enter_t enter;
And this
> + struct fbtk_widget_s *btnul; /* scroll button up/left */
Oh, but here you have struct fbtk_widget_s rather than fbtk_widget_t?
Consistency please.
> + } u;
I find the 'u' particularly uninformative, but I understand the need for
brevity.
> Index: framebuffer/fbtk/osk.c
> + * Copyright 2008 Vincent Sanders <[email protected]>
2010? Indentation.
> +fbtk_widget_t *osk;
Global, exported symbol? Should this be static?
> Index: framebuffer/fbtk/bitmap.c
> +//#include <libnsfb_plot_util.h>
No C++ style comments please
> +//#include "desktop/plotters.h"
Ditto
> Index: framebuffer/gui.c
Indentation issues here too.
> /* pump up the volume. dance, dance! lets do it */
80's lyrics have no place in commentary. Perhaps tell us something useful here?
> +static int
> +fb_osk_click(fbtk_widget_t *widget,fbtk_callback_info *cbi)
Spaces?
> Index: framebuffer/localhistory.c
> + * Copyright 2008 Vincent Sanders <[email protected]>
2010? Indentation issues.
> + //fbtk_set_handler(gw->localhistory, FBTK_CBT_INPUT,
> fb_browser_window_input, gw);
> + //fbtk_set_handler(gw->localhistory, FBTK_CBT_POINTERMOVE,
> fb_browser_window_move, bw);
C++ comments? Icky!
> Index: framebuffer/thumbnail.c
> bool thumbnail_create(struct hlcache_handle *content, struct bitmap *bitmap,
> const char *url)
> + if (url)
If url is what?
> Index: framebuffer/gui.h
Indentation issues
> Index: framebuffer/fbtk.h
> +/** Retrive the framebuffer library handle from toolkit widget.
Retrieve
> +/** Get a widgets absolute horizontal screen co-ordinate.
widget's
> +/** Get a widgets absolute vertical screen co-ordinate.
Ditto
> +/** Get a widgets width.
Ditto
> +/** Get a widgets height.
Ditto
> +/** get widgets bounding box in absolute screen co-ordinates.
Get a widget's
> +/** Change the widgets position and size.
widget's
> +/** Map a widget and request it is redrawn.
request that it be redrawn
> +/** Destroy a widget and all its descendents.
all of its descendants.
D.
--
Daniel Silverstone http://www.netsurf-browser.org/
PGP mail accepted and encouraged. Key Id: 3CCE BABE 206C 3B69
