Bug 156071

Summary: Thai Support for Konsole
Product: [Applications] konsole Reporter: Pattara Kiatisevi <ott>
Component: generalAssignee: Konsole Developer <konsole-devel>
Status: RESOLVED DUPLICATE    
Severity: normal CC: adaptee, ott, robertknight, thep
Priority: NOR    
Version: unspecified   
Target Milestone: ---   
Platform: Compiled Sources   
OS: Linux   
Latest Commit: Version Fixed In:
Sentry Crash Report:
Attachments: Patch to fix Thai-related problem in Konsole
update patch for Thai Support for konsole (add QChar::category(c) checking) :)
Patch to fix Thai-related problems in Konsole
Anchored monospace font for testing
konsole Thai patch
Updated Thai patch for Konsole KDE 4.1.3
Updated patch for svn trunk

Description Pattara Kiatisevi 2008-01-18 04:45:39 UTC
Version:            (using KDE 4.0.0)
Installed from:    Compiled From Sources
OS:                Linux

Thai characters differ from English in that some characters should be combined and displayed in the same cell. This patch fixes the rendering issues of Thai in Konsole (upper and lower vowels not shown, input method, cursor moving and mouse selection).

Ref: 
[1] patch: http://linux.thai.net/viewvc/viewvc.cgi/software/kde/kdebase-3.96.0/konsole_thai_patch.diff?revision=1.2&view=markup

[2] TlwgMono font for testing.
http://suriyan.in.th/download/fonts/TlwgMono.ttf
Comment 1 Pattara Kiatisevi 2008-01-18 04:48:30 UTC
Created attachment 23114 [details]
Patch to fix Thai-related problem in Konsole

Patch by Jakkapun Kwanroeangjai <jakkapun@mm.co.th>
Comment 2 Robert Knight 2008-01-18 05:41:11 UTC
Hello, thank-you for the patch, I have some comments and questions on it.  

+  // force combining character at column 0 to always take the first cell
+  if (w == 0 && cuX == 0)
+    w = 1;

I don't understand the comment here.  Could you clarify what this does?

+  //Set CharSequence
+  ushort u_char_combind[5];

Where does the '5' come from?

