Bug 372629 - [OS X] line height too low issue(s)
Summary: [OS X] line height too low issue(s)
Status: RESOLVED DUPLICATE of bug 379727
Alias: None
Product: frameworks-ktexteditor
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: 5.27.0
Platform: Compiled Sources macOS
: NOR normal
Target Milestone: ---
Assignee: KWrite Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-18 15:29 UTC by RJVB
Modified: 2018-01-27 15:42 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
glyph clipping due to too low lineheight; QtCurve (57.04 KB, image/png)
2016-11-18 15:29 UTC, RJVB
Details
glyph clipping in v5.2.7; Macintosh native style (56.06 KB, image/png)
2016-11-18 15:30 UTC, RJVB
Details
no clipping with v5.24.0 (12.29 KB, image/png)
2016-11-18 15:31 UTC, RJVB
Details
touching lines with 5.27.0 (29.25 KB, image/png)
2016-11-18 16:10 UTC, RJVB
Details
better line spacing with 5.24.0 (25.77 KB, image/png)
2016-11-18 16:10 UTC, RJVB
Details

Note You need to log in before you can comment on or make changes to this bug.
Description RJVB 2016-11-18 15:29:51 UTC
Created attachment 102303 [details]
glyph clipping due to too low lineheight; QtCurve

I have been noticing cases where the lineheight was too low in Kate-based editors on OS X, and have just traced this to a regression in KTextEditor between 5.24.0 and 5.27.0 .

It strikes reproducibly on the last line (when not terminated with a newline); see the screenshots. It happens only with the native CoreText font engine, not when using the FreeType engine (-platform cocoa:fontengine=freetype); the application style in use is of no importance.

Reverting to KTextEditor 5.24.0 solves the issue.
Comment 1 RJVB 2016-11-18 15:30:41 UTC
Created attachment 102304 [details]
glyph clipping in v5.2.7; Macintosh native style
Comment 2 RJVB 2016-11-18 15:31:10 UTC
Created attachment 102305 [details]
no clipping with v5.24.0
Comment 3 Christoph Cullmann 2016-11-18 15:40:25 UTC
We altered how lines are painted to actually avoid things like that (perhaps between that versions).
Comment 4 RJVB 2016-11-18 16:05:28 UTC
Hah, well, that misfired then, at least on OS X. I didn't check if 5.24.0 does worse with the FreeType font engine.

This commit? 4c83bac52716a08a549dca50a76be8aa47c11ca3
Comment 5 RJVB 2016-11-18 16:10:14 UTC
Created attachment 102307 [details]
touching lines with 5.27.0
Comment 6 RJVB 2016-11-18 16:10:33 UTC
Created attachment 102308 [details]
better line spacing with 5.24.0
Comment 7 RJVB 2016-11-18 17:00:51 UTC
(In reply to RJVB from comment #4)

> This commit? 4c83bac52716a08a549dca50a76be8aa47c11ca3

Clearly not only that commit. Reverting just that makes line spacing too generous (total text height increases by approx. 16%).
Comment 8 RJVB 2016-11-18 17:09:17 UTC
(In reply to RJVB from comment #4)

> This commit? 4c83bac52716a08a549dca50a76be8aa47c11ca3

Clearly not only that commit. Reverting just that makes line spacing too generous (total text height increases by approx. 16%).
Comment 9 RJVB 2016-11-18 17:45:39 UTC
FWIW, this seems to work acceptably for me:

```C++
void KateRenderer::updateFontHeight()
{
    // use height of font + round down, ensure we have at least one pixel
    // we round down to avoid artifacts: line height too large vs. qt background rendering of text attributes
    const qreal height = config()->fontMetrics().height();
#ifdef Q_OS_OSX
    m_fontHeight = qMax(1, qFloor(1.005f * height));
#else
    m_fontHeight = qMax(1, qFloor(height));
#endif
}

```
Comment 10 Christoph Cullmann 2016-11-19 16:30:47 UTC
Hmm, any idea why we need that adjust? Seems strange, given no problem on other operating systems.
Comment 11 RJVB 2016-11-19 19:16:47 UTC
What other OSes have you tried?

I guess it must have something to do with the font engine, an observation that I think is supported by the fact that the adjustment isn't required when I use the Freetype font engine on OS X.

Sadly the Freetype font engine isn't really ready for prime time on OS X; it doesn't appear to use FontConfig for one, and it requires the use of a commandline argument without any way the make it the default as far as I have seen.

To what extent do you intervene in the standard Qt text rendering mechanisms, iow to what extent could this be considered a Qt bug?
Comment 12 Dominik Haumann 2016-11-19 21:39:14 UTC
What about this in ::updateFontHeight?

-    m_fontHeight = qMax(1, qFloor(height));
+    m_fontHeight = qMax(1, qCeil(height));

Is this better?
Comment 13 RJVB 2016-11-20 15:38:40 UTC
Yes, that seems to have about the same effect as my tweak. `qCeil(1.005f * height)` is too much though (I tried that too, tuning down the extra space that was obtained in the original code).
Comment 14 Christoph Cullmann 2016-11-20 16:43:06 UTC
I would be fine with changing that to ceil, floor is a bad idea, no clue why we/I did not use ceil :/
Comment 15 RJVB 2016-11-20 17:28:13 UTC
Did you compare the spacing with qCeil and qFloor on Linux and/or MS Windows? I find the line spacing just fine on Linux, and would regret it getting significantly more ample *). Maybe it can be tied to the hidpi setting, but when your screen is only 768 pixels high like mine I tend prefer a somewhat more compact layout than I might otherwise prefer. It just got too compact on OS X.

*) using ceil instead of floor makes lines approx. 10% higher for me, iow 10 lines instead of 11 lines in the same vertical space with Ubuntu Mono 12pt.
Comment 16 Dominik Haumann 2016-11-20 17:50:00 UTC
> I would be fine with changing that to ceil, floor
> is a bad idea, no clue why we/I did not use ceil :/

