Download it now!
Bug 124109 - Justified Arabic text has gaps in place of kashida
Summary: Justified Arabic text has gaps in place of kashida
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: graphics stack (show other bugs)
Version:
(earliest affected)
6.2.0.0.alpha1+
Hardware: All All
: medium major
Assignee: Khaled Hosny
URL:
Whiteboard: target:6.3.0 target:6.1.6 target:6.2.3
Keywords: bibisected, bisected, regression
Depends on:
Blocks:
 
Reported: 2019-03-15 20:47 UTC by Khaled Hosny
Modified: 2019-04-21 07:25 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
Document showing issue (907.52 KB, application/vnd.oasis.opendocument.text)
2019-03-15 20:47 UTC, Khaled Hosny
Details
6.2.x defective output (14.30 KB, image/png)
2019-03-15 20:49 UTC, Khaled Hosny
Details
6.1.x correct output (14.52 KB, image/png)
2019-03-15 21:20 UTC, Khaled Hosny
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Khaled Hosny 2019-03-15 20:47:40 UTC
Created attachment 150007 [details]
Document showing issue

In the attached document there should be kashida (long horizontal lines) inside justified Arabic words, but instead there are gaps.

This is a regression as kashida was showing correctly in 6.1.x
Comment 1 Khaled Hosny 2019-03-15 20:49:04 UTC
Created attachment 150008 [details]
6.2.x defective output
Comment 2 Khaled Hosny 2019-03-15 21:20:47 UTC
Created attachment 150010 [details]
6.1.x correct output
Comment 3 Khaled Hosny 2019-03-15 21:27:37 UTC
I’m trying to debug this but getting nowhere, so hopefully someone can bisect this and find the commit that broke it.

(When testing, sometimes it is necessary to change the text when opening the document, e.g., by adding a space, for the kashida justification to work).
Comment 4 Julien Nabet 2019-03-16 09:38:52 UTC
I noticed that lcl_CheckKashidaPositions returned false at first test since rInf.GetOut()->GetMinKashida() returns 0

With gdb, I found that LogicalFontInstance::GetKashidaWidth could return 0
because in some cases, we don't enter if block with hb_font_get_glyph

