GNOME Bugzilla – Bug 752005
Patch to support TrueType Collection Font Format
Last modified: 2016-03-05 15:17:54 UTC
In TrueType Collection font (TTC fonts), the font file may contains several font faces. I write a patch for gnome-font-viewer to show all font faces from one TTC font. Also I changed one asynchronous call to synchronous call to avoid to start to two asynchronous calls when call: g_object_set (self->font_widget, "uri", uri, "face-index", face_index, NULL); Please review the patch, thanks!
Created attachment 306894 [details] [review] Patch for TrueType Collection Font
Review of attachment 306894 [details] [review]: (In reply to Peng Wu from comment #0) > Also I changed one asynchronous call to synchronous call to avoid to start > to two asynchronous calls when call: > g_object_set (self->font_widget, "uri", uri, > "face-index", face_index, NULL); I don't understand why you needed to do this. What was the issue with the async font loader? Note that this causes calling code to miss the "loaded" signal from the font widget since now the signal is emitted when the object is created meaning that callers can't listen to it. This needs to be fixed ::: src/font-thumbnailer.c @@ +238,3 @@ g_object_unref (file); + face = sushi_new_ft_face_from_uri (library, uri, 0, &contents, &gerror); We should generate a thumbnail for each index, otherwise all of them look the same. Since the thumnailing API doesn't support parameters, I guess one way to do it would be to generate all the thumbnails in a single thumbnail file, stitched together like: +---+---+ | 0 | 1 | +---+---+ for a font file with 2 indexes. Then on the reading side, we'd split the various thumbnails from the same file into different pixmaps.
Because both "uri" and "face-index" properties will call load_font_face. g_object_set (self->font_widget, "uri", uri, "face-index", face_index, NULL); The above code will start two load_font_face call asap, and two sushi_new_ft_face_from_uri_async call will starts two async font loaders, the font widget seems to use a random loaded font to show. I will try to fix the "loaded" signal issue. Thanks for pointing out how to fixes the thumbnail problem. It will take me some days to figure out how to implement the thumbnail, please be patient.
Created attachment 307886 [details] [review] Updated Patch Set Fixes the "loaded" signal issue, and generates the tiled thumbnails.
Created attachment 309995 [details] [review] Support TrueType Collection Font Format In TrueType Collection font (TTC fonts), the font file may contains several font faces. Fixes to display several different face names from one TTC font. -- I have just tried the patches, nice work. However, I have encountered a minor problem. With the latest patch, the "loaded" signal is emitted from an idle context, while the signal handler is connected from the main context. That sometimes leads to an error: (gnome-font-viewer:4824): Gtk-CRITICAL **: gtk_widget_get_style_context: assertion 'GTK_IS_WIDGET (widget)' failed On my environment, it can be reproduced by selecting WenQuanYi Zen Hei font and clicking "<" button on the headerbar. (In reply to Peng Wu from comment #3) > Because both "uri" and "face-index" properties will call load_font_face. > > g_object_set (self->font_widget, "uri", uri, > "face-index", face_index, NULL); Perhaps you could change the logic of the callee, so: - don't call load_font_face() in the property setters - make "uri" and "face" as G_PARAM_CONSTRUCT and call load_font_face() in GObject::constructed() - add a new method, say sushi_font_widget_load(widget, uri, face_index), to set those properties and trigger load_font_face() I am attaching a slightly modified patch doing that.
One more thing I noticed is that TTC thumbnails are shown with blur, since non-square thumbnails are scaled down in libgnome-desktop. To prohibit scaling, perhaps a .thumbnailer file could have a special keyword, say "NoScale=true"?
Right, that is the remained problem of my patch.
Hi Daiki, I will use your modified patch after we figure out how to fix the blur problem.
Created attachment 313907 [details] [review] Generate some thumbnail image on the fly It seems gnome-desktop only support to store one thumbnail image for each file. This patch try to generate multiple font faces of TTC fonts on the fly, without storing into the thumbnail cache.
It sounds clever, but I am not sure if it is a good idea to use URI fragments for this non-standard usage, and if TTC fonts are really minority. For what it's worth I have tried to generate thumbnails for non-zero face indices under ~/.cache/gnome-font-viewer. I will attach the rebased patches soon.
Created attachment 319663 [details] [review] Fix some unused variable GCC warnings -- This is not related to this bug, but anyway.
Created attachment 319664 [details] [review] sushi-font-widget: Add missing include <hb-glib.h> -- Ditto.
Created attachment 319665 [details] [review] sushi-font-loader: Add face_index argument sushi_new_ft_face_from_uri{,_async} now takes face index as the third argument. All callers changed.
Created attachment 319666 [details] [review] font-utils: Add face_index argument font_utils_get_font_name_for_file() now takes face index as the third argument. All callers changed.
Created attachment 319667 [details] [review] sushi-font-widget: Add font_index argument sushi_font_widget_new() now takes font index as the second argument. All callers changed. Setting the "uri" property on the font widget no longer triggers loading font. Use sushi_font_widget_load() after setting "uri" (and "face-index" if any) property.
Created attachment 319668 [details] [review] font-thumbnailer: Add option to specify face index gnome-thumbnail-font now takes --face option to specify face index. As the argument is parsed as 32-bit integer, to use the FreeType extension for GX fonts, use 0x... format, such as --face=0x00030004: http://freetype.org/freetype2/docs/reference/ft2-base_interface.html#FT_Open_Face
Created attachment 319669 [details] [review] font-model: Refactor thumbnail loading To make it easier to support non-zero face indices, factor out pixbuf loading code to separate functions load_thumbnail_pixbuf() and create_thumbnail_from_file().
Created attachment 319670 [details] [review] font-model: Support TTC fonts For TTC fonts, font faces with non-zero index will be thumbnailed under ~/.cache/gnome-font-viewer. The default font face (index 0) is still stored under ~/.cache/thumbnails using the desktop thumbnailer mechanism.
Created attachment 319671 [details] [review] font-view: Support TTC fonts
Comment on attachment 319663 [details] [review] Fix some unused variable GCC warnings A similar patch was already committed to master a while ago
Comment on attachment 319664 [details] [review] sushi-font-widget: Add missing include <hb-glib.h> A similar patch was already committed to master a while ago
Review of attachment 319665 [details] [review]: Looks good to me.
Review of attachment 319666 [details] [review]: Looks good to me.
Review of attachment 319667 [details] [review]: I am not entirely sure you need to add the explicit "load" method. What happens if you just assume face index = 0 if it's not present and just let it load as before? Looks good to me otherwise.
Review of attachment 319668 [details] [review]: This one is OK but I have a comment in a later patch that may require you to change this patch.
Review of attachment 319669 [details] [review]: ::: src/font-model.c @@ +237,3 @@ uri, (time_t) mtime); + g_free (uri); Is this change a leak fix? If so it would be best in a separate commit. @@ +242,2 @@ out: + g_clear_object (&info); It looks to me the changes here only touch whitespace/indentation and could be avoided.
Review of attachment 319670 [details] [review]: ::: src/font-model.c @@ +256,3 @@ +static GdkPixbuf * +create_thumbnail (ThumbInfoData *thumb_info, GError **error) I am wondering whether the manual invocation of the thumbnailer here is really needed. Would it be possible to use, let's say, an URI fragment to optionally specify the index? In other words you would pass an URI to the gnome-desktop API such as file:///path/to/file#face-index. I don't think gnome-desktop does any sort of I/O on the URI before calling into the thumbnailer.
Review of attachment 319671 [details] [review]: Looks good to me.
Actually I dunno whether gnome-desktop can store the image into the thumbnail cache with an URI fragment, or call g_file_query_info with an URI fragment. And some glib code seems not support the URI fragment, bicbw. I think that is why Daiki Ueno's patch store the thumbnail image in ~/.cache/gnome-font-viewer .
In a quick testing, it actually seems to work, as the thumbnail file name is calculated from a URI: https://git.gnome.org/browse/gnome-desktop/tree/libgnome-desktop/gnome-desktop-thumbnail.c#n1016 while we need to bypass the G_FILE_ATTRIBUTE_THUMBNAIL_PATH handling, which seems to be set by gvfs, if face index is non-zero. I'll update the patches to use URI fragments soon.
Created attachment 319732 [details] [review] sushi-font-loader: Add face_index argument sushi_new_ft_face_from_uri{,_async}() now takes face index as the third argument. All callers changed. -- Fix usage of this function in font-thumbnailer.c.
Created attachment 319733 [details] [review] sushi-font-widget: Add face_index argument sushi_font_widget_new() now takes face index as the second argument. All callers changed. The face index of the widget can be accessed through the "face-index" property. Setting the "uri" property with g_object_set() no longer starts font loading, while it does when it is set through the constructor. This is to prevent the same font from being loaded twice, when both "uri" and "face-index" are set with g_object_set(). To start loading, you can explicitly call sushi_font_widget_load() after setting the properties.
Created attachment 319734 [details] [review] font-thumbnailer: Extract face index from URI The URI passed to gnome-thumbnail-font can now have a face index, embedded as a URI fragment. The fragment part is parsed as a 32-bit integer, and it may include a "0x" prefix, to support FreeType extension for GX fonts: http://freetype.org/freetype2/docs/reference/ft2-base_interface.html#FT_Open_Face
Created attachment 319735 [details] [review] font-model: Support TTC fonts
Created attachment 319736 [details] [review] font-view: Support TTC fonts
Comment on attachment 319669 [details] [review] font-model: Refactor thumbnail loading I have dropped this patch, as it is not really useful if we use the default thumbnailer.
(In reply to Cosimo Cecchi from comment #24) > Review of attachment 319667 [details] [review] [review]: > > I am not entirely sure you need to add the explicit "load" method. What > happens if you just assume face index = 0 if it's not present and just let > it load as before? > Looks good to me otherwise. This change is to prevent the same font from being loaded twice at: g_object_set (widget, "uri", uri, "face-index", index, NULL); as in font-view.c:font_view_application_do_open. I have added a comment in the commit log.
(In reply to Cosimo Cecchi from comment #25) > Review of attachment 319668 [details] [review] [review]: > > This one is OK but I have a comment in a later patch that may require you to > change this patch. Yes, I have removed the option, and added support for file:///path/to/file#face-index style URI instead.
Review of attachment 319732 [details] [review]: Looks good to me.
Review of attachment 319733 [details] [review]: Looks good.
Review of attachment 319734 [details] [review]: ::: src/font-thumbnailer.c @@ +238,3 @@ + fragment = strrchr (arguments[0], '#'); + if (fragment) + face_index = strtol (fragment + 1, NULL, 0); Don't you need to still initialize the face_index to zero when the fragment is not set?
Review of attachment 319735 [details] [review]: Looks good - it's a little unfortunate that we can't get away without manually building the thumbnail path here, but GIO doesn't really support the concept of one file providing multiple thumbnails. Still better than spawning the thumbnailer ourselves though :-)
Review of attachment 319736 [details] [review]: OK
Comment on attachment 313907 [details] [review] Generate some thumbnail image on the fly Assuming this is obsolete now
Daiki, feel free to push directly to master with that initialization issue fixed, and thanks again for the patches to both of you!
Thanks for the prompt and thorough review, Cosimo. I've fixed the initialization issue and pushed them. Attachment 319666 [details] pushed as a62d0bf - font-utils: Add face_index argument Attachment 319732 [details] pushed as 28bab70 - sushi-font-loader: Add face_index argument Attachment 319733 [details] pushed as 3c48f73 - sushi-font-widget: Add face_index argument Attachment 319734 [details] pushed as 920c834 - font-thumbnailer: Extract face index from URI Attachment 319735 [details] pushed as e512b0b - font-model: Support TTC fonts Attachment 319736 [details] pushed as 11d181c - font-view: Support TTC fonts
Thanks all for the comments and reviews!