D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Anthony Fieroni]

anthonyfieroni added a comment.


  In D9537#300367 , @boemann wrote:
  
  > 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

REPOSITORY
  R8 Calligra

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

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Camilla Boemann]

boemann added a comment.


  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

REPOSITORY
  R8 Calligra

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

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Anthony Fieroni]

anthonyfieroni added a comment.


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

REPOSITORY
  R8 Calligra

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

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Dag Andersen]

danders added a comment.


  Yeah :)
  Seems a unit test needs adjustments, could you have a look?
  
https://build.kde.org/view/Calligra/job/Calligra%20calligra%20kf5-qt5%20FreeBSDQt5.10/lastCompletedBuild/testReport/(root)/TestSuite/libs_kotextlayout_TestBlockLayout/

REPOSITORY
  R8 Calligra

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

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Anthony Fieroni]

anthonyfieroni closed this revision.
anthonyfieroni added a comment.


  
https://cgit.kde.org/calligra.git/commit/?id=805df22766026fac22154ae89b64dad532a6cbb4

REPOSITORY
  R8 Calligra

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

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Camilla Boemann]

boemann accepted this revision.
boemann added a comment.


  Thanks

REPOSITORY
  R8 Calligra

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

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Anthony Fieroni]

anthonyfieroni updated this revision to Diff 38209.

REPOSITORY
  R8 Calligra

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9537?vs=38205=38209

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

AFFECTED FILES
  libs/textlayout/KoTextLayoutArea.cpp
  plugins/textshape/dialogs/ParagraphIndentSpacing.cpp

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Camilla Boemann]

boemann added a comment.


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

REPOSITORY
  R8 Calligra

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

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Anthony Fieroni]

anthonyfieroni updated this revision to Diff 38205.

REPOSITORY
  R8 Calligra

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9537?vs=35423=38205

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

AFFECTED FILES
  libs/textlayout/KoTextLayoutArea.cpp
  plugins/textshape/dialogs/ParagraphIndentSpacing.cpp

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Camilla Boemann]

boemann added a comment.


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

REPOSITORY
  R8 Calligra

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

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Anthony Fieroni]

anthonyfieroni added a comment.


  This -> 
https://phabricator.kde.org/source/calligra/browse/master/plugins/textshape/dialogs/ParagraphIndentSpacing.cpp$124-132
 ?

REPOSITORY
  R8 Calligra

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

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Camilla Boemann]

boemann added a comment.


  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

REPOSITORY
  R8 Calligra

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

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Anthony Fieroni]

anthonyfieroni added a comment.


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

REPOSITORY
  R8 Calligra

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

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Anthony Fieroni]

anthonyfieroni added a comment.


  I don't understand what you mean.

REPOSITORY
  R8 Calligra

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

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Anthony Fieroni]

anthonyfieroni added a comment.


  In D9537#270932 , @boemann wrote:
  
  > And finally the code you already wrote can be simplyfied by by not having 
an else but always multiplying
  
  
  You mean to not percentage and multiply always 1.16?

REPOSITORY
  R8 Calligra

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

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Camilla Boemann]

boemann added a comment.


  no just:
  
  if (percent != 0)
  
height *= percent/100
  
  height *= 1.16;

REPOSITORY
  R8 Calligra

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

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Camilla Boemann]

boemann added a comment.


  So now we are just missing the texttool ui changes

REPOSITORY
  R8 Calligra

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

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Camilla Boemann]

boemann added a comment.


  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

REPOSITORY
  R8 Calligra

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

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Anthony Fieroni]

anthonyfieroni added a comment.


  Test documents -> 
https://europass.cedefop.europa.eu/documents/curriculum-vitae/templates-instructions

REPOSITORY
  R8 Calligra

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

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Camilla Boemann]

boemann added a comment.


  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 ?

REPOSITORY
  R8 Calligra

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

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Anthony Fieroni]

anthonyfieroni added a comment.


  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.

REPOSITORY
  R8 Calligra

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

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever

Re: D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Dag]

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

D9537: [kotextlayoutarea] Make percentage line height relative to the default height

[Anthony Fieroni]

anthonyfieroni added a comment.
Restricted Application added a subscriber: Calligra-Devel-list.


  Ping?

REPOSITORY
  R8 Calligra

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

To: anthonyfieroni, danders, mecir, boemann
Cc: Calligra-Devel-list, cochise, vandenoever