[Draft] Support SheenBidi - #139

Can you added a GitHub Actions workflow with sheenbidi enabled? I’d like to see what tests are failing.

Can you added a GitHub Actions workflow with sheenbidi enabled? I’d like to see what tests are failing.

Just pushed a new commit. I don't know GitHub Actions but hopefully it just works

The test failures show that bidi is not being performed at all, all runs have level 0.

The test failures show that bidi is not being performed at all, all runs have level 0.

I'm afraid I won't be of any help about this, unfortunately.

Something is not right, but I don’t think I’ll get to the base of it without debugging the code locally.

FWIW this patch gives me 38 passes and 8 fails

diff --git a/src/raqm.c b/src/raqm.c
index a82dd87..6fe6491 100644
--- a/src/raqm.c
+++ b/src/raqm.c
@@ -1176,6 +1176,7 @@ _raqm_itemize (raqm_t *rq)
 #ifdef RAQM_SHEENBIDI
   raqm_bidi_char_type_t *sheenbidi_types;
   raqm_bidi_level_t *sheenbidi_levels;
+  const SBRun *sheenbidi_runs;
   SBAlgorithmRef bidiAlgorithm;
   SBParagraphRef firstParagraph;
   SBLineRef paragraphLine;
@@ -1250,7 +1251,11 @@ _raqm_itemize (raqm_t *rq)
       SBAlgorithmCreateParagraph (bidiAlgorithm,
                                   0,
                                   INT32_MAX,
+#if 0
                                   par_type);
+#else
+                                  SBLevelDefaultLTR);
+#endif
 
     SBUInteger paragraphLength =
       SBParagraphGetLength (firstParagraph);
@@ -1297,11 +1302,13 @@ _raqm_itemize (raqm_t *rq)
      rq->resolved_dir = RAQM_DIRECTION_RTL;
   }
 
+#if 0
   if (max_level == 0)
   {
     ok = false;
     goto done;
   }
+#endif
 
   if (!_raqm_resolve_scripts (rq))
   {
@@ -1311,11 +1318,23 @@ _raqm_itemize (raqm_t *rq)
 
   /* Get the number of bidi runs */
 #ifdef RAQM_SHEENBIDI
+#if 0
   runs = _raqm_reorder_runs (sheenbidi_types,
                              rq->text_len,
                              par_type,
                              sheenbidi_levels,
                              &run_count);
+#else
+  runs = malloc (sizeof (_raqm_bidi_run) * run_count);
+  sheenbidi_runs = SBLineGetRunsPtr(paragraphLine);
+
+  for (size_t i = 0; i < run_count; ++i)
+  {
+    runs[i].pos = sheenbidi_runs[i].offset;
+    runs[i].len = sheenbidi_runs[i].length;
+    runs[i].level = sheenbidi_runs[i].level;
+  }
+#endif
 #else
   runs = _raqm_reorder_runs (types,
                              rq->text_len,

Looks good. May be refactor out the old bidi run creation code into a separate function, say _raqm_itemize_bidi(), and add a sheenbidi version of and call it from _raqm_itemize()

(in order to fully ustilize shhenbidi API and don’t try to reorder the runs ourselves)

Looks good. May be refactor out the old bidi run creation code into a separate function, say _raqm_itemize_bidi(), and add a sheenbidi version of and call it from _raqm_itemize()

I'm not sure I understand what should be done. Wouldn't that imply changing the parameters of the function depending on the bidi library we use? Could you review & annotate e2ba7da?

Looks good. May be refactor out the old bidi run creation code into a separate function, say _raqm_itemize_bidi(), and add a sheenbidi version of and call it from _raqm_itemize()

I'm not sure I understand what should be done. Wouldn't that imply changing the parameters of the function depending on the bidi library we use? Could you review & annotate e2ba7da?

Looks good.My suggestion was since now the two bidi implementations don’t share any code, it would be better to untangle it and move each to as separate function. Some thing in the line of:

#ifndef RAQM_SHEENBIDI
static _raqm_bidi_run *
_raqm_bidi_itemize (raqm_t *rq) {
// All FriBiDi code goes here
}
#else
static _raqm_bidi_run *
_raqm_bidi_itemize (raqm_t *rq) {
// All SheenBiDi code goes here
}
#endif

static bool
_raqm_itemize (raqm_t *rq)
{
...
  /* Get the number of bidi runs */
  runs = _raqm_bidi_itemize (rq);
...
}

It looks like we have to return other arrays (levels, types) as well as runs;
Maybe we can define a struct to communicate the required pointers and ifdef its fields depending on the Bidi backend used?
Or maybe we can just use pointers to pointers as parameters to _raqm_bidi_itemize?

Or maybe we can just use pointers to pointers as parameters to _raqm_bidi_itemize?

That would be my preferred way.

Actually I was wrong it can be kept simple: 98c0e5b

Of the currently failing tests, cursor_position_GB3 is bad test as it involves LF and that shouldn't be part of the text feed to raqm. We should disable for now. I need to investigate the rest.

OK, I think I know how to fix the rest of the failures. I’ll be leaving some cosmetic comments and once this is done, I’ll push the fixes.

Please merge all the commits into three commits. One for the typo in the README.me, one for replacing FriBiDi utility functions and one for the rest.

Please merge all the commits into three commits. One for the typo in the README.me, one for replacing FriBiDi utility functions and one for the rest.

I just force-pushed, and will be addressing your comments in new commits before re-squashing them into the last one

Of the currently failing tests, cursor_position_GB3 is bad test as it involves LF and that shouldn't be part of the text feed to raqm. We should disable for now. I need to investigate the rest.

Indeed, this is the only test that still fails :)

Indeed, this is the only test that still fails :)

Yes, please just comment it out for now.

Indeed, this is the only test that still fails :)

Yes, please just comment it out for now.

Done.

Linking SheenBiDi is failing on Windows, if you can’t debug this I’m OK with disabling the SheenBiDi Windows job for now.

Linking SheenBiDi is failing on Windows, if you can’t debug this I’m OK with disabling the SheenBiDi Windows job for now.

I should be able to debug this

Well, simply using mingw under Windows works fine for me so I actually have no idea what's going on, sorry.

Should I remove the windows job for SheenBidi ?

Should I remove the windows job for SheenBidi ?

Yes. Probably the new SheenBiDi meson build is not working well with MSVC (at least as a submodule), but that OK for me.

Should be fine now.

Thanks!

No problem. Thank you for your help.