On Mon, 2009-07-20 at 16:18 +0100, John-Mark Bell wrote:
I note that you have changed all the front ends, how many have you at
least compile-tested?
> Index: render/html_redraw.c
There are some whitespace change nits in this file, but mostly I don't
think it'll affect anyone else (other than your libcss branch) so I'm
not highlighting them.
> @@ -914,10 +918,8 @@
>
> if (!plot.text(x, y + (int)
> (height * 0.75 * scale),
This *0.75 malarky... If this is for RISC OS's
baseline-rather-than-top-left behaviour for font plotting, then why not
do it in the RISC OS frontend instead of here to be undone in all the
other frontends? Or am I simply mistaken?
Since you're refactoring the font plotting API a bit, now is the time to
fix this particular nit.
> Index: render/textplain.c
> @@ -332,8 +336,7 @@
> + float line_height = (textplain_style.size * css_screen_dpi / 72) * 1.2;
In many places, we appear to use css_screen_dpi / 72 as a handy-dandy
value. Perhaps we should cache that? (Since you do it in this branch, is
it worth considering?)
> Index: render/layout.c
> + font_plot_style_from_css(b->style ? b->style
> + : b->parent->parent->style,
Is this guaranteed? That anything without a style is guaranteed to have
a grandparent which does?
> + font_plot_style_from_css(b->style ? b->style
> + : b->parent->parent->style,
And again.
> @@ -2406,7 +2423,7 @@
> for (j = i; j != b->length &&
> b->text[j] != ' '; j++)
> ;
I hate this idiom. But since you didn't change it, I can't ask you to
fix it :-(
> Index: image/svg.c
> + plot_style_font->background = 0xffffff;
> + plot_style_font->foreground = 0x000000;
Is it reasonable to mutate this rather than copying?
> Index: riscos/font.c
> + static const rufl_style weight_table[] = {
> + rufl_WEIGHT_100,
> + rufl_WEIGHT_200,
> + rufl_WEIGHT_300,
> + rufl_WEIGHT_400,
> + rufl_WEIGHT_500,
> + rufl_WEIGHT_600,
> + rufl_WEIGHT_700,
> + rufl_WEIGHT_800,
> + rufl_WEIGHT_900
> + };
> + *font_style |= weight_table[(fstyle->weight / 100) - 1];
There's no bounds checking on this operation. Are you certain that with
CSS you won't be able to set a weight above 999 ?
> Index: desktop/plot_style.h
Your re-indentation didn't really belong in this diff, but I'm not going
to request it be removed. Just try not to do it at the same time as code
changes in the future. Alternatively, set your svn diff to ignore
whitespace when you're issuing reviews, then I won't even see it :-)
> + PLOT_FONT_FAMILY_FANTASY
Several times this is used as the "end of the enum" marker. Could we
have an additional:
PLOT_FONT_FAMILY_COUNT
which would then be used instead of PLOT_FONT_FAMILY_FANTASY + 1
> +typedef unsigned long plot_font_flags_t;
> +#define FONTF_NONE 0
> +#define FONTF_ITALIC 1
> +#define FONTF_OBLIQUE 2
> +#define FONTF_SMALLCAPS 4
This *can* be an enum, a'la
typedef enum {
FONTF_NONE = 0,
FONTF_ITALIC = 1,
FONTF_OBLIQUE = 2,
FONTF_SMALLCAPS = 4
} plot_font_flags_t;
This has the side-effect that we get basic type checking in the case of
setting only a single value. Dunno if it's worthwhile for this change,
but something to consider in future perhaps.
> +
> +typedef struct {
> + plot_font_generic_family_t family;
> + int size; /* In points */
> + int weight;
> + plot_font_flags_t flags;
> + colour background;
> + colour foreground;
> +} plot_font_style_t;
Docstring! Come on John-Mark. You know better than this.
> Index: desktop/plotters.h
> - /* shape primatives */
> - bool (*arc)(int x, int y, int radius, int angle1, int angle2, const
> plot_style_t *pstyle);
> + /* shape primitives */
> + bool (*arc)(int x, int y, int radius, int angle1, int angle2,
> + const plot_style_t *pstyle);
This reflow doesn't belong to this change really.
> + bool (*line)(int x0, int y0, int x1, int y1,
> + const plot_style_t *pstyle);
> + bool (*rectangle)(int x0, int y0, int x1, int y1,
> + const plot_style_t *pstyle);
> + bool (*polygon)(const int *p, unsigned int n,
> + const plot_style_t *pstyle);
Ditto.
> Index: desktop/knockout.c
> +static void knockout_calculate(int x0, int y0, int x1, int y1,
> + struct knockout_box *box);
Ditto
> +static bool knockout_plot_fill_recursive(struct knockout_box *box,
> + plot_style_t *plot_style);
Ditto
> +static bool knockout_plot_line(int x0, int y0, int x1, int y1,
> + const plot_style_t *pstyle);
> +static bool knockout_plot_polygon(const int *p, unsigned int n,
> + const plot_style_t *pstyle);
> +static bool knockout_plot_rectangle(int x0, int y0, int x1, int y1,
> + const plot_style_t *plot_style);
Ditto
> +static bool knockout_plot_disc(int x, int y, int radius,
> + const plot_style_t *pstyle);
> +static bool knockout_plot_arc(int x, int y, int radius, int angle1, int
> angle2,
> + const plot_style_t *pstyle);
Ditto.
Other than that, the only issues I would raise are related to using
FANTASY + 1 as a COUNT replacement all over the place, but I mentioned
that when I reviewed the enum itself.
Seems solidly done, but basically the majority of the work was
mechanical so I'll trust its behaviour.
D.
