Text wrapping

[saitonakamura]

It's ugly and buggy right now (code is just plain shit), but it's working

UPD:
It's better now

Fixes #165

[OhadRau]

The code isn't that bad right now. There's a lot of imperative stuff you can't really get around for this type of problem if you want it to perform well. I do have a few suggestions so I'll open up a review to discuss those.

[OhadRau]

Just wanted to say this is really awesome to have and it's a huge issue that we've been wanting to tackle for a while now. I left some comments on some ideas I think we could add to improve the API (let me know what you think of these) as well as some questions about the current code/some cleanup we can do. I'm also playing around with it on my machine to see if I can spot any more bugs, so I'll keep you updated on that front but otherwise this looks pretty good and we can hopefully merge this in pretty soon.

[bryphe]

I like that you created a module to encapsulate our TextWrapping logic - thanks! Just thinking through this interface and using it in TextNode is a big step forward 🎉

[saitonakamura]

I fully rewrote the wrapping algorithm. Read a plethora of stuff on the matter. Unfortunately, most of the algorithms are quite naive in terms of thinking about kerning and different spaces sizes, so I had to use my own. It still has bugs (like not taking non-breaking space into consideration, for instance), but it's pretty solid in terms of measurement methink

[OhadRau]

This is awesome @saitonakamura! Thanks so much for taking the time to work on this.

This looks super good on the Text example, but this seems to have broken some other stuff:

Looks like all fonts are also rendering a few pixels too high now, which results in the todo text getting cut off. I think maybe the size given to the text node by the layout engine doesn't match the final size after rendering, which causes it to overflow its bounds or the size given to it has too little vertical height at least... (see the Focus example or the fact that the text is rendering on top of the border instead of inside it)

Will take a look at the code and see if I can track it down

EDIT: I guess one of the issues here is that these other examples don't expect wrapping to be enabled by default, so this may be old code we have to fix. But maybe we can come up with a good default for the lineHeight style?

[OhadRau]

The code really does look great right now, really glad to see this PR progress. I pointed out 2 bugs I found, but these should hopefully be quick fixes. Other than that, this PR is basically ready to be merged 🎉

[saitonakamura]

@OhadRau I fixed the bugs and all other review points. Now I only have two concerns:

1. CI builds if failing, because tests are failing with

Error: No implementations provided for the following modules:
         Str referenced from src/UI/Revery_UI.cmxa(Revery_UI__TextNode)

I'm not sure what to do with this, I need Str module to do the regex matching for spaces and stuff

2. @bryphe did an excellent job on calculating font baseline in Fix #301 - Set up baseline when rendering text #326 , so I rebased on that to fix font placement issue. However current placement is a bit off to the bottom. I'm not sure if I introduced this or a baseline still bit inaccurate.

[OhadRau]

Wow that was super fast! I'm playing with it now and it feels awesome. For #1, I believe the Str module in OCaml isn't loaded by default. Adding str into the dune file should fix this (not sure why it partially works without right now...), but we could also probably do this without a regex (e.g. using a loop). For example:

let breakpoints = str => {
  let found = ref([]);
  for (i in 0 to String.length(str) - 1) {
    switch (str.[i]) {
    | ' ' | '\t' | '\n' | '\x0c' => found := [i, ...found^];
    | _ => ()
    }
  };
  found^ /* Note, this list is backwards here */
};

As for #2, this looks very similar to what I see on the master branch. The only place where I noticed slightly different results was on the animation where the text is rendered above the outrun logo -- I have no idea why that happened. Will update if I find out why (edit: actually it looks like it may have been a fluke because it's working now ...)

One last thing before we merge this, can we change the printing to either depend on a debug flag (like REVERY_DEBUG being set) or just be disabled altogether? Disabling the printing gives a significant performance boost on Windows

[bryphe]

Just tried it out and it works well - awesome work @saitonakamura ! So nice to have a text wrapping solution.

One last thing before we merge this, can we change the printing to either depend on a debug flag (like REVERY_DEBUG being set) or just be disabled altogether? Disabling the printing gives a significant performance boost on Windows

Noticed this too - yes, we should definitely disable the printing or defer it to a debug logging.

The only other blocker I see is the formatting check is failing - might need to run esy format to get those checks green. Let me know if you need any help with this or the CI 👍

Otherwise, looks great!

[saitonakamura]

we could also probably do this without a regex

I used regex to be able to detect all kinds of spaces. For now, I moved to just string equality. However I can't seem to make unicode escaping like "\u{2001}" to work (Took the syntax from this PR). The good news (well, not so good) are that we can't render unicode yet, hence we can forget about other unicode spaces for now (I left a todo).

can we change the printing to either depend on a debug flag

I decided to clean the logging altogether.

@bryphe I fixed the tests by not using Str. module, cleaned the logging and gave some vars nicer names. I think it's ready to be merged, but CI is failing with

File "examples/TextExample.re", line 44, characters 45-62:
Error: Unbound type constructor React.Hooks.empty

https://dev.azure.com/revery-ui/revery/_build/results?buildId=973&view=logs
I can't reproduce this locally and I don't understand why is that. Slider example, for instance, doesn't complain, despite it's almost identical in terms of using React.Hooks

[bryphe]

However I can't seem to make unicode escaping like "\u{2001}" to work (Took the syntax from this PR). The good news (well, not so good) are that we can't render unicode yet, hence we can forget about other unicode spaces for now (I left a todo).

Interesting. We can follow up and investigate adding Str. In theory we should be able to use Unicode - it just might involve the user of a separate lib. But we can investigate this in another issue 👍

@bryphe I fixed the tests by not using Str. module, cleaned the logging and gave some vars nicer names. I think it's ready to be merged, but CI is failing with

Sorry about this! I upgraded to the latest brisk-reconciler which had the following API changes:

• Removed the need for React.Hooks.empty
• Change the return of components to require a tuple of (hooks, <View ... />) instead of just <View />

I'll help you merge this up 👍

[bryphe]

Build is green! 🎉 I'm ready to merge this in if you are @saitonakamura .

Thanks for the incredible work & investigation on this - huge step forward for Revery to have text wrapping! And thanks @OhadRau for your help and reviewing the changes.

[saitonakamura]

@bryphe go for it

And thank you actually, @bryphe and @OhadRau for your immense help on this!