Hbox fixes for RTL languages

[mahmadzaid]

Closes #1796 (for me at least).

I know that bidi.reorder() was deprecated, and here I'm trying to export bidi.splitNodelistIntoBidiRuns() as well, as I'm not able to find a better way.

Packages that call typesetter:makeHbox() and wraps the results somehow are responsible for calling the above two exported functions, as found in the rules and rotate packages, like so:

diff --git a/packages/rules/init.lua b/packages/rules/init.lua
index 22f6885..28bb5d1 100644
--- a/packages/rules/init.lua
+++ b/packages/rules/init.lua
@@ -3,6 +3,8 @@ local base = require("packages.base")
 local package = pl.class(base)
 package._name = "rules"
 
+local bidi = require("packages.bidi")
+
 local function getUnderlineParameters ()
   local ot = require("core.opentype-parser")
   local fontoptions = SILE.font.loadDefaults({})
@@ -211,6 +213,8 @@ function package:registerCommands ()
     SU.deprecated("\\boxaround (undocumented)", "\\framebox (package)", "0.12.0")
 
     local hbox, hlist = SILE.typesetter:makeHbox(content)
+    hbox.value = bidi.splitNodelistIntoBidiRuns(hbox.value, SILE.typesetter)
+    hbox.value = bidi.reorder(nil, hbox.value, SILE.typesetter)
     -- Re-wrap the hbox in another hbox responsible for boxing it at output
     -- time, when we will know the line contribution and can compute the scaled width
     -- of the box, taking into account possible stretching and shrinking.

[Omikhleia]

Packages that call typesetter:makeHbox() and wraps the results somehow are responsible for calling the above two exported functions

That's a very strong assumption which is hard to guarantee in all packages that might build and use hboxes, no? I'd expect boxes to work without that kind of weird magic being exposed to calling packages! 🤕

[mahmadzaid]

Correct, they work fine as they did before, but with no bidi, since, well, bidi is never used on the content, they're already shaped in typesetter:makeHbox(). The manual, which I assume has enough cases, can be built with these changes and seems to look fine where rules and rotate are used.

I tried to handle the split into bidi runs and reordering in the bidi package itself, and it works, but I found that I have to deal with every new way of wrapping hboxes a new package would come up with, for example you have these in the rules package:

sile/packages/rules/init.lua

Lines 153 to 158 in faf0b57

	-- Re-wrap the hbox in another hbox responsible for boxing it at output
	-- time, when we will know the line contribution and can compute the scaled width
	-- of the box, taking into account possible stretching and shrinking.
	SILE.typesetter:pushHbox({
	inner = hbox,
	width = hbox.width,

And this in the rotate package:

sile/packages/rotate/init.lua

Lines 96 to 98 in faf0b57

	SILE.typesetter:pushHbox({
	value = { orig = origbox, theta = theta},
	height = height,

[mahmadzaid]

On a similar note, to have this done properly, UBA should be a core part (hopefully in the future), and in our case it would be called in typesetter:makeHbox(), there is this old discussion (#76) for reference.

Sorry if I come off as strongly opinionated 😟

[Omikhleia]

Packages that call typesetter:makeHbox() and wraps the results somehow are responsible for calling the above two exported functions (...)

Really, I don't think this would be acceptable (and I'm not going to do it in my 3rd party packages, which do a lot of hbox constructions). We'd need to solve the core issue, not monkey-patch it in unrelated packages.

[mahmadzaid]

Really, I don't think this would be acceptable (and I'm not going to do it in my 3rd party packages, which do a lot of hbox constructions).

And you don't have to. Your packages should work as they did before.

We'd need to solve the core issue, not monkey-patch it in unrelated packages.

I know it's not ideal, and as I mentioned in my last comment, bidi should be a core part, but since it's an optional package, I didn't think it would be fine to call it from typesetter:makeHbox(), which, has its share of hacks as per the comments.

If nothing else, this is, at least, supposed to fix our basic \hbox with bidi text.

Thank you for taking the time to look into this.

[Omikhleia]

but since it's an optional package (...)

BTW, bidi being loaded by the plain class, it's unlikely it's that "optional" in many workflows -- most other classes are most likely derived from plain, at least to get the very basic and general commands it provides.

[Omikhleia]

Plus, bidi overrides typesetter methods when on, and restores them when off...

sile/packages/bidi/init.lua

Lines 235 to 249 in 83d1423

	function package:bidiEnableTypesetter (typesetter)
	if typesetter.nobidi_boxUpNodes and self.class._initialized then
	return SU.warn("BiDi already enabled, nothing to turn on")
	end
	typesetter.nobidi_boxUpNodes = typesetter.boxUpNodes
	typesetter.boxUpNodes = bidiBoxupNodes
	end
	function package:bidiDisableTypesetter (typesetter)
	if not typesetter.nobidi_boxUpNodes and self.class._initialized then
	return SU.warn("BiDi not enabled, nothing to turn off")
	end
	typesetter.boxUpNodes = typesetter.nobidi_boxUpNodes
	typesetter.nobidi_boxUpNodes = nil
	end

So why not have the weird hacks there, similarly?

[mahmadzaid]

So why not have the weird hacks there, similarly?

A very good point, I did not think of that. This seems like a much better way.
Please take a look at the last commit. Thank you :)

[alerque]

Hey @mahmadzaid thanks for the contribution! This came it at a time that I was pretty overwhelmed, but I'm sorry the ball seems to have gotten dropped a little. I'll be giving it a go and figuring out how to get it in the next minor or major release.

Maybe you can answer a question though: I don't see any tests being changed here. Is there a specific test case we can add to our suite that would show what this is fixing and make sure it doesn't break again without warning us?

@Omikhleia Since you reviewed it before do you have anything to add or suggestions about versioning?

[Omikhleia]

Calling a package method with a nil self pointer and bypassing inheritance has heavy code smell.

[Omikhleia]

@Omikhleia Since you reviewed it before do you have anything to add or suggestions about versioning?

TBH, I don't really understand what it does and how bidi reordering is supposed to work. A review from someone actually using mixed LTR/RTL languages would be welcome. I am a bit embarrassed that bibi is enabled by default via the plain class, so this will be running even in contexts it's not needed, and we have to be careful.

[alerque]

I messed with this a bit locally because I'm not sure why the CI runs in this PR didn't even run, but what I found is that it isn't passing regression tests. This doesn't mean it is wrong, the test expectations themselves might actually be wrong. But it wasn't clear to me why they were different and whether the old or new result was correct.

At the very least before we merge this we have to either update the existing expectations with the new results and identify why the changes and note that the new results are the correct expectations, OR make it match the existing expectations.

Failed tests:
❌ tests/bug-1343-dotfill-stretch.sil
❌ tests/bug-1580.sil
❌ tests/bug-192.sil
❌ tests/bug-243.sil
❌ tests/bug-262.sil
❌ tests/bug-479-line-disappear.sil
❌ tests/bug-892.sil
❌ tests/feat-875-leaders-alignment.sil
❌ tests/twoside.sil

[Omikhleia]

I'm not sure why the CI runs in this PR didn't even run

I think first time contributors don't get the CI to be triggered?

[alerque]

I think first time contributors don't get the CI to be triggered?

First time contributors don't trigger CI jobs automatically, but it usually shows repo admins a button to approve them so they run manually. I don't have that for this PR (or perhaps I hit it earlier, I don't remember) the jobs show up as triggered but never finished ("waiting for status to be reported"). That's a bit unusual.