Closed Bug 207438 Opened 21 years ago Closed 21 years ago

Glyph-detection code for iso10646-1 fonts should conform more to the Xlib specs

Categories

(Core Graveyard :: GFX: Xlib, defect)

Product:
Core Graveyard
Component:
GFX: Xlib
Platform:
All
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: roland.mainz, Assigned: roland.mainz)

Details

Attachments

(1 file)

smontagu's patch from bug 192088 (same as attachment 123952)
3.52 KB, patch
roland.mainz
: review+
rbs
: superreview+
asa
: approval1.4.1-
Details | Diff | Splinter Review 
[Follow-up to the discussion in bug 192088 ("zwnj show up as [zwnj]")]:
Glyph-detection code for iso10646-1 fonts should conform more to the Xlib specs.
The current code only looks at the |ascent| and |descent| fields which may cause
that some valid glyphs are missed.
Status: NEW → ASSIGNED
I tried testing this with some extra printf statements, and found a number of
fonts in my system with glyphs for whitespace characters which had width but no
ascent or descent.

Here is some output from the test:

GetMapFor10646Font -misc-console-medium-r-normal--16-160-72-72-c-*-iso10646-1
rejected valid glyph for U+0020
rejected valid glyph for U+00a0
GetMapFor10646Font -misc-fixed-medium-r-normal--15-140-75-75-c-*-iso10646-1
rejected valid glyph for U+0020
rejected valid glyph for U+00a0
rejected valid glyph for U+2000
rejected valid glyph for U+2001
rejected valid glyph for U+2002
rejected valid glyph for U+2003
rejected valid glyph for U+2004
rejected valid glyph for U+2005
rejected valid glyph for U+2006
rejected valid glyph for U+2007
rejected valid glyph for U+2008
rejected valid glyph for U+2009
rejected valid glyph for U+200a
rejected valid glyph for U+202f
rejected valid glyph for U+2800
GetMapFor10646Font -mutt-clearlyu-medium-r-normal--17-120-100-100-p-*-iso10646-1
rejected valid glyph for U+0020
rejected valid glyph for U+00a0
rejected valid glyph for U+1360
rejected valid glyph for U+2800
rejected valid glyph for U+3164
rejected valid glyph for U+f200
GetMapFor10646Font -adobe-utopia-regular-r-normal--15-140-75-75-p-*-iso10646-1
rejected valid glyph for U+0020
rejected valid glyph for U+00a0

rbs:
Can you sr= the patch, please ?
Comment 3 seems to suggest that there are some inaccuracies with the patch.
I know that a whitespace with a non-zero height will be picked in some other
font, so it is not really critical. But are you aware of a better test to avoid
these false positives?
Let me clarify the circumstances in which the "Rejected valid glyph" is
displayed. These are glyphs which are not picked up by the existing condition,
and would be picked up with the patch.
Or are you saying that we don't want to use glyphs which have width but zero
height so the original code is better?
> we don't want to use glyphs which have width but zero
> height so the original code is better?

No. I am okay with any genuine whitespace glyph with a non-zero width. The
height is not critical for the whitespace. If it has a non-zero height, it is
fine. If it has a zero-height, it is fine too. It is whistepace.

My understanding of your comment was that the patch now also excludes any
genuine whitespace glyph with zero height.

The implication of rejecting a glyph is that it slows down the lookup -- since
another glyph has to be searched. And so I was wondering if there wasn't another
test to avoid rejecting the whitespace. But I am okay to go along with the patch
if there isn't another way. Just making sure that there is no oversight.
Here is a diff for the code referred to in comment 3:

       XCharStruct* bounds = &aFont->per_char[offset + cell];
       if (bounds->ascent || bounds->descent) {
         ccmapObj.SetChar((row << 8) | cell);
+      } else if (bounds->lbearing ||
+                 bounds->rbearing ||
+                 bounds->width ||
+                 bounds->attributes) {
+        printf("rejected valid glyph for U+%04x\n",
+               (row << 8) | cell);
       }
     }
   }

So what the patch is doing is preventing these glyphs from being rejected.
I just re-read what you said in bug 192088 comment 29, the patch isn't making
any difference for the problems at hand. So what does this bug really buy us now?
Comment on attachment 124407 [details] [diff] [review]
smontagu's patch from bug 192088 (same as attachment 123952 [details] [diff] [review])

Taking this off my list since it is not pursuing anymore.
Attachment #124407 - Flags: superreview?(rbs) → superreview-
rbs wrote:
> So what does this bug really buy us now?

There are at least two benefits:
- We would stop looking for whitespace chars in other fonts (and therefore stop
loading all the *-iso10646-1 fonts, search them for the requested glyph and
finally end-up with "no glyph found")
- We would be slightly closer to the Xlib spec
rbs wrote:
> (From update of attachment 124407 [details] [diff] [review])
> Taking this off my list since it is not pursuing anymore.

huh?!
Comment on attachment 124407 [details] [diff] [review]
smontagu's patch from bug 192088 (same as attachment 123952 [details] [diff] [review])

rbs:
The patch can't hurt (at least not from the "perf" nor from the "correctness"
point of view... :) ...
Attachment #124407 - Flags: superreview- → superreview?(rbs)
Comment on attachment 124407 [details] [diff] [review]
smontagu's patch from bug 192088 (same as attachment 123952 [details] [diff] [review])

sr=rbs
Attachment #124407 - Flags: superreview?(rbs) → superreview+
rbs wrote:
> (From update of attachment 124407 [details] [diff] [review])
> sr=rbs

Thanks! :)
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment on attachment 124407 [details] [diff] [review]
smontagu's patch from bug 192088 (same as attachment 123952 [details] [diff] [review])

Requesting a= for the 1.4_branch...
Attachment #124407 - Flags: approval1.4?
Comment on attachment 124407 [details] [diff] [review]
smontagu's patch from bug 192088 (same as attachment 123952 [details] [diff] [review])

moving approval request forward.
Attachment #124407 - Flags: approval1.4? → approval1.4.x?
Has there been any fallout from this on the trunk? Has anyone looked? What
impact does this have for the user?
Roland? rbs? Any response to my question above?
Seems to me that it is not really worth bothering to chase this on the branch.
Comment on attachment 124407 [details] [diff] [review]
smontagu's patch from bug 192088 (same as attachment 123952 [details] [diff] [review])

This is not going to make 1.4.1.  Please re-request aproval after 1.4.1 ships
if you'd like to get this in for 1.4.2.
Attachment #124407 - Flags: approval1.4.x? → approval1.4.x-
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: