[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.

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

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.

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... :) ...

Comment on attachment 124407 [details] [diff] [review] smontagu's patch from bug 192088 (same as attachment 123952 [details] [diff] [review]) sr=rbs

rbs wrote: > (From update of attachment 124407 [details] [diff] [review]) > sr=rbs Thanks! :)

Fix checked in.

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...

Comment on attachment 124407 [details] [diff] [review] smontagu's patch from bug 192088 (same as attachment 123952 [details] [diff] [review]) moving approval request forward.

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.