On Fri, Mar 21, 2014 at 3:38 AM, Michael Drake <mike@smoothartist.com> 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.I'm not sure this forcing of non-negative y-offset is correct.
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;
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.
That would work, but you've accumulated more potential for error.+ text_y_offset_baseline += text_y_offset + (ta->line_height * 3 + 2) / 4;
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
This change is not right.}
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 ;
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.
Again, this change isn't relevant to the bug as far as I can see.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;
I guess we don't need to convert FONT_SIZE_SCALE to fixed point.
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 <mike@smoothartist.com> 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.I'm not sure this forcing of non-negative y-offset is correct.
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;
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.That would work, but you've accumulated more potential for error.
+ text_y_offset_baseline += text_y_offset + (ta->line_height * 3 + 2) / 4;
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;This change is not right.
}
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 ;
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?Again, this change isn't relevant to the bug as far as I can see.
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;
Cheers,
Michael
Achal
No comments:
Post a Comment