Subject: bug#41005: problem with rendering Persian text in Emacs 27

From: Eli Zaretskii <[email-masked]>
Date: Thu, 04 Jun 2020 16:15:07 +0300

> From: Pip Cet <[email-masked]>
> Cc: Eli Zaretskii <[email-masked]>,  [email-masked],
>   [email-masked]
> Date: Thu, 04 Jun 2020 08:28:24 +0000
> 
> What happens is that font-shape-gstring is called with direction == L2R,
> mis-shapes the text, then caches that version in the composition gstring
> cache. The cache doesn't distinguish directions, and it's never cleared,
> so this "infects" other buffers, but only if they're opened afterwards,
> and only for the same words.
> 
> shr appears to force bidi-display-reordering off. Removing that let
> binding from shr-insert-document avoids the problem.

Right, thanks.  When I added that binding of bidi-display-reordering,
we didn't yet have HarfBuzz, and the other shapers' Arabic shaping
wasn't affected by the local text direction like HarfBuzz is.

> We should consider adding direction to our gstrings, on master. While
> we're there, let's also add script, language, and harfbuzz features to
> the gstrings so we know we've captured the precise harfbuzz parameters?

On emacs-27, I can fix this by a simpler band-aid below.

On master, I think we should indeed add direction to the cached
gstrings, as there could be other much more subtle situations where
looking at bidi-display-reordering is not enough, and we could then
still cache a composition with the wrong direction.  Patches welcome.

As for script and language, for now adding them would just waste
memory, since we don't yet have any meaningful support for
buffer-local, let-alone paragraph-local, scripts and languages.  When
we added HarfBuzz support, I considered adding some functionality to
determine language and/or script from the codepoints, but decided that
it made little sense, since HarfBuzz already does exactly that in
hb_buffer_guess_segment_properties.  So I think we should add to the
FIXME in hbfont.c the fact that when we do support those internally in
Emacs, we should add these attributes to cached gstrings, but not yet
actually add them for now.

Here's the patch I propose for emacs-27:

diff --git a/src/hbfont.c b/src/hbfont.c
index 576c5fe..4b3f64e 100644
--- a/src/hbfont.c
+++ b/src/hbfont.c
@@ -26,6 +26,7 @@
 #include "composite.h"
 #include "font.h"
 #include "dispextern.h"
+#include "buffer.h"
 
 #ifdef HAVE_NTGUI
 
@@ -438,7 +439,11 @@ hbfont_shape (Lisp_Object lgstring, Lisp_Object direction)
 
   /* If the caller didn't provide a meaningful DIRECTION, let HarfBuzz
      guess it. */
-  if (!NILP (direction))
+  if (!NILP (direction)
+      /* If they bind bidi-display-reordering to nil, the DIRECTION
+	 they provide is meaningless, and we should let HarfBuzz guess
+	 the real direction.  */
+      && !NILP (BVAR (current_buffer, bidi_display_reordering)))
     {
       hb_direction_t dir = HB_DIRECTION_LTR;
       if (EQ (direction, QL2R))