refactor(typesetters): Simplify discretionary logic and line ratio co…

…mputation (#1980)

core/nodefactory.lua

@@ -287,6 +287,28 @@ function nodefactory.discretionary:toText ()
   return self.used and "-" or "_"
 end
 
+function nodefactory.discretionary:markAsPrebreak ()
+  self.used = true
+  if self.parent then
+    self.parent.hyphenated = true
+  end
+  self.is_prebreak = true
+end
+
+function nodefactory.discretionary:cloneAsPostbreak ()
+  if not self.used then
+    SU.error("Cannot clone a non-used discretionary (previously marked as prebreak)")
+  end
+  return SILE.nodefactory.discretionary({
+    prebreak = self.prebreak,
+    postbreak = self.postbreak,
+    replacement = self.replacement,
+    parent = self.parent,
+    used = true,
+    is_prebreak = false,
+  })
+end
+
 function nodefactory.discretionary:outputYourself (typesetter, line)
   -- See typesetter:computeLineRatio() which implements the currently rather
   -- messy hyphenated checks.
@@ -300,22 +322,13 @@ function nodefactory.discretionary:outputYourself (typesetter, line)
 
   -- It's possible not to have a parent (e.g. on a discretionary directly
   -- added in the queue and not coming from the hyphenator logic).
-  -- Eiher that, or we have a hyphenate parent.
+  -- Eiher that, or we have a hyphenated parent.
   if self.used then
     -- This is the actual hyphenation point.
-    -- Skip margin glue and zero boxes.
-    -- If we then reach our discretionary, it means its the first in the line,
-    -- i.e. a postbreak. Otherwise, its a prebreak (near the end of the line,
-    -- notwithstanding glues etc.)
-    local i = 1
-    while (line.nodes[i].is_glue and line.nodes[i].value == "margin")
-      or line.nodes[i].type == "zerohbox" do
-      i = i + 1
-    end
-    if (line.nodes[i] == self) then
-      for _, node in ipairs(self.postbreak) do node:outputYourself(typesetter, line) end
-    else
+    if self.is_prebreak then
       for _, node in ipairs(self.prebreak) do node:outputYourself(typesetter, line) end
+    else
+      for _, node in ipairs(self.postbreak) do node:outputYourself(typesetter, line) end
     end
   else
     -- This is not the hyphenation point (but another discretionary in the queue)

typesetters/base.lua

@@ -945,6 +945,11 @@ function typesetter:breakpointsToLines (breakpoints)
       local slice = {}
       local seenNonDiscardable = false
       for j = linestart, point.position do
