Closed
Bug 207438
Opened 22 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)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: roland.mainz, Assigned: roland.mainz)
Details
Attachments
(1 file)
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.
Assignee | ||
Comment 1•22 years ago
|
||
Assignee | ||
Updated•22 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•22 years ago
|
||
Comment on attachment 124407 [details] [diff] [review]
smontagu's patch from bug 192088 (same as attachment 123952 [details] [diff] [review])
r=roland.mainz@informatik.med.uni-giessen.de
Attachment #124407 -
Flags: superreview?(rbs)
Attachment #124407 -
Flags: review+
Comment 3•22 years ago
|
||
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
Assignee | ||
Comment 4•21 years ago
|
||
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?
Comment 6•21 years ago
|
||
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.
Comment 7•21 years ago
|
||
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.
Comment 9•21 years ago
|
||
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.
Comment 10•21 years ago
|
||
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 11•21 years ago
|
||
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-
Assignee | ||
Comment 12•21 years ago
|
||
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
Assignee | ||
Comment 13•21 years ago
|
||
rbs wrote:
> (From update of attachment 124407 [details] [diff] [review])
> Taking this off my list since it is not pursuing anymore.
huh?!
Assignee | ||
Comment 14•21 years ago
|
||
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 15•21 years ago
|
||
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+
Assignee | ||
Comment 16•21 years ago
|
||
rbs wrote:
> (From update of attachment 124407 [details] [diff] [review])
> sr=rbs
Thanks! :)
Comment 17•21 years ago
|
||
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 18•21 years ago
|
||
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 19•21 years ago
|
||
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?
Comment 20•21 years ago
|
||
Has there been any fallout from this on the trunk? Has anyone looked? What
impact does this have for the user?
Comment 21•21 years ago
|
||
Roland? rbs? Any response to my question above?
Comment 22•21 years ago
|
||
Seems to me that it is not really worth bothering to chase this on the branch.
Comment 23•21 years ago
|
||
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-
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
•