Qt patches for thai - fix

[John Tapsell]

Hi,

The current libthai Qt patches have a small but important error.
Could someone please update the patches? They need the following
modification:

commit e0ba0fcc9da1616c903b6fc94a505efe0215875b
Author: John Tapsell <john.tap...@basyskom.de>
Date: Wed Dec 14 17:24:56 2011 +0000

Harfbuzz - fix libthai patch to set the logcluster correctly

The result of this being incorrect was that thai did not word wrap
correctly

diff --git a/src/3rdparty/harfbuzz/src/harfbuzz-thai.c
b/src/3rdparty/harfbuzz/src/harfbuzz-thai.c
index b846264..f759216 100644
--- a/src/3rdparty/harfbuzz/src/harfbuzz-thai.c
+++ b/src/3rdparty/harfbuzz/src/harfbuzz-thai.c
@@ -210,7 +210,7 @@ static HB_Bool HB_ThaiConvertStringToGlyphIndices
(HB_ShaperItem *item)
const hb_uint32 len = item->item.length;
unsigned short *logClusters = item->log_clusters;

- hb_uint32 i = 0, slen = 0;
+ hb_uint32 i = 0, slen = 0, cluster_start = 0;

if (!init_libthai())
return HB_BasicShape (item);
@@ -245,7 +245,7 @@ static HB_Bool HB_ThaiConvertStringToGlyphIndices
(HB_ShaperItem *item)
haveSaraAm = (cstr[i + cell_length - 1] == (char)0xd3);

/* set shaper item's log_clusters */
- logClusters[i] = slen;
+ logClusters[i] = cluster_start;
for (int j = 1; j < cell_length; j++) {
logClusters[i + j] = logClusters[i];
}
@@ -259,6 +259,7 @@ static HB_Bool HB_ThaiConvertStringToGlyphIndices
(HB_ShaperItem *item)

/* Add glyphs to glyphs string and setting some attributes */
for (int lgi = 0; lgi < lgn; lgi++) {
+ cluster_start = slen;
if ( rglyphs[lgi] == 0xdd/*TH_BLANK_BASE_GLYPH*/ ) {
//if ( !item->fixedPitch ) {
glyphString[slen++] = C_DOTTED_CIRCLE;

[John Tapsell]

After I made this patch, I found that it broke the word wrapping. I
needed the following to fix the word wrapping again:

diff --git a/src/3rdparty/harfbuzz/src/harfbuzz-thai.c
b/src/3rdparty/harfbuzz/src/harfbuzz-thai.c
index f759216..7b2d925 100644
--- a/src/3rdparty/harfbuzz/src/harfbuzz-thai.c
+++ b/src/3rdparty/harfbuzz/src/harfbuzz-thai.c
@@ -387,9 +387,9 @@ static void HB_ThaiAssignAttributes(const
HB_UChar16 *string, hb_uint32 len, HB_
attributes[0].wordBoundary = true;
numbreaks = th_brk((const unsigned char *)cstr,
break_positions, brp_size);
for (i = 0; i < numbreaks; ++i) {
- attributes[break_positions[i]].wordBoundary = true;
- if (break_positions[i] > 0)
- attributes[break_positions[i]-1].lineBreakType = HB_Break;
+ if (break_positions[i]+1 < len)
+ attributes[break_positions[i]+1].wordBoundary = true;
+ attributes[break_positions[i]].lineBreakType = HB_Break;
}

if (break_positions != brp)

But I'm really shooting in the dark here. Could someone have a look
at these patches and see if I'm doing this wrongly? It seems wrong to
be changing the wordBoundary code here, and instead it looks like I
should be fixing the logcluster code. But I don't know how!

Please could someone look at this, or it will end up being merged and
released as I've done this :-/

John

[Pattara Kiatisevi (list)]

Hi John,

We've included your patch and tested on 4.7.4. The whole patch is here:

http://linux.thai.net/websvn/wsvn/software.qt/trunk/qt-everywhere-opensource-4.7.4/qt-4.7.4-harfbuzz_thai.patch

Thank you very much,
Pattara

[Pattara Kiatisevi (list)]

OK we will try it.

Golf/P'Thep: any comments krab? :)

Ott

[Pattara Kiatisevi (list)]

Hi John,

Sorry we are a little confused - what exactly was the problem that you had to made these two patches?
What's the test text or scenario that word wrapping won't work?

We tested with the old patch the word wrapping still works file so just wondering what problems
you are trying to solve here.

Thank you very much,
Pattara (Ott)

On 23/12/2554 2:04, John Tapsell wrote:

[John Tapsell]

Hi Pattara,

I'm trying to put together a test case that works only in Qt, but a
quick question - I've only just noticed that the patches attached to :
https://bugreports.qt.nokia.com//browse/QTBUG-12930 are not the same
as the ones on the website.

The patches attached to the bug look a bit newer, but they use the
HB_TextCodecForMib functions which were removed and instead hard
coded in. Any chance that you could port these patches to Qt 4.7 ?

John

2012/1/5 Pattara Kiatisevi (list) <pattar...@mm.co.th>:

[Pattara Kiatisevi (list)]

Hi John,

We tested using this patch http://linux.thai.net/websvn/wsvn/software.qt/trunk/qt-everywhere-opensource-4.7.4/qt-4.7.4-harfbuzz_thai.patch ,
word wrapping works quite fine as in the picture.

You have libthai installed?

Cheers,
Pattara



On 5/1/2555 21:52, John Tapsell wrote:

Hi Pattara and all,

  I put together a test case that demonstrates the bug.  You should
see a label with some thai text and word wrap enabled.  If you resize
the label you will see that the text does wordwrap, but that it's
always "off" - text gets truncated.

This is fixed by the patch that I proposed, but I know think that my
patch is not correct.  But I don't know what the correction solution.

  Attached also is a screenshot, just in case you can't reproduce.

John

On 5 January 2012 13:53, John Tapsell <john...@gmail.com> wrote:

[John Tapsell]

Hi Pattara and all,

I put together a test case that demonstrates the bug. You should
see a label with some thai text and word wrap enabled. If you resize
the label you will see that the text does wordwrap, but that it's
always "off" - text gets truncated.

This is fixed by the patch that I proposed, but I know think that my
patch is not correct. But I don't know what the correction solution.

Attached also is a screenshot, just in case you can't reproduce.

John

On 5 January 2012 13:53, John Tapsell <john...@gmail.com> wrote:

[jakkapun kwanroeangjai]

Hi, everybody

I tested the patch http://linux.thai.net/websvn/wsvn/software.qt/trunk/qt-everywhere-opensource-4.7.4/qt-4.7.4-harfbuzz_thai.patch, but I found a little compile error about parenthesis at first.

After I fixed it and built it again, I found the same problem about word wrap like John, even though I have already installed libthai. 

Now I'm looking through the patch again and trying to figure out this problem.

If anyone has suggestions or solutions about this problem, please let me know.

Thank you, 

Jakkapun

2012/1/6 Pattara Kiatisevi (list) <pattar...@mm.co.th>

--
คุณได้รับข้อความนี้เนื่องจากคุณสมัครรับข้อมูลจากกลุ่ม "Thai Linux/FOSS developers" ของ Google Groups
หากต้องการโพสต์ถึงกลุ่มนี้ ให้ส่งอีเมลไปที่ thai-linux...@googlegroups.com
หากต้องการยกเลิกการสมัครสำหรับกลุ่มนี้ โปรดส่งอีเมลไปที่ thai-linux-foss-...@googlegroups.com
สำหรับตัวเลือกเพิ่มเติม โปรดเข้าสู่กลุ่มนี้ที่ http://groups.google.com/group/thai-linux-foss-devel?hl=th

[Thanomsub Noppaburana]

With the patch http://linux.thai.net/websvn/wsvn/software.qt/trunk/qt-everywhere-opensource-4.7.4/qt-4.7.4-harfbuzz_thai.patch,i found the problems same as Jakkapun, the word warp feature does not work.

I guess that the new variable 'cluster_start' in HB_ThaiConvertStringToGlyphIndices function may has an incorrect value,
so i try to move the statement 'cluster_start = slen;' out of the for loop like these:

        /* Add glyphs to glyphs string and setting some attributes */
        for (int lgi = 0; lgi < lgn; lgi++) {

            //cluster_start = slen;

            if ( rglyphs[lgi] == 0xdd/*TH_BLANK_BASE_GLYPH*/ ) {

            ...
            ....
            }
        }
        cluster_start = slen;

Now, the word warp work fine.

Cheers,
donga

เมื่อ 10 มกราคม 2555, 10:23, jakkapun kwanroeangjai <gloo...@gmail.com> เขียนว่า:

[Thanomsub Noppaburana]

Hi all,

I attached the new harfbuzz_thai patch (fix the old qt-4.7.4-harfbuzz_thai.patch)  with this reply.

Cheers,
Donga

เมื่อ 10 มกราคม 2555, 14:09, Thanomsub Noppaburana <dong...@gmail.com> เขียนว่า:

[John Tapsell]

2012/1/10 Thanomsub Noppaburana <dong...@gmail.com>

Hi all,

I attached the new harfbuzz_thai patch (fix the old qt-4.7.4-harfbuzz_thai.patch)  with this reply.

I think your change is exactly equivalent to just reverting the latest commit (which I wrote, and think is wrong).

John

[Thanomsub Noppaburana]

2012/1/10 John Tapsell <john...@gmail.com>:

If let 'cluster_start = slen' stay in loop, when process the next cell
on the next loop, the value of 'cluster_start' still point to the last
cell not the current cell which processing, i think that is wrong.

Cheers,
Donga.

>
> John

[John Tapsell]

> If let 'cluster_start = slen' stay in loop, when process the next cell
> on the next loop, the value of 'cluster_start' still point to the last
> cell not the current cell which processing,  i think that is wrong.

Right. That's what I'm saying.

[Theppitak Karoonboonyanan]

Hi all,

Sorry that I don't have a proper Qt environment to test. (I'm a GNOME
user.) So, I just follow your discussions by reading the source.

2012/1/10 John Tapsell <john...@gmail.com>:
>
> 2012/1/10 Thanomsub Noppaburana <dong...@gmail.com>
>>

>> I attached the new harfbuzz_thai patch (fix the old
>> qt-4.7.4-harfbuzz_thai.patch)  with this reply.
>
> I think your change is exactly equivalent to just reverting the latest
> commit (which I wrote, and think is wrong).

Yes, it's effectively the same. So, we can fix the word wrapping issue
by either updating the patch to Thanomsub's or applying the reverse
patch of your commit at beginning of the thread.

But what's the problem that led you to that commit, anyway?

Regards,
-Thep.
--
Theppitak Karoonboonyanan
http://linux.thai.net/~thep/

[John Tapsell]

I'm becoming increasingly frustrated

Is there noone that is willing to work with me on this? Taking a day
for each email, and then repeatedly asking me questions that I have
answered several times now is getting nowhere.

Is there anyone willing to work with me today on this?

John

[John Tapsell]

Aha, I've got progress!

It seems that the reason that you can't reproduce is because our font
is different from your font. We have ligatures in our font, and I
think that's the reason.

If I change in harfbuzz-thai.c :

- return HB_OpenTypePosition (shaper_item, availableGlyphs,
/*doLogClusters*/false);
+ return HB_OpenTypePosition (shaper_item, availableGlyphs,
/*doLogClusters*/true);

Then it works correctly (without any of my patches).

John

[John Tapsell]

On 11 January 2012 09:40, John Tapsell <john...@gmail.com> wrote:
> If I change in harfbuzz-thai.c :
>
> -        return HB_OpenTypePosition (shaper_item, availableGlyphs,
> /*doLogClusters*/false);
> +        return HB_OpenTypePosition (shaper_item, availableGlyphs,
> /*doLogClusters*/true);
>
> Then it works correctly (without any of my patches).

The Qt guys said that this should indeed be true:

The doLogClusters parameter to HB_OpenTypePosition should be true for
all languages that are not shaped one syllable at a time

So, please revert *all* of my patches (one or two) and make this log
cluster change? I am submitting this change to Qt as well.

[Thanomsub Noppaburana]

Hi all,

John, where is your patches, i'll try it for you.
Please provide it to us. :-)

Cheers,
Donga

2012/1/11 John Tapsell <john...@gmail.com>:

[John Tapsell]

It's a pretty simple patch - I pasted below :-)

To reproduce the problem that we were having though, you need a font
with ligatures. I don't know which particular fonts though.

John

commit 96993932ef0911176f96a9b4e400ba6c99bf7602
Author: John Tapsell <john.tap...@basyskom.de>
Date: Wed Jan 11 09:52:00 2012 +0000

Harfbuzz-thai - fix fonts with ligatures by setting doLogClusters parameter

The doLogClusters parameter to HB_OpenTypePosition should be true
for all languages that are not shaped one syllable at a time

diff --git a/src/3rdparty/harfbuzz/src/harfbuzz-thai.c
b/src/3rdparty/harfbuzz/src/harfbuzz-thai.c
index d3fc5a9..d00c220 100644
--- a/src/3rdparty/harfbuzz/src/harfbuzz-thai.c
+++ b/src/3rdparty/harfbuzz/src/harfbuzz-thai.c
@@ -334,7 +334,7 @@ HB_Bool HB_ThaiShape (HB_ShaperItem *shaper_item)
const int availableGlyphs = shaper_item->num_glyphs;
if ( HB_SelectScript (shaper_item, thai_features) ) {
HB_OpenTypeShape (shaper_item, /*properties*/0);

- return HB_OpenTypePosition (shaper_item, availableGlyphs,
/*doLogClusters*/false);
+ return HB_OpenTypePosition (shaper_item, availableGlyphs,
/*doLogClusters*/true);
}

#endif

[Thanomsub Noppaburana]

2012/1/11 John Tapsell <john...@gmail.com>:

Ok, and should i put the 'cluster_start = slen' into the for loop again?

p.s.
I've just tried doLogClusters = true with cluster_start = slen in the for loop
and use Droid Sans font (don't know it have ligatures or not),
but seem the word warpping does not work perfectly :-P

Cheers,
Donga.

[John Tapsell]

> Ok, and should i put the 'cluster_start = slen' into the for loop again?

Outside. You want to revert all of my patches.

[Thanomsub Noppaburana]

2012/1/11 John Tapsell <john...@gmail.com>:

>> Ok, and should i put the 'cluster_start = slen' into the for loop again?
>
> Outside.  You want to revert all of my patches.
>

Done with cluster_start = slen outside the for loop.
John, word warpping and shaping work fine same as doLogClusters = false !!
:-)

Anyone have tried this?

Cheers,
Donga

[John Tapsell]

On 11 January 2012 12:51, Thanomsub Noppaburana <dong...@gmail.com> wrote:
> 2012/1/11 John Tapsell <john...@gmail.com>:
>>> Ok, and should i put the 'cluster_start = slen' into the for loop again?
>>
>> Outside.  You want to revert all of my patches.
>>
>
> Done with cluster_start = slen  outside the for loop.
> John, word warpping and shaping work fine same as  doLogClusters = false !!

With the specific font you are using, yes. If you use a font with
ligatures, however, it makes a difference and only works if it is
true.

John

[Thanomsub Noppaburana]

2012/1/11 John Tapsell <john...@gmail.com>:

LOL
Can you provide your suggest font for this test?

> John

[Theppitak Karoonboonyanan]

2012/1/11 John Tapsell <john...@gmail.com>:

> With the specific font you are using, yes.  If you use a font with
> ligatures, however, it makes a difference and only works if it is
> true.

In fact, most Thai fonts from the thaifonts-scalable >= 0.4.10
(released since 2008), which I guess people here are using,
do contain ff, fi, fl, ffi, ffl ligatures, with 'liga' GSUB rule.
Is that relevant?

Regards,

[Pattara Kiatisevi]

So based on this theory K.Donga and Golf Jakkapun must be using the same kind of fonts
and Sukoom the different one. OK guys which font you are using??

Ott

2012/1/11 Theppitak Karoonboonyanan <th...@linux.thai.net>

[Pattara Kiatisevi]

John,

We are all volunteers here and trying to work with you as much as we can.

The fact that we submitted the patch some 3-4 years ago and noone at Qt 

seems to care to review it make me feel that your effort is very valuable -- we do 

want to cooperate, but please just have enough patience.

Cheers,

Ott

2012/1/11 John Tapsell <john...@gmail.com>

[John Tapsell]

2012/1/11 Pattara Kiatisevi <pat...@mm.co.th>:

> John,
>
> We are all volunteers here and trying to work with you as much as we can.
> The fact that we submitted the patch some 3-4 years ago and noone at Qt
> seems to care to review it make me feel that your effort is very valuable --
> we do
> want to cooperate, but please just have enough patience.

I know - I really do appreciate it. I am very sorry for my
frustration. I'm also sorry that Qt took 4 years to merge your
patches.

As for the font, my hands are tied. Frustratingly, I am not allowed
to send it to you!

So, please don't worry about this any more. I have a fix now that
works, and the Qt guys has approved it.

John