Text module Refactoring *WIP*

[jazmit]

WIP on Text refactoring. Anyone interested is welcome to comment and advise.
The idea is to normalize the internal representation of Text objects to {Array.<{text: string, style: Object.<string, string}>}. The keys and values of the style object will still be valid CSS keys and values, but this will make it easier to manipalate text objects without resorting to using the DOM API.
I've put a TODO by every function I think needs to change.

TODO

• Add tests for Text module
• Change Text representation
• Move text-related functions from Util to Text
• Add comments for Text module

[evancz]

I'm not sure what you have proposed as the data structure, but I think it'd be best to model it like any other union type in elm:

type Text = Literal String | Styled String String Text | Append Text Text

You can do all that in JS and mess with it to make it more efficient, but I'd think of like a union type.

Another thing to think about is how to get toString to still work.

[evancz]

I'm not sure what you have proposed as the data structure, but I think it'd be best to model it like any other union type in elm:

type Text = Literal String | Styled String String Text | Append Text Text

You can do all that in JS and mess with it to make it more efficient, but I'd think of like a union type.

Another thing to think about is how to get toString to still work.

[jazmit]

Ah.. lightbulb.. I was wondering why everything was implemented as linked lists.. so maybe we should keep the current representation but just replace the current CSS string with [key, value] pairs?

[jazmit]

Ah.. lightbulb.. I was wondering why everything was implemented as linked lists.. so maybe we should keep the current representation but just replace the current CSS string with [key, value] pairs?

[jazmit]

Is this the way to go for testing otherwise 'opaque' modules such as Text?

[jazmit]

Is this the way to go for testing otherwise 'opaque' modules such as Text?

[jazmit]

toString on a Text object returns "" for me.. is this intended behaviour?

[jazmit]

toString on a Text object returns "" for me.. is this intended behaviour?

[evancz]

Don't do this aligning stuff. It's really frail. When any line changes, lots of things need to change. Not worth it in the long run!

[evancz]

Always use curly braces!

[evancz]

Always bring the definition down a line:

tests =
  suite "Text Tests"
    [ ...
    ]

[evancz]

Same here, always bring it down.

[evancz]

testFromString =
  let result = toHtmlString (fromString "Hello")
  in
      test "simple" <| assertEqual "Hello" result 

[evancz]

I am unsure about how to ideally test this. If it is represented directly as a Union Type in Elm, we can run tests by making sure it's the value we expect. Perhaps we can do it this way and add a "secret" tag that only toString looks for. That'd mean making a special Union Type in JS:

function append(text1, text2) {
    return {
        ctor: 'Append',
        _0: text1,
        _1: text2,
        isText: true
    };
}

So it'll look like a union type to all of Elm, but when toString looks at it, it can check for isText.

And yes, I made it <internal structure> on purpose. It was not clear to me what the "right" way to visualize it given that it really is not a string and that Text.fromString "hello" is not the same as Text.bold (Text.fromString "hello"). I don't really know the right answer here, but I don't feel that we should expose the internal structure. What do you think?

Also, we were talking about moving some Text functions around. Might be something to consider here.

[evancz]

I am unsure about how to ideally test this. If it is represented directly as a Union Type in Elm, we can run tests by making sure it's the value we expect. Perhaps we can do it this way and add a "secret" tag that only toString looks for. That'd mean making a special Union Type in JS:

function append(text1, text2) {
    return {
        ctor: 'Append',
        _0: text1,
        _1: text2,
        isText: true
    };
}

So it'll look like a union type to all of Elm, but when toString looks at it, it can check for isText.

And yes, I made it <internal structure> on purpose. It was not clear to me what the "right" way to visualize it given that it really is not a string and that Text.fromString "hello" is not the same as Text.bold (Text.fromString "hello"). I don't really know the right answer here, but I don't feel that we should expose the internal structure. What do you think?

Also, we were talking about moving some Text functions around. Might be something to consider here.

[jazmit]

I think exposing type Text = Literal String | Styled String String Text | Append Text Text is probably not such a frightening idea.. after all I would think we'd want it to be possible to manipulate and process Text values from client code without depending on Native modules. However, this can be left for another pull request, as it won't necessarily make for great tests - they'd turn out kinda tautological:

assertEqual (Text.append text1 text2) (Append text1 text2)

If we're testing conversion to and from another format I'd be more confident that code changes aren't breaking anything. Since it's not outrageous that we might one day provide a toHtmlString : Text -> String function, I think that continuing with the current approach is probably OK. If one day this function does exist it just requires changing one line of the test.

[jazmit]

I think exposing type Text = Literal String | Styled String String Text | Append Text Text is probably not such a frightening idea.. after all I would think we'd want it to be possible to manipulate and process Text values from client code without depending on Native modules. However, this can be left for another pull request, as it won't necessarily make for great tests - they'd turn out kinda tautological:

assertEqual (Text.append text1 text2) (Append text1 text2)

If we're testing conversion to and from another format I'd be more confident that code changes aren't breaking anything. Since it's not outrageous that we might one day provide a toHtmlString : Text -> String function, I think that continuing with the current approach is probably OK. If one day this function does exist it just requires changing one line of the test.

[jazmit]

I'll have a think about moving the asText function to the Element module.

[jazmit]

I'll have a think about moving the asText function to the Element module.

[jazmit]

Any objections to pulling in the Elm QuickCheck library to help with the testing here?

[jazmit]

Any objections to pulling in the Elm QuickCheck library to help with the testing here?

[evancz]

Okay, things should be refactored now. Not sure about speed, but at least we are no longer going through HTML in the Collage code!

I think we need to look into how \n behaves in Text rendered to the collage? My guess is badly.

I don't mind about elm-check, but I think do that kind of thing separate from the Text stuff.

[evancz]

Okay, things should be refactored now. Not sure about speed, but at least we are no longer going through HTML in the Collage code!

I think we need to look into how \n behaves in Text rendered to the collage? My guess is badly.

I don't mind about elm-check, but I think do that kind of thing separate from the Text stuff.

[evancz]

As an aside, this whole addition is really cool :) I was testing it out to make sure everything was in order, and it was very fun!

[evancz]

As an aside, this whole addition is really cool :) I was testing it out to make sure everything was in order, and it was very fun!

[evancz]

All the Graphics.* modules have moved to evancz/elm-graphics so it makes sense to retarget stuff like this. That module also did get refactored for this change and I think it's a lot nicer.

[evancz]

All the Graphics.* modules have moved to evancz/elm-graphics so it makes sense to retarget stuff like this. That module also did get refactored for this change and I think it's a lot nicer.