toptensoftware / RichTextKit

Rich text rendering for SkiaSharp
Other
367 stars 73 forks source link

Trailing \n is ignored in measurements #52

Closed Jeremy-Price closed 2 years ago

Jeremy-Price commented 2 years ago

When a string ending with \n is added to a TextBlock, measurements and hit testing do not take the newline into account unless there are other characters after it. For example,

tb = new TextBlock();
tb.AddText("Abc\n", new Style());

has tb.LineCount == 1, and it has a MeasuredHeight of a single line.

Where,

tb.AddText("Abc\nxyz", new Style());

or

tb.AddText("Abc\n\n", new Style());

have, as expected, tb.LineCount == 2 and a MeasuredHeight of two lines.

This makes even simple text editing difficult, as it's reasonable for a user to append a single \n at the end of the last line, and it's difficult for developers to special-case correctly to handle this to, for example, put the cursor on the new empty line or correctly size a container to the TextBlock.

toptensoftware commented 2 years ago

Thanks for reporting this. I seem to remember when I originally wrote this that I was thinking that paragraphs have an implicit trailing \n and so if there was an explicit one to just ignore it.

Thinking about it more, that's probably not true and I should fix it, but I'm a bit busy atm. If you'd like to send a pull request I'll consider merging it, otherwise could be a least a couple of weeks till I get to it.

Jeremy-Price commented 2 years ago

Thanks for the quick response! I'm not really a text expert so it would probably be better for you to fix it.

toptensoftware commented 2 years ago

I've had a closer look at this and I'm in two minds.

The problem here is that after the trailing \n there's actually no text in the line to measure.

So technically it's doing the right thing, but perhaps logically it should assume a line with the same height as the font of the \n.

Not sure but I'll think I'll leave it for now unless there's a compelling reason to change it.

toptensoftware commented 2 years ago

Fixed by this:

https://github.com/toptensoftware/RichTextKit/commit/05480108b06f0f6712f80a79c17e7747bea9e9b3

Gillibald commented 2 years ago

I've had a closer look at this and I'm in two minds.

The problem here is that after the trailing \n there's actually no text in the line to measure.

So technically it's doing the right thing, but perhaps logically it should assume a line with the same height as the font of the \n.

Not sure but I'll think I'll leave it for now unless there's a compelling reason to change it.

For editing purposes it makes sense to measure the line with the font metrics of the new line character otherwise this isn't hit testable. For static text this can be ignored.

toptensoftware commented 2 years ago

For editing purposes it makes sense to measure the line with the font metrics of the new line character otherwise this isn't hit testable.

In RichTextKit's editor there is always an implicit end of paragraph character at the end of each TextBlock, which provides the required measurement for hit testing.