D25339: update lineHeight if boundingRect indicates a larger value.

[Christoph Cullmann]

cullmann commandeered this revision.
cullmann edited reviewers, added: xuetianweng; removed: cullmann.
cullmann added a comment.


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

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D25339

To: cullmann, #ktexteditor, dhaumann, #frameworks, rjvbb, xuetianweng
Cc: ahmadsamir, brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D25339: update lineHeight if boundingRect indicates a larger value.

[Christoph Cullmann]

cullmann abandoned this revision.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D25339

To: cullmann, #ktexteditor, dhaumann, #frameworks, rjvbb, xuetianweng
Cc: ahmadsamir, brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D25339: update lineHeight if boundingRect indicates a larger value.

[Christoph Cullmann]

cullmann added a comment.


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

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D25339

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: ahmadsamir, brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D25339: update lineHeight if boundingRect indicates a larger value.

[Xuetian Weng]

xuetianweng added a comment.


  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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D25339

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: ahmadsamir, brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D25339: update lineHeight if boundingRect indicates a larger value.

[Dominik Haumann]

dhaumann added a comment.


  In D25339#666832 , @cullmann wrote:
  
  > 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! :-)

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D25339

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: ahmadsamir, brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D25339: update lineHeight if boundingRect indicates a larger value.

[René J . V . Bertin]

rjvbb added a comment.


  >   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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D25339

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: ahmadsamir, brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D25339: update lineHeight if boundingRect indicates a larger value.

[Christoph Cullmann]

cullmann added a comment.


  F8304597: lineheight.patch 
  
  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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D25339

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: ahmadsamir, brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D25339: update lineHeight if boundingRect indicates a larger value.

[Christoph Cullmann]

cullmann added a comment.


  In D25339#666877 , @rjvbb wrote:
  
  > >   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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D25339

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: ahmadsamir, brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D25339: update lineHeight if boundingRect indicates a larger value.

[René J . V . Bertin]

rjvbb added a comment.


  >   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?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D25339

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: ahmadsamir, brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D25339: update lineHeight if boundingRect indicates a larger value.

[Christoph Cullmann]

cullmann added a comment.


  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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D25339

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: ahmadsamir, brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D25339: update lineHeight if boundingRect indicates a larger value.

[René J . V . Bertin]

rjvbb added a comment.


  > 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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D25339

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: ahmadsamir, brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D25339: update lineHeight if boundingRect indicates a larger value.

[Ahmad Samir]

ahmadsamir added a comment.


  In D25339#665827 , @xuetianweng 
wrote:
  
  >
  
  
  [...]
  
  > 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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D25339

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: ahmadsamir, brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D25339: update lineHeight if boundingRect indicates a larger value.

[Xuetian Weng]

xuetianweng marked an inline comment as done.
xuetianweng added a comment.


  In D25339#665432 , @rjvbb wrote:
  
  > 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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D25339

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D25339: update lineHeight if boundingRect indicates a larger value.

[René J . V . Bertin]

rjvbb added a comment.


  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
  
  F8293110: emoji.txt 

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D25339

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D25339: update lineHeight if boundingRect indicates a larger value.

[René J . V . Bertin]

rjvbb added a comment.


  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...

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D25339

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D25339: update lineHeight if boundingRect indicates a larger value.

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> katerenderer.cpp:1192
> +// trigger view update, if any!
> +QMetaObject::invokeMethod(m_view, "updateRendererConfig", 
> Qt::QueuedConnection);
> +}

Can you use functor here, instead of string.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D25339

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D25339: update lineHeight if boundingRect indicates a larger value.

[Xuetian Weng]

xuetianweng retitled this revision from "KateRenderer: Use representitive 
character in CJK to estimate the fontHeight." to "update lineHeight if 
boundingRect indicates a larger value.".
xuetianweng edited the summary of this revision.
xuetianweng edited the test plan for this revision.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D25339

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann