D25339: update lineHeight if boundingRect indicates a larger value.

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,

D25339: update lineHeight if boundingRect indicates a larger value.

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,

D25339: update lineHeight if boundingRect indicates a larger value.

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,

D25339: update lineHeight if boundingRect indicates a larger value.

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

D25339: update lineHeight if boundingRect indicates a larger value.

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

D25339: update lineHeight if boundingRect indicates a larger value.

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

D25339: update lineHeight if boundingRect indicates a larger value.

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

D25339: update lineHeight if boundingRect indicates a larger value.

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

D25339: update lineHeight if boundingRect indicates a larger value.

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

D25339: update lineHeight if boundingRect indicates a larger value.

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

D25339: update lineHeight if boundingRect indicates a larger value.

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

D25339: update lineHeight if boundingRect indicates a larger value.

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

D25339: update lineHeight if boundingRect indicates a larger value.

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

D25339: update lineHeight if boundingRect indicates a larger value.

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

D25339: update lineHeight if boundingRect indicates a larger value.

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

D25339: update lineHeight if boundingRect indicates a larger value.

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

D25339: update lineHeight if boundingRect indicates a larger value.

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.