search

revery-ui / revery

:zap: Native, high-performance, cross-platform desktop apps - built with Reason!
https://www.outrunlabs.com/revery/
MIT License
8.06k stars 196 forks source link

chore(monorepo): bring in revery-text-wrap #947

Closed zbaylin closed 4 years ago

zbaylin commented 4 years ago

This brings in @OhadRau's revery-text-wrap into the monorepo. In order to make fallback work properly, we have to make this lib Unicode/UTF-8 aware. Since we already have Zed in here as of #941, I think it makes sense to also bring this into the monorepo.

Note that this PR does not patching to the lib for Zed, just brings it into the repo.

Thanks again @OhadRau for your text-wrap implementation!

bryphe commented 4 years ago

Thanks for bringing this in @zbaylin , and @OhadRau for the initial implementation!

Is there a reason the test wasn't brought over? https://github.com/OhadRau/revery-text-wrap/blob/master/wrapTest.ml

It'd be great to have some test cases in place, so that we can build / add them for the UTF-8 implementation.

The wrap tests might be a good fit for inline tests, maybe?

zbaylin commented 4 years ago

Thanks for bringing this in @zbaylin , and @OhadRau for the initial implementation!

Is there a reason the test wasn't brought over? https://github.com/OhadRau/revery-text-wrap/blob/master/wrapTest.ml

It'd be great to have some test cases in place, so that we can build / add them for the UTF-8 implementation.

The wrap tests might be a good fit for inline tests, maybe?

Thanks for reminding me! I added them in 059bcd0 as ReveryTextWrapCli and added the native and bytecode tests to CI in 059bcd0!

OhadRau commented 4 years ago

Hey sorry! Haven't had a whole lot of time to look at this today, but one thing I did notice is that we're not carrying over the OPAM package right now? I'm not sure what the situation looks like for our unicode lib (Zed I guess?) but I think it would make sense to make both of these available as OPAM libs. Not a huge priority but it might be a little confusing to keep developing our internal version while not publishing those changes back to the existing OPAM package.

zbaylin commented 4 years ago

Thanks @OhadRau! As of right now it's bundled with the Revery opam package -- not sure if we can include it in two separate packages but it's something I can look into. Since it will soon depend on Zed I'm not sure how feasible that would be either.

Thanks for the approval!