GNOME Bugzilla – Bug 787759
Show more information
Last modified: 2017-09-26 10:05:19 UTC
Admittedly, this is a bit font nerdy, but I like to be able to see some detailed information about fonts, like what OpenType features they expose, how many glyphs they contain, and if there are multiple styles available.
Created attachment 359890 [details] [review] Add more general information Display designer, manufacturer, license and glyph count. This is generally interesting information about a font.
Created attachment 359891 [details] [review] Do multiline labels differently I think we should show multiple lines of text when it is present. Cutting things off is just rude. But we don't want to break the UI by producing an enormous dialog, so don't allow more than 10 lines per label.
Created attachment 359892 [details] [review] Add font variation information Display the available axes and named instances.
Created attachment 359893 [details] [review] Show OpenType layout features Making such font features visible is the first step towards getting them to be exposed and used in appliations, where appropriate.
Created attachment 359894 [details] [review] Add a details tab The OpenType details are fairly obscure, and mainly interesting to font nerds. Move them to a separate tab so they don't clutter the info dialog.
Created attachment 359895 [details] how it looks
Created attachment 359896 [details] how it looks, 2
(In reply to Matthias Clasen from comment #7) > Created attachment 359896 [details] > how it looks, 2 I think it would be more consistent for "Glyph Count" and "Color Glyphs" to use Title Case too.
Review of attachment 359890 [details] [review]: Thanks, looks good with this small comment. (Should probably also branch for gnome-3-26 before pushing this, which reminds me I need to do a release...) ::: src/font-view.c @@ +316,3 @@ + { + char *s = g_strdup_printf ("%ld", face->num_glyphs); + add_row (grid, _("Glyph count"), s, FALSE); Agreed that these should probably use title case too.
Review of attachment 359891 [details] [review]: ::: src/font-view.c @@ +179,2 @@ gtk_label_set_max_width_chars (GTK_LABEL (label), 64); + gtk_label_set_width_chars (GTK_LABEL (label), 64); Does this not make the dialog always wide?
Review of attachment 359892 [details] [review]: ::: src/font-view.c @@ +192,3 @@ +describe_axis (FT_Var_Axis *ax) +{ + return g_strdup_printf (_("%s %g — %g, default %g"), ax->name, This really needs a comment for translators explaining the various specifiers @@ +193,3 @@ +{ + return g_strdup_printf (_("%s %g — %g, default %g"), ax->name, + FixedToFloat(ax->minimum), Missing spaces before paren @@ +228,3 @@ +is_valid_subfamily_id (guint id) +{ + if (FT_Get_Sfnt_Name (face, i, &sname) != 0) Where do these magic numbers come from? It would be nice to use defines, or have a link to some documentation @@ +232,3 @@ + +static void + Prefer one argument per line @@ +236,3 @@ + if (is_valid_subfamily_id (ns->strid)) { + char *str = get_sfnt_name (face, ns->strid); + continue; I'd prefer to consolidate the logic here, and make str = _("Instance %d") when empty/invalid, with a single call to append it to the GString at the end of the function @@ +385,3 @@ add_row (grid, _("Color glyphs"), FT_HAS_COLOR (face) ? _("yes") : _("no"), FALSE); + + if (FT_Get_MM_Var (face, &ft_mm_var) == 0) { ft_mm_var is leaked AFAICT
Review of attachment 359893 [details] [review]: Commit message has a typo ("appliations") ::: src/font-view.c @@ +257,3 @@ + const char *tag; + const char *name; +} named_features[] = { How did you make this list? Would be nice to be able to recreate it (or even better, if there was an API somewhere that we could use to do this for us!)
Review of attachment 359894 [details] [review]: Looks good
Created attachment 359907 [details] [review] Add more general information Display designer, manufacturer, license and glyph count. This is generally interesting information about a font.
Created attachment 359908 [details] [review] Do multiline labels differently I think we should show multiple lines of text when it is present. Cutting things off is just rude. But we don't want to break the UI by producing an enormous dialog, so don't allow more than 10 lines per label.
Created attachment 359909 [details] [review] Add font variation information Display the available axes and named instances.
Created attachment 359910 [details] [review] Show OpenType layout features Making such font features visible is the first step towards getting them to be exposed and used in appliations, where appropriate.
Created attachment 359911 [details] [review] Add a details tab The OpenType details are fairly obscure, and mainly interesting to font nerds. Move them to a separate tab so they don't clutter the info dialog.
Created attachment 359912 [details] [review] Pull out the OpenType data This commit moves the OpenType feature tags list to a separate file that can more easily be shared with other users. It also completes the list to have all registered tags from the spec.
Thanks for the reviews. I think I've addressed all feedback in the new series.
Created attachment 359913 [details] [review] Pull out the OpenType data
Created attachment 359914 [details] [review] Pull out the OpenType data
Review of attachment 359907 [details] [review]: Thanks, looks good to me. gnome-3-26 is branched, so you can push this to master.
Review of attachment 359908 [details] [review]: OK
Review of attachment 359909 [details] [review]: Looks good
Review of attachment 359910 [details] [review]: Looks good to me (not sure why there's another copy of font-view.c in this patch though)
Review of attachment 359911 [details] [review]: OK
Review of attachment 359914 [details] [review]: Nice, thanks!
Attachment 359907 [details] pushed as 3b35b8a - Add more general information Attachment 359908 [details] pushed as c44df5b - Do multiline labels differently Attachment 359909 [details] pushed as 96a51ff - Add font variation information Attachment 359910 [details] pushed as 5eabb1f - Show OpenType layout features Attachment 359911 [details] pushed as c3a4ed3 - Add a details tab Attachment 359914 [details] pushed as 3e00abb - Pull out the OpenType data
(In reply to Cosimo Cecchi from comment #26) > Review of attachment 359910 [details] [review] [review]: > > Looks good to me (not sure why there's another copy of font-view.c in this > patch though) It looks like this one fell through the cracks.
(In reply to Piotr Drąg from comment #30) > (In reply to Cosimo Cecchi from comment #26) > > Review of attachment 359910 [details] [review] [review] [review]: > > > > Looks good to me (not sure why there's another copy of font-view.c in this > > patch though) > > It looks like this one fell through the cracks. Fixed in https://git.gnome.org/browse/gnome-font-viewer/commit/?id=7e8950c83ea2e0ba82720770cff79714f4d1c827