Created attachment 15631 [details] [review]
Patch against pango-1.2.1 that kludges this bug

Tor - could you look at this?

My initial thought is that the patch could well be applied, even if 
kludgy. Has the original poster noticed any unwanted side-effects?

One thing does appear to be different with using oblique fonts instead
of a real italic font.  With real italic, all the glyphs fit inside
their boxes, even the italic ones.  With oblique, some glyphs may
extend slightly outside their boxes on the right.  This is not really
a pango problem.  Having an oblique font that's very slightly messy is
better than nothing at all, at least for me.

I also wonder if there's something wrong with only generating a single
FW_BOLD font, not the several weights that may be available.  I'm not
 even sure if win32 can generate any other weights using overstrike,
and it's unlikely that anyone will be using them anyway.

I guess there are two questions here:

 A) Should Pango use artificial bold/oblique if the programmer
    asks for "Tahoma Italic"
 B) Should Pango _list_ these fonts when asked.
    (pango_font_family_list_faces())

My feeling is yes for A, no for B. The font selector dialog
shouldn't give the option for "Tahoma Italic" if the font doesn't
actually exist. But probably a better guide than my feeling
is what Windows apps typically do. People using GTK+ apps
on Windows should see the same set of fonts as in other Windows
apps.

As for the question of fonts extending outside their "box",
that actually can occur for real italic as well. There are
two boxes:

 logical rect - spacing to adjacent elements
 ink rect - extents of pixels drawn onto the screen

That will be different in this case of highly sloped elements.

The system of GTK+/Pango doesn't always handle the case of 
fonts extending past the boundaries very well ... Pango probably
needs the idea of "italic correction" - a per-font amount of
space to leave after italic text - to handle this well.

Based on a quick look around WinXP, font selectors provide a list of
font families and, optionally, seperate bold and italic checkbuttons.
 Some applications (e.g. Internet Explorer) do not provide bold and
italic options as these would not make sense.  Where bold and italic
are provided (e.g. setting the font used on window title bars etc.)
the bold and italic buttons always work, using the real fonts or
generated fonts transparently.  Also, the list of familiy names is
often filtered to exclude symbol fonts or to include only
serif/sans/monospace fonts.

To support this, pango needs to provide a list of font family names,
what 'type' of font each one is, and to supply bold, italic, and
bold-italic fonts for each family.

Well, that doesn't really answer the question since the
font selector lists styles explicitely instead
of providing check buttons (the Windows "4 styles" approach,
though simple, doesn't reflect real world fonts well.)

But I think my suggestion of A) but not B) above should
be OK for most uses.

Comments From Tim Evans 2003-04-12 00:15
> I also wonder if there's something wrong with only
> generating a single FW_BOLD font, not the several
> weights that may be available.  I'm not even sure
> if win32 can generate any other weights using
> overstrike, and it's unlikely that anyone will
> be using them anyway.

Win32 certainly allows you to specify a wide spectrum of weights for
your font. What I have seen in practice is that most TTFs are only
designed to generate two weights, the normal and bold that are
accessible from Windows apps.

Tor, I'll let you make the call on whether we want to go
with the patch above (always list all four weights) 
vs. a different change so that we only synthesize the
fonts on request, but don't list them.

The second approach sounds better to me, but I don't
have a good intuition for the right UI for font handling
on Windows.

It looks like the changes to add "simulate but not list" are
more than a few lines, so punting until after 1.4.0.

Created attachment 32466 [details] [review]
Updated kludge for pango-1.6.0

Blocking on a decision from Tor, and possibly on a patch to implement
the alternate approach.

The standard windows font selector always lists regular/italic/bold/bold italic
for all true type or open type fonts.

Also, for end users, if a font cannot be selector via the font selector dialog,
then the font is not availalble/visible.

So for these two reasons, I would say yes to both 'A' and 'B' questions.

