157394 - *Gdk-WARNING* when creating input fields in non-en_US locales

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Lina Kemmel]

When creating input fields in non-US (e.g. iw_IL) locale, "Gdk-WARNING **: can 
not create input context with specified input style" error message is displayed 
in the shell window.
As a result, X Input Method doesn't start, and mozilla doesn't accept 
international input.

Comment 1

[Lina Kemmel]

Attachment: proposed patch

Degrade the IM style, so that input context can be created.

Comment 2

[Roy Yokoyama]

Lina:  is this bug Bidi specific?  (i see #ifdef IBMBIDI)
Simon:  is IBMBIDI on by default and are you familiar with AIX input method?

Comment 3

[Roy Yokoyama]

>Lina:  is this bug Bidi specific?  (i see #ifdef IBMBIDI)
Sorry, I should have checked for other bug before making a comment here.
I see bug 157397 which indicates it is not a bidi specific.

Comment 4

[Lina Kemmel]

Roy: Sorry about the late reply and thanks for the clarification. Of course, I 
had to mention explicitly that this bug is not bidi specific..

Comment 5

[Lina Kemmel]

Attachment: Another fix for the bug

The previous fix set IM editing style named "root window", which supposed
creating a separate window for pre-edited text.

The current fix suggests querying the system for the styles supported by the
current IM server and then choosing one of them (actually it falls on
"over-the-spot" style).
Another changes in this fix:
  - remove IBMBIDI ifdef;
  - change AIX4_3 to _AIX ifdef.

Comment 6

[Roy Yokoyama]

Lina: do you want us to review your code or is not ready yet?

Comment 7

[Philip K. Warren]

Lina is on vacation for the next few weeks, but I believe the latest fix is 
ready to be reviewed. Thanks.

Comment 8

[Roy Yokoyama]

Comment on attachment 92570 [details] [diff] [review]
Another fix for the bug

/r=yokoyama
I am not very familiar w/ AIX platform; but the
code looks clean.  I will 
give /r here; but we should ask kin or Pinkerton for /sr.

lina/philip: do you have a checkin privilage? If you don't, then I can checkin
for you.

Comment 9

[Philip K. Warren]

Neither of us have checkin privileges at this time. I have asked for 
privileges, but am still waiting for approval. I would appreciate you checking 
this fix in for us.

Comment 10

[kinmoz]

Comment on attachment 92570 [details] [diff] [review]
Another fix for the bug

Isn't this:

    +  if ((curr_style & best_style) == curr_style) {

the same as:

    +  if (best_style == curr_style) {

or were you trying to see if all of best_style was supported in a give
curr_style:

    +  if ((curr_style & best_style) == best_style) {

The rest of the code looks ok, though I really dislike the fact that we are
leaving unreachable code active, doesn't the AIX compiler complain about this?

    +  return gIMStyle;
    +#endif // _AIX
    +
       nsCOMPtr<nsIPref> prefs(do_GetService(kPrefServiceCID, &rv));
       if (NS_SUCCEEDED(rv) && (prefs)) {
	 char *input_style;

Comment 11

[Jim Dunn]

removing "branchOEM+", till patch gets r=/sr= for trunk

Comment 12

[Philip K. Warren]

    +  if ((curr_style & best_style) == curr_style) {

This line is not the same as either of your suggestions. I believe the above
line simply wants to ensure that the current style doesn't contain any styles
which are not allowed. (best_style is a combination of all allowed styles)

Regarding the following line:

    +  return gIMStyle;
    +#endif // _AIX
    +

The AIX compiler does not complain with leaving unreachable code. HP-UX has a
similar workaround above which returns early. I could submit a new patch which
add ifdef/elif/else sections to separate out the platform specific code if you
would find that to be an improvement over the current patch.

Comment 13

[kinmoz]

Comment on attachment 92570 [details] [diff] [review]
Another fix for the bug

>This line is not the same as either of your suggestions. I believe the above
>line simply wants to ensure that the current style doesn't contain any styles
>which are not allowed. (best_style is a combination of all allowed styles)


Ok, thanks for the clarification. Add a comment before the |if| check, that
states what you said above, and you got an sr=[e-mail].

As for the unreachable code, it's not a big deal, hopefully your compiler will
optimize out the code after the return. I was just expressing my dislike of
leaving code in that will never execute.

Comment 14

[Philip K. Warren]

Attachment: Same patch - added comment

Comment 15

[Jim Dunn]

Roy with your's and kin's reviews, do you plan
to check this into the trunk?  Or do you want me
too?

Comment 16

[kinmoz]

Comment on attachment 96183 [details] [diff] [review]
Same patch - added comment

I think the comment that was added is missing the keyword "only":

+	   // Ensure that curr_style contains allowed styles

Comment 17

[Philip K. Warren]

You're right. The comment should read "Ensure that curr_style only contains 
allowed styles". This change should be made when the fix is checked in.

Comment 18

[Jim Dunn]

reassigning to me

Comment 19

[Jim Dunn]

fix checked into trunk

Comment 20

[Rui Xu]

Lina or someone else: 
Could you please verify with the latest build to see if it is still 
reproducible? since we don't have AIX here. Thanks!

Comment 21

[Mary Hoetzel]

Lina is back on Monday ...
I do not see the Gdk Warning anymore with the latest fix. It works fine for me.

Comment 22

[Rui Xu]

Mark bug as VERIFIED. Please reopen if it is still reproducible.