On Tue, Jul 06, 2010 at 06:10:33PM +0100, Daniel Silverstone wrote:
> 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?
>
all files re-intented and whitespace cleaned
> Also, some places you have mixed
>
> rettype
> funcname(args)
> {
> }
>
> with
>
> rettype funcname(args)
> {
> }
>
> for exported functions.
>
> Please make it consistent.
it is now consistant
>
> > 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?
>
well i just applied the indentation all the rest of the files have.
> > 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.
>
fixed them all, I wont respond to them all
> > +/** 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.
>
dropped
> > +static int
> > +vscrollarea_click(fbtk_widget_t *widget, fbtk_callback_info *cbi)
> > + fbtk_tgrab_pointer(widget);
>
> tgrab?
>
toggle grab, documented.
> > +/* Horizontal scroll widget */
>
> Aah, this isn't doc, so drop the ** from the above.
>
cleaned it up a bit
> 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?
>
yes...I just cannot figure out how to do it and it look clean
> > Index: framebuffer/fbtk/event.c
> > + * Copyright 2008 Vincent Sanders <[email protected]>
>
> Copyright 2010?
>
all changed as i said
> > +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
thanks
> > + /* 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.
>
all gone
> > +/*
> > + * 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.
>
not any more
> > 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.
>
did
> > +#define TEXT_WIDGET_BORDER 3
>
> Can this please be given some kind of comment?
#define TEXT_WIDGET_BORDER 3 /**< The pixel border round a text widget. */
>
> > +/* 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?
some of them are in desktop/plot_style.h some elsewhere, guidence please?
>
> > Index: framebuffer/fbtk/fbtk.c
> > + * Copyright 2008 Vincent Sanders <[email protected]>
>
> 2010? Also, indentation snafus in this file too
fixified
>
> > +static void dump_tk_tree(fbtk_widget_t *widget)
> > + LOG(("%*s%p",indent,"", widget));
>
> spaces?
fixed
>
> > + if (widget->first_child != NULL) {
> > + widget = widget->first_child;
> > + indent+=6;
>
> spaces?
fixed
>
> > + } else if (widget->next != NULL) {
> > + widget = widget->next;
> > + } else {
> > + while ((widget->parent != NULL) &&
> > + (widget->parent->next == NULL)) {
> > + widget = widget->parent;
> > + indent-=6;
>
> spaces?
fixed
>
> > + }
> > + if (widget->parent != NULL) {
> > + indent-=6;
>
> spaces?
fixed
>
> > + 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.
removed, I already fixed em
>
> > +int
> > +fbtk_set_mapping(fbtk_widget_t *widget, bool map)
> > +{
> > + LOG(("setting mapping on %p to %d",widget,map));
>
> spaces?
>
fixed
> > +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.)
>
fixified
> > +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
>
added some basic comments
> > +#define SWAP(a, b) do { int t; t=(a); (a)=(b); (b)=t; } while(0)
>
> Ick, but again, comments please.
>
added.
> > +/* 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?
>
is now
> > + 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();
>
yeah, changed
> > + if ((region1 | region2) && (region1 & region2))
> > + return false;
>
> Just like you did here.
>
yup
> > +/** 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.
>
changed
> > 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?
>
we probably should just fix the bloody fill interface to pass the
right transparancy parameters, right now we do not in the core so we
need these.
> > Index: framebuffer/fbtk/widget.h
> > + * Copyright 2008 Vincent Sanders <[email protected]>
>
> 2010?
changed
>
> > +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.
bah its in fbtk.h which is a pre-requisite...but I think I shall alter
them to struct fbtk_widget_s * to pacify you.
>
> > + fbtk_callback callback[FBTK_CBT_END];
>
> Ditto.
I need to fix this better in general, I need to think about it.
>
> Also you're using bool without including stdbool.h
>
fixed
> > + nsfb_t *fb;
>
> Where has this come from?
>
nsfb.h but I think this whole file suffers from simply being the
extended private interface which is not exposed in fbtk.h and you must
include teh former to use this header.
> > + struct {
> > + struct bitmap *bitmap;
>
> And this?
see previous comment
>
> > + } bitmap;
> > +
> > + fbtk_enter_t enter;
>
> And this
>
see previous comment
> > + 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.
I went with the struct thing but see the above comment
>
> > + } u;
>
> I find the 'u' particularly uninformative, but I understand the need for
> brevity.
yeah I do not care for the idom but it is the universal union way is it not?
>
> > Index: framebuffer/fbtk/osk.c
> > + * Copyright 2008 Vincent Sanders <[email protected]>
>
> 2010? Indentation.
>
> > +fbtk_widget_t *osk;
>
> Global, exported symbol? Should this be static?
>
yup
> > Index: framebuffer/fbtk/bitmap.c
> > +//#include <libnsfb_plot_util.h>
>
> No C++ style comments please
>
> > +//#include "desktop/plotters.h"
>
> Ditto
>
all gone
> > 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?
>
well it was your comment originally ;-p altered to
/* if the pan exceeds the viewport size just redraw the whole area */
> > +static int
> > +fb_osk_click(fbtk_widget_t *widget,fbtk_callback_info *cbi)
>
> Spaces?
>
fixified
and all the indentation in the file etc.
> > 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!
>
cleaned up
> > Index: framebuffer/thumbnail.c
> > bool thumbnail_create(struct hlcache_handle *content, struct bitmap
> > *bitmap,
> > const char *url)
>
> > + if (url)
>
> If url is what?
! NULL and it says that now
>
> > Index: framebuffer/gui.h
>
> Indentation issues
>
no more
> > Index: framebuffer/fbtk.h
> > +/** Retrive the framebuffer library handle from toolkit widget.
>
> Retrieve
>
ta
> > +/** 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.
>
all done
> D.
>
> --
> Daniel Silverstone http://www.netsurf-browser.org/
> PGP mail accepted and encouraged. Key Id: 3CCE BABE 206C 3B69
>
--
Regards Vincent
http://www.kyllikki.org/
