Bug Hunting Session
Bug 120525 - percent linespacing <100% bug with fonts with big descent
Summary: percent linespacing <100% bug with fonts with big descent
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Impress (show other bugs)
Version:
(earliest affected)
5.4.5.1 release
Hardware: All All
: medium normal
Assignee: Aldric RENAUDIN
URL:
Whiteboard: target:6.2.0 target:6.1.4
Keywords: bibisected, bisected, regression
Depends on:
Blocks: Font-Rendering
  Show dependency treegraph
 
Reported: 2018-10-11 17:29 UTC by Aldric RENAUDIN
Modified: 2018-11-28 22:13 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments
font with big descent (108.66 KB, application/x-font-ttf)
2018-10-11 17:29 UTC, Aldric RENAUDIN
Details
patch (1.07 KB, patch)
2018-10-11 17:31 UTC, Aldric RENAUDIN
Details
test presentation with EB Garamond 12 embedded (1.07 MB, application/vnd.oasis.opendocument.presentation)
2018-10-11 19:28 UTC, V Stuart Foote
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Aldric RENAUDIN 2018-10-11 17:29:11 UTC
Created attachment 145624 [details]
font with big descent

If font have "big" descent values, setting linespacing percent has no effect until a certain value.
to reproduce :
- use the attached font (descent=646 ascent=963)
- write some text, and try to set linespacing to 90% => no change
- changes can be seen for percents < 80%

This doesn't happen with version <5.4.
git Bisecting show that the regression start with this commit : https://cgit.freedesktop.org/libreoffice/core/commit/?h=libreoffice-6-0&id=81827ab543d27af82dd7b432f860bf1c473508dc

I don't understand all the mechanisms behind the code, but the culprit is clearly in this line : if ( !nAscent || nAscent > nNewAscent )

As I'm not familiar at all with libreoffice code, I have done a "minimal" patch, just to avoid the regression and so the bug #114628 stay fixed by just applying the new code path for the first line and revert back to the old code path for the others.
Comment 1 Aldric RENAUDIN 2018-10-11 17:31:08 UTC
Created attachment 145625 [details]
patch

This is my first contribution here. I hope I've done it the right way
Comment 2 V Stuart Foote 2018-10-11 19:24:57 UTC
Confirmed on Windows 10 Ent 64-bit en-US with
Version: 6.2.0.0.alpha0+ (x64)
Build ID: 5f5d890c242b8a092804991dba809f6f4287cfb2
CPU threads: 8; OS: Windows 10.0; UI render: GL; VCL: win; 
TinderBox: Win-x86_64@42, Branch:master, Time: 2018-10-03_23:04:03
Locale: en-US (en_US); Calc: CL

With the attached document using EB Garamond 12 with lines "Qui Qwake Quail"

Using Sidebar Properties Deck -> Paragraph Spacing, or from context menu Paragraph dialog--values between 100 and 90 percent proportional spacing all scale to 100%. 89% and down will scale, 101% and up will scale.

Thanks for the patch, but more efficient to submit via git/gerrit and link here.
Comment 3 V Stuart Foote 2018-10-11 19:28:58 UTC
Created attachment 145626 [details]
test presentation with EB Garamond 12 embedded
Comment 4 Julien Nabet 2018-10-11 20:00:43 UTC
Aldric: considering all the analysis until even proposing a patch (!), thought you might be interested in this link:
https://wiki.documentfoundation.org/Development/GetInvolved
Comment 5 Xisco Faulí 2018-10-15 08:05:17 UTC
Adding Cc: to Szymon Kłos
Comment 6 Xisco Faulí 2018-10-15 08:06:55 UTC
@Aldric, Could you please submit your patch to gerrit as explained here -> https://wiki.documentfoundation.org/Development/gerrit/SubmitPatch ?
Thanks
Comment 7 Aldric RENAUDIN 2018-10-15 17:46:54 UTC
Ok, thanks for your comments. I'll try to submit the patch via gerrit ASAP
Comment 8 Aldric RENAUDIN 2018-10-15 19:40:42 UTC
Well... after some more tests, it's not so simple and my patch produce regression too :
if you set linespacing percent to low values, the linespacing between line 0 and 1 is lower than the next ones.
This is obviously due to the fact that the linespacing calculation doesn't use the same path.
I'll have to dig a little more inside the code.
btw, I *think* I'm now able to send patch via gerrit :)
Comment 9 Commit Notification 2018-10-25 14:10:09 UTC
AlicVB committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=87239dbf7d57d812cdecbf75f6e86afaa4864abb

tdf#120525 fix linespacing <100% for impress

It will be available in 6.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 10 Xisco Faulí 2018-11-28 13:04:08 UTC
A polite ping to Aldric RENAUDIN:
Is this bug fixed? if so, could you please close it as RESOLVED FIXED ? Otherwise, Could you please explain what's missing?
Thanks
Comment 11 Aldric RENAUDIN 2018-11-28 18:25:40 UTC
Well, this bug is fixed in master (for next 6.2 release) but not for the previous releases.
This should be straightforward as it's just copy-paste, but I have not spend the time to do it (to be honest, I've forgotten)
So I've just to figure how to propose a patch for older releases in gerrit !
Comment 12 Xisco Faulí 2018-11-28 18:28:14 UTC
Done -> https://gerrit.libreoffice.org/#/c/64190/
Meanwhile, you can close it as RESOLVED FIXED
Comment 13 Commit Notification 2018-11-28 21:49:13 UTC
AlicVB committed a patch related to this issue.
It has been pushed to "libreoffice-6-1":

https://git.libreoffice.org/core/+/6a25245b37437e66f501338f17d6b54825a266d3%5E%21

tdf#120525 fix linespacing <100% for impress

It will be available in 6.1.4.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.