Bug 156071

Created attachment 23114 [details]
Patch to fix Thai-related problem in Konsole

Patch by Jakkapun Kwanroeangjai <jakkapun@mm.co.th>

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?

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

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

> +  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)

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

+        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. ;)

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

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

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

There is a related report which this patch might also fix: http://bugs.kde.org/show_bug.cgi?id=96536 

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

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

Created attachment 23135 [details]
update patch for Thai Support for konsole (add QChar::category(c) checking) :)

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?

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.

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

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>

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>

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

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.

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>

Created attachment 27099 [details]
konsole Thai patch

Updated patch for KDE 4.1

(patch by Jakkapun Kwanroengjai)

Please kindly review, cheers,
Pattara

Created attachment 28516 [details]
Updated Thai patch for Konsole KDE 4.1.3

Updated patch, successfully tested with KDE 4.1.3.

Created attachment 32179 [details]
Updated patch for svn trunk

Please test this if you can.

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.

This patch changes a lot of stuff I'm not familiar with.  It will have to wait for Robert's approval I think.

So any comments for the patch? 

Commit it?

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.

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