update lineHeight if boundingRect indicates a larger value.
AbandonedPublic

Authored by cullmann on Nov 16 2019, 5:19 PM.

Details

Summary

When a font has fallback, the fontHeight is broken can may cut the string
in the middle in the line. Try to use actual lineheight from document.

BUG: 404907

Test Plan

Manually tested.

Type chinese / arabic / delete and save.

Diff Detail

Repository
R39 KTextEditor
Branch
arcpatch-D25339
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25492
Build 25510: arc lint + arc unit
xuetianweng requested review of this revision.Nov 16 2019, 5:19 PM
xuetianweng created this revision.

Add comment about the actual characters.

Add screenshot to demonstrate the problem.

And what I'd like to point is, for CJK users, it is uncommon for them to select a single font to cover all the characters, because such fonts are really rare. People usually select one latin only font and just let system (fontconfig) select the fallback for them.
So this could be a common problem for them to see the "half character".

The issue with this approach is, that you get "too high" lines for the case that none of this characters ever appear.
Beside that, unfortunately, you get rendering artifacts in addition, as Qt will not paint e.g. selection high enough for lines without such chars.

See e.g. attached screenshot.

xuetianweng added a comment.EditedNov 17 2019, 8:42 PM

Any idea about how konsole derive the value?.. I'm not so sure if they did a better job (probably not because IIRC I have seen similar issue in konsole).

The issue with this approach is, that you get "too high" lines for the case that none of this characters ever appear.
Beside that, unfortunately, you get rendering artifacts in addition, as Qt will not paint e.g. selection high enough for lines without such chars.

See e.g. attached screenshot.

Then what is the ideal solution to you for this case?

Having different font height for every line? Or keep this "slightly higher line" by do vertical center align and fix the artifact?

At least the current state doesn't look right to me and I think it need to be fixed.

Having different font height for every line?

  1. We don't want a bigger lines
  2. We don't want a lines that are bigger that other in a view.
  3. We don't want different font height in a view.

I don't know which is best.

Actually, I could live with:

  1. All lines are a bit higher, for me that makes reading even easier. But the rendering shall have no glitches.
  2. Some lines have different heights. But I assume this is hard to implement at the moment.

Try to fill the gap if we increase the line height with representitive char.

handle the range with multiple lines.

Actually, I could live with:

  1. All lines are a bit higher, for me that makes reading even easier. But the rendering shall have no glitches.
  2. Some lines have different heights. But I assume this is hard to implement at the moment.

After some experiment and reading the Qt code, seems there is no easy way to extend the line height..
E.g. QPlainTextEdit will show the text with different line height, so I believe I couldn't do it with in Qt.

So I tried to use a small trick to fill the gap.
If there is some gap need to be filled, then the code tried to draw some transparent text so the background will be extended to fill the gap. Then draw the real text.

The solution is hacky though.

Fix the handling when layout formats has background.

Add a screenshot to demostrate the change.

+1, looks great.

I appreciate work on this issue.
I am not sure about how well this "hack" will solve the issue, thought.
I will give it a try here in any case.

Hmm, after applying this patch, for me, no text is visible at all.
By selecting a bit stuff, one at least sees an outline (CMakeLists.txt of ktexteditor toplevel dir).

Hmm, after applying this patch, for me, no text is visible at all.
By selecting a bit stuff, one at least sees an outline (CMakeLists.txt of ktexteditor toplevel dir).

let me test with some different colorscheme..

Hmm, after applying this patch, for me, no text is visible at all.
By selecting a bit stuff, one at least sees an outline (CMakeLists.txt of ktexteditor toplevel dir).

Are you sure it is the latest patch? I know some old version in this patch might have this effect.
What's your color & font in kwrite setup BTW?

Clean up the code a little bit and adding comments. Also fix a small bug if m_fontHeight has
big difference with m_fontMetrics's height.

Fix the blank text issue by alway setLineWidth.

