Closed
Bug 100868
Opened 23 years ago
Closed 23 years ago
Add GfxMac support for getting actual string height (GetStringDimensions)
Categories
(Core Graveyard :: GFX, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla0.9.6
People
(Reporter: rbs, Assigned: ftang)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
pierre
:
review+
sfraser_bugs
:
superreview+
|
Details | Diff | Splinter Review |
The fix for bug 99010 only has a fix-up patch for the Mac. Therefore, the Mac
will be missing the newly added dynamic font heights -- until
nsUnicodeRenderingToolkit::GetStringDimensions() is implemented. That's the
piece that is lacking.
LXR gives the blame to ftang for nsUnicodeRenderingToolkit::GetWidth(). I think
he will not have that much trouble hooking GetStringDimensions() once/if he sets
out to do it. Some of the i18n people have become quite familiar with what is
going on too.
The code for nsUnicodeRenderingToolkit::GetStringDimensions() will be a slight
extension to nsUnicodeRenderingToolkit::GetWidth(). At the place where
GetTextSegmentWidth() is called, the maxAscent/maxDescent will also need to be
sync:ed. This means that the ascent/descent of the "inner" fonts used in
GetTextSegmentWidth() may have to be cached when these fonts are created. This
is basically the scenario that is happening on other platforms.
The tracker bug for GetStringDimensions() is bug 96609.
Assignee | ||
Comment 1•23 years ago
|
||
Not sure when will I have time to deal with this. we are really under staff now.
Especially Mac resource on I18N. I cannot do 3 people's job anymore.
Assignee | ||
Comment 2•23 years ago
|
||
rbs- do we need to do anything with DrawString2 ?
No, no special action is needed there (apart from the simple renaming which is
due in bug 102088).
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•23 years ago
|
Target Milestone: mozilla0.9.7 → mozilla0.9.6
Assignee | ||
Comment 5•23 years ago
|
||
Is there good test case I can use to valid my fix ?
I used MathML fonts for my testings -- they have widely different glyphs, but
since the Mac doesn't have MathML support, the composer problem (bug 103002)
might help. Also, you can take a string 'xyz...', where 'x', 'y', 'z', ..., are
Unicode points from different fonts to trigger font-switching and see how things
go.
Assignee | ||
Comment 7•23 years ago
|
||
Assignee | ||
Comment 8•23 years ago
|
||
Before this patch, the Indic glyph (Devanagri, etc) always got chop the top in
the composer, after the patch, it look fine.
pierre- can you r= this one ?
Assignee | ||
Comment 9•23 years ago
|
||
In additional to add the dimension, I also add the following change with the
patch:
1. fix tab/space and other formatting issue in surronding code.
2. remove unnecessary #ifdef IBMBIDI code, basically, we cannot swith back
without IBMBIDI now. So... remove it to make it cleaner
3. change NS_IMETHOD and NS_IMPL_IMETHOD to nsresult for non virtual function.
We can speed up a little bit by doing so since these places do not need vtable
at all.
Comment 10•23 years ago
|
||
Can we have a unified diff please? That one is hard to read.
Comment 11•23 years ago
|
||
Franck, to get a unified diff on the Mac, go to "Status | Advanced Diff..." and
select "Unified Diff". If you changed the identations or the text aligment,
select "Ignore White Space" too. On other platforms, enter "cvs diff -u2" or
"cvs diff -w -u2".
Assignee | ||
Comment 12•23 years ago
|
||
Assignee | ||
Comment 13•23 years ago
|
||
pierre- can you review it ? :)
Reporter | ||
Comment 14•23 years ago
|
||
Comment on attachment 54528 [details] [diff] [review]
v2 (fix some tab) and cvs diff -u with space ignored.
r=rbs
Nits:
+ if (NS_SUCCEEDED(rv))
+ rv = mUnicodeRenderingToolkit.GetTextDimensions(aString, aLength, aDimensions, aFontID);
}
>>> no return rv
+nsATSUIToolkit::GetDimensions()
>>>renamed to nsATSUIToolkit::GetTextDimensions()
Attachment #54528 -
Flags: review+
Comment 15•23 years ago
|
||
In nsRenderingContextMac::GetWidth() and GetTextDimensions(), you don't need to
check mGS->mFontMetrics because SetPortTextState() does it for you.
Rename GetFontInfo() into ::GetFontInfo().
I tested what I could but I'm no unicode expert. Do you have a testcase that
goes through a couple of fallbacks in nsUnicodeRenderingToolkit::
GetTextSegmentDimensions()?
Assignee | ||
Comment 16•23 years ago
|
||
>I tested what I could but I'm no unicode expert. Do you have a testcase
> that goes through a couple of fallbacks in nsUnicodeRenderingToolkit::
1. install Devenagri, Korean, Hewbrew and Arabic language kits from the MacOS 9
CD (optional install)
2. open composer, and type some english, select the Arabic or Devangri and type
some key. And then you selec the text. In particlar for Indic script like
Devanagri, you will see some text chop off on the top without the fix and
selection and rendering fine after the fix.
Also,
http://warp/u/ftang/utf8test/ncr.cgi?out=dncr&p=0x0&r=0xa&c=0xc
should exercise some fallback too.
rbs may have some other test page.
Assignee | ||
Comment 17•23 years ago
|
||
I will prepare a new patch to address pierre and rbs comment tomorrow. thanks
Assignee | ||
Comment 18•23 years ago
|
||
found some bug in the v2 patch. IME does not work well. Need to debug.
Assignee | ||
Comment 19•23 years ago
|
||
ok, find the problem, should call oDim.Clear() in one place.
Assignee | ||
Comment 20•23 years ago
|
||
Assignee | ||
Comment 21•23 years ago
|
||
pierre and rbs, I address your comment in the v5 of the patch. Please r= it. The
v2 have one bug which shown up on CJK text IME with composer. I forget to add a
oDim.Clear() in the beginning of one method.
Assignee | ||
Updated•23 years ago
|
Attachment #54321 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #54528 -
Attachment is obsolete: true
Assignee | ||
Comment 22•23 years ago
|
||
I also clean up some tab / space and format issue in v5. The v5 patch may not
show them since it is diff with Ignore case, but when I check in the white space
change will also get in.
Comment 23•23 years ago
|
||
Comment on attachment 55125 [details] [diff] [review]
v5 of the patch
r=pierre - good catch for oDim.Clear() in GetTextSegmentDimensions
Attachment #55125 -
Flags: review+
Assignee | ||
Comment 24•23 years ago
|
||
sfraser- can you sr= this one?
Comment 25•23 years ago
|
||
Comment on attachment 55125 [details] [diff] [review]
v5 of the patch
sr=sfraser
Attachment #55125 -
Flags: superreview+
Assignee | ||
Comment 26•23 years ago
|
||
fixed and check in.
Assignee | ||
Comment 27•23 years ago
|
||
fixed and check in.
Updated•16 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•