-
Notifications
You must be signed in to change notification settings - Fork 942
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for multi-line text to SkiaRenderContext (#1538) #1549
Conversation
var paint = this.GetTextPaint(fontFamily, fontSize, fontWeight, out var shaper); | ||
var width = this.MeasureText(text, shaper, paint); | ||
var height = paint.GetFontMetrics(out _); | ||
var height = paint.GetFontMetrics(out _) * lines.Length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels wrong, but I can't tell from looking at the examples (e.g. Issue 1545 example). It reads like it is adding extra line spacing for the 'last' line (i.e. lineHeight
is the sum of the line height and spacing). Sorry if I've just got myself confused (this is something that OxyPlot.Pdf
gets wrong, and I think Svg
does as well but will be fixed with #1531)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well paint.GetFontMetrics(out _)
returns the suggested line height. To get the total height of the text block we multiply this by the number of lines, so for a text with one line nothing changes, for two lines we get double the height etc. Sounds right to me? Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I am confused...
My reading of the GetFontMetrics
method documention is that it returns the line height (i.e. |descender| + |ascender|
) plus the line gap. This would mean that the height of a box with 1 line of text should be less than half of the height of a box with 2 lines of text. If the line gap is small then this won't be noticeable (and will dimish further with more lines). However, the opposite seems to happen with WinForms (more lines means shorter boxes by number of lines), so clearly I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is just part and parcel of not really having a specification for the MeasureText
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think your last point is probably the key issue: It is not really specified what height the MeasureText
method should actually return. I interpreted this as the line height in SkiaRenderContext
, as that happened to coincide fairly well with what CanvasRenderContext
gives, but other interpretations are also valid and possibly make more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given I can't tell the difference, I'd suggest we pretend I never said anything, and instead we open a new issue discussing the possibility of qualifying MeasureText
. We are going to have to write custom implementations of multi-text support for WPF/WinForms anyway to support text-alignment properly, so it would be good to nail down the requirements, and perhaps put the text-alignment code in a common location. That way, the individual render contexts only have to worry about measuring and drawing single lines of text. Nope... they have to measure whole blocks otherwise you can't observe line-gap (if it's an issue), but not render them.
Good to merge in my opinion. |
Fixes #1538.
Checklist
Changes proposed in this pull request:
SkiaRenderContext
using theStringHelper.SplitLines(...)
method@oxyplot/admins