On Wed, 2004-06-09 at 09:50, Jody Goldberg wrote: > On Thu, Jun 03, 2004 at 08:54:58PM -0400, Owen Taylor wrote: > > > > * I've changed how PangoFont => GnomeFont works so that instead of > > doing it at a "naming" level, it actually maps according to font > > file. > > In the longer term do you see any value in maintaining GnomeFont vs > PangoFont ? or would you rather just see us move to a cairo backend ? I was thinking of this in terms of a stop-gap measure for the next year or so until we move to Cairo. With this patch, if you are using the new APIs, GnomeFont is just an implementation detail. Getting rid of GnomeFont as an implementation detail would take more work; you'd have to extend gnome-glyphlist and the metafile format. And since you need GnomeFont for backwards compatibility anyways, it doesn't really seem worthwhile to me. (Well, if most apps using gnome-print were switched to pure Pango and we could avoid populating gnome-print's list of fonts, it might save a noticeable amount of memory, but still a lower priority.) > > * I'd consider this patch "done" other than review comments; > > it's reasonably well documented and commented and should > > be pretty much complete. The one thing I can think of that > > needs fixing is: > > > > http://bugzilla.gnome.org/show_bug.cgi?id=114431 > > > > But that mostly comes up for menu-style partial-word underlines. > > I don't see that as terribly significant for the time being. Definitely not a blocker for landing the patch. > > * The patch makes gnome-print require CVS head Pango but for a > > pretty surface reason; all that is being used is the > > new underline and strikethrough font metrics. It would be > > pretty easy to add a configure check and fallbacks for that. > > It will depend on the pango release schedule. If you still plan to > release a new stable version before gnome-2.8 then it seems > unnecessary. This code isn't going to be backported to gp-2.6. Yeah, I'm still planning on doing a new Pango for gnome-2.8. (Less necessary than when I thought I'd need a bunch of Pango changes to get this working, but easier to reduce the number of different combinations.) > > +/** > > + * gnome_print_pango_update_context: > > + * @context: a #PangoContext from gnome_print_pango_create_context(). > > + * @gpc: a #GnomePrintContext > > + * > > + * Update a context so that layout done with it reflects the > > + * current state of @gpc. In general, every time you use a > > + * #PangoContext with a different #GnomePrintContext, or > > + * you change the transformation matrix of the #GnomePrintContext > > + * (other than pure translations) you should call this function. > > + * You also need to call pango_layout_context_changed() for any > > + * #PangoLayout objects that exit for the #PangoContext. > > 'Change the transformation matrix' ? > I'll assume that is not necessary for translation. That was what I meant by "(other than pure translations)": I'm not really how to word it better. (If the CTM changes from old_ctm to new_ctm and old_ctm[i] != new_ctm for i == 0,1,2,3, then you must call this function...) In fact (as noted elsewhere in the docs) it's not necessary ever to call this function for gnome-print; it's just there to keep the structure the same when moving forward to Cairo. > > +static void > > +get_item_properties (PangoItem *item, ItemProperties *properties) > > +{ > ... > > + switch (attr->klass->type) { > > + case PANGO_ATTR_UNDERLINE: > > + case PANGO_ATTR_STRIKETHROUGH: > > + case PANGO_ATTR_FOREGROUND: > > + case PANGO_ATTR_BACKGROUND: > > + case PANGO_ATTR_SHAPE: > > + case PANGO_ATTR_RISE: > > + default: > Where is ATTR_SCALE handled ? There are a lot of things that aren't handled here; the attributes that are handled here are the attributes that affect rendering *beyond* the choice of PangoFont and glyphs. ATTR_SCALE will get bundled into the PangoFont. > > +void > > +gnome_print_pango_layout (GnomePrintContext *gpc, PangoLayout *layout) > > +{ > > + PangoLayoutIter *iter; > > + > > + g_return_if_fail (GNOME_IS_PRINT_CONTEXT (gpc)); > > + g_return_if_fail (PANGO_IS_LAYOUT (layout)); > > A sanity check to ensure that the layout is associated with a > context that has a PangoFT2FontMap seems like a good idea. Can we > get more specific to ensure that it's really one of the gnome-print > created fontmaps with hinting disabled ? Hmm, unfortunately, there is no pango_context_get_fontmap(). If there was such PANGO_IS_FC_FONTMAP (pango_context_get_fontmap (pango_layout_get_context (layout)); would be sufficient to check whether things would "basically work". Though there certainly is an argument that "basically working" is pretty evil. I spent quite a bit of time trying to figure out why layout was slightly wrong for my GtkHTML patch before realizing that GtkHTML was using the wrong PangoContext. So, there might be an advantage of doing something like setting a magic GObject data key on the PangoContexts we create and requiring it to be there. But the question that then comes up is gnome_print_pango_layout_print(); there would be no benefit in keeping that function around for backwards compatibility if we required using new APIs to create the fontmap. I have no idea how this function was being used / who was using it, so I have a hard time judging what the best approach is here. Or by having public wrappers for private functions we could make that function still work with random contexts, and require our contexts for the new API. Does that sound reasonable? > Other than those minor questions this looks good to go. OK, once you answer the above, I'll commit the patch. Thanks for the review, Owen
Attachment:
signature.asc
Description: This is a digitally signed message part