230235 - AIX+GTK2: Preedit string is not highlighted properly

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Philip K. Warren]

This is the same problem as was reported in Bug 199683 for GTK1. AIX returns the
feedback style XIMUnderline | XIMReverse to signify that the preedit string
should be both underlined and reversed. Currently, the GTK2 code does the following:

if (PANGO_ATTR_UNDERLINE)
  feedback = NS_TEXTRANGE_CONVERTEDTEXT
else if (PANGO_ATTR_FOREGROUND)
  feedback = NS_TEXTRANGE_SELECTEDRAWTEXT

I would like to make the GTK2 version work similarly to the GTK1 version, so
that it uses text style NS_TEXTRANGE_SELECTEDCONVERTEDTEXT if both attributes
are present.

Comment 1

[Philip K. Warren]

Attachment: Patch v1

- Adds support for preedit style XIMReverse | XIMUnderline.
- Removes duplicate code.
- Uses g_free on strings allocated with g_utf8_to_utf16 (leaking these before).

Comment 2

[Christopher Blizzard (:blizzard)]

Comment on attachment 138539 [details] [diff] [review]
Patch v1


>+        uniStr = NULL;
>+        if (aPangoAttr->start_index > 0)
>+            uniStr = g_utf8_to_utf16(aPreeditString,
>+                                     aPangoAttr->start_index,
>+                                     NULL, &uniStrLen, NULL);

Multiple lines after an if (), please use braces like below:

>+        if (uniStr) {
>+            START_OFFSET(count) = uniStrLen;
>+            g_free(uniStr);
>+        }
>+        
>+        uniStr = NULL;
>+        uniStr = g_utf8_to_utf16(aPreeditString + aPangoAttr->start_index,
>+                                 aPangoAttr->end_index - aPangoAttr->start_index,
>+                                 NULL, &uniStrLen, NULL);

Whitespace here.

>+        if (uniStr) {
>+            END_OFFSET(count) = START_OFFSET(count) + uniStrLen;
>+            SET_FEEDBACKTYPE(count, feedbackType);
>+            g_free(uniStr);
>         }
>     } while ((count < aMaxLenOfTextRange - 1) &&
>              (pango_attr_iterator_next(aFeedbackIterator)));

That's all I have from a coding standpoint but I didn't really look at it for
technical correctness.	I'm hoping katakai can do that for me.

Comment 3

[Philip K. Warren]

I will add the braces to the final patch, however I don't understand what
whitespace error you want fixed. Can you elaborate?

Comment 4

[Masaki Katakai]

Comment on attachment 138539 [details] [diff] [review]
Patch v1

>+        PRUint32 feedbackType;
>+        // XIMReverse | XIMUnderline
>+        if (aPangoAttrUnderline && aPangoAttrReverse) {
>+            feedbackType = NS_TEXTRANGE_SELECTEDCONVERTEDTEXT;
>+            // Use the feedback attribute with the smaller end_index
>+            aPangoAttr = aPangoAttrUnderline;

>+            if (aPangoAttrUnderline->end_index > aPangoAttrReverse->end_index)
>+                aPangoAttr = aPangoAttrReverse;

Could you explain the purpose of this code?

>+        }

Comment 5

[Philip K. Warren]

The purpose of the code which chooses the smaller end_index is to address the
issue of when some of the preedit string should be highlighted while some of the
preedit string should be underlined. On AIX, when you hit the 'convert' key to
change to kanji, and then hit the 'non-convert' key, sometimes the preedit
string is shown with certain portions highlighted+underlined and other portions
underlined. For example, look at the following (with a custom GTK build which
outputs XIM debug information):

XIMPreeditDraw
(
        caret=0
        chg_first=0
        chg_length=2
        text = {
                length=3
                feedback[0]=0x3 (XIMReverse | XIMUnderline)
                feedback[1]=0x2 (XIMUnderline)
                feedback[2]=0x2 (XIMUnderline)
                encoding_is_wchar=0
                string.multi_byte[0]=164
                string.multi_byte[1]=171
                string.multi_byte[2]=164
        };
)

This is the output of the call_data pointer which is passed to the
PreeditDrawCallback. It says that there should be a preedit string of three
characters, with the first character underlined and highlighted, and the last
two characters only underlined. This gets passed through pango and ends up
looking like this in the IM_set_text_range loop in Mozilla:

1st iteration:
aPangoAttrUnderline: 0x214752c8, start_index: 0, end_index: 9
aPangoAttrReverse: 0x214a3ec8, start_index: 0, end_index: 3

2nd iteration:
aPangoAttrUnderline: 0x214752c8, start_index: 0, end_index: 9
aPangoAttrReverse: NULL

3rd iteration:
aPangoAttrUnderline: NULL
aPangoAttrReverse: NULL

So to get the correct output in the example above, we need to choose the
attribute with the smaller end_index. If we don't, the entire string will be
shown in the style XIMUnderline|XIMReverse.

I hope this makes sense. I am not very familiar with pango, so I'm sure there is
probably a better method than my current one. Maybe pango_attr_iterator_range
would be good to use here instead?

Comment 6

[Philip K. Warren]

Attachment: Patch v2

This seems like a better solution. I have switched to using
pango_attr_iterator_range instead of using each attribute's start_index and
end_index.

Comment 7

[Philip K. Warren]

Just thought I'd add some additional information. In my example above, this was
the output of the pango attributes in the IM_set_text_range loop:

1st iteration:
aPangoAttrUnderline: 0x214752c8, start_index: 0, end_index: 9
aPangoAttrReverse: 0x214a3ec8, start_index: 0, end_index: 3

2nd iteration:
aPangoAttrUnderline: 0x214752c8, start_index: 0, end_index: 9
aPangoAttrReverse: NULL

3rd iteration:
aPangoAttrUnderline: NULL
aPangoAttrReverse: NULL

So the first patch would do the following:
- Use style NS_TEXTRANGE_SELECTEDCONVERTEDTEXT from index 0-3.
- Use style NS_TEXTRANGE_CONVERTEDTEXT from index 0-9.

Somehow Mozilla did the right thing and used the style 
NS_TEXTRANGE_SELECTEDCONVERTEDTEXT for index 0-3, and NS_TEXTRANGE_CONVERTEDTEXT
for index 3-9. With the change to use pango_attr_iterator_range, I get the
following in the IM_set_text_range loop:

1st iteration:
pango_attr_iterator_range returning start: 0, end: 3

2nd iteration:
pango_attr_iterator_range returning start: 3, end: 9

3rd iteration:
pango_attr_iterator_range not called because both attributes are NULL.

So it seems that the second patch is doing the right thing - using style
NS_TEXTRANGE_SELECTEDCONVERTEDTEXT for index 0-3 and NS_TEXTRANGE_CONVERTEDTEXT
for index 3-9.

Comment 8

[Masaki Katakai]

Comment on attachment 138634 [details] [diff] [review]
Patch v2

OK for me.
Also verified working properly with Solaris ATOK, Linux ATOK and kinput2.

Comment 9

[Philip K. Warren]

Fixed. Thanks a lot for the reviews and testing!

Checking in nsWindow.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.95; previous revision: 1.94
done