So is this technically redundant with HB defaults? I'm slightly confused by the comment following this line.

Half :) I've indeed been unclear, but that's mostly just cut and pasted from crengine.
And what the HB defaults are are also hard to track. It's in its src/hb-ot-shape.cc.
So, it looks to me +liga is the default, but not +kern (or may be it is...).
I'm just setting here the defaults we use in crengine HB full/best, even if one of them is already the default.
The commented next line is the list of all stuff mentionned in HB "good", that I just cut and pasted with all disabled to see if enabling +liga / +kern made a difference :)
I don't plan on being the reference as I'm not sure what it is :|. I'll update the comment to not state anything about "default" but "If we'd wanted to disable all the features HB may enable" :)

The order sounds a bit weird to me.

Disable enhanced UI text shaping

Excellent!

(Nothing excellent :) that's some previous comment that I just moved - it appears added here, but it's the same that is deleted below :)

Oh right, I remember now. 🤦‍♂️

Still excellent though. ;-D

When does this happen?

Should not, but can if we provide a number or else.
We'll see it displayed instead of crashing, and it simplify my next check:
if not self.text or #self.text == 0 then self._is_empty = true self._length = 0
It avoid having nil fixed/handled in lower level code, like in:

Would something this work in this context? (That'd go underneath the method.) It's a convenient way to ensure conformity without bothering users.

dbg:guard(TextWidget, "updateSize",
    function()
        assert(type(self.text) == "string",
            "Wrong text type (expected string)")
    end)

I can, it works as you expect (crash on the emulator). Same in setText(text) then.
I think I've seen some output from logger.warn("renderUtf8Text called without text") in the past, so it may happen we get a nil. It's ok if we crash then when that happens?
(Can't cause any crash while tapping around - but I'm sure we'll get some.)

Assuming it only crashes when it comes across a problem so that the problem can be addressed, that's indeed the idea. ;-)

But if it's too big an issue atm then it'd probably be best to leave that for a follow-up PR.

I've already added and tested it, so it will go in :)
If you confirm dbg.guard() does not kick in nor add overhead on our devices, it's fine with me - as long as I get there the safety of my tostring() (othewise, XText Lua check will throw an error).

Yeah, we possibly have a few nil renderUtf8Text lying around, that does ring a bell ;).

No overhead, when debug is disabled it's just nothing:

And when enabled:

This still relevant?

Not with the use_xtext code (Haven't checked, but I hope HB does the right thing with monospace fonts.
But for the legacy code, it might (it would still use kerning with monospaced fonts).

OK, I'll remove the @todo and keep that comment in parenthesis. As we'll probably don't have to fix it if we keep using xtext.

So this is always disabled unless you change it in the code?

(Might be of some interest in that developer settings menu.)

So this is always disabled unless you change it in the code?

Right, always disabled.
Not worth busying developper settings with it I think - was useful when doing this PR, but I guess RTL is alright now, and I shouldn't need it - unless we feel it's needed when we play with mirroring the UI, but I doubt it.

Free when UIManager:close() was called.

(I mean, it just looks so out of place with the next comment. :-P)

Yes :) That's verbatim from the other widgets that have it, so, I just kept it for coherency even if it sounds gramatically strange.
And... it's I that introduced that strange wording :)

ressources is French ^_^

French ressources need freeing too ! :)

Doing the proper math in a real matching function in ffi/framebuffer.lua might be worth it?

(i.e., having a real px to pt helper function available).

Probably, we used that same trick in coverbrowser some weeks ago.
(But I suck at proper math :)

Apparently as simple as basically copying scaleBySize but changing the return to math.floor(px / (size_scale + dpi_scale) * 2) ;).

So, should it be that (scaleBySize() input being pt and not px)?

-function fb:scaleBySize(px)
+function fb:scaleBySize(pt)
     -- larger screen needs larger scale
@@ -299,3 +299,15 @@ function fb:scaleBySize(px)
     -- scaled positive px should also be positive
