[kotextlayoutarea] Make percentage line height relative to the default height
ClosedPublic

Authored by anthonyfieroni on Dec 28 2017, 7:32 PM.

Details

Summary

When it used percentage line height it *cuts* lower part of the text even at 100%, if percentage is lower it looks even worse. I'm not sure it's proper fix.

Test Plan

{F5577014}before
{F5577017}after
Test it with other documents/templates, please verify or suggest other fix.

Diff Detail

Repository
R8 Calligra
Lint
Lint Skipped
Unit
Unit Tests Skipped
anthonyfieroni created this revision.Dec 28 2017, 7:32 PM
Restricted Application added a project: Calligra: 3.0. · View Herald TranscriptDec 28 2017, 7:32 PM
anthonyfieroni requested review of this revision.Dec 28 2017, 7:32 PM

It's not a proper fix no - the height is correct - it's when drawing we have a problem.
The lines below will be drawn on top, and due to some other problem drawing lines will always clear the area first thus cutting away some of the text

Not sure if we can do anything about it

The lines below will be drawn on top, and due to some other problem drawing lines will always clear the area first thus cutting away some of the text

Not sure if we can do anything about it

I vote to improve situation, it shouldn't have a reading/writing penalty/glitches in word processing program, no? I can try to correct drawing if it not fit the box, it will be more acceptable to you?

Yes you are more than welcome to try and fix the drawing - however the drawing is done like it is due to an issue with hoe qt renders compare to how it's supposed to look., so if we can fix both the original problem and the new without breaking anything else i'm all for fixing it

see KoTextLayoutArea_paint.cpp line 250

I investigate in drawing but i can't see how FormatRange is responsible for that. I think the problem is minimum height of the cell, if it changed in paintings it will allow to resize down, if we increase it, it will cannot.

can you please supply a test document then - is there a bug number?

Test document It's not present bug number.

Did you have a time to investigate? If you have suggest how to fix, i open document with Qt4 version it looks good.

No my linux installation has malfunctioned - could you please add some screenshots so i can see what the problem is more specifically?

Hmm the line is inside a table, maybe that is what goes wrong - unfortunately the code is not easy to understand, but at least you can try and see why the value is different

It looks indeed like the descend of the line is not used when figuring out the row height - try doubling the font size and see if the cut is still everything below the baseline?

Bigger font will result with same *cutting* and this is only on PercentLineHeight.

Ok please make the following test:

create 10 lines of text in LO
see where the text ends
change the lines to 101% and see it becomes just a little bit higher and not lower

This will tell us if LO works like your proposal - and if so then I will accept you patch :)

anthonyfieroni added a comment.EditedJan 15 2018, 8:22 AM

In Table -> Size -> Row Height... it has 'Fit to size' if it's checked you cannot cut text or make cell higher, but if it's not you can make it whatever you want that cutting or exceeded lines.

// Edit
It's not a percentage but a cm value to edit ?!

Any further steps?

I have npt had time to try it out myself - and since you didn't make the test i asked for i have nothing new to tell

I try what you want (screenshot)
https://phabricator.kde.org/file/data/lpjuzqc2vi4a74potaml/PHID-FILE-53wfjewtgqjv4wm7tvgq/Screenshot_20180115_102720.png
As you can see if fit to size isn't checked text is *cutted* (selected one) but you can see it does not have percentage but cm.

Restricted Application added a subscriber: Calligra-Devel-list. · View Herald TranscriptMay 27 2018, 5:31 AM
danders accepted this revision.May 28 2018, 6:00 PM

I won't pretend I know the code well, but it looks good to me...

This revision is now accepted and ready to land.May 28 2018, 6:00 PM

This is not correct

I refer to https://www.w3.org/TR/xsl11/#line-height

which states that percentage is releative to fontheight - if not percentage is give a default of 120 percent is used

but you should not multiply 1.2 to the percentage

I say again that the fix should be found in the table code or rendering

Camilla Boemann skrev den 2018-05-28 20:20:

boemann added a comment.
View Revision [1]

This is not correct

I refer to https://www.w3.org/TR/xsl11/#line-height

which states that percentage is releative to fontheight - if not
percentage is give a default of 120 percent is used

but you should not multiply 1.2 to the percentage

Hmmm, it says:
"The computed value of the property is this percentage multiplied by the
element's computed font size"
My question: what is "element's computed font size"?
Could it be what is computed for "normal"?

A thought experiment:
If percent is 100, should we not get the same height as in "normal"?

I say again that the fix should be found in the table code or
rendering

You are probably right, but the the text isn't crystal clear to me...

anthonyfieroni added a comment.EditedMay 29 2018, 7:58 AM

I investigate couple of days, last year, in rendering and i think problem isn't there. So probably table, but i don't get suspicious code, so text layout can be.
Edit: And the problem persist only in percentage layout.

I have my linux partiotion working again, so I'l take a look this weekend - is there a bug number and a test document somewhere ?

Okay I basically believe you are right, but before I accept I ask you to make the corresponding changes in
textshape/dialogs/ParagraphIndentSpacing.cpp

Where the 120% should be 100% etc

Also having tested with Libreoffice I think the correct scale is 1.16

And finally the code you already wrote can be simplyfied by by not having an else but always multiplying

Also 5 lines below a similar factor for dropcaps should probably also be 1.16

Sorry it took me this long to look into it

anthonyfieroni added a comment.EditedJun 2 2018, 6:44 PM

And finally the code you already wrote can be simplyfied by by not having an else but always multiplying

You mean to not read percentage and multiply always 1.16?

no just:

if (percent != 0)

height *= percent/100

height *= 1.16;

So now we are just missing the texttool ui changes

I don't understand what you mean.

Ping, let put in this fix, what should be done?

in plugins/textshape/dialogs there is a file that handles line height (can't remember which exactly)

when converting between the 1x, 2x etc and a percentage it right now aplies the 1.2

but this is wrong: it should just convert 1x as 100%, 2x as 200%

The 1.2 (or actually 1..16) is just a behind the scenes detail handled in layout

yes and line 233-235 in the same file - fix that and we are ready to push

except that its 1, 1½, 2 and not 1,2,3 then it looks good

boemann accepted this revision.Jul 22 2018, 4:01 PM

Thanks

Values like 14.4, 28.8, 28.0-12.0, should be correct, how are they results?

given that we no multiply with1.16 the values should be: 12*1.16, 24*1.16, no
Don't know what the last is supposed to be but it should probably have the same treatment

given that we no multiply with1.16 the values should be: 12*1.16, 24*1.16, no

It looks like - no

Actual (blockLayout->lineAt(1).y()): 120.875
Expected (0.0 + 18.0 + 100.0) : 118

and other it does not multiply by 1.16