On Tue, 2009-07-21 at 11:36 +0200, Daniel Silverstone wrote:
> 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?
The only ones I haven't compiled and run are the BeOS and Amiga
frontends.
> > 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?
I'm not sure. Michael?
> 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?)
Possibly. I'm not sure I particularly like the above code, anyway :)
Certainly, there's now multiple places where line height is calculated
in this way. This makes it prone to breakage should we ever decide to
change the constant. The conversion to points won't change, of course.
> > 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.
It should be, yes. The only case where a box doesn't have a style is
because it's a FLOAT within an INLINE_CONTAINER, which gets its style
from the parent BLOCK/TABLE_CELL/INLINE_BLOCK.
> > @@ -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 :-(
Good :)
> > Index: image/svg.c
> > + plot_style_font->background = 0xffffff;
> > + plot_style_font->foreground = 0x000000;
>
> Is it reasonable to mutate this rather than copying?
I thought so. That said, copying is less prone to failure elsewhere
because someone else forgot to set up the colours they wanted. I'll make
this change (and elsewhere, too).
> > 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 ?
Currently, yes. The CSS weights are multiples of 100 in the range
[100,900].
> > 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
Yes. Done.
> > +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.
I just followed the idiom used for the bitmap flags in plotters.h. If we
change the above, we should change that, too.
> > +
> > +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.
Consider me slapped :)
> > 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.
I agree. The question arises, however, as to when formatting changes are
appropriate.
J.