We have used this patch on a big and complex application (GPS, see
http://libre.adacore.com/gps), and this change is a big improvement
compared to the current situation, so would be in favor of integrating
this patch.

Arno

Actually, using the pango 1.2.1 patch (with pango 1.4.1), there is an unwanted
side effect in the font selector: fonts that already had a italic/bold variant
now display these variants twice.

I do not know whether the patch against 1.6 has the same issue (I would guess so),
so the patch probably needs some extra check before inclusion.

Arno

Marking patches...

Tor, still blocking on you, right?

Created attachment 66786 [details] [review]
Synthesizing italics only if they are not already there

As noted in bug #343796 the italic variant is much more crucial than the bold variant. While the latter just gets replaced by a font with different weight the former gives unknown glyphs drawing.

The attached patch addresses the issue by synthesizing italic variants for every font which not already has one. It currently does only synthesize one variant (often the bold one) and I'm unsure how to change this.

Other than that there is nothing kludgy with it. IMHO it matches the users expectations to have italic and bold variants available - most users dont 
care if they are real or synthesized. Winwords character format dialog always
shows at least four variants for a fonts style (normal,italic,bold,
bold-italic) - only named differently dependening on the font (e.g. demi bold). The only excetption I found was a font called "Aharoni" where only bold and bold-italic where offered.

Ok to commit?

Yay! Patch seems to work OK, but are you sure it's correct not to look at lfItalic in first_match(), though? Or do you need two matching functions, one that looks at lfItalic and one that doesn't? The comment "we have a non italic variant, look for the italic" in ensure_italic() and setting logfont.lfItalic=1 before calling g_hash_table_find() seems to indicate that you then want to use a match function that looks at lfItalic? Or am I confused?

The data structure confused me as well. Even so much I needed do draw a diagram ;)
At first I thought the SizeInfo::logfonts would include both variants: the italic and the non italic once (as in: if we dont have italic fall back to non italic).

Two functions would work as well, but there shouldn't be a need of it, i.e. I 
dont think there would be any performance benefit. Restructering the whole
data handling of PangoWin32FontMap could help, though.

The "look for italic" case is due to the hash function of FontMap::size_infos.
The key needs to be modified to make a difference.

Ah yes ok ;) Commit away then, to HEAD and pango-1-12, unless Behdad opposes? (btw, two unused variables in ensure_italic(): lfp2 amd win32family.)

Go ahead and rock!

Applied to HEAD and pango-1-12:

2006-06-05  Hans Breuer  <hans@breuer.org>

	* pango/pangowin32-fontmap.c(pango_win32_font_map_init) : synthesize
	some italic variants for fonts no having them already. This fixes
	bug #343796 and for the italic case also bug #110521.

Hans, Tor,

Anything remaining in this bug that can be fixed?

While looking into converting pangowin32 to use wide character API I came across odd code introduced by the patch here. Sorry for not noticing earlier. Surely the code does not really do what it is claimed to do? We have:

+static gboolean 
+first_match (gpointer key, 
+            gpointer value, 
+	    gpointer user_data)
+{
+  LOGFONT *lfp = (LOGFONT *)key;
+  LOGFONT *lfp2 = (LOGFONT *)((PangoWin32SizeInfo *)value)->logfonts->data;
+  gchar *name = (gchar *)user_data;
+  
+  if (strcmp (lfp->lfFaceName, name) == 0 && lfp->lfWeight == lfp2->lfWeight)
+    return TRUE;
+  return FALSE;
+}

Note how the user_data is assumed to be a char pointer. first_match() is used in two cases as the predicate function passed to g_hash_table_find(). In the second case what is passed as user_data is however not a char pointer, but a LOGFONT pointer:

+	      LOGFONT logfont = *lfp;
+	      LOGFONT *lfp2;
+	      logfont.lfItalic = 1;
+	      sip = (PangoWin32SizeInfo *)g_hash_table_find (helper->fontmap->size_infos, first_match, &logfont);

Should the last parameter to g_hash_table_find() be logfont.lfFaceName here?

Another thing is that first_match() uses strcmp() to compare lfFaceName with user_data string. In other places in the file ASCII case insensitive comparisons are used. Are we sure that a straight strcmp() is the correct thing to do in this case? Oh well, probably yes, as no casfolding is done on the stuff stored in win32fontmap->size_infos. Case insensitivity is used only to look up fonts based on user input I assume.

And note that even if the last parameter to g_hash_table_find() is changed to logfont.lfFaceName as suggested in my previous comment so that at least the data types are correct, the code still doesn't make sense. The comment says "we have a non italic variant, look if there is an italic", but then it calls g_hash_table_find() with the first_match() function as predicate, which doesn't check italicness at all. I noticed this already in comment #17 but didn't insist, thinking I was imagining things... There probably has been a confusion between g_hash_table_find() and g_hash_table_lookup() here?

