On Fri, Mar 21, 2014 at 3:38 AM, Michael Drake <[email protected]> wrote:
> Hi Achal-Aggarwal, > > Thanks for this patch. We need to fix up a few things, but you did a good > job tracking down the source of the bug in our rather large code base. I > hope this is an instructive and valuable experience for you. > > > On 20/03/14 21:32, Achal-Aggarwal wrote: > >> --- >> desktop/textarea.c | 9 +++++---- >> render/font.c | 3 +-- >> 2 files changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/desktop/textarea.c b/desktop/textarea.c >> index 584642d..dcaa2e8 100644 >> --- a/desktop/textarea.c >> +++ b/desktop/textarea.c >> @@ -1786,7 +1786,9 @@ static void textarea_setup_text_offsets(struct >> textarea *ta) >> /* Single line text area; text is vertically centered */ >> int vis_height = ta->vis_height - 2 * ta->border_width; >> text_y_offset += (vis_height - ta->line_height + 1) / 2; >> - text_y_offset_baseline += (vis_height * 3 + 2) / 4; >> + if (text_y_offset < 0) >> + text_y_offset = 0; >> > > I'm not sure this forcing of non-negative y-offset is correct. > > Previously if the visible height of the textarea was less than the line > height we were clipping from the top and bottom to show the middle, but > with that change we would only see the top. The top of lower case latin > letters doesn't often contain much. Yeah you are right, centering text is much better than cropping its bottom. I guess I got motivated by how chrome does it. I thinking about sending a patch about this to their repo also. hehe. > > + text_y_offset_baseline += text_y_offset + (ta->line_height >> * 3 + 2) / 4; >> > > That would work, but you've accumulated more potential for error. > e.g. there is a +/- half pixel error from the text_y_offset calculation's > division, and another +/- half pixel error from the division in the > text_y_offset_baseline calc. > > Better would be to get the text_y_offset_baseline with only one division, > and therefore less places to lose fractions of pixels. > > I expect something like this would work: > > text_y_offset_baseline += (2 * vis_height + ta->line_height + 2) / 4; Again you are right in this one also, with this we will have error of only +-0.5 pixel > > > } >> >> ta->text_y_offset = text_y_offset; >> @@ -1894,9 +1896,8 @@ struct textarea *textarea_create(const >> textarea_flags flags, >> ret->show = &ret->text; >> } >> >> - ret->line_height = FIXTOINT(FDIV((FMUL(FLTTOFIX(1.3), >> - FMUL(nscss_screen_dpi, >> INTTOFIX((setup->text.size))))), >> - FONT_SIZE_SCALE * F_72)); >> + ret->line_height = FIXTOINT(FDIV(FMUL(nscss_screen_dpi, >> + INTTOFIX((setup->text.size)/FONT_SIZE_SCALE)),F_72)) * >> 1.3 ; >> > > This change is not right. > > First, you're introducing a multiply 1.3, which is a floating point > operation. The rest of that calculation is using fixed point to avoid > using floating point. > > Second you're doing setup->text.size/FONT_SIZE_SCALE which will lose any > fractional part of the original font size. E.g. your change would make > 12.8pt get treated as if it were 12pt. Third, I'm not sure why you're > modifying that calculation. I don't see anything wrong with it. Have I > missed something? > earlier FONT_SIZE_SCALE was treated as a fixed point which is actually an integer and multiplied with another fixed point ( F_72 ) as FONT_SIZE_SCALE * F_72 instead of using FMUL I changed a bit, now it is ret->line_height = FIXTOINT(FDIV(FMUL(nscss_screen_dpi, FLTTOFIX((setup->text.size * 1.3) / FONT_SIZE_SCALE)), F_72)); incorporating 1.3 and not loosing precision of text.size. I checked that when I set font-size:100px I got line_height:100 and when I set 75pt, I got 100px equivalent to 75pt. > > > diff --git a/render/font.c b/render/font.c >> index 03c5a36..f594678 100644 >> --- a/render/font.c >> +++ b/render/font.c >> @@ -45,8 +45,7 @@ void font_plot_style_from_css(const css_computed_style >> *css, >> css_computed_font_family(css, &families)); >> >> css_computed_font_size(css, &length, &unit); >> - fstyle->size = FIXTOINT(FMUL(nscss_len2pt(length, unit), >> - INTTOFIX(FONT_SIZE_SCALE))); >> + fstyle->size = FIXTOINT(nscss_len2pt(length, unit)) * >> FONT_SIZE_SCALE; >> > > Again, this change isn't relevant to the bug as far as I can see. > > I guess we don't need to convert FONT_SIZE_SCALE to fixed point. checkout https://github.com/Achal-Aggarwal/netsurf/blob/master/render/textplain.c#L1323 size is point scaled using FONT_SIZE_SCALE. I am attaching a test html file, please do test the patch on it. I have one more patch applying first two changes, how can I send that patch to this thread? And thank you for reviewing it, I really appreciate it. I suppose until you try head on with such a large code base you never get to get familiar with it. So I will keep doing that. Regards Achal On Fri, Mar 21, 2014 at 3:38 AM, Michael Drake <[email protected]>wrote: > Hi Achal-Aggarwal, > > Thanks for this patch. We need to fix up a few things, but you did a good > job tracking down the source of the bug in our rather large code base. I > hope this is an instructive and valuable experience for you. > > > On 20/03/14 21:32, Achal-Aggarwal wrote: > >> --- >> desktop/textarea.c | 9 +++++---- >> render/font.c | 3 +-- >> 2 files changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/desktop/textarea.c b/desktop/textarea.c >> index 584642d..dcaa2e8 100644 >> --- a/desktop/textarea.c >> +++ b/desktop/textarea.c >> @@ -1786,7 +1786,9 @@ static void textarea_setup_text_offsets(struct >> textarea *ta) >> /* Single line text area; text is vertically centered */ >> int vis_height = ta->vis_height - 2 * ta->border_width; >> text_y_offset += (vis_height - ta->line_height + 1) / 2; >> - text_y_offset_baseline += (vis_height * 3 + 2) / 4; >> + if (text_y_offset < 0) >> + text_y_offset = 0; >> > > I'm not sure this forcing of non-negative y-offset is correct. > > Previously if the visible height of the textarea was less than the line > height we were clipping from the top and bottom to show the middle, but > with that change we would only see the top. The top of lower case latin > letters doesn't often contain much. > > > + text_y_offset_baseline += text_y_offset + >> (ta->line_height * 3 + 2) / 4; >> > > That would work, but you've accumulated more potential for error. > e.g. there is a +/- half pixel error from the text_y_offset calculation's > division, and another +/- half pixel error from the division in the > text_y_offset_baseline calc. > > Better would be to get the text_y_offset_baseline with only one division, > and therefore less places to lose fractions of pixels. > > I expect something like this would work: > > text_y_offset_baseline += (2 * vis_height + ta->line_height + 2) / 4; > > > } >> >> ta->text_y_offset = text_y_offset; >> @@ -1894,9 +1896,8 @@ struct textarea *textarea_create(const >> textarea_flags flags, >> ret->show = &ret->text; >> } >> >> - ret->line_height = FIXTOINT(FDIV((FMUL(FLTTOFIX(1.3), >> - FMUL(nscss_screen_dpi, >> INTTOFIX((setup->text.size))))), >> - FONT_SIZE_SCALE * F_72)); >> + ret->line_height = FIXTOINT(FDIV(FMUL(nscss_screen_dpi, >> + INTTOFIX((setup->text.size)/FONT_SIZE_SCALE)),F_72)) * >> 1.3 ; >> > > This change is not right. > > First, you're introducing a multiply 1.3, which is a floating point > operation. The rest of that calculation is using fixed point to avoid > using floating point. > > Second you're doing setup->text.size/FONT_SIZE_SCALE which will lose any > fractional part of the original font size. E.g. your change would make > 12.8pt get treated as if it were 12pt. Third, I'm not sure why you're > modifying that calculation. I don't see anything wrong with it. Have I > missed something? > > > > diff --git a/render/font.c b/render/font.c >> index 03c5a36..f594678 100644 >> --- a/render/font.c >> +++ b/render/font.c >> @@ -45,8 +45,7 @@ void font_plot_style_from_css(const css_computed_style >> *css, >> css_computed_font_family(css, &families)); >> >> css_computed_font_size(css, &length, &unit); >> - fstyle->size = FIXTOINT(FMUL(nscss_len2pt(length, unit), >> - INTTOFIX(FONT_SIZE_SCALE))); >> + fstyle->size = FIXTOINT(nscss_len2pt(length, unit)) * >> FONT_SIZE_SCALE; >> > > Again, this change isn't relevant to the bug as far as I can see. > > Cheers, > > Michael > > > -- AchalTitle: I am a title H:NIL FS:NIL
H:120 FS:40
H:NIL FS:40
H:120 FS:100
H:80 FS:100
H:50 FS:100
Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.
