Closed Bug 389520 Opened 14 years ago Closed 14 years ago

Complex Text Line Breaking with ATSUI/Carbon for Mac

Categories

(Core :: Internationalization, enhancement)

PowerPC
macOS
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: thep, Assigned: smontagu)

Details

(Keywords: intl)

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en; rv:1.8.1.5) Gecko/20070712 (Debian-1.8.1.5-1thai1) Epiphany/2.18
Build Identifier: 

A similar solution to Bug #336969 is needed to have proper line breaking support for complex text on Mac.

Currently, some people have been working together at a Scratchpad [1] and a solution based on Carbon API has been proposed. It should be time to bring the patch here for discussion.

  [1] http://scratchpad.wikia.com/wiki/Firefox_Thai

Reproducible: Always
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: intl
Version: unspecified → Trunk
It's Bug #336959 not Bug #336969.
Attachment #273761 - Flags: superreview?(roc)
Attachment #273761 - Flags: review?(roc)
Similar comments apply to the ones I just gave about the Uniscribe patch. In particular,

+  for(UniCharArrayOffset i = 0; i < aLength; ++i)
+  {
+    aBreakBefore[i] = PR_FALSE;
+  }

Use memset

+  if(status == noErr) 
+  {

Fix space after if. Also there's less indenting if you write it as
  if (status != noErr)
    return;

+    for(position = 0; position < aLength;)

Declare 'position' here.

+                    (const UniChar *)aText, 

Is this needed? I suspect we might compile without this. If it is needed, use reinterpret_cast.

+      if(status != noErr)
+      {
+        break;
+      }

Drop the braces
Attachment #273761 - Attachment is obsolete: true
Attachment #274411 - Flags: superreview?(roc)
Attachment #274411 - Flags: review?(roc)
Attachment #273761 - Flags: superreview?(roc)
Attachment #273761 - Flags: review?(roc)
+ifeq  ($(MOZ_WIDGET_TOOLKIT),windows)
+CPPSRCS		+= \
+		nsUniscribeBreaker.cpp \
+		$(NULL)
+else

Now you've dragged this in from your other patch. Please keep them separate.

+  OSStatus status = noErr;

Declare this where you first set it. You don't need this line.

+  if(status != noErr)

Space after "if".

+  UniCharArrayOffset offset;

Make this local to the loop body.

+  for(UniCharArrayOffset position = 0; position < aLength;)

Space after "for".

+  for(UniCharArrayOffset position = 0; position < aLength;)
+  {

Brace on the line with the "for".

+    if(status != noErr)

Space after "if".
roc: Thank you for your comments. :-)
Attachment #274411 - Attachment is obsolete: true
Attachment #274431 - Flags: superreview?(roc)
Attachment #274431 - Flags: review?(roc)
Attachment #274411 - Flags: superreview?(roc)
Attachment #274411 - Flags: review?(roc)
Comment on attachment 274431 [details] [diff] [review]
A patch for using Carbon to find line break position

great!
Attachment #274431 - Flags: superreview?(roc)
Attachment #274431 - Flags: superreview+
Attachment #274431 - Flags: review?(roc)
Attachment #274431 - Flags: review+
the lastest patch is work fine on my mac box.
Attachment #274431 - Flags: approval1.9? → approval1.9+
Note that patches don't get checked in of their own accord.  In the future, please mark bugs with the checkin-needed keyword so patches don't get lost.
Keywords: checkin-needed
This patch is broken by the checked-in for bug 390048, please update the Makefile.in.
A patch for using Carbon to find line break position as Masayuki's suggestion.
Attachment #276819 - Flags: approval1.9?
Oops, i did something wrong?
Comment on attachment 276819 [details] [diff] [review]
 A patch for using Carbon to find line break position   

a1.9 should be carried over from previous patch, because this is not changing the actual behavior/logic.
Attachment #276819 - Flags: approval1.9?
Attachment #276819 - Flags: approval1.9?
(In reply to comment #13)
> (From update of attachment 276819 [details] [diff] [review])
> a1.9 should be carried over from previous patch, because this is not changing
> the actual behavior/logic.
> 
I see, thanks for your suggestion.
Comment on attachment 276819 [details] [diff] [review]
 A patch for using Carbon to find line break position   

I think you missed the point... you don't need to re-request approval because there are no changes, so just don't set the flag.  (There are enough approval requests in the queue already.)
Attachment #276819 - Flags: approval1.9?
checked-in, thanks!
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Verified with hourly build 2007081523 on Mac OS X 10.4
Please close this bug.
You need to log in before you can comment on or make changes to this bug.