For QTextLine, the layout information is not filled until certain function is called.
If dynamic wrap is not enabled, the line width is -1 so at the point we do the calculation,
we will have no size information of the text. Use INT_MAX to set the lineWidth as workaround.

I tried the current version.
For me this looks OK now.
Thought I would like to have more people trying this out before we merge.
Some volunteers?

Beside this: Thanks for working on this issue.

fakefred added a subscriber: fakefred.EditedMay 4 2020, 9:18 AM

So I tested this, great improvements regarding line height can be seen:

With CJK fonts, it still works as expected.

rjvbb requested changes to this revision.May 4 2020, 5:47 PM
rjvbb added a subscriber: rjvbb.

I can't speak for the special cases where this change would improve matters, but for me it introduces a clear regression (waste of vertical space: 12 lines less) in a basic ascii code editing context. Font used is Ubuntu Mono 10pt.

Granted, the current display (left) is a tad crowded but I actually like it like that for code editing. For reading prose it might be too much, but that's hardly the domain of application for a text editing framework.

This revision now requires changes to proceed.May 4 2020, 5:47 PM
pshinjo added a subscriber: pshinjo.May 4 2020, 7:39 PM

And what I'd like to point is, for CJK users, it is uncommon for them to select a single font to cover all the characters, because such fonts are really rare. People usually select one latin only font and just let system (fontconfig) select the fallback for them.

This statement is valid even for non-CJK users who only occasionally see the said characters.

For all screenshots, left hand side is git master+this patch, right hand side is Kate 19.12.3/Frameworks 5.68.0 on Ubuntu 20.04.

Test 1 - Hack + Noto Sans Mono CJK KR 9pt (Hack does not contain CJK glyphs, so for missing glyphs the latter will be used). Notice that only in the left hand side the underline in my e-mail address is visible:

Test 2 - Nanum Square 9pt (contains both Latin-1 and Korean glyphs):

Test 3 - Ubuntu Mono + Noto Sans Mono CJK KR 9pt. For me the right hand side is a regression, since neither Cyrillic nor Korean scripts are clearly visible when both of them are in the same line. Also in this setup most of Korean texts are cut by half even when there are only Korean texts. Compare it with the other lines where only Cyrillic is visible:

I can also confirm about 6-7 lines of loss, but for me the right hand side has issues. Need to find some compromise.

src/render/katerenderer.cpp
1040

I think this causes the regression mentioned in

I can't speak for the special cases where this change would improve matters, but for me it introduces a clear regression (waste of vertical space: 12 lines less) in a basic ascii code editing context. Font used is Ubuntu Mono 10pt.

But this part is the raison d'etre for this patch: provide enough font height for non-latin-1 text. Also not considered is glyphs from other scripts, such as Cyrillic, Arabic, ... What could be another solution?

rjvbb added a comment.May 4 2020, 8:37 PM

This patch is only needed when mixing a main Latin1 (like) alphanumeric font with occasional glyphs from a font that have a different, taller height?

Am I right that any text that uses only a single font will see some form of significant loss of the number of lines that fit within a given vertical space?

Test 3 - Ubuntu Mono + Noto Sans Mono CJK KR 9pt. For me the right hand side is a regression, since neither Cyrillic nor Korean scripts are clearly visible when both of them are in the same line. Also in this setup most of Korean texts are cut by half even when there are only Korean texts. Compare it with the other lines where only Cyrillic is visible:...

But this part is the raison d'etre for this patch: provide enough font height for non-latin-1 text. Also not considered is glyphs from other scripts, such as Cyrillic, Arabic, ... What could be another solution?

If local lineheight adaptation isn't possible with the current implementation (not even by inserting 1 or more virtual linebreaks?) then the behaviour introduced with this patch c/should become optional. Selected automatically if possible (with an off toggle in the settings dialog) but otherwise via a menu action that's not too well hidden so people who need to turn it on and off on any kind of regular basis can do so without having to hunt the command down too much.

This patch is only needed when mixing a main Latin1 (like) alphanumeric font with occasional glyphs from a font that have a different, taller height?

