Closed
Bug 266551
Opened 20 years ago
Closed 20 years ago
[Mac] BiDi caret ignores bidi keyboard layouts (LTR marker instead of RTL)
Categories
(Core Graveyard :: Widget: Mac, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.8alpha6
People
(Reporter: asaf, Assigned: asaf)
References
(Blocks 1 open bug)
Details
(Keywords: rtl)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
stay tuned :)
Assignee | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
QA Contact: jrgmorrison → prognathous
Target Milestone: --- → mozilla1.8alpha6
Assignee | ||
Updated•20 years ago
|
Attachment #163955 -
Flags: superreview?(bzbarsky)
Attachment #163955 -
Flags: review?(jhpedemonte)
![]() |
||
Comment 2•20 years ago
|
||
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•20 years ago
|
||
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(¤tKeyboard); + + if (err == noErr) { + err = fpKLGetKeyboardLayoutProperty(currentKeyboard, kKLIdentifier, + ¤tKeyboardResID); + 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 5•20 years ago
|
||
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•20 years ago
|
||
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+
Assignee | ||
Comment 7•20 years ago
|
||
thanks for reviews.
Attachment #163955 -
Attachment is obsolete: true
Assignee | ||
Comment 8•20 years ago
|
||
err, wrong diff.
Attachment #167216 -
Attachment is obsolete: true
Comment 9•20 years ago
|
||
Patch checked in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 10•20 years ago
|
||
Verified with 1.8a6/20041215. Thanks Asaf! Prog.
Status: RESOLVED → VERIFIED
Blocks: 276367
Depends on: 298430
Assignee | ||
Updated•20 years ago
|
Component: XP Toolkit/Widgets → Widget: Mac
Comment 11•17 years ago
|
||
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
You need to log in
before you can comment on or make changes to this bug.
Description
•