After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 346942 - gnome-font-viewer doesn't render complex scripts correctly
gnome-font-viewer doesn't render complex scripts correctly
Status: RESOLVED FIXED
Product: gnome-font-viewer
Classification: Core
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: gnome-font-viewer-maint
gnome-font-viewer-maint
: 442750 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2006-07-07 20:05 UTC by Behdad Esfahbod
Modified: 2014-01-14 22:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
shot (90.44 KB, image/png)
2006-07-07 20:07 UTC, Behdad Esfahbod
  Details
Patch using HarfBuzz (8.67 KB, patch)
2013-09-17 23:07 UTC, Khaled Hosny
reviewed Details | Review
Patch to fix showing fonts with non-Unicode charmaps (1.91 KB, patch)
2013-09-17 23:09 UTC, Khaled Hosny
accepted-commit_now Details | Review
Patch using HarfBuzz, V2 (8.82 KB, patch)
2014-01-14 00:13 UTC, Khaled Hosny
accepted-commit_now Details | Review

Description Behdad Esfahbod 2006-07-07 20:05:40 UTC
I'm not sure what gome-font-viewer is doing, but seems like it's accessing FreeType directly instead of using Pango.  That makes it not render Persian text correctly for example, and it makes the text non-selectable too.
Comment 1 Behdad Esfahbod 2006-07-07 20:07:12 UTC
Created attachment 68592 [details]
shot

The letters should be connected, and the layout right-to-left and right-aligned..
Comment 2 James Henstridge 2006-07-10 06:02:34 UTC
Behdad: as the pango maintainer, would you know how to use Pango here?

The program needs to render the sample text using the particular font file, which may not be on the fontconfig path or may only be accessible via gnome-vfs.  I can create an FT_Face for the font quite easily (what the app does currently), but I don't see a way to get Pango to render using that font.

As this is a font previewer, we don't want to be doing any font substitution even if the font in question lacks coverage (it might be worth picking different sample text in this case).
Comment 3 Behdad Esfahbod 2006-07-11 16:18:50 UTC
With PangoFT2 and PangoXft you probably can use pango_ft2_font_map_set_default_substitute() and pango_xft_set_default_substitute() to set FC_FILE/FC_INDEX on the pattern, to force the font file to be used, or FC_FT_FACE to point to the face.


For PangoCairo, I'm trying to come up with a generic solution and so far this is what I've come up with:

Add a new PangoCairoFontMap implementation that uses a single font face:

PangoFontMap *
pango_cairo_font_map_create_for_cairo_font_face (cairo_font_face_t *);

Such a font map then can be used to create a context and then a layout that will only use the specificed font face.  You can then use cairo-ft.h to create a cairo_font_face_t for your FT_Face.