Yes, though the "occasion" will highly depend on the user locale. For someone this will likely never happen, for someone this will happen everyday.

Am I right that any text that uses only a single font will see some form of significant loss of the number of lines that fit within a given vertical space?

Yes, given that the font contains all required glyphs to represent the string "AHIygあ你말" is limited. That's also visible in my test 2. Previous tests were done in artificially high font size (32pt for example) and less dense texts so the original authors might overlooked this problem.

but that's hardly the domain of application for a text editing framework.

It's "KTextEditor", not "KCodeEditor". This rendering issue, though not from an "intended purpose", makes it painful to write CJK LaTeX articles in Kile. Now that KDE declares to be serving a worldwide userbase, the foundation of numerous applications has to see some change somehow.

sars added a subscriber: sars.May 5 2020, 5:29 AM

I'm starting to think that we need an option for enabling/disabling this change/feature. I would not want to have the extra space between the lines, but at the same time I can see that actually not seeing the whole character is not an acceptable situation...

rjvbb added a comment.May 5 2020, 9:23 AM
It's "KTextEditor", not "KCodeEditor".

Yes, but look at the traditional meaning of a text editor, which typically means "plain text" editor. KTextEditor's design decision to use a single lineheight puts it squarely in that domain - to reply in style: It's "TKextEditor", not "KRichTextEditor" (and even less "KWordProcessor") ...

write CJK LaTeX articles in Kile

Like it or not, TeX is code (or a universal language, depending on how you look at it). That's intentional (WYSIWIG vs. WYSIAYG and all that). And frankly, TeX code is the last place where I'd expect this kind of rendering problem: doesn't it give you US-ASCII canonical representations of every possible glyph? (I know that doesn't help manuscript readability, but hey, that's the choice you make :) ) One could also make the argument that maybe Kile should look for another rendering engine if they want to support something more advanced than KTextEditor can offer. There are Qt classes that have "rich text" support which might be more appropriate (if they offer editing support), and on the other end of the scale there's the Calligra project which probably has the required libraries.

Anyway, serving a worldwide userbase isn't a new goal, but also shouldn't IMHO drag down usability to some lowest common denominator IMHO. Hence my firm request to make any chance that does have that effect optional, one way or another - I do agree with @sars .

doesn't it give you US-ASCII canonical representations of every possible glyph?

Whether it is possible or not, no CJK users will write TeX document using US-ASCII representation for all glyphs instead of their native alphabet, especially when the document is in their mother tongue. Welcome to the world of transliteration problems.

I second the as-an-option proposal. Hey, why not automatically increase the line height when CJK characters are detected?

brauch added a subscriber: brauch.May 5 2020, 3:50 PM

I second the as-an-option proposal. Hey, why not automatically increase the line height when CJK characters are detected?

This issue has been around for years and actually yeah, I think that is the only viable solution unless somebody dives in and reworks the renderer to support variable line heights.

I second the as-an-option proposal. Hey, why not automatically increase the line height when CJK characters are detected?

So the thing is we need to maintain a such height depending on the document, not so sure if that is even possible (or whether it could be costly) or you could give me some hint on that?
Actually, I'd rather to calculate the height based on actual character in the document because during my test, mixing Arabic together to CJK and latin could still have problem (likely same for Cyrillic).

... or you could give me some hint on that?

Too bad I can't; it's been no more than a month since I began my study into KDE applications. I'd love to test out if someone provides a genius solution, though.

xuetianweng updated this revision to Diff 82170.May 7 2020, 5:33 AM

Use actual line height instead of representitive character.

I'm not sure if this is the right way to do it or it might cause any glitch, but here we go. Upon line rendering, update the maximum height we've ever seen.

Though line height won't shrink during the edit phase, it will back to the actual value upon save.

