Add support for formatted text/vt-100 formatting escape codes #69

New Issue

asdf · 2019-11-01T01:04:09Z

asdf commented

2019-11-01 01:04:09 +00:00

It seems that the client has changed so much that phlogs with colours and other formatted text now show the escape sequences as plain text. Some examples:

gopher://tilde.pink:70/1/~fc03cf/
gopher://tilde.black:70/1/users/genin

In bombadillo 1.0 these were displayed as intended by the author.

What are your thoughts about this @sloum?

It seems that the client has changed so much that phlogs with colours and other formatted text now show the escape sequences as plain text. Some examples: - gopher://tilde.pink:70/1/~fc03cf/ - gopher://tilde.black:70/1/users/genin In bombadillo 1.0 these were displayed as intended by the author. What are your thoughts about this @sloum?

sloum was assigned by asdf

2019-11-01 01:04:09 +00:00

sloum commented

2019-11-01 03:08:46 +00:00

There isnt a reason that I can conjure that would prevent formatted text from working. However, there are a good many reasons to not allow formatted text to work:

If the bookmarks are open then the formatting will either continue for the rest of the whole screen (if the close or end to the formatting would have been found underneath the bookmarks) or will affect the bookmarks themselves (if the end/close to a style would have been on a future line due to wrapping).
Our string wrapping does not handle escape sequences well as far as how they get counted. As far as len(somestring) is concerned, the escape sequences take up length.
They make the client unreliable and it is very difficult to manage every edge case. For example, what if a requested file uses escape sequences that move the cursor around? Suddenly the whole screen output is donked.

Again though, I'm not sure what in the current code would prevent this from happening currently. I'll have to dig through and see what I can find.

Actually, that was easy. It is line 13 of page.go. When wrapping a line, I have things set to ignore ascii character 27 (escape), among others. I think the reasons listed above are good enough to warrant this behavior... but I am open to hearing other suggestions on how to handle it. There are a lot of different escape codes and a lot of different versions of those escape codes (though people mostly use VT100)... it would create a huge ball of code to try to allow some but not others :-/ I'm not sure what the best action is here.

There isnt a reason that I can conjure that would prevent formatted text from working. However, there are a good many reasons to not allow formatted text to work: - If the bookmarks are open then the formatting will either continue for the rest of the whole screen (if the close or end to the formatting would have been found underneath the bookmarks) or will affect the bookmarks themselves (if the end/close to a style would have been on a future line due to wrapping). - Our string wrapping does not handle escape sequences well as far as how they get counted. As far as `len(somestring)` is concerned, the escape sequences take up length. - They make the client unreliable and it is very difficult to manage every edge case. For example, what if a requested file uses escape sequences that move the cursor around? Suddenly the whole screen output is donked. Again though, I'm not sure what in the current code would _prevent_ this from happening currently. I'll have to dig through and see what I can find. Actually, that was easy. It is line `13` of `page.go`. When wrapping a line, I have things set to ignore ascii character 27 (escape), among others. I think the reasons listed above are good enough to warrant this behavior... but I am open to hearing other suggestions on how to handle it. There are a lot of different escape codes and a lot of different versions of those escape codes (though people _mostly_ use VT100)... it would create a huge ball of code to try to allow some but not others :-/ I'm not sure what the best action is here.

sloum added the

non-urgent

bug

rendering

labels 2019-11-01 17:39:14 +00:00

sloum commented

2019-11-01 17:50:22 +00:00

Thinking about this a little bit more, this seems like an easy candidate for another configuration option. I dont want to go overboard with a billion config options... but since this can end up causing buggy behavior I would like users to have to opt in to it. It could be called something like renderescapecodes (I am regretting having made the configuration options all lowercase without spaces, but it feels too late to shift). It would be a true/false option with a default of false. The documentation for the option would acknowledge that it could result in buggy display/visual artifacts.

If we go with this, the following will need to be updated:

Update man page to add the configuration option
Add the option to defaults.go with a default of false
Add true and false to the SET checker method (cant remember the name) so that it only accepts those values
In page.go update the wrapping method to take in an argument re: whether or not to render escape codes. It has to be passed in, since the page struct is contained within the client struct (which has the config values). Then have it conditionally write or not write rune(27) as needed (right now it ignores it).
Update everywhere that the aforementioned wrapping is called (should all be in client.go I think) to pass the value of the option.

I do not think it would take long to do. If you think this is a good approach, then I can get it done this weekend (unless we think another things needs attention first).

Thinking about this a little bit more, this seems like an easy candidate for another configuration option. I dont want to go overboard with a billion config options... but since this can end up causing buggy behavior I would like users to have to opt in to it. It could be called something like `renderescapecodes` (I am regretting having made the configuration options all lowercase without spaces, but it feels too late to shift). It would be a true/false option with a default of false. The documentation for the option would acknowledge that it could result in buggy display/visual artifacts. If we go with this, the following will need to be updated: - Update man page to add the configuration option - Add the option to `defaults.go` with a default of `false` - Add `true` and `false` to the SET checker method (cant remember the name) so that it only accepts those values - In `page.go` update the wrapping method to take in an argument re: whether or not to render escape codes. It has to be passed in, since the page struct is contained within the client struct (which has the config values). Then have it conditionally write or not write `rune(27)` as needed (right now it ignores it). - Update everywhere that the aforementioned wrapping is called (should all be in `client.go` I think) to pass the value of the option. I do not think it would take long to do. If you think this is a good approach, then I can get it done this weekend (unless we think another things needs attention first).

sloum commented

