95721 - Add GTK+/Xlib support for for getting actual string height (GetDimensions)

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[kill this account]

bug 74189 is adding code to actually measure a strings height.

This would help text not overlap when the glyph fill in code ends up picking
a larger glyph.

nsFontMetricsGTK.cpp needs to be modified to add similar code to the new
code added to nsFontMetricsWin.cpp.

Comment 1

[rbs]

s/bug 74189/bug 74186/

Comment 2

[rbs]

Attachment: FYI - patch to hook GetDimensions()

Comment 3

[kill this account]

How complete is attachment 46317 [details]?
ie: if it were added to the tree would it solve this bug?

Comment 4

[rbs]

The complete patches to apply are those that I recently attached to bug 74186:
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=46323
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=46324
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=46325

Comment 5

[kill this account]

So you have done all the work and are looking for a review?

Comment 6

[rbs]

Yes, nearly all. I am looking for testing and review on your system (with
possible corrections that may ensue on the patch that affected the GTK part).

[Also, on a related matter, I would appreciate if you emulate what I did for
the support of the list of multiple fallback fonts in nsFontMetricsWin so that,
at least, both platforms will be in sync. (If you apply the patch, you will see
the GenericFontEnumCallback stuff more clearly).]

Comment 7

[rbs]

In particular the handling of the min-size in GfxGTK wouldn't be necessary
anymore since it is now handled from within the Style System. This can be tested
for example by trying a large value of the min-size and seeing how the sidebar
contents are enlarged while the tabs themselves remain unchanged.

Comment 8

[rbs]

Cc:ing [e-mail] for the Xlib corner.
nsIRenderingContext::GetDimensions() is needed on all flavors of GFX.

Comment 9

[rbs]

+nsRenderingContextGTK::DrawString2(const PRUnichar* aString, PRUint32 aLength,
                                   nscoord aX, nscoord aY,
                                   PRInt32 aFontID,
                                   const nscoord* aSpacing)
@@ -1458,11 +1550,6 @@
 
     nscoord x = aX;
     nscoord y;

Sould be...
     nscoord y = aY;

Comment 10

[Roland Mainz]

Uhm... OK... TMP - Too Many Patches... :-)

Which attachments are required to patch a GTK+-toolkit Zilla ?

Comment 11

[rbs]

Given that there has been a lot of checkins since my earlier diffs, the patches 
are likely out of sync now. Let me update my build to the tip, and I will 
provide other patches shortly -- that's why there are TMP... and the fact that 
the added functionality is much involved and requires several iterations to get 
things right doesn't help in that respect either.

Comment 12

[rbs]

Attachment: FYI - patch in GTK v2 (not for application)

Comment 13

[rbs]

Attachment: FYI - patch in Xlib - v1 (not for application)

Comment 14

[rbs]

I am going to attach the full "tip-ready" patches for application on bug 74186.

Comment 15

[rbs]

gisburn, I just attached the latest patches to bug 74186, inlcuding a tentative 
port to Xlib. I am eager to hear the outcome of your testing on Xlib (and 
possibly GTK?). The needed patches are:

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=46957
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=46958
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=46961

Comment 16

[rbs]

Typo in xlib (from copy-pasting...)
 NS_IMETHODIMP
+nsRenderingContextGTK::DrawString(const char *aString, PRUint32 aLength,

Of course, the namespace should be nsRenderingContextXlib.

Comment 17

[rbs]

Guys, how are things coming along on this one?

Comment 18

[Roland Mainz]

I need two or three days until I have a fresh&clean build to test this one ...
is that acceptable ? Or should I swing the whips on myself ? :-)

Comment 19

[rbs]

By then the pacthes would have been long outdated and I will have to attach even 
more patches :( My suggested way of testing is actually to duplicate your trees 
(if you don't have many trees already) and then try the stuff into one.

Comment 20

[rbs]

Attachment: FYI - little cleanup to the proposed pacthes

Comment 21

[rbs]

Attachment: Correct diff (ignore the previous one...)

Comment 22

[kill this account]

rbs: now that bug 99010 is checked in should we close this?

Comment 23

[rbs]

Yes, resolving as FIXED to to get it off the radars.

Comment 24

[Andreas Becker]

Switching qa contact to ylong for now.  Yuying, please check with ftang if you
need a test case for verifying this.

Comment 25

[kill this account]

Attachment: html test case

Comment 26

[kill this account]

if one sets the min-size for japanese in unix.js to 6

  pref("font.min-size.variable.ja", 6);
  pref("font.min-size.fixed.ja", 6);

and displays the test case, attachment 51742 [details],

  without this fix the test case will have glyphs touching

  with this fix the test case will not have glyphs touching

Comment 27

[Yuying Long]

Thanks, Brian!

The build with bug 99010 fix checked in is not overlapping any more but still
toughed, by looking into Brian's test case.

Did we really resolved this problem?

Comment 28

[Yuying Long]

Attachment: Before bug 99010 checked in.

Comment 29

[Yuying Long]

Attachment: After bug 99010 checked in

Comment 30

[rbs]

Do you also have another screenshot for another application/editor that is
rendering the same text?

Comment 31

[Yuying Long]

Attachment: same test case with N4.7 on same system

Comment 32

[rbs]

Interesting comparisons... Either the fonts are not telling the full story or it
is yet again the dilemma of whether the external leading (or some approximation
thereof) should be included in the metrics or not. 

Cc:ing some people with whom we discussed this in bug 76097.

Now that each nsFont[Win|GTK] keeps its own metrics, it might be possible to
special case certain Japanese fonts without regressing ASCII documents at all,
e.g,  nsFontGTK::mMaxAscent can be tweaked a little bit in the case where it
belongs to a Japanese font and it is known that the direct metrics returned by
the font are not satisfactory. (From there on, the supporting infrastructure of
GetTextDemensions() will cater for the rest whilst the nsIFontMetrics API will
continue to be associated to the existing ASCII metrics.)

Comment 33

[rbs]

BTW, I wonder why N6 glyphs are bigger than in Nav4.x (using '1' as reference
enables to see marked differences) -- shouldn't all the glyphs be identical to
begin with.

Comment 34

[kill this account]

4.7 may be selecting a Japanese font that has a smaller size.

Comment 35

[Yuying Long]

I'm re-openning it for now, please mark it as resolve when we have better
solution or if we can get good envidence.

Comment 36

[rbs]

Let's move the ongoing data to a separate bug given that the primary issue of
this bug was to hook GetTextDimensions() to GTK/Xlib and it was resolved. By
design, the metrics returned come from the fonts themselves as originally
intended.

It appear that adding an extra leading may be needed, and so it is worth having
a separate, focussed bug, where the amount of leading and other details could be
agreed upon.

Comment 37

[Yuying Long]

Mark as verified according rbs's last comments.

Maybe after bug 96535 fixed, we don't even need adding an extra leading.