I wonder if https://cgit.freedesktop.org/libreoffice/core/commit/?id=23c5125148a8110d88385b29570bf0b7d4400458 may be the cause.
Difficult to test since it can't be reverted without conflicts.
Comment 5 Julien Nabet 2019-03-16 09:39:13 UTC
On pc Debian x86-64 with master sources updated today, I could reproduce this.
Comment 6 Khaled Hosny 2019-03-18 11:52:14 UTC
(In reply to Julien Nabet from comment #4)
> I noticed that lcl_CheckKashidaPositions returned false at first test since
> rInf.GetOut()->GetMinKashida() returns 0
> 
> With gdb, I found that LogicalFontInstance::GetKashidaWidth could return 0
> because in some cases, we don't enter if block with hb_font_get_glyph
> 
> I wonder if
> https://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=23c5125148a8110d88385b29570bf0b7d4400458 may be the cause.
> Difficult to test since it can't be reverted without conflicts.

I don’t think so, the kashida positions are calculated and they are inserted in the glyph items vector, but they are not drawn, so much later than this code.
Comment 7 raal 2019-03-18 17:16:53 UTC
bisected with 6.1 to this commit. But I have not Noto fonts on my computer, so I'm not sure if it's correct. Can you check it?

e7b772a9c8a1c688c438e0f72ffff0a9b4263783 is the first bad commit
commit e7b772a9c8a1c688c438e0f72ffff0a9b4263783
Author: Norbert Thiebaud <nthiebaud@gmail.com>
Date:   Mon Dec 18 14:36:58 2017 -0800

    source sha:e45e2c4897933f14c90a65fa74d0ad2a0b620ede

author	Yousuf Philips <philipz85@hotmail.com>	2017-12-16 16:18:18 +0400
committer	Yousuf Philips <philipz85@hotmail.com>	2017-12-18 23:26:39 +0100
commit	e45e2c4897933f14c90a65fa74d0ad2a0b620ede (patch)
tree	6c3fbff8304e52d01a66b04e29163d27a14ee117
parent	bdcbf601410f29642aa27e2b8fab312f929d569f (diff)
tdf#103080 October 2017 update to Noto fonts
Comment 8 Khaled Hosny 2019-03-18 21:26:47 UTC
(In reply to raal from comment #7)
> bisected with 6.1 to this commit. But I have not Noto fonts on my computer,
> so I'm not sure if it's correct. Can you check it?
> 
> e7b772a9c8a1c688c438e0f72ffff0a9b4263783 is the first bad commit
> commit e7b772a9c8a1c688c438e0f72ffff0a9b4263783
> Author: Norbert Thiebaud <nthiebaud@gmail.com>
> Date:   Mon Dec 18 14:36:58 2017 -0800
> 
>     source sha:e45e2c4897933f14c90a65fa74d0ad2a0b620ede
> 
> author	Yousuf Philips <philipz85@hotmail.com>	2017-12-16 16:18:18 +0400
> committer	Yousuf Philips <philipz85@hotmail.com>	2017-12-18 23:26:39 +0100
> commit	e45e2c4897933f14c90a65fa74d0ad2a0b620ede (patch)
> tree	6c3fbff8304e52d01a66b04e29163d27a14ee117
> parent	bdcbf601410f29642aa27e2b8fab312f929d569f (diff)
> tdf#103080 October 2017 update to Noto fonts

This only adds the fonts, but the issue should be reproducible with any Arabic font. Try changing the font to an Arabic font you have on your system (avoid non-Arabic fonts as font fallabcks breaks kashida in a different, unrelated bug.
Comment 9 Xisco Faulí 2019-03-20 22:58:52 UTC
Hi Khaled,
my bisection points to https://cgit.freedesktop.org/libreoffice/core/commit/?id=c45b23377bb2fe44c26f1287ff38495344e4ca50, however, the commit is from LibreOffice 6.0 and you can't reproduce it in 6.1... this is weird
Comment 10 Khaled Hosny 2019-03-21 12:57:18 UTC
(In reply to Xisco Faulí from comment #9)
> Hi Khaled,
> my bisection points to
> https://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=c45b23377bb2fe44c26f1287ff38495344e4ca50, however, the commit is from
> LibreOffice 6.0 and you can't reproduce it in 6.1... this is weird

I reverted this commit locally and it didn’t fix the gap *inside* words issue, however it fixed the issue I had with kashida justfication not being done when the file is first open (gap *between* words).

While bisecting, try to always add a space somewhere in the line then remove it, you might need this to activate kashida justification.
Comment 11 Commit Notification 2019-03-21 14:46:10 UTC
Khaled Hosny committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/+/0a8e9cc5c1782f1cd50ef338ec2aa4f6776a4c0e%5E%21

tdf#124109: Revert "Only do kashida insertion with fonts that have non-zero width kashidas"

It will be available in 6.3.0.

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.
Comment 12 Xisco Faulí 2019-03-21 14:54:00 UTC
Hi Khaled,
second try pressing the space bar -> https://cgit.freedesktop.org/libreoffice/core/commit/?id=436b829f5b904d76039db0818cff5dedf1ae89f1.
Give it a try and let me know if this bisection is the right one...
Comment 13 Khaled Hosny 2019-03-21 15:39:19 UTC
(In reply to Xisco Faulí from comment #12)
> Hi Khaled,
> second try pressing the space bar ->
> https://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=436b829f5b904d76039db0818cff5dedf1ae89f1.
> Give it a try and let me know if this bisection is the right one...

Thanks Xisco, that is the commit indeed.
Comment 14 Xisco Faulí 2019-03-21 15:41:06 UTC
Adding Cc: to Miklos Vajna
Comment 15 Commit Notification 2019-03-22 08:04:30 UTC
Khaled Hosny committed a patch related to this issue.
It has been pushed to "master":

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

tdf#124109: Fix missing kashida glyphs

It will be available in 6.3.0.

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.
Comment 16 Miklos Vajna 2019-03-22 08:05:33 UTC
This is resolved now, thanks Khaled!
Comment 17 Commit Notification 2019-03-22 09:03:28 UTC
Khaled Hosny committed a patch related to this issue.
It has been pushed to "libreoffice-6-1":

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

tdf#124109: Revert "Only do kashida insertion with fonts that have non-zero width kashidas"

It will be available in 6.1.6.

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.
Comment 18 Commit Notification 2019-03-22 09:03:38 UTC
Khaled Hosny committed a patch related to this issue.
It has been pushed to "libreoffice-6-2":

https://git.libreoffice.org/core/+/76848756909cb969b4689ebd2dd3b5fa27b0dc23%5E%21

tdf#124109: Revert "Only do kashida insertion with fonts that have non-zero width kashidas"

It will be available in 6.2.3.

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.
Comment 19 Xisco Faulí 2019-03-22 09:22:00 UTC
Verified in

Version: 6.3.0.0.alpha0+
Build ID: 4c5d0e4822dcd0c6c9397a45e3afb66d53ebaafc
CPU threads: 4; OS: Linux 4.15; UI render: default; VCL: gtk3; 
Locale: ca-ES (ca_ES.UTF-8); UI-Language: en-US
Calc: threaded

@Khaled, thanks for fixing this issue!

Any chance we could have a unittest for this issue ?
Comment 20 Commit Notification 2019-03-22 13:59:07 UTC
Khaled Hosny committed a patch related to this issue.
It has been pushed to "libreoffice-6-2":

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

tdf#124109: Fix missing kashida glyphs

It will be available in 6.2.3.

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.
Comment 21 Commit Notification 2019-03-22 14:03:53 UTC
Miklos Vajna committed a patch related to this issue.
It has been pushed to "master":

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

Related: tdf#124109 sw: save one vcl layout call in SwFntObj::DrawText()

It will be available in 6.3.0.

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.
Comment 22 Miklos Vajna 2019-03-22 14:48:19 UTC
> Any chance we could have a unittest for this issue ?

Good question, certainly not trivial. I guess what the test has to do is pre-compute a glyph list, then call DrawTextArray() with the pre-computed glyph list and somehow assert that GenericSalLayout::ApplyDXArray() inserts the kashida glyphs. I'll give it a try, but I can't promise it.

(From my point of view, usually VCL "just works" and then we can assert what VCL calls Writer does, but here the root cause is in VCL itself.)
Comment 23 Commit Notification 2019-03-22 17:46:58 UTC
Miklos Vajna committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/+/8308d6a38ff283a2feebf84a95198159887cd685%5E%21

Related: tdf#124109 vcl: make glyph item caching work with kashida glyphs

It will be available in 6.3.0.

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.
Comment 24 Babak Razmjoo 2019-04-21 07:25:51 UTC
Great! it works for my in:
Version: 6.2.4.0.0+
Build ID: ca0ac3938020e8a544071fc6b08eb6b2830b5cc1
CPU threads: 1; OS: Linux 5.0; UI render: default; VCL: gtk2; 
TinderBox: Linux-rpm_deb-x86_64@86-TDF, Branch:libreoffice-6-2, Time: 2019-04-19_06:33:21
Locale: en-US (en_US.UTF-8); UI-Language: en-US
Calc: threaded