Interestingly, I wondered why you changed if from ceil to floor, but I trusted you enough that this was intentional :-)

But then it's easy: back to qCeil() and that's it.

Btw: The #ifdef is the worst solution of all, so this is a no-go in my opinion.
Comment 17 RJVB 2016-11-20 18:19:56 UTC
I'd say that the most important aspect here is that behaviour is as much the same as possible across platforms. In this case that would mean that they all have a sufficient line spacing, and that a same font size will give a number of lines per height unit that's as similar as possible.

I was surprised to see the difference qCeil can make vs. qFloor in that latter aspect, so I hope you test this properly before making a decision. #ifdefs don't look very nice, but nice looking code isn't the goal here.
Comment 18 Christoph Cullmann 2016-11-21 15:29:45 UTC
I think the issue with ceil was that you get space between lines that is not painted correctly. Perhaps we should just start to use float's for the height...
Comment 19 Dominik Haumann 2017-02-18 12:23:07 UTC
So we have not made any decisiton. Rene, any news?
Comment 20 RJVB 2017-02-18 13:13:22 UTC
What kind of news are you hoping for?

I'm currently using the patch below with 5.29.0 on Mac and that works well enough. I should maybe try the patch on Linux too (not sure why I never did) because I do see glyph clipping for instance in KDevelop's "Find in Files" toolview.
Comment 21 Christoph Cullmann 2017-12-20 21:32:00 UTC
The problem is we end up with ugly spacing between the lines if we ceil or scale, see bug 379727, we have them already in some cases :/

I guess we need to redo that rendering stuff :///

*** This bug has been marked as a duplicate of bug 379727 ***
Comment 22 RJVB 2017-12-25 09:37:26 UTC
I still use 

m_fontHeight = qMax(1, qCeil(height));

on Mac, stock code on Linux, and that's perfectly satisfactory to me. Maybe you should accept that there are subtle platform differences in font rendering that interact with how KTextEditor handles line-spacing and allow platform specific code here.

FWIW, one would expect that difference to be due to using FreeType vs. CoreText (i.e. in QPA code) but strangely that does not appear to be the case. I build the XCB QPA plugin on Mac and don't notice any real differences running apps side-by-side in X11 (XQuartz) and Cocoa mode.

I also cannot reproduce the transversal line glitch of #379727 under XQuartz, at least not as easily as described.
Comment 23 Christoph Cullmann 2018-01-27 15:42:36 UTC
Could you try if https://phabricator.kde.org/D10145 improves that?
It includes the qCeil you wanted + includes the font leading.