search

timjs / elm-collage

Create interactive vector graphics and position them relative to each other
BSD 3-Clause "New" or "Revised" License
58 stars 19 forks source link

Simplistic size measurement of text causes overflow #36

Open hossameldeen opened 5 years ago

hossameldeen commented 5 years ago

Screenshot

image

Code

view : () -> Html Never
view _ =
    Collage.Text.fromString "hamada"
      |> Collage.rendered
      |> Collage.Render.svg
<body>
  <script>
    var app = Elm.Main.init({ node: document.querySelector('body') })
  </script>
</body>

Ellie

https://ellie-app.com/4mB5pLNhSJPa1

Description

Using the simplest way to render a string, sometimes some part of the beginning and end of it disappears. When using a string like foobarbaz, its rendered correctly. But when using, e.g., m, it is not.

Didn't go over all the characters that work and don't work correctly.

If you need any more details, please tell me.

danfishgold commented 5 years ago

I’m pretty sure this is due to the fact that width of text elements isn’t accurate: calculating the real width of the string requires a synchronous call to JavaScript (using Native code, which is forbidden in Elm 0.19), so instead the package gives an estimate based on the font size.

Since m is a relatively wide character the estimation is off.

The package determines the size of the svg based on the size of the elements within, so the width is a little too small.

One possible solution would be to use a monospace font.

Another would be to use Collage.Render.svgBox with manual width and height.

I’m guessing this is a known issue, but I don’t think there’s a way to fix it 😕

hossameldeen commented 5 years ago

No problem. Thank you for your reply.

If I'd use Collage.Render.svgBox with manual width and height, how would I determine the width and height?

timjs commented 5 years ago

Yes, @danfishgold is completely right. In Elm 0.18, the library used a native call to the HTML canvas to measure the width of some text. In Elm 0.19 this is not allowed any more and I reverted to a very simplistic calculation using the font size and the string length. I'm open to any suggestions to make this work again.

Ideally, we would use some font library that measures text. Obviously, we can't make bindings to an existing JavaScript one any more, due to the restrictions in Elm 0.19.

Regrettably, I don't think ports are the way to go. If my understanding is correct, this would require every developer using this library to wire up to a port themselves. I think requiring every developer to do this is bad library design.

hossameldeen commented 5 years ago

@timjs Thank you for your comprehensive reply.

For me, I wouldn't mind wiring ports, but my problem with ports is asynchronicity. I've tried to think of a design that would allow the user to just wire some ports but couldn't. Do you have one in mind?

hossameldeen commented 5 years ago

Note: I've made a post on Elm Discourse in case anyone there has a work-around in mind.

timjs commented 5 years ago

@timjs Thank you for your comprehensive reply.

You're welcome.

For me, I wouldn't mind wiring ports, but my problem with ports is asynchronicity. I've tried to think of a design that would allow the user to just wire some ports but couldn't. Do you have one in mind?

Nope, when using ports it will always be async I guess.

Note: I've made a post on Elm Discourse in case anyone there has a work-around in mind.

Wonderful! Keep me posted if any solution comes up. I see decoding font files could be an option, but it is also async.

michaelmesser commented 4 years ago

Forking the compiler works. Change this line to always return true: https://github.com/elm/compiler/blob/6c19c6e202984e2be98d6abe46b616751f712e94/compiler/src/Elm/Package.hs#L85 Rework to the package to use the new Kernel system instead of Native. I did it and it works.

michaelmesser commented 4 years ago

@timjs I believe I figured out a way around this so that users would not have to use a modified compiler. It would require a couple of steps:

If you are interested I can I perform further testing.

michaelmesser commented 4 years ago

@timjs This command installs a prebuilt package for 0.19.1. I built it with a custom elm compiler. The elm file in the archive is just dummy code that returns 0, but the artifacts.dat was built from the other elm code which calls a kernel function.

mkdir -p ~/.elm/0.19.1/packages/2426021684/elm-text-width/1.0.1 && curl -L https://github.com/2426021684/elm-text-width/archive/1.0.1.tar.gz | tar -xz --strip 1 -C ~/.elm/0.19.1/packages/2426021684/elm-text-width/1.0.1

After running that command you can use 2426021684/elm-collage with normal elm 0.19.1 and everything works.

timjs commented 4 years ago

Nice workaround! Don’t have time to test it right away though, maybe somewhere next week.

How would a user of the package use and install this library? In the same way as above? If Elm itself won’t provide a convenient way to solve our problem, this is a good one for everybody that wants to do some extra work to get it

Sent with GitHawk

michaelmesser commented 4 years ago

2426021684/elm-text-width is installed with the command I wrote. 2426021684/elm-collage can be installed the normal elm way after 2426021684/elm-text-width is installed.

michaelmesser commented 4 years ago

If you want to use this method, I wrote a guide on how.

timjs commented 4 years ago

Excellent! Can you maken a PR with te required code and add a description how to use it to the readme? I’ll review it

Sent with GitHawk

michaelmesser commented 4 years ago

@timjs So this consists of three parts:

  1. Dummy text width package with simple calculation
  2. The prebuilt text width package with the kernel code
  3. Small changes to elm-collage

Only 3 makes sense as a PR to this repo. How do you want 1 and 2?

timjs commented 4 years ago

I take 3 is a small change to elm-collage instead of elm-diagrams? And can the dummy text width be built in in this library or do you really need a separate package? Otherwise we’ll add a note to the readme how to use the dummy package and your setup

Sent with GitHawk

michaelmesser commented 4 years ago

While it is possible to put it all in elm-collage, it is better to put it in a separate package. Otherwise the examples don’t work and updating elm-collage becomes more difficult when using the kernel code. There needs to be two versions of the separate package. Do you want to maintain the second package? The dummy package would be install by elm-collage automatically. I could make the dummy fail to compile and require users to use the real one. I will most likely not have the time to maintain the package in the future.