266551 - [Mac] BiDi caret ignores bidi keyboard layouts (LTR marker instead of RTL)

Status: VERIFIED FIXED

(Core Graveyard :: Widget: Mac, defect)

Description

[Asaf Romano (gone)]

stay tuned :)

Comment 1

[Asaf Romano (gone)]

Attachment: patch

It's amazing how broken is the old good TSM API.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 163955 [details] [diff] [review]
patch

It will take me probably at least a week to get to this review....  I'm a
little swamped with other reviews right now.

Comment 3

[jhp (no longer active)]

Comment on attachment 163955 [details] [diff] [review]
patch

Just some nits:

+typedef OSStatus (*fpKLGetKeyboardLayoutProperty_type) (KeyboardLayoutRef,
+    KeyboardLayoutPropertyTag, void**);

I would write it like this:
+typedef OSStatus (*fpKLGetKeyboardLayoutProperty_type)
+			 (KeyboardLayoutRef, KeyboardLayoutPropertyTag,
void**);

Try to keep the lines in the patch to 80 characters wide or less.

I would write the second code block in IsLangRTL with one 'return', like this:
+  nsresult rv = NS_OK;
+  if (fpKLGetCurrentKeyboardLayout) {
+    OSStatus err;
+    KeyboardLayoutRef currentKeyboard;
+    const void* currentKeyboardResID;
+  
+    err = fpKLGetCurrentKeyboardLayout(&currentKeyboard);
+
+    if (err == noErr) {
+      err = fpKLGetKeyboardLayoutProperty(currentKeyboard, kKLIdentifier,
+					   &currentKeyboardResID);
+      if (err == noErr) {
+	 *aIsRTL = IsRTLLangauge((SInt32)currentKeyboardResID);
+	 rv = NS_OK;
+      }
+    }
+  }
+
-   return NS_OK;
+  return rv;

Otherwise, the code looks good.

Comment 4

[Asaf Romano (gone)]

Comment on attachment 163955 [details] [diff] [review]
patch


bz?

Comment 5

[Boris Zbarsky [:bzbarsky]]

I've still got 4 reviews in my queue that were requested before this one and
this code is really not something I know well, so I would need some time to
review this....

I'll try to get to this within the next 3-4 days, but it may not happen.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 163955 [details] [diff] [review]
patch

>Index: widget/src/mac/nsBidiKeyboard.cpp
>+  // Check if the resource id is BiDi associated (Arabic, Persion, Hebrew)
>+  // (Persion is included in the Arabic range)

"Persian", not "Persion".

sr=bzbarsky. Sorry this took so long... :(

Comment 7

[Asaf Romano (gone)]

Attachment: patch for checkin

thanks for reviews.

Comment 8

[Asaf Romano (gone)]

Attachment: patch for checkin

err, wrong diff.

Comment 9

[Simon Montagu :smontagu]

Patch checked in.

Comment 10

[Prognathous]

Verified with 1.8a6/20041215.

Thanks Asaf!

Prog.

Comment 11

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).