D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

rjvbb added a comment. On, with the partially reverted patch: CoreText font engine: F6650185: Screen Shot 2019-03-02 at 22.51.09.png FontConfig font engine (= identical to the one on Linux; FreeType & FontConfig have the Infinality patches):

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

rjvbb added a comment. > But now the state is just like it was before: If you have a font that does fallbacks for some glyphs It's the fontengine that handles this of course ;) Note that on Unix the fallback mechanism is actually provided by/through fontconfig, so there might be

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

cullmann added a comment. If somebody provides a patch that allows artifact free line height modification, I will happily merge it. The same for any improvement to the line height computation. It is only fixed for all lines to make it easy (and fast). If somebody fixes that all, I am more

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

rjvbb added a comment. > I would rather keep the current behavior and let people use sane fonts if the want unicode. That's not current acceptable IMHO, not with the cumbersome way of selecting the font. If proper document display depends on "user picking a sane font", they

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

cullmann closed this revision. cullmann added a comment. Actually, if you use a "proper" font, it all works fine with this patch, too. Here screenshots using DejaVu with current KTextEditor master. F6649178: arab.png F6649180: smileys.png

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

thomassc added a comment. I guess the previous situation is still more practical, since by temporarily adding a newline there below the oversized line, it is at least possible to edit that line. With the line cut off, it is impossible to edit. Also, in cases where a line is only slightly

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

cullmann reopened this revision. This revision is now accepted and ready to land. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D19283 To: cullmann, dhaumann Cc: rjvbb, loh.tar, thomassc, kwrite-devel, kde-frameworks-devel, domson, michaelh, ngraham, bruns,

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

cullmann added a comment. Here you have a pre and post picture, both suck, either way, you can't read a thing. F6649112: pre_change.png F6649114: post_change.png REPOSITORY R39 KTextEditor REVISION DETAIL

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

cullmann added a comment. I have no idea how they do that, but we paint line by line ourself, if we just change the height, you get interleaved "something" between the lines. And I don't see a benefit in supporting that at all, how should the user set it to the "right value" if it can

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

cullmann added a comment. We can not arbitrary scale the height, that must match what Qt does, else you get maximal ugly empty trash in-between the lines, e.g. if you partially select stuff or have different backgrounds for some glyphs. REPOSITORY R39 KTextEditor REVISION DETAIL

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

rjvbb added a comment. > Just load the XML file from bug 404713. > Before this changes here, you did overpaint the next line randomly with the "oversized one", now you "cut" the oversized line. I don't know where to look specifically in that huge file. I do notice that with the

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

loh.tar added a comment. Sorry when I'm wrong, but it sounds to me that this bug is somehow relatated or maybe a solution, there is patch offered. Bug 328837 - Add configurable line height to katepart REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D19283 To:

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

cullmann added a comment. Just load the XML file from bug 404713. Before this changes here, you did overpaint the next line randomly with the "oversized one", now you "cut" the oversized line. In both cases, one line is garbage. REPOSITORY R39 KTextEditor REVISION DETAIL

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

rjvbb added a comment. So it seems that the partial revert works; you lost me re: what would break again by doing that? F6646645: Screen Shot 2019-03-01 at 13.07.52.png (background: full revert; foreground partial revert (only

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

thomassc added a comment. > If you keep the overpainting, you will just overpaint the next line partially with it, that won't help that much, IMHO. > > Take a look at bug 404713. I totally agree, but this is a different issue that is out of scope here as far as I see. Bugs 403868

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

rjvbb added a comment. > We just can't use one fixed height for such texts. You can, but it'd have to be the maximum of all lineheights (of the entire document or of the visible section). REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D19283 To: cullmann,

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

cullmann added a comment. If you keep the overpainting, you will just overpaint the next line partially with it, that won't help that much, IMHO. Take a look at bug 404713. We just can't use one fixed height for such texts. REPOSITORY R39 KTextEditor REVISION DETAIL

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

rjvbb added a comment. > That is a good observation: @cullmann Could you give this partial revert a try? I'll try to do that too, tomorrow. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D19283 To: cullmann, dhaumann Cc: rjvbb, loh.tar, thomassc,

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

thomassc added a comment. > The summary of this diff reads a bit as if it doesn't noticeably improve behaviour The line height change (a part of this commit) does noticeably improve behavior for the font that is used on my system. REPOSITORY R39 KTextEditor REVISION DETAIL

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

rjvbb added a comment. > Next Frameworks Tag is on Saturday, 2nd, i.e. in 2 days. Revert, or give it a try? TBH I didn't notice any issues with the before code and am pretty certain I couldn't do better. FWIW I noticed that Scribus (1.5.5) filters out pure emoji fonts, from what I

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

dhaumann added a comment. That is a good observation: @cullmann Could you give this partial revert a try? REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D19283 To: cullmann, dhaumann Cc: rjvbb, loh.tar, thomassc, kwrite-devel, kde-frameworks-devel, domson,

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

thomassc added a comment. > We trade one bug for another. Which one is worse? Without having had a detailed look, it seems to me like the problem reported by @rjvbb might be caused by the removal of the workaround with the second drawing passes in this commit (the changes in

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

dhaumann added a comment. We trade one bug for another. Which one is worse? Next Frameworks Tag is on Saturday, 2nd, i.e. in 2 days. Revert, or give it a try? PS: This is never going to be fixed, since some lines (in this case with emojis, but could also be bidi text or arabic

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

rjvbb added a comment. Causes https://bugs.kde.org/show_bug.cgi?id=404907 REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D19283 To: cullmann, dhaumann Cc: rjvbb, loh.tar, thomassc, kwrite-devel, kde-frameworks-devel, domson, michaelh, ngraham, bruns, demsking,

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

cullmann added a comment. Hmpf :/ I improved my commit message and amended, but arc at it :/ I will BUG: and CCBUG: the bugs manually :/ REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D19283 To: cullmann, dhaumann Cc: loh.tar, thomassc, kwrite-devel,

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

This revision was automatically updated to reflect the committed changes. Closed by commit R39:ce4194b5bc2e: try to improve painting height for text lines - bug 403868 avoid to cut _ and… (authored by cullmann). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D19283?vs=52466=52544#toc

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

dhaumann accepted this revision. dhaumann added a comment. This revision is now accepted and ready to land. Please add the comment as suggested by Thomas. REPOSITORY R39 KTextEditor BRANCH master REVISION DETAIL https://phabricator.kde.org/D19283 To: cullmann, dhaumann Cc: loh.tar,

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

loh.tar added a comment. Personally I would avoid doxygen style or multiline comments in code and prefer // INLINE COMMENTS > katerenderer.cpp:988 > // ensure minimal height of one pixel to not fall in the div by 0 trap > somewhere > // use font line spacing, this includes the

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

thomassc added a comment. I think that this change will fix bug 403470 as well. Suggestion: in updateFontHeight() in katerenderer.cpp, add a comment saying that the line height needs to match that which is used by Qt, since Qt may handle some parts of the drawing, e.g. selection

D19283: try to improve painting height for text lines - bug 403868 avoid to cut _ and other parts still broken: double height things like mixed english/arab, see bug 404713

cullmann created this revision. cullmann added a reviewer: dhaumann. Herald added projects: Kate, Frameworks. Herald added subscribers: kde-frameworks-devel, kwrite-devel. cullmann requested review of this revision. REVISION SUMMARY using a better height and removing the overpainting stuff