I'd love to mark this up but given other i18n issues and the gtk
team's workload this is not a release blocker. Sorry; I wish it had
been reported earlier.

Too bad this is an Gnome Blocker for Arabic language ..  Luis Villa, are 
you stating your opnion or you have a role in the GTK development and 
what you said is a statement  ? 
I am currently the Gnome Translation coordinator for Arabic, and the 
Arabic team reached 48% in 3 weeks.. it seems that the Arabic 
translation would be called off until this is fixed.. 

This is a *really* hard bug to fix; the trouble is that
a different attribute is set on the underline character,
and Pango shapes each attributed section separately.
(attributes within words weren't really planned for.)

There are three ways I can think of fixing it:

 - Don't use Pango attributes for the underline characters
   but draw them separetely.

 - Do some sort of trick to merge adjacent runs with
   attributes "that don't affect the shaping"
   (color, underline)

 - Change the Pango engine interface to pass, along with the text
   of the run being shaped, some characters of context.

None of these is at all easy to do, and certainly won't
happen before the Pango-1.2 timescale.

I'd suggest that to the Arabic Team, they may need to
omit underline accelerators from the menu items for
now .... I understand this is highly unsatisfacotary
but it's the best I can suggest at the moment.

Owen, I was wondering how hard is it to tackle this. Do you recommend
waiting until Pango 1.2 or you think one like me can try his hand on it?
If the latter, which of your three suggestions should be easier?

Well, waiting for pango-1.2 won't do much good by itself :-)

Thinking about it:

 Solution #3 doesn't work becuase the underline could fall
 on half of a ligature pair.

 Solution #2 is possibly right but would require extensive
 changes to Pango. What I think you'd need to do is add: 
  
   pango_glyph_string_extents_with_attrs (PangoGlyphString *glyphs,
                                          PangoAttrList    *extra_attrs,
                                          ...)

   And a variants of pango_x_render()/pango_xft_render() that took 
   an extra list of attrs as well.

  So, when Pango got a string, it would separate it into 
  "shape relevant" and "shape irrelevant" attributes, and
  pass the first set to the shaper, and pass the second set
  as "extra attrs" when getting the extents/rendering.
  
 Solution #1 is probably the most feasible in the short term, 
 since it could be done entirely within GtkLabel, though it 
 wouldn't be  *easy* to do so.

 What I'd do to implement #1 is first just forget about trying
 to adjust the spacing between lines to fit in underlines
 on multiline labels. This makes it just a rendering problem,
 not a layout problem. Multiline labels with underlined characters
 are going to be very rare.

 Then all you have to do is:
 
 a) Add a constant two pixels to the requisition of the label
    to put in space for the character
  
 b) Identifiy the position of the character on the screen
    by iterating through the PangoLayout and draw a line under  
    it.

Created attachment 10608 [details] [review]
A first patch

Attached a first hack to solve this. It has problems with ligatures
across boundaries, but works otherwise. It also fixes the subword
coloring bug.

Finally! We've all been waiting for this ;) If it doesn't break
anything (outside of Arabic-based scripts) please please incorporate.
Translated interfaces are simply undesirable.

Roozbeh Pournader are you still working on this? I heard you and Owen
had discussing 
how the patch needed to be and you where working on an improved
version, but that seems to be a while ago? 

Anything happening here? Should I replace the PATCH_NEEDED keyword
with PATCH?

I had some time today and decided to see if I could fix
this one.

Approach I took was to add two functions:


/**
 * pango_attr_list_filter:
 * @list: a #PangoAttrList
 * @types: an array of #PangoAttrType
 * @n_types: number of elements in @type
 *
 * Given a PangoAttrList and a set of types, remove any elements
 * from @list of those types, and insert them into a new list.
 *
 * Return value: a newly allocated %PangoAttrList or %NULL if
 *  no attributes of the given types were found.
 **/
PangoAttrList *
pango_attr_list_filter (PangoAttrList *list,
                        PangoAttrType *types,
                        int            n_types);

/**
 * pango_glyph_item_apply_attrs:
 * @glyph_item: a shaped item
 * @text: text that @list applies to
 * @list: a #PangoAttrList
 *
 * Splits a shaped item (PangoGlyphItem) into multiple items based
 * on an attribute list. The idea is that if you have attributes
 * that don't affect shaping, such as color or underline, to avoid
 * affecting shaping, you filter them out (pango_attr_list_filter()),
 * apply the shaping process and then reapply them to the result using
 * this function.
 *
 * This function takes ownership of @glyph_item; it will be reused
 * as one of the elements in the list.
 *
 * Return value: a list of glyph items resulting from splitting
 *   @glyph_item. Free the elements using pango_glyph_item_free(),
 *   the list using g_slist_free().
 **/
GSList *
pango_glyph_item_apply_attrs (PangoGlyphItem   *glyph_item,
                              const char       *text,
                              PangoAttrList    *list);

So PangoLayout can remove the troublesome attributes, do it's
shaping, then add them back in.

Created attachment 12366 [details]
Image of underlines not breaking Arabic text

(Yes, the underlines are ugly on such large text...)

One somewhat problematical issue was what to do with "clusters"
such as the lam-alef ligature in the screenshot. What I decided
to do was to simply say that if the attribute applied to the
first character in the cluster, it applied to the entire cluster.

So, though only the lam of the lam-alef is logically underlined,
the underline extends across the entire ligature. (This is 
different than the GTK+ selection behavior where we actually
draw the selection selecting half of the ligature.)

Created attachment 12367 [details]
pango-glyph-item.c [new file]

Created attachment 12368 [details]
pango-glyph-item.h [new file]

Created attachment 12369 [details] [review]
Remaining diffs

Note that there are some references to pango-script[-table].[ch]
in the above patch that would need to be removed after applying;
they are part of another change I was working on my local tree.

The main remaining thing that needs doing here is:

 a) Test the patch a bunch in practice
 b) Think about writing some automated test cases, especially
    for pango_glyph_item_apply_attrs() which is a bit tricky.

Just tried this patch. It works well for arabic, but it breaks latin
text. I made a screenshot here
http://amaoui.free.fr/misc/images/accels.jpeg

Tried on pango-1.1.1 that comes with RH 8.0

Yeah, I fixed that after posting the patch - there is

  if (ltr)
    g_slist_reverse (result);

At the and of pango-glyph-item.c should be:

  if (ltr)
    result = g_slist_reverse (result);

Works great now
http://amaoui.free.fr/gnome2/images/accels.png

Thanks a lot Owen

Sun Nov 17 23:28:26 2002  Owen Taylor  <otaylor@redhat.com>

        * pango/pango-glyph-item.[ch] pango/pango-layout.h:
        Rename PangoLayoutRun to PangoGlyphItem (with a
        typedef for compat), add pango_glyph_item_split(),
        pango_glyph_item_apply_attrs().

        * pango/pango-attributes.[ch]: Add
        pango_attr_list_filter(), pango_attr_iterator_get_attrs().

        * pango/pango-layout.c: Remove attributes that don't
        affect shaping before shaping, shape and then add
        them back. Fixes the infamous "underscores break
        arabic shaping" bug (#83058)

*** Bug 157019 has been marked as a duplicate of this bug. ***