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

VERIFIED FIXED in mozilla1.8alpha6

Status

Core Graveyard
Widget: Mac
--
major
VERIFIED FIXED
13 years ago
8 years ago

People

(Reporter: mano, Assigned: mano)

Tracking

(Blocks: 1 bug, {rtl})

Trunk
mozilla1.8alpha6
PowerPC
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

stay tuned :)
Status: NEW → ASSIGNED
QA Contact: jrgmorrison → prognathous
Target Milestone: --- → mozilla1.8alpha6

Updated

13 years ago
Blocks: 265070
Created attachment 163955 [details] [diff] [review]
patch


It's amazing how broken is the old good TSM API.
Attachment #163955 - Flags: superreview?(bzbarsky)
Attachment #163955 - Flags: review?(jhpedemonte)
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 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.
Attachment #163955 - Flags: review?(jhpedemonte) → review+
Comment on attachment 163955 [details] [diff] [review]
patch


bz?
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 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... :(
Attachment #163955 - Flags: superreview?(bzbarsky) → superreview+
Created attachment 167216 [details] [diff] [review]
patch for checkin


thanks for reviews.
Attachment #163955 - Attachment is obsolete: true
Created attachment 167246 [details] [diff] [review]
patch for checkin

err, wrong diff.
Attachment #167216 - Attachment is obsolete: true
Patch checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 10

13 years ago
Verified with 1.8a6/20041215.

Thanks Asaf!

Prog.
Status: RESOLVED → VERIFIED
Blocks: 276367
No longer blocks: 276367
Depends on: 298430
Component: XP Toolkit/Widgets → Widget: Mac

Comment 11

10 years ago
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl

Updated

8 years ago
Component: Widget: Mac → Widget: Mac
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.