-    return math.ceil(px * (size_scale + dpi_scale) / 2)
+    return math.ceil(pt * (size_scale + dpi_scale) / 2)
+end
+
+function fb:unscaleBySize(px)
+    -- Revert of previous function:
+    -- px = scaleBySize(pt)
+    -- pt = unscaleBySize(px)
+    local size_scale = math.min(self:getWidth(), self:getHeight())/600
+    local dpi_scale = size_scale
+    if self.device and self.device.display_dpi ~= self.dpi then
+        dpi_scale = self.dpi / 160
+    end
+    return math.floor(px / (size_scale + dpi_scale) * 2)
 end

Well, it's not technically points per-se, as that's pretty much set in stone as px = round(dpi / 72.0 * pt), which isn't what we're doing.

It's more something like absolute pixels or whatever (I only mentioned point earlier because the theory is similar ;)).

Not a Python quirk ;).

#!/usr/bin/env luajit

dpi = 167
width = 600
height = 800

function scaleBySize(px)
   -- larger screen needs larger scale
   size_scale = math.min(width, height)/600
   -- if users custom screen dpi, also scale by dpi
   dpi_scale = size_scale

   dpi_scale = dpi / 160

   -- scaled positive px should also be positive
   scaled_px = px * (size_scale + dpi_scale) / 2
   return {scaled_px, math.ceil(scaled_px)}
end

function unScaleBySize(px)
   -- larger screen needs larger scale
   size_scale = math.min(width, height)/600
   -- if users custom screen dpi, also scale by dpi
   dpi_scale = size_scale

   dpi_scale = dpi / 160

   -- scaled positive px should also be positive
   unscaled_px = px / (size_scale + dpi_scale) * 2
   return {unscaled_px, math.floor(unscaled_px)}
end

---

function tprint(tbl, indent)
  if not indent then indent = 0 end
  for k, v in pairs(tbl) do
    formatting = string.rep("  ", indent) .. k .. ": "
    if type(v) == "table" then
      print(formatting)
      tprint(v, indent+1)
    elseif type(v) == 'boolean' then
      print(formatting .. tostring(v))
    else
      print(formatting .. v)
    end
  end
end

print("scaleBySize(500)")
tprint(scaleBySize(500))
print("unScaleBySize(511)")
tprint(unScaleBySize(511))

print("scaleBySize(20)")
tprint(scaleBySize(20))
print("unScaleBySize(21)")
tprint(unScaleBySize(21))

dpi = 167 * 3

print("scaleBySize(18)")
tprint(scaleBySize(18))
print("unScaleBySize(38)")
tprint(unScaleBySize(38))

print("18 via * 10**6")
tprint(scaleBySize(18 * 1000000 / scaleBySize(1000000)[2]))

scaleBySize(500)
1: 510.9375
2: 511
unScaleBySize(511)
1: 500.06116207951
2: 500
scaleBySize(20)
1: 20.4375
2: 21
unScaleBySize(21)
1: 20.550458715596
2: 20
scaleBySize(18)
1: 37.18125
2: 38
unScaleBySize(38)
1: 18.39636913767
2: 18
18 via * 10**6
1: 18
2: 18

TL;DR: Smells like yet another case of "I don't know what the hell's happening inside the testsuite, but it doesn't make any sense" ;).

(GH is still being clunky, I've only just now got your previous answers interleaved in the thread -_-").

GH has recently been "helpfully" hiding comments behind a "More comments" type thing for me. D:

The presumed default used to be 800x600 @ 167 but that was changed to 160 for easier math. See koreader/koreader-base@c978857

Otherwise the unit test would make no sense! ;-)

        fb:setDPI(167)
        assert.are.equals(31, fb:scaleBySize(30))

        fb:setDPI(167 * 3)
        assert.are.equals(62, fb:scaleBySize(30))

Or at least, I supported it for that reason: koreader/koreader-base#843 (comment)

I'm wondering if maybe we should switch the other one to 160 instead since it's basically just a bit of legacy. It's practically the same thing. I doubt you'll even notice a difference in pixels except at really high DPI, except 160 DPI is exactly one inch making for much easier (head) calculations.

Yeah, we possibly have a few nil renderUtf8Text lying around, that does ring a bell ;).