(Anyway, even if this problem is fixed, I certainly agree with comment #18 that a general cleanup of the very messy code and data structures in pangowin32 is needed.)

Now I think I have patches that make the code at least work as it is supposed to currently. But as Owen says in comment #5, listing synthesized italic (or bold, but the code doesn't do that) is not the right thing to do. Pango should just synthesize them if asked for when instantiating fonts. Will try to fix that in trunk only, though.

Created attachment 83331 [details] [review]
Suggested patch to the stable pango-1-14 branch

Will commit this in a few days unless somebody screams. A visible improvement is that, at least for me, the current code makes the font picker list only the Normal, Bold and Bold Italic styles for the Tahoma family. With this patch, also Italic is listed.

(But as Owen said, that is not *really* what it should do. It should list only those styles that actually exist (Normal and Bold), but the code should then if asked for Tahoma Italic just let Windows do its thing and synthesize it from the Normal variant.)

Created attachment 83341 [details] [review]
Suggested patch to trunk

Will commit this in a few days unless somebody screams. Similar as the patch to the stable branch, and additionally the patch also starts using only the wide character API. Sorry for handling these two as such unrelated things in the same patch.

Patch worked on a bit more and committed to trunk from which in the meantime Pango 1.16.0 had been released. I also committed the ensure italicness fix to pango-1-14, but there presumably won't be any more 1.14.x releases. 

To have a clear separation that Pango 1.16.x uses the wide character Win32 API, I won't distribute any 1.16.0 binaries for Win32, but start with 1.16.1.

2007-02-26  Tor Lillqvist  <tml@novell.com>

	This change was supposed to go in the trunk before 1.16.0, but it
	didn't quite get there in time. So, to have a clear cut for this
	somewhat fundamental change in underlying workings (although there
	should be no user-visible changes), I will build and distribute
	Win32 binaries only starting from 1.16.1.

	Use wide character API for fonts on Windows. Rename functions and
	variables that deal with LOGFONTW structs to emphasize
	this. (#407315)

	* pango/pangowin32.c
	* pango/pangowin32-fontcache.c
	* pango/pangowin32-fontmap.c: Use LOGFONTW all over the place
	instead of LOGFONT, and adapt code accordingly. Use wide character
	Win32 API.

	* pango/pangowin32.c (pango_win32_font_neww): Renamed from
	pango_win32_font_new(), as it now takes a LOGFONTW pointer. This
	is a private function and can be renamed though it is exported as
	it is used from the pangocairo DLL.
	(pango_win32_font_logfont): Mention explicitly in doc comment that
	it returns a LOGFONTA, and recommend to use
	pango_win32_font_logfontw() instead.
	(pango_win32_font_logfontw): New function.

	* pango/pangowin32-fontcache.c
	(pango_win32_font_cache_load): Must keep this function that takes
	a LOGFONTA pointer as it is declared in the public header.
	(pango_win32_font_cache_loadw): New public function that takes a
	LOGFONTW pointer.

	* pango/pangowin32-fontmap.c
	(pango_win32_font_description_from_logfont): Mention explicitly in
	the doc comment that it takes a LOGFONTA pointer.
	(pango_win32_font_description_from_logfontw): New public function
	that takes a LOGFONTW pointer.
	(pango_win32_make_matching_logfontw): Rename from
	pango_win32_make_matching_logfont() to emphasize it takes a
	LOGFONTW pointer.

	* pango/pangowin32.h: Declare new public functions.

	* pango/pangowin32-private.h: Declare new private functions, drop
	removed ones.

	* pango/pangocairo-win32font.c (_pango_cairo_win32_font_new):
	Simplify now that we call pango_win32_make_matching_logfontw().

	* pango/pangowin32.def: Add new functions, rename internal
	functions that now use LOGFONTW.

2007-02-26  Tor Lillqvist  <tml@novell.com>

	Fix brokenness in the code that tries to ensure that all fonts
	also have italic variants. Now the code hopefully actually does
	what it was supposed to. (Which is not necessarily the right thing
	to do, though. It can be argued that we should not list
	synthesized italic font styles, we should just silently generate
	them if asked for. We don't want synthesized italic (or synthesized
	bold) styles showing up in the font selector. They don't show up
	when using a fontconfig-based Pango backend either.) (#110521)
	
	* pango/pangowin32-fontmap.c
	(logfont_nosize_hash, logfont_nosize_equal): Don't use the
	lfItalic field as such, just its nonzeroness. When being
	enumerated, italic fonts show up with lfItalic=255, but our code
	looks up italic versions of fonts by passing a key LOGFONT with
	lfItalic=1.
	(first_match): Not needed any more, see below.
	(ensure_italic): This is now called on the entries in the
	size_infos hash table, not families. The code used to randomly
	look for the first matching font in size_infoswith the family name
	being handled.
	(pango_win32_font_map_init): Iterate through the size_infos hash
	table with ensure_italic, not through the families table.

	* pango/pangowin32-fontcache.c
	(logfontw_hash, logfontw_equal): Look at just nonzeroness of
	lfItalic here, too.