2019-11-01 17:53:04 +00:00

I did just think of another potential issue:

If an escape code starts near the bottom of the screen and its close is below the bottom (needs to be scrolled to) then the bombadillo information panel at the bottom will be affected by it. I am ok with this, but only as an opt in (I assume only eople that know what to expect from escape codes will notice).

I could also try to write a function that can parse escape codes from the string and do a ton of work to make everything that deals with color work correctly with bookmarks bar and other ui... but i think it would be really difficult to do right.

I did just think of another potential issue: If an escape code starts near the bottom of the screen and its close is below the bottom (needs to be scrolled to) then the bombadillo information panel at the bottom will be affected by it. I am ok with this, but only as an opt in (I assume only eople that know what to expect from escape codes will notice). I could also try to write a function that can parse escape codes from the string and do a ton of work to make everything that deals with color work correctly with bookmarks bar and other ui... but i think it would be really difficult to do right.

asdf commented

2019-11-02 04:35:34 +00:00

Thanks for explaining this all and coming up with so many different approaches for changing Bombadillo to support this.

I completely agree with the non-urgent tag. While there is a quick change available, perhaps this could wait until after 2.0.0 so there's more time to think about the approach.

Are you aware of much history regarding escape sequences on gopher? Is it likely to be a gopher-only thing, or would gemini also allow it?

Thanks for explaining this all and coming up with so many different approaches for changing Bombadillo to support this. I completely agree with the non-urgent tag. While there is a quick change available, perhaps this could wait until after 2.0.0 so there's more time to think about the approach. Are you aware of much history regarding escape sequences on gopher? Is it likely to be a gopher-only thing, or would gemini also allow it?

sloum commented

2019-11-02 05:09:07 +00:00

I think in theory it would be allowable on either, but fairly uncommon on both.

sloum commented

2019-11-03 02:47:26 +00:00

Ok. So I was trying to find a way to optionally parse escape codes. When I enabled them they broke the crap out of the layout. Part of the issue is that they consist of a good number of non-printing characters... but still get counted as part of the line length. Then, when they dont take up that space, things get really misaligned (the bookmarks panel is really bad at that point).

Something I did get working though, is removing them entirely. Rather than just not printing the initial ascii char 27 (escape) I found out how to just not render the whole escape code at all. So at the very least they dont make the output of the client look gross.

I have that on a branch (remove-escapes) if you want to take a look.

I am not sure that this is the best way to go long term, but it would clean up the output for the time being. I would imagine that those using escape codes for color would prefer the codes to not be seen at all whether color is functional or not. What do you think @asdf ?

Ok. So I was trying to find a way to optionally parse escape codes. When I enabled them they broke the crap out of the layout. Part of the issue is that they consist of a good number of non-printing characters... but still get counted as part of the line length. Then, when they dont take up that space, things get really misaligned (the bookmarks panel is really bad at that point). Something I _did_ get working though, is removing them entirely. Rather than just not printing the initial ascii char 27 (escape) I found out how to just not render the whole escape code at all. So at the very least they dont make the output of the client look gross. I have that on a branch (`remove-escapes`) if you want to take a look. I am not sure that this is the best way to go long term, but it would clean up the output for the time being. I would imagine that those using escape codes for color would prefer the codes to not be seen at all whether color is functional or not. What do you think @asdf ?

asdf commented

2019-11-05 11:21:11 +00:00

This is really good as it makes the documents readable. A definite improvement over the current state.

When escape codes are enabled, assuming only colours and formatting, will printing "�33[0m" before printing UI elements like the bookmark bar prevent the colour bleeding?

This is really good as it makes the documents readable. A definite improvement over the current state. When escape codes are enabled, assuming only colours and formatting, will printing "�33[0m" before printing UI elements like the bookmark bar prevent the colour bleeding?

sloum commented

2019-11-05 14:06:48 +00:00

Cool. I also think it improves readability.

We do not write 33[0m before bookmarks currently. However, the bookmarks already start with an escape code to handle dimming or not based on focus. Doing so would fix color issues when bookmarks are open, sort of. They will still break if the text gets wrapped since we would only be allowing color to go for one line then, and the wrapped line would not have color then. The spacing is still a problem. I have some ideas for how to deal with counting the space and doing some math, but it feels like it will add a lot of instability to the Draw receiver.

If you want to move forward with the removal of escape codes entirely, let me know and I can open a PR for this.

Cool. I also think it improves readability. We do not write `�33[0m` before bookmarks currently. However, the bookmarks already start with an escape code to handle dimming or not based on focus. Doing so would fix color issues when bookmarks are open, sort of. They will still break if the text gets wrapped since we would only be allowing color to go for one line then, and the wrapped line would not have color then. The spacing is still a problem. I have some ideas for how to deal with counting the space and doing some math, but it feels like it will add a lot of instability to the `Draw` receiver. If you want to move forward with the removal of escape codes entirely, let me know and I can open a PR for this.

asdf commented

2019-11-07 01:47:24 +00:00

I think the removal of escape codes adds readability for now. It could also be the basis for future work in supporting colours/formatting. I'd recommend merging this in.

sloum changed title from ~~Support for phlogs with formatted text?~~ to Add support for formatted text/vt-100 formatting escape codes

2019-12-01 03:42:37 +00:00

sloum commented

2019-12-01 03:43:39 +00:00

I have re-titled this issue. I am glad we found a good place to land for 2.0.0, but I do think we will need to tackle this eventually and just wanted to leave a comment to the effect of this still being an active issue that should get solved in the post 2.0.0 period