xuetianweng retitled this revision from KateRenderer: Use representitive character in CJK to estimate the fontHeight. to update lineHeight if boundingRect indicates a larger value..May 7 2020, 5:37 AM
xuetianweng edited the summary of this revision. (Show Details)
xuetianweng edited the test plan for this revision. (Show Details)
anthonyfieroni added inline comments.May 7 2020, 5:58 AM
src/render/katerenderer.cpp
1188 ↗(On Diff #80606)

Can you use functor here, instead of string.

rjvbb added a comment.May 7 2020, 8:31 AM

With "we've ever seen" you do mean that lineheight only changes when a line that requires it scrolls into view?

Though line height won't shrink during the edit phase, it will back to the actual value upon save.

Have you tried to reset the max. lineheight on each redraw (I presume the scrollbars could give you a signal that a view scroll/jump was initiated, if need be).
Something tells me that it'd be nicer if lineheight always is as small as possible. Imagine you're using a smallish window to edit a document that just has some of these "offending", much higher characters at the bottom. If it gets into view only once, lineheight increases and it's as if you lose a lot of screen estate until you save the document. Now I mustn't be the only one who often doesn't save for a while, esp. when doing things like moving blocks of text around, and it's exactly in that kind of scenario where having to save to get a more space-efficient rendering back can be very annoying.

As long as you can determine the max. lineheight required for the view that's about to be drawn before the view is actually drawn there should be no glitches. You'd just see a jump in lineheight and there would probably be an interesting problem to tackle with edge cases where the higher glyphs fall outside the view area because of the increased lineheight, but that's something your current implementation cannot avoid completely either. As to the changing lineheight: I think users will understand why it happens and get used to it. It's comparable to what you see in graphics programmes that show cursor co-ordinates next to the cursor; that indicator has to jump when it would get "pushed out" of the view, and that doesn't even feel weird.

I presume your new approach would work not just for CJK characters, but also for anything else that changes the lineheight. That's and advantage but could also lead to regressions for some (who never use CJK characters or who, like me, wouldn't care how they display because they can't read them anyway). Emoticons come to mind; here too I don't really care if they get cut off. Scrap that, I *really* don't care if they are...

rjvbb added a comment.May 7 2020, 12:39 PM

This new version does not cause a lineheight regression for me (after backporting it to 5.60.0). However, contrary to what I thought it does not react to emoji

xuetianweng marked an inline comment as done.May 7 2020, 5:48 PM

With "we've ever seen" you do mean that lineheight only changes when a line that requires it scrolls into view?

Though line height won't shrink during the edit phase, it will back to the actual value upon save.

Have you tried to reset the max. lineheight on each redraw (I presume the scrollbars could give you a signal that a view scroll/jump was initiated, if need be).
Something tells me that it'd be nicer if lineheight always is as small as possible. Imagine you're using a smallish window to edit a document that just has some of these "offending", much higher characters at the bottom. If it gets into view only once, lineheight increases and it's as if you lose a lot of screen estate until you save the document. Now I mustn't be the only one who often doesn't save for a while, esp. when doing things like moving blocks of text around, and it's exactly in that kind of scenario where having to save to get a more space-efficient rendering back can be very annoying.

As long as you can determine the max. lineheight required for the view that's about to be drawn before the view is actually drawn there should be no glitches. You'd just see a jump in lineheight and there would probably be an interesting problem to tackle with edge cases where the higher glyphs fall outside the view area because of the increased lineheight, but that's something your current implementation cannot avoid completely either. As to the changing lineheight: I think users will understand why it happens and get used to it. It's comparable to what you see in graphics programmes that show cursor co-ordinates next to the cursor; that indicator has to jump when it would get "pushed out" of the view, and that doesn't even feel weird.

I presume your new approach would work not just for CJK characters, but also for anything else that changes the lineheight. That's and advantage but could also lead to regressions for some (who never use CJK characters or who, like me, wouldn't care how they display because they can't read them anyway). Emoticons come to mind; here too I don't really care if they get cut off. Scrap that, I *really* don't care if they are...

To be honest, I didn't see issue about color emoji (until select them). My "fill" gap code seems can't make color emoji bitmap transparent.... The fill gap code is indeed a hack.

Probably when I get time I might try to make it able to render view with different height...

As for higher line, it might not as bad as you thought as it actually might improve readability for many people.

[...]

As for higher line, it might not as bad as you thought as it actually might improve readability for many people.

I agree with this statement :). Thanks to this diff I found out where the line height can be manipulated; the way the code computed it, fontHeight was 34 (IIRC), I've hardcoded it to 40 and I very much prefer reading text that way.