+        if nodes[j].is_discretionary and nodes[j].used then
+          -- This is the used (prebreak) discretionary from a previous line,
+          -- repeated. Replace it with a clone, changed to a postbreak.
+          nodes[j] = nodes[j]:cloneAsPostbreak()
+        end
         slice[#slice+1] = nodes[j]
         if nodes[j] then
           if not nodes[j].discardable then
@@ -957,10 +962,12 @@ function typesetter:breakpointsToLines (breakpoints)
         SU.debug("typesetter", "Skipping a line containing only discardable nodes")
         linestart = point.position + 1
       else
-        -- If the line ends with a discretionary, repeat it on the next line,
-        -- so as to account for a potential postbreak.
         if slice[#slice].is_discretionary then
+          -- The line ends, with a discretionary:
+          -- repeat it on the next line, so as to account for a potential postbreak.
           linestart = point.position
+          -- And mark it as used as prebreak for now.
+          slice[#slice]:markAsPrebreak()
         else
           linestart = point.position + 1
         end
@@ -985,49 +992,31 @@ function typesetter:breakpointsToLines (breakpoints)
 end
 
 function typesetter.computeLineRatio (_, breakwidth, slice)
-  -- This somewhat wrong, see #1362 and #1528
-  -- This is a somewhat partial workaround, at least made consistent with
-  -- the nnode and discretionary outputYourself routines
-  -- (which are somewhat wrong too, or to put it otherwise, the whole
-  -- logic here, marking nodes without removing/replacing them, likely makes
-  -- things more complex than they should).
-  -- TODO Possibly consider a full rewrite/refactor.
   local naturalTotals = SILE.length()
 
-  -- From the line end, check if the line is hyphenated (to account for a prebreak)
-  -- or contains extraneous glues (e.g. to account for spaces to ignore).
-  local n = #slice
-  while n > 1 do
-    if slice[n].is_glue or slice[n].is_zero then
-      -- Skip margin glues (they'll be accounted for in the loop below) and
-      -- zero boxes, so as to reach actual content...
-      if slice[n].value ~= "margin" then
-        -- ... but any other glue than a margin, at the end of a line, is actually
-        -- extraneous. It will however also be accounted for below, so subtract
-        -- them to cancel their width. Typically, if a line break occurred at
-        -- a space, the latter is then at the end of the line now, and must be
-        -- ignored.
-        naturalTotals:___sub(slice[n].width)
-      end
-    elseif slice[n].is_discretionary then
-      -- Stop as we reached an hyphenation, and account for the prebreak.
-      slice[n].used = true
-      if slice[n].parent then
-        slice[n].parent.hyphenated = true
+  -- From the line end, account for the margin but skip any trailing
+  -- glues (spaces to ignore) and zero boxes until we reach actual content.
+  local npos = #slice
+  while npos > 1 do
+    if slice[npos].is_glue or slice[npos].is_zero then
+      if slice[npos].value == "margin" then
+        naturalTotals:___add(slice[npos].width)
       end
-      naturalTotals:___add(slice[n]:prebreakWidth())
-      slice[n].height = slice[n]:prebreakHeight()
-      break
     else
-      -- Stop as we reached actual content.
       break
     end
-    n = n - 1
+    npos = npos - 1
   end
 
+  -- Due to discretionaries, keep track of seen parent nodes
   local seenNodes = {}
+  -- CODE SMELL: Not sure which node types were supposed to be skipped
+  -- at initial positions in the line!
   local skipping = true
-  for i, node in ipairs(slice) do
+
+  -- Until end of actual content
+  for i = 1, npos do
+    local node = slice[i]
     if node.is_box then
       skipping = false
       if node.parent and not node.parent.hyphenated then
@@ -1043,29 +1032,25 @@ function typesetter.computeLineRatio (_, breakwidth, slice)
     elseif node.is_discretionary then
       skipping = false
       local seen = node.parent and seenNodes[node.parent]
-      if not seen and not node.used then
-        naturalTotals:___add(node:replacementWidth():absolute())
-        slice[i].height = slice[i]:replacementHeight():absolute()
+      if not seen then
+        if node.used then
+          if node.is_prebreak then
+            naturalTotals:___add(node:prebreakWidth())
+            node.height = node:prebreakHeight()
+          else
+            naturalTotals:___add(node:postbreakWidth())
+            node.height = node:postbreakHeight()
+          end
+        else
+          naturalTotals:___add(node:replacementWidth():absolute())
+          node.height = node:replacementHeight():absolute()
+        end
       end
     elseif not skipping then
       naturalTotals:___add(node.width)
     end
   end
 
-  -- From the line start, skip glues and margins, and check if it then starts
-  -- with a used discretionary. If so, account for a postbreak.
-  n = 1
-  while n < #slice do
-    if slice[n].is_discretionary and slice[n].used then
-      naturalTotals:___add(slice[n]:postbreakWidth())
-      slice[n].height = slice[n]:postbreakHeight()
-      break
-    elseif not (slice[n].is_glue or slice[n].is_zero) then
-      break
-    end
-    n = n + 1
-  end
-
   local _left = breakwidth:tonumber() - naturalTotals:tonumber()
   local ratio = _left / naturalTotals[_left < 0 and "shrink" or "stretch"]:tonumber()
   ratio = math.max(ratio, -1)