+  if (w == 0 && cuX > 0) {
+    if (screenLines[cuY][cuX-1].rendition & RE_EXTENDED_CHAR)
+    {

Looking at the table in konsole_wcwidth.cpp, it seems that w can be 0 when the character is not a combining character, for example the null character and there are two other cases which I don't understand.  Do we need to check the value of QChar::category(c) here to be sure this is the right kind of character to build an extended char sequence?      

+        if (extendedCharLength > 5)
+            return;

Is it possible for extendedCharLength to be above 5 in normal use or can that only happen if there is a bug?

+void TerminalDisplay::drawCharSequence(   QPainter& painter,const QRect& rect, const QString& str, 
+                                    const Character* attributes)
+{ 
+    painter.drawText(rect,str[0]);
+    Qt::BGMode bgMode = painter.backgroundMode();
+    painter.setBackgroundMode(Qt::TransparentMode);
+    for (int i = 1; i < str.length(); i++)
+        painter.drawText(rect,str[i]);
+    painter.setBackgroundMode(bgMode);
+}

Character-by-character text drawing is very slow.  Complete strings should be drawn if at all possible.  What is the background mode change here for?

+            {   
+                // Do not try to even peek screen buffer. The real text buffer
+                // owner is the process that runs on konsole, not konsole
+                // itself. Trying to do so would cause text input in remote
+                // sessions to depend on konsole's response, which can block
+                // one from typing continuously without waiting the characters
+                // to appear on screen, for instance.
+                return QString();

How will this affect languages other than Thai?  Is this also a problem with Chinese or Japanese text for example?
Comment 3 Jakkapun Kwanroengjai 2008-01-18 12:13:00 UTC
> +  // force combining character at column 0 to always take the first cell
> +  if (w == 0 && cuX == 0)
> +    w = 1;

> I don't understand the comment here.  Could you clarify what this does? 

In case combining character appears at column 0 without a base character, this would force it to take the column, rather than falling of the left edge of screen.
Comment 4 Jakkapun Kwanroengjai 2008-01-18 12:14:55 UTC
> +  //Set CharSequence
> +  ushort u_char_combind[5];

> Where does the '5' come from? 
For Thai it is 3 but I heard that the safe number is 5 (for example Khmer needs 5). xterm is using 5 too.
Comment 5 Jakkapun Kwanroengjai 2008-01-18 13:46:51 UTC
> +  if (w == 0 && cuX > 0) {
> +    if (screenLines[cuY][cuX-1].rendition & RE_EXTENDED_CHAR)
> +    {

> Looking at the table in konsole_wcwidth.cpp, it seems that w can be 0 when the character is not a combining character, for example the null character and there are two other cases which I don't understand.  Do we need to check the value of QChar::category(c) here to be sure this is the right kind of character to build an extended char sequence?       

thank you for pointing this out. i attached a new patch (which uses QChar::category(c )==QChar::Mark_NonSpacing instead)
Comment 6 Jakkapun Kwanroengjai 2008-01-18 13:50:27 UTC
> +void TerminalDisplay::drawCharSequence(   QPainter& painter,const QRect& rect, const QString& str,
> +                                    const Character* attributes)
> +{
> +    painter.drawText(rect,str[0]);
> +    Qt::BGMode bgMode = painter.backgroundMode();
> +    painter.setBackgroundMode(Qt::TransparentMode);
> +    for (int i = 1; i < str.length(); i++)
> +        painter.drawText(rect,str[i]);
> +    painter.setBackgroundMode(bgMode);
> +}

> Character-by-character text drawing is very slow.  Complete strings should be drawn if at all possible.  What is the background mode change here for? 

The problem is that, for this kind of font (pure monospace), drawing the whole string using QPainter::DrawText() would yield incorrect output -- combining characters will be in different cells, hence not combined. We set the background mode to transparent to that the to-be-combined upper or lower vowles will be drawn on top of the previous character (i.e. "combine" them).
 Or should we fix QPainter::DrawText() to detect this kind of font and draw string accordingly instead?  but i imagine that won't be too easy (as it could affect a lot of things...)
Advise welcomed.
Comment 7 Jakkapun Kwanroengjai 2008-01-18 14:01:18 UTC
+        if (extendedCharLength > 5)
+            return;

Is it possible for extendedCharLength to be above 5 in normal use or can that only happen if there is a bug? 

In thai it is impossible, and i think other language is too. ;)
Comment 8 Jakkapun Kwanroengjai 2008-01-18 15:12:49 UTC
> In thai it is impossible, and i think other language is too. ;)
in case using normal input method, 
but if it happens (maybe in junk file), it will draw only 5 characters.

> How will this affect languages other than Thai?  Is this also a problem with Chinese or Japanese text for example? 
As far as I know, languages whose input methods need to retrieve surrounding include Thai, Lao and Sinhala. All should be affected by the problem for slow remote sessions. CJK, however, use preedit rather than surrounding retrieval. So, they are not affected.
Comment 9 Pattara Kiatisevi 2008-01-18 15:38:02 UTC
In addition to comment # 4
That 5 is supposed to be the maximum number of characters that can possibly be combined in a single cell
Comment 10 Robert Knight 2008-01-18 16:00:51 UTC
> For Thai it is 3 but I heard that the safe number
> is 5 (for example Khmer needs 5). xterm is using 5 too. 

Ok.  A named constant would be better, eg. "const int MAX_COMBINING_CHARS = 5"

> i attached a new patch (which uses QChar::category(c )==QChar::Mark_NonSpacing > instead)

The same check should presumably also be used in the "if (w==0 && cuX==0)" test.
I think you forgot to attach the updated patch.  

> Or should we fix QPainter::DrawText() to detect this kind of font and
> draw string accordingly instead?  but i imagine that won't be too easy
> (as it could affect a lot of things...) Advise welcomed. 

Thanks for explaining.  In this case I suggest leaving the code as you have done already but add a comment to explain the problem that is being worked around.  It would be worth checking against Qt 4.4 to see if there has been any change.

Can Kate/KWrite render Thai text correctly?  If so it would be worth looking at how the kate part draws the text.

> but if it happens (maybe in junk file), it will draw only 5 characters. 

Ok, that is sensible.  

Comment 11 Robert Knight 2008-01-18 16:15:39 UTC
There is a related report which this patch might also fix: http://bugs.kde.org/show_bug.cgi?id=96536 
Comment 12 Theppitak Karoonboonyanan 2008-01-19 02:30:04 UTC
> Can Kate/KWrite render Thai text correctly?  If so it would be worth looking at 
> how the kate part draws the text.

I think the situations are different. Kate/KWrite has no restriction about
display grid. So it's free to use proportional fonts (or fake monospace fonts, 
i.e. duo-space with zero-width combining characters and fixed-width base 
characters) and enjoy any string-based linguistic preprocessing done by the
renderer. And most users are satisfied with using such non-monospace fonts.

OTOH, Konsole is a strict display grid, where cells are rendered separately 
from one another. Some string-based preprocessings that involve more than one
column become too complicated to handle. (In case of Thai, Sara Am (U+0E33) 
decomposition is an example. We had fought to solve some display glitches
before finally giving up.) So, true monospace fonts with character cell
composition (as already used in Emacs and XTerm) seem to fit it more.

Kate/KWrite currently displays every combining character in separate cell when 
using monospace font. Fixing this would be good as well, anyway. But I have no
idea yet whether this should be application-specific feature or a toolkit 
provision.
Comment 13 Robert Knight 2008-01-19 05:49:58 UTC
"OTOH, Konsole is a strict display grid, where cells are rendered separately
from one another."

Konsole actually renders text in batches of equally formatted text.  So if you have a plain line of text all with default formatting, it will be dispatched to QPainter::drawText() in one go.  This means that you can actually use proportional fonts with Konsole under KDE 4, if you modify the code that creates the font selector dialog so that it doesn't restrict the user to monospace fonts.  The only problem is that the decision about where to draw to start drawing a particular block of text on a line is based on (average latin character width * number of columns drawn so far), so if you have several blocks of text with different formatting on a line, the blocks might overlap or have gaps between them if a proportional font is used.  That is not unfixable though.   

"But I have no idea yet whether this should be application-specific feature or a toolkit provision." 

The more of this that can be handled automatically by the toolkit the better.  Text input. processing and rendering is a complex subject.  Hence Trolltech have at least one employee dedicated to it.  Application authors such as myself who work in their spare time will generally know very little about it. 
Comment 14 Jakkapun Kwanroengjai 2008-01-19 13:01:08 UTC
Created attachment 23135 [details]
update patch for Thai Support for konsole (add QChar::category(c) checking) :)
Comment 15 Pattara Kiatisevi 2008-02-04 16:05:30 UTC
Created attachment 23413 [details]
Patch to fix Thai-related problems in Konsole

After some trials and also a fix in the font (adding anchors between base
characters and upper/lower vowels(By Theppitak, see
http://groups.google.com/group/thai-linux-foss-devel/browse_thread/thread/2b5d151b674b2bd2)
and in Qt
(http://linux.thai.net/viewvc/viewvc.cgi/software/qt/x11-free-4.3.1/qt-4.3.1-thai-script-engine-patch.diff?view=log),
Thai text seems to be rendered OK in konsole using QPainter::drawText() with
this new patch by Jakkapun Kwanroengjai.

Is it OK?
Comment 16 Theppitak Karoonboonyanan 2008-02-05 03:08:15 UTC
Created attachment 23422 [details]
Anchored monospace font for testing

This experimental font is monospace, with OpenType features to manage the
CharCell composition. Anchors are added for mark-to-base positioning, and
advanced widths subtracted to zero for combining marks. So, it's supposed to
render Thai properly, given proper OpenType support in rendering engines. And
non-OpenType apps would just see it as pure monospace.
Comment 17 Theppitak Karoonboonyanan 2008-02-05 04:01:00 UTC
"Konsole actually renders text in batches of equally formatted text.  So if you have a plain line of text all with default formatting, it will be dispatched to QPainter::drawText() in one go."

I hope the OpenType-based CharCell composition is the right way to handle monospace fonts. Given that, there would be no difference of interface between monospace and proportional rendering of single cells, then.

But I'd like to clarify my points about Konsole's different nature a little bit.

The proposed font modification may solve the cell-by-cell rendering of monospace fonts with QPainter::drawText(), but the problem of inter-cell composition cases still remains for proportional fonts in general.

I mentioned the case of Thai SARA AM (U+0E33) in comment #12. This character is normally rendered by decomposition into NIKHAHIT (U+0E4D) and SARA AA (U+0E32). NIKHAHIT is combining mark, which has to be composed to the previous cell, while SARA AA stays in its own cell.

For applications like Kate, text lines can be stored as continuous strings, with necessary interface to access each column by the aids of logical clusters analyzed by the toolkit's language engine.

Konsole, on the other hand, stores and handles cells as separate "Character"s [Character.h]. So, the case that involves more than one cell like SARA AM will need special accessing method. This is what I said about its difference from Kate.

For the proposed patch and font here, the font still carries the composition/decomposition rules for SARA AM cases, which will only be effective when rendering inter-cell strings with sufficient context (e.g. with Kate). For the single SARA AM without base character, Jakkapan's proposed patch to Qt4 will render it with dotted circle, *except* when using monospace font, which is supposed to be the case for console-based apps like Konsole.
Comment 18 Rohit 2008-02-05 14:56:04 UTC
hi,
i have the same problem with konsole..
but not in Thai,,, In devanagari script..
Konsole does not display combing characters of devanagari script..

Can this problem solved by putting some patches?????

If so then can u suggest the patch..



On 5 Feb 2008 03:01:01 -0000, Theppitak Karoonboonyanan <thep@linux.thai.net>
wrote:

[bugs.kde.org quoted mail]
hi,<br>i have the same problem with konsole..<br>but not in Thai,,, In devanagari script..<br>Konsole does not display combing characters of devanagari script..<br><br>Can this problem solved by putting some patches?????<br>
<br>If so then can u suggest the patch..<br><br>&nbsp;  <br><br><div class="gmail_quote">On 5 Feb 2008 03:01:01 -0000, Theppitak Karoonboonyanan &lt;<a href="mailto:thep@linux.thai.net">thep@linux.thai.net</a>&gt; wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="Ih2E3d">------- You are receiving this mail because: -------<br>You are the assignee for the bug, or are watching the assignee.<br><br><a href="http://bugs.kde.org/show_bug.cgi?id=156071" target="_blank">http://bugs.kde.org/show_bug.cgi?id=156071</a><br>
<br><br><br><br></div>------- Additional Comments From thep linux thai net &nbsp;2008-02-05 04:01 -------<br>&quot;Konsole actually renders text in batches of equally formatted text. &nbsp;So if you have a plain line of text all with default formatting, it will be dispatched to QPainter::drawText() in one go.&quot;<br>
<br>I hope the OpenType-based CharCell composition is the right way to handle monospace fonts. Given that, there would be no difference of interface between monospace and proportional rendering of single cells, then.<br><br>
But I&#39;d like to clarify my points about Konsole&#39;s different nature a little bit.<br><br>The proposed font modification may solve the cell-by-cell rendering of monospace fonts with QPainter::drawText(), but the problem of inter-cell composition cases still remains for proportional fonts in general.<br>
<br>I mentioned the case of Thai SARA AM (U+0E33) in comment #12. This character is normally rendered by decomposition into NIKHAHIT (U+0E4D) and SARA AA (U+0E32). NIKHAHIT is combining mark, which has to be composed to the previous cell, while SARA AA stays in its own cell.<br>
<br>For applications like Kate, text lines can be stored as continuous strings, with necessary interface to access each column by the aids of logical clusters analyzed by the toolkit&#39;s language engine.<br><br>Konsole, on the other hand, stores and handles cells as separate &quot;Character&quot;s [Character.h]. So, the case that involves more than one cell like SARA AM will need special accessing method. This is what I said about its difference from Kate.<br>
<br>For the proposed patch and font here, the font still carries the composition/decomposition rules for SARA AM cases, which will only be effective when rendering inter-cell strings with sufficient context (e.g. with Kate). For the single SARA AM without base character, Jakkapan&#39;s proposed patch to Qt4 will render it with dotted circle, *except* when using monospace font, which is supposed to be the case for console-based apps like Konsole.<br>
<div><div></div><div class="Wj3C7c">_______________________________________________<br>konsole-devel mailing list<br><a href="mailto:konsole-devel@kde.org">konsole-devel@kde.org</a><br><a href="https://mail.kde.org/mailman/listinfo/konsole-devel" target="_blank">https://mail.kde.org/mailman/listinfo/konsole-devel</a><br>
</div></div></blockquote></div><br>
Comment 19 Rohit 2008-02-09 13:47:16 UTC
hi..

I wanted to install the patch given for the thai lang..

Can you please tell me how to run or load the patch..

 can i load the patch for kde 3.5.7or it is only valid for kde 4 ???

Regards
Rohit.
On 8 Feb 2008 10:33:56 -0000, <owner@bugs.kde.org> wrote:

[bugs.kde.org quoted mail]
hi..<br><br>I wanted to install the patch given for the thai lang..<br><br>Can you please tell me how to run or load the patch..<br><br>&nbsp;can i load the patch for kde 3.5.7or it is only valid for kde 4 ???<br><br>Regards <br>
Rohit.<br><div class="gmail_quote">On 8 Feb 2008 10:33:56 -0000,  &lt;<a href="mailto:owner@bugs.kde.org">owner@bugs.kde.org</a>&gt; wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="Ih2E3d">------- You are receiving this mail because: -------<br>You are the assignee for the bug, or are watching the assignee.<br><br><a href="http://bugs.kde.org/show_bug.cgi?id=156071" target="_blank">http://bugs.kde.org/show_bug.cgi?id=156071</a><br>
</div>glooffy23 gmail com changed:<br><br> &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; What &nbsp; &nbsp;|Removed &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; |Added<br>----------------------------------------------------------------------------<br> &nbsp;Attachment #23135 [details]|0 &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; |1<br>
 &nbsp; &nbsp; &nbsp; &nbsp;is obsolete| &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;|<br><div><div></div><div class="Wj3C7c">_______________________________________________<br>konsole-devel mailing list<br><a href="mailto:konsole-devel@kde.org">konsole-devel@kde.org</a><br>
<a href="https://mail.kde.org/mailman/listinfo/konsole-devel" target="_blank">https://mail.kde.org/mailman/listinfo/konsole-devel</a><br></div></div></blockquote></div><br>
Comment 20 Pattara Kiatisevi 2008-02-09 16:54:05 UTC
-it is valid for kde4.
-the steps should be: 

1.download the KDE4's kdebase source code and extract it 
2.patch it (something like "patch -p1 < patch_file")
3.build kdebase (follow the normal build instruction)

-you might need to patch/build Qt4 too:
see the patches we did at http://linux.thai.net/viewvc/viewvc.cgi/software/qt/x11-free-4.3.1/
Comment 21 Theppitak Karoonboonyanan 2008-02-10 06:53:07 UTC
Rohit, you may also be interested in this "Quantum Font" thread from dejavu-fonts mailing list:
  http://sourceforge.net/mailarchive/forum.php?thread_name=416e2cf10802081213t17a4fd78w88c4327bb7115a91%40mail.gmail.com&forum_name=dejavu-fonts

It's still a proposal, though.
Comment 22 Rohit 2008-02-28 07:45:10 UTC
hi..
 can you please tell me how to install Kde 4 on fedora core 7...


please relpy..

Regards,
Rohit


On 10 Feb 2008 05:53:08 -0000, Theppitak Karoonboonyanan <
thep@linux.thai.net> wrote:

[bugs.kde.org quoted mail]
hi..<br>&nbsp;can you please tell me how to install Kde 4 on fedora core 7...<br><br><br>please relpy..<br><br>Regards,<br>Rohit<br><br><br><div class="gmail_quote">On 10 Feb 2008 05:53:08 -0000, Theppitak Karoonboonyanan &lt;<a href="mailto:thep@linux.thai.net">thep@linux.thai.net</a>&gt; wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="Ih2E3d">------- You are receiving this mail because: -------<br>
You are the assignee for the bug, or are watching the assignee.<br>
<br>
<a href="http://bugs.kde.org/show_bug.cgi?id=156071" target="_blank">http://bugs.kde.org/show_bug.cgi?id=156071</a><br>
<br>
<br>
<br>
<br>
</div>------- Additional Comments From thep linux thai net &nbsp;2008-02-10 06:53 -------<br>
Rohit, you may also be interested in this &quot;Quantum Font&quot; thread from dejavu-fonts mailing list:<br>
 &nbsp;<a href="http://sourceforge.net/mailarchive/forum.php?thread_name=416e2cf10802081213t17a4fd78w88c4327bb7115a91%40mail.gmail.com&amp;forum_name=dejavu-fonts" target="_blank">http://sourceforge.net/mailarchive/forum.php?thread_name=416e2cf10802081213t17a4fd78w88c4327bb7115a91%40mail.gmail.com&amp;forum_name=dejavu-fonts</a><br>

<br>
It&#39;s still a proposal, though.<br>
<div><div></div><div class="Wj3C7c">_______________________________________________<br>
konsole-devel mailing list<br>
<a href="mailto:konsole-devel@kde.org">konsole-devel@kde.org</a><br>
<a href="https://mail.kde.org/mailman/listinfo/konsole-devel" target="_blank">https://mail.kde.org/mailman/listinfo/konsole-devel</a><br>
</div></div></blockquote></div><br>
Comment 23 Pattara Kiatisevi 2008-08-28 10:51:35 UTC
Created attachment 27099 [details]
konsole Thai patch

Updated patch for KDE 4.1

(patch by Jakkapun Kwanroengjai)

Please kindly review, cheers,
Pattara
Comment 24 Pattara Kiatisevi 2008-11-12 13:28:43 UTC
Created attachment 28516 [details]
Updated Thai patch for Konsole KDE 4.1.3

Updated patch, successfully tested with KDE 4.1.3.
Comment 25 Kurt Hindenburg 2009-03-17 04:51:22 UTC
Created attachment 32179 [details]
Updated patch for svn trunk

Please test this if you can.
Comment 26 Pattara Kiatisevi 2009-03-30 11:55:14 UTC
Tested on Kubuntu KDEBASE 4.2.1-0ubuntu6, seems to work OK! Will that differ from testing on SVN trunk? 

I somehow couldn't svn update kdebase from trunk till the end, always get timeout and have to cancel.
Comment 27 Kurt Hindenburg 2009-04-01 04:28:09 UTC
This patch changes a lot of stuff I'm not familiar with.  It will have to wait for Robert's approval I think.
Comment 28 Pattara Kiatisevi 2009-09-20 06:36:53 UTC
So any comments for the patch? 

Commit it?
Comment 29 Robert Knight 2009-09-20 11:09:25 UTC
Hello Pattara,

I'll try to get a drive to review all the outstanding patches for KDE 4.4.  Meanwhile it would be very useful if there was a test case in kdebase/apps/konsole/src/tests for the Thai support to make it less likely to be accidentally broken in future.  Since most people working on Konsole use western languages exclusively we likely won't spot any regressions otherwise.
Comment 30 Jekyll Wu 2011-08-16 07:50:02 UTC

*** This bug has been marked as a duplicate of bug 96536 ***