[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kfm-devel
Subject:    Re: [PATCH] [Bug 91955] Bidi algorithm: incorrect implementation of
From:       Mitz Pettel <bugs.kde.org () mitzpettel ! com>
Date:       2004-11-03 11:26:46
Message-ID: 3F170C1D-2D8B-11D9-99B1-0003937C3DB0 () mitzpettel ! com
[Download RAW message or body]

On Nov 3, 2004, at 12:03 PM, Lars Knoll wrote:
>
> Did you check khtmltests/i18n/bidi.html that you don't break any 
> existing test
> cases? The patch looks good at first sight but without extensive 
> testing I
> can't really tell.

And on Nov 3, 2004, at 12:36 PM, Stephan Kulow wrote:

> The patch breaks more than that. But bidi.html shows already enough of 
> them.

Oh dear, as Stephan pointed out, what I submitted breaks mostly 
everything. I simply submitted the wrong diff. Sorry about that!

This is the correct diff now, which as far as I could see doesn't break 
anything in bidi.html and also corrects bug 91955.


["bidi.cpp.diff" (bidi.cpp.diff)]

Index: bidi.cpp
===================================================================
RCS file: /home/kde/kdelibs/khtml/rendering/bidi.cpp,v
retrieving revision 1.198
diff -u -r1.198 bidi.cpp
--- bidi.cpp	26 Oct 2004 18:58:41 -0000	1.198
+++ bidi.cpp	3 Nov 2004 11:18:33 -0000
@@ -856,26 +856,19 @@
                 case QChar::DirS:
                 case QChar::DirWS:
                 case QChar::DirON:
-                    if(dir != QChar::DirL) {
+                     if( bidi.status.eor != QChar::DirL ) {
                         //last stuff takes embedding dir
-                        if( bidi.context->dir == QChar::DirR ) {
-                            if(!(bidi.status.eor == QChar::DirR)) {
-                                // AN or EN
-                                appendRun( bidi );
-                                dir = QChar::DirR;
-                            }
-                            else
-                                bidi.eor = bidi.last;
+                        if(bidi.context->dir == QChar::DirL || bidi.status.lastStrong == QChar::DirL) {
                             appendRun( bidi );
                             dir = QChar::DirL;
+                            bidi.eor = bidi.current;
                             bidi.status.eor = QChar::DirL;
                         } else {
-                            if(bidi.status.eor == QChar::DirR) {
-                                appendRun( bidi );
-                                dir = QChar::DirL;
-                            } else {
-                                bidi.eor = bidi.current; bidi.status.eor = QChar::DirL; break;
-                            }
+                            dir = QChar::DirR; // stuff before neutrals must have been R
+                            bidi.eor = bidi.last;
+                            appendRun( bidi );
+                            dir = QChar::DirL;
+                            bidi.status.eor = QChar::DirL;
                         }
                     } else {
                         bidi.eor = bidi.current; bidi.status.eor = QChar::DirL;
@@ -911,16 +904,18 @@
                 case QChar::DirON:
                     if( !(bidi.status.eor == QChar::DirR) && !(bidi.status.eor == QChar::DirAL) ) {
                         //last stuff takes embedding dir
-                        if(bidi.context->dir == QChar::DirR || bidi.status.lastStrong == QChar::DirR) {
+                        if(bidi.context->dir == QChar::DirR || bidi.status.lastStrong == QChar::DirR
+                            || bidi.status.lastStrong == QChar::DirAL) {
                             appendRun( bidi );
                             dir = QChar::DirR;
                             bidi.eor = bidi.current;
-			    bidi.status.eor = QChar::DirR;
+                            bidi.status.eor = QChar::DirR;
                         } else {
+                            dir = QChar::DirL; // stuff before neutrals must have been L
                             bidi.eor = bidi.last;
                             appendRun( bidi );
                             dir = QChar::DirR;
-			    bidi.status.eor = QChar::DirR;
+                            bidi.status.eor = QChar::DirR;
                         }
                     } else {
                         bidi.eor = bidi.current; bidi.status.eor = QChar::DirR;
@@ -940,10 +935,7 @@
             if(!(bidi.status.lastStrong == QChar::DirAL)) {
                 // if last strong was AL change EN to AN
                 if(dir == QChar::DirON) {
-                    if(bidi.status.lastStrong == QChar::DirAL)
-                        dir = QChar::DirAN;
-                    else
-                        dir = QChar::DirL;
+                    dir = QChar::DirL;
                 }
                 switch(bidi.status.last)
                     {
@@ -1115,7 +1107,6 @@
                 break;
             case QChar::DirEN:
                 if ( bidi.status.last == QChar::DirL ) {
-                    bidi.status.last = QChar::DirL;
                     break;
                 }
                 // fall through


I deeply apologize for the erroneous submission.

-- Mitz Pettel

[prev in list] [next in list] [prev in thread] [next in thread]