Filed that as bug 347237
Comment 4 Behdad Esfahbod 2006-07-11 16:19:30 UTC
Jamed, does that make sense?
Comment 5 Behdad Esfahbod 2006-07-11 16:21:08 UTC
As for fallbacks, we already have pango attributes to disable fallback.  That would trigger hexboxes.  I'm going to add more attributes to disable hexboxes too, eventually.  But with the above approach, the hexbox you get will use the same font.
Comment 6 James Henstridge 2006-07-20 02:50:27 UTC
Behdad: sorry for not replying earlier.  I think mail for your last three comments on this bug never got through (see http://blogs.gnome.org/view/ovitters/2006/07/11/0).

The pango_xft_set_default_substitute() would affect other rendering though, right?  That'd make things a bit more complicated for the other text rendering in the font viewer app (I still want the rest of the widgets to render normally).

So I suppose the pangoft2 method (possible now) or pangocairo method (possible with bug 347237) are worth trying.

So I suppose the rendering code would look something like this:

 1. create FT_Font for the file being previewed.
 2. create a PangoFT2FontMap with pango_ft2_font_map_new()
 3. set DPI of font map with pango_ft2_font_map_set_resolution() (to what?)
 4. set a default substitute function for the font map to a function that
    ensures that said font gets used.
 5. create a PangoContext with pango_ft2_font_map_create_context()
 6. create a PangoLayout with the context, making sure that the "fallback"
    PangoAttribute is set to false for the entire run of text
 7. Render the layout with pango_ft2_render_layout() or
    pango_ft2_render_layout_subpixel() (which one?)

I guess the fact that this isn't too obvious isn't too bad, since font viewers are pretty much the only apps that'd want to use pango this way.
Comment 7 Behdad Esfahbod 2006-07-30 23:30:34 UTC
(In reply to comment #6)
> Behdad: sorry for not replying earlier.  I think mail for your last three
> comments on this bug never got through (see
> http://blogs.gnome.org/view/ovitters/2006/07/11/0).

No problem.  Thanks for the detailed reply.


> The pango_xft_set_default_substitute() would affect other rendering though,
> right?

That's true, because with the Xft backend you cannot create a new fontmap for the same display.  However, you can use a magic string as the font family and match FC_FAMILY in the susbtitution function before overwriting the matched font file/face.


> So I suppose the pangoft2 method (possible now) or pangocairo method (possible
> with bug 347237) are worth trying.

With the pangoft2 you still cannot use a textview but that's not a big deal.

 
> So I suppose the rendering code would look something like this:
> 
>  1. create FT_Font for the file being previewed.
>  2. create a PangoFT2FontMap with pango_ft2_font_map_new()
>  3. set DPI of font map with pango_ft2_font_map_set_resolution() (to what?)

to what you get from pango_cairo_context_get_resolution() from a context created by gtk_widget_create_pango_context(), such that you match the resolution of user's desktop.

>  4. set a default substitute function for the font map to a function that
>     ensures that said font gets used.
>  5. create a PangoContext with pango_ft2_font_map_create_context()
>  6. create a PangoLayout with the context, making sure that the "fallback"
>     PangoAttribute is set to false for the entire run of text
>  7. Render the layout with pango_ft2_render_layout() or
>     pango_ft2_render_layout_subpixel() (which one?)

First one.  If you read the docs for the latter, it's not really that different.  Only the positioning parameters are in subpixel accuracy, but the final rendering is still rounded to pixel boundaries.


> I guess the fact that this isn't too obvious isn't too bad, since font viewers
> are pretty much the only apps that'd want to use pango this way.

Exactly.  Pango wants to be really sure that it can find a suitable font for the text and defeating that is a bit hard.
Comment 8 Jens Granseuer 2007-06-01 16:17:41 UTC
*** Bug 442750 has been marked as a duplicate of this bug. ***
Comment 9 Khaled Hosny 2007-06-08 09:33:21 UTC
Any updates regarding this issue? this is really an annoying bug for Arabic script users.
Comment 10 Behdad Esfahbod 2007-06-11 03:17:14 UTC
(In reply to comment #9)
> Any updates regarding this issue? this is really an annoying bug for Arabic
> script users.

I'm still making up my mind about the right API to add.  Owen didn't like my previously proposed API, with good reason.  I'm working on it, hopefully this will be fixed this cycle.
Comment 11 Behdad Esfahbod 2007-06-16 23:39:35 UTC
So I'm working on a solution that I hope to get into tomorrow's pango devel release.  James, do you mind if I hack gnome-font-viewer to use my new API?  And are you planning a release tomorrow?
Comment 12 Khaled Hosny 2007-09-11 17:48:53 UTC
Any plans for this to get in gnome 2.20?
Comment 13 Behdad Esfahbod 2007-09-11 17:58:31 UTC
I've not found a way to implement the needed feature in Pango yet.  This is constantly brought up again and again when Owen and I are talking.  Hopefully we find a solution for 2.20, but no promises at this point.
Comment 14 Khaled Hosny 2008-10-16 16:32:42 UTC
Interesting thing I found today, font-viewer of serif project (http://code.google.com/p/serif/source/browse/#svn/fontview/trunk) does use pango to render fonts (it gets Arabic rendered right) without installing the font.
Comment 15 Behdad Esfahbod 2008-10-16 17:11:02 UTC
Khaled,

It does that by using fontconfig API to add app-specific fonts.  That works for files on your system but not for more general types of streams...  Though, thinking about it, using the ~/.gvfs fuse mounts we can always use that trick.
Comment 16 Cosimo Cecchi 2010-11-04 08:51:19 UTC
-> gnome-utils

gnome-font-viewer has now moved to gnome-utils, reassigning.
Comment 17 Khaled Hosny 2013-09-17 23:07:21 UTC
Created attachment 255145 [details] [review]
Patch using HarfBuzz

Since Pango does not (yet) have a API to use a cairo_font_face_t for rendering, HarfBuzz is used directly instead to do the actual text layout, but Pango is still (ab)use to do text itemization.

The thumbnailer still does not do complex text, but I don’t think it actually need to, so it was left alone.

This should fix Bug 109269 as well.
Comment 18 Khaled Hosny 2013-09-17 23:09:30 UTC
Created attachment 255146 [details] [review]
Patch to fix showing fonts with non-Unicode charmaps

This is not a new issue, but it seems the use of HarfBuzz exposed it.
Comment 19 Khaled Hosny 2013-09-17 23:11:22 UTC
I think the patches should have been submitted against Sushi, but I don’t know how to test that code there.
Comment 20 Behdad Esfahbod 2013-09-18 02:18:01 UTC
*Very* neat.  Thanks Khaled!  Humm.  James, you still maintainer here?
Comment 21 Khaled Hosny 2014-01-12 10:12:19 UTC
Any chance of getting those patches for the next GNOME release?
Comment 22 Behdad Esfahbod 2014-01-13 13:04:27 UTC
Cosimo, can you please take these patches?  I support them.
Comment 23 Cosimo Cecchi 2014-01-13 19:26:44 UTC
Review of attachment 255145 [details] [review]:

Hi Khaled, sorry it took me so long to review this.
Code looks mostly good, apart from some memory leaks that I found below. The original code for SushiFontWidget lives here [1] though, so any modification will need to be made on both copies.

[1] https://git.gnome.org/browse/sushi/tree/src/libsushi/sushi-font-widget.c

::: src/sushi-font-widget.c
@@ +102,3 @@
+   * pango will split runs at font change */
+  context = pango_cairo_create_context (cr);
+  gint i;

This should be free-d with pango_attr_list_unref()

@@ +108,3 @@
+                                       text, 0, strlen (text),
+                                       attr_list, NULL);
+  *glyphs = NULL;

Missing space before paren.

@@ +111,3 @@
+
+  /* reorder the items in the visual order */
+  base_dir = pango_find_base_dir (text, -1);

pango_reorder_items() will return a new list, so here you're leaking the original items list.

@@ +153,3 @@
+  }
+
+

Can use g_list_free_full() here instead.
Comment 24 Cosimo Cecchi 2014-01-13 19:28:03 UTC
Review of attachment 255146 [details] [review]:

Looks good to me.
Comment 25 Khaled Hosny 2014-01-14 00:13:12 UTC
Created attachment 266217 [details] [review]
Patch using HarfBuzz, V2

Here a new patch that should address the comments above.
Comment 26 Cosimo Cecchi 2014-01-14 00:38:26 UTC
Review of attachment 266217 [details] [review]:

This looks good to me, thanks for the quick update.
Do you have a GNOME git account, or should I push these for you?
Comment 27 Khaled Hosny 2014-01-14 06:31:34 UTC
(In reply to comment #26)
> Review of attachment 266217 [details] [review]:
> 
> This looks good to me, thanks for the quick update.
> Do you have a GNOME git account, or should I push these for you?

I have an account, should I push both commits?
Comment 28 Khaled Hosny 2014-01-14 22:12:53 UTC
Pushed both patches.