Note that Konsole tries and compute a sane line height to accommodate CJK characters... etc, but it also has a config option to change the line height (Settings -> Appearance -> Miscellaneous)... the code can be smart, but it can't know how a user prefers to read text, there is no one-size fits all.

rjvbb added a comment.May 7 2020, 6:42 PM

the code can be smart

No, cleverly written at best (don't get me started! ;) )

but it can't know how a user prefers to read text, there is no one-size fits all.

EXACTLY. Words becomes a lot more readable for me when I use 64pt or larger so I read from my recliner ... but given the size of my screens it would texts less readable. IOW, there's a trade-off there and that also applies to lineheight, and esp. with lineheight it also depends on the context. Many forms of code are easier to work with with a minimal lineheight because that makes it easier to see code organisation (blocks/scopes identified by indentation). So yeah, a user-controllable scale factor could be a good idea, independent from the issue at hand.

I didn't mention Konsole because AFAIK it doesn't use KTextEditor.

Looking at the code, might it make more sense to just move away from the fixed height we have?
It isn't used that often and in most cases one could just query the height of the current line.
That would solve this issue without needing any hacks for the rendering I think.

rjvbb added a comment.May 9 2020, 12:06 PM
Looking at the code, might it make more sense to just move away from the fixed height we have?
It isn't used that often and in most cases one could just query the height of the current line.

I'd be all for that, provided it doesn't introduce any regressions in the rendered result nor in rendering time.

It could be trickier to implement than you think though?

Looking at the code, might it make more sense to just move away from the fixed height we have?
It isn't used that often and in most cases one could just query the height of the current line.

I'd be all for that, provided it doesn't introduce any regressions in the rendered result nor in rendering time.

It could be trickier to implement than you think though?

It is a non-trivial change.
But I think any other change, like this, will lead to issues, too, and is just a "hack".

I have taken a closer look.
I assume the lineHeight usages in the renderer are easy to replace with the proper "height()" of the individual lines of the layout.

Harder to replace is the use of the lineHeight outside of the renderer.

see e.g. here for a start of using the right heights inside the renderer.

for a dynamically wrapped line, the lines before the last one get now the right height for me.

But there are a lot more places to handle.

rjvbb added a comment.May 9 2020, 2:56 PM
I assume the lineHeight usages in the renderer are easy to replace with the proper "height()" of the individual lines of the layout.

This is what I think could be tricky. One wouldn't want fluctuating line heights when only a single font is used (at least not when using a fixed-width/"mono" font). That concern is of course moot is lineheight is calculated from a font property that doesn't depend on the actual text being rendered.

Looking at the code, might it make more sense to just move away from the fixed height we have?

Yes please. Instead of hacks and options, which will make the code and the UI harder to use, the correct fix is to switch to variable line heights.

Of course you need to touch many places, but this approach is so much better than hacks. Please give it a try! :-)

Actually I was trying to use this approach in the patch because I was afraid that variable line height may need to estimate the whole document height to make scroll work correctly.

But during this patching experience I realized that the ktexteditor first line is always a whole line. So the renderer only need to care about the current page instead of whole document, which makes variable line height might not as hard as thought.

I won't have time recently to look into the variable line height though.

I think we went with the solution in https://phabricator.kde.org/D29789, could we close this?

cullmann commandeered this revision.Oct 10 2020, 7:13 PM
cullmann edited reviewers, added: xuetianweng; removed: cullmann.

I think the solution we have in current master is ok enough.

cullmann abandoned this revision.Oct 10 2020, 7:13 PM