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
Overhaul Text Arrangement and Trimming #1556
base: develop
Are you sure you want to change the base?
Conversation
0ca628c
to
1df261a
Compare
MaxSize support is probably pretty slow, and will be caught out by strings with combining characters (need to do some research on that), but gives pretty reasonable results. It could be performed more quickly by render contexts with access to glyph information (e.g. SkiaSharp), where cropping the line would be logarithmic (i.e. irrelevant) rather that log-linear. WPF (the bottom-most line is drawn clipped) Skia on WPF (the bottom-most line is skipped completely) |
@Jonarw would you mind taking a gander at the changes in Skia, and let me know if this looks too abhorrent to you to be an acceptable change? Having done the provisional implementation of |
Generally I think the I am a bit sceptical if clipping on character level is really the way to go for implementing I am more concerned that we might be trying to do something very complex that we don't actually really need (see also my comment in #1559) - after all, I haven't missed this feature on SkiaSharp so far. |
I am also not too sure how about I feel about guessing the font metrics (ascent, descent, leading) by doing multiple text measurements. At the current state, the exact algorithm for text height measurement should be an implementation detail of the render context, which we shouldn't rely on. If we effectively move the responsibility for vertical text arrangement out of the render context and into the
|
@Jonarw yeah, it's pretty horrendous, and I think what you describe is actually close to how OxyPlot was designed: To that end, I would suggest keeping the current API as it is (i.e. |
You are right of course, from the perspective of I haven't completely wrapped my mind around this, I need to think about it some more... |
Thanks for the comments on what must look a complete mess at the moment. I'll probably next implement your suggestions about abstracting out the 'basic' measurement stuff (so we don't have to infer metrics), and put the whole thing in its own class. Rough plan:
The additional interface will duly provide the information the managed only |
f6ced01
to
e5c9a80
Compare
e5c9a80
to
5eb86c3
Compare
Recent changes:
Next tasks:
Any feedback (esspecially negative) welcome as always. |
Refitting done, so Also refitted the TextTrimmer so it supports Unicode and adding additional tests. This of course has made it slower... but I don't think that should be a concern: it is rarely used, and would be providing a capability that is presently unavailable almost everywhere. I doubt it is perfect (i.e. there will be strings that it gives nonsense results on), but it should be fairly easy to extend by making the central regexes more complicated. Annoyingly we can't compile the regexes because we started .NET Standard 1.0; however, the architecture means that if someone really cared they can swap out the text trimmer for their own; whether this is a good architecture is another issue. WinForms is still broken: either I'm having difficultly extracting the font metrics (i.e. |
Fixed WinForms. I think that means everything is working as I understand it should. I've not StyleCop'd this, but I'm going to open for review because there is nothing else I'm intending to do at the moment. |
8a10280
to
1927d3d
Compare
Looks very promising! Just for my understanding: This adapts ImageSharp, SkiaSharp, WinForms and Pdf, at the moment, right? So WPF would still have to be adjusted for this (but could also be its own issue and PR). Also we should then add more sophisticated |
1bd167b
to
f6aa96c
Compare
@Jonarw thanks. I'll eagerly await your feedback, and perhaps put my efforts into #1516 It also adapts the built-in SVG renderer, which now depends on an I've not refitted WPF yet because frankly I'm frightened of it, but I can do that work for this PR or it can be done later if @objorke approves. For the purposes of consistent vertical alignment it is probably a good idea, but it may still be that we leave the trimming to it (though as I've discussed elsewhere, trimming by character seems a much better strategy to me) which could complicate matters with multi-line text Regards advanced trimmers: yep, that was pretty much what I had in mind. The performance so far is perfectly good in my opinion, but there is plenty of scope for improvement. I won't claim the current trimmer behaviour is perfect, but it should be fairly easy to adapt if we find difficulties, and the architecture means it should be easy to support users without regular releases. |
Rebase against develop. |
Should idealy review https://www.unicode.org/reports/tr14/tr14-40.html and http://www.unicode.org/reports/tr14/tr14-22.html to try to conform to unicode standards, but I don't think this is essential for this PR given there was no support for anything prior and it will probably be very difficult to write something standard complaint (if anyone knows better, please do speak up), so we should take our time over it. |
Not sure why the test is failing |
af1067f
to
9c825e3
Compare
Failing on the XKCD examples, something to do with font. |
New code is crashing because it insists on measuring the text when rendering, which the old code didn't. Not immediately sure how to deal with this. Should we be throwing if the font isn't available, or should the drawing routines trap the exception and use a default? Certainly need to be able to detect when the font is missing so we can use a consistent default if we go that way. Example stack trace:
|
Text layout always involves measuring so that line breaks can be handled. |
Aye, I think it can't be avoided, but we need to decide how to handle a missing font when rendering: previously it would just be passed through to WPF and it would default to a different font. My preference at the moment would probably be to implement that behaviour explicitly, so we handle falling back to a different font, maybe allowing the user to configure if they want it to throw in this case or not. |
Pushed a change so that |
a51a5fd
to
4892d5b
Compare
Rebased. @Jonarw do you have any opinion on the issue of default fonts? (see recent comments above and most recent commit) |
I think falling back to the default font seems sensible. How do other renderers handle this? As there are no crashes in the examples I assume they also just fall back to the default? |
4892d5b
to
5aea6f4
Compare
Rebased against master: @Jonarw given how slowly things are moving at the moment, I'd suggest we (squash) and merge this, and give ourselves time to find any remaining issues with the understanding that this isn't necessary a completely fixed API yet. If this massive PR gets out of the way, I might get up the courage to start re-writing the Axis abstractions per #1640. |
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 am sorry for my prolonged absence, this came during a quite busy time for me, and then I just forgot about it.
I agree with you and would be happy to have this merged! I just found one property that I think is a probably a remnant of an earlier version of CanvasRenderContext
?
/// </summary> | ||
/// <value><c>true</c> if stream geometry should be used; otherwise, <c>false</c> .</value> | ||
/// <remarks>The XamlWriter does not serialize StreamGeometry, so set this to <c>false</c> if you want to export to XAML. Using stream geometry seems to be slightly faster than using path geometry.</remarks> | ||
public bool UseStreamGeometry { get; set; } |
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 don't think this is used anywhere?
Fixes #1531, #1559, and #1554
Checklist
Changes proposed in this pull request:
ITextMeasurer
interface, and implementations for all RenderContexts that can support itITextTrimmer
interface, andSimpleTextTrimmer
implementation which uses 'character ellipsis' trimming mode as its defaultTextArranger
class, which arranges multiline text as per Unify multi-line text alignment on all the platforms #1554 and What is MeasureText #1551TextArranger
for text arrangement (thereby resolving Improve vertical text alignment of OxyPlot.SvgExporter #1531 and Add support for DrawText maxSize parameter to all platforms #1559 )OxyPlot.ImageSharp
with additional testsUseVerticalAlignmentWorkaround
property fromOxyPlot.Svg
as it is redundant and will only get in the wayBig Issues
ITextTrimmer
is weak: the render context determines the type of trimming that is done, which may not be ideal@oxyplot/admins