Make line wrapping a command line argument (since it breaks color) #3

tunas · 2020-03-06T05:56:25Z

tunas commented

2020-03-06 05:56:25 +00:00

First-time contributor

No description provided.

tunas commented

2020-03-06 05:58:05 +00:00

First-time contributor

Hah, now that I look at the diff, it seems that VSCode reformatted the file and changed some comments around. No differences in the content, it's all just removal of #'s and whitespaces. I hope that's okay, if not I can re-push the commit.

lel commented

2020-03-06 20:59:00 +00:00

There are at least two issues here, one of which I really don't know what the best way to fix is:

At line 1038, you check for the wraplines flag with an elif so if you start AV-98 with a starting URL (e.g., by running it via av98 -w gemini.circumlunar.space, the wraplines flag doesn't get used.
Even if you change that to a regular if, starting AV-98 with -w and a starting URL still results in the wraplines flag not being respected until you enter a command (e.g., refresh). This appears to be because all you're doing is appending the wraplines command to the command buffer, and only one of those is read per command line entered, so you have to wait one command. Apparently, AV-98 wasn't written with extensibility of the command line arguments in mind, because this means it can have at maximum two command line arguments. That would I guess explain why there are only two, and why they are mutually exclusive.

So I guess in order to fix both of these, you'd want to change the elif on line 1038 to an if, and you'd either want to directly call do_wraplines by something like gc.do_wraplines("") from there or just not use a cli flag.

Oh, and on an unrelated note, you'll want to add a docstring to the do_wraplines method so that help doesn't show it as undocumented.

All of that being said, I think it's a good idea to make this configurable in some fashion, because I personally still don't feel like I need word-wrapping in a console client (although I get why others like it obviously and it's fine by me if it's the default).

But I also feel like unless solderpunk intentionally did this because he was part of the "no ansi colors on gemini" tribe (I don't remember if he was or wasn't; I personally am pro colors and I think konpeito.media is awesome tbh), but if not, I think it would be worth dragging in a dependency to entirely get rid of the bug you're trying to work around. This means using ansiwrap. It's a drop-in replacement for the built-in textwrap library which properly respects ANSI color codes.

You can test by running pip3 install ansiwrap then changing line 27 from import textwrap to import ansiwrap as textwrap. It should just work.

There are at least two issues here, one of which I really don't know what the best way to fix is: 1. At [line 1038](https://tildegit.org/solderpunk/AV-98/src/commit/5e4f844770dbb006a9e9566add7fc4d6d483d7fa/av98.py#L1038), you check for the `wraplines` flag with an `elif` so if you start `AV-98` with a starting URL (e.g., by running it via `av98 -w gemini.circumlunar.space`, the `wraplines` flag doesn't get used. 2. Even if you change that to a regular if, starting `AV-98` with `-w` and a starting URL still results in the `wraplines` flag not being respected until you enter a command (e.g., `refresh`). This appears to be because all you're doing is appending the `wraplines` command to the command buffer, and only one of those is read per command line entered, so you have to wait one command. Apparently, `AV-98` wasn't written with extensibility of the command line arguments in mind, because this means it can have at maximum two command line arguments. That would I guess explain why there are only two, and why [they are mutually exclusive](https://tildegit.org/solderpunk/AV-98/src/branch/master/av98.py#L1020). So I guess in order to fix both of these, you'd want to change the `elif` on [line 1038](https://tildegit.org/solderpunk/AV-98/src/commit/5e4f844770dbb006a9e9566add7fc4d6d483d7fa/av98.py#L1038) to an `if`, and you'd either want to directly call `do_wraplines` by something like `gc.do_wraplines("")` from there or just not use a cli flag. Oh, and on an unrelated note, you'll want to add a docstring to the `do_wraplines` method so that `help` doesn't show it as undocumented. All of that being said, I think it's a good idea to make this configurable in some fashion, because I personally still don't feel like I need word-wrapping in a console client (although I get why others like it obviously and it's fine by me if it's the default). But I also feel like unless solderpunk intentionally did this because he was part of the "no ansi colors on gemini" tribe (I don't remember if he was or wasn't; I personally am pro colors and I think [konpeito.media](gemini://konpeito.media/) is awesome tbh), but if not, I think it would be worth dragging in a dependency to entirely get rid of the bug you're trying to work around. This means using [ansiwrap](https://pypi.org/project/ansiwrap/). It's a drop-in replacement for the built-in `textwrap` library which properly respects ANSI color codes. You can test by running `pip3 install ansiwrap` then changing [line 27](https://tildegit.org/solderpunk/AV-98/src/commit/5e4f844770dbb006a9e9566add7fc4d6d483d7fa/av98.py#L27) from `import textwrap` to `import ansiwrap as textwrap`. It should just work.

tunas commented

2020-03-06 23:33:02 +00:00

First-time contributor

I didn't really know there was a debate about ANSI color on gemini. I was thinking that adding a command line argument would be easier than rewriting the wrapping code, but I do agree that if things are done, they ought to be done properly.
Besides, if it really is as easy as substituting textwrap for ansiwrap, then there's probably no reason not to.

I didn't really know there was a debate about ANSI color on gemini. I was thinking that adding a command line argument would be easier than rewriting the wrapping code, but I do agree that if things are done, they ought to be done properly. Besides, if it really is as easy as substituting textwrap for ansiwrap, then there's probably no reason not to.

lel commented

2020-03-07 01:51:57 +00:00

Yeah color/escape codes have been fought over, and like half of the bigger names on the mailing list are in favor and like half are against, although looking back through the archives it's actually been a while since anyone talked about it, and looking back through, this is the most recent thing I've seen solderpunk say about it, and it seems pretty middle-of-the-road.

I can only assume that solderpunk intentionally didn't use something like ansiwrap b/c he didn't want external dependencies. Thinking back on it, someone at some point in the debate over reflow/soft/hard-wrapping definitely made the argument that wrapping libraries in most languages are pretty bad at handling ansi escape codes, and the fact that external libraries may be required for properly handling wrapping was one of the arguments that seemed like it was going to win the fight for the hard-wrap side because external libraries were considered so problematic.

I definitely think that making wrapping optional is really useful, so something like your PR is cool and good and important imo (assuming the bug[s] in it are fixed) but I would be very surprised if the idea of pulling in ansiwrap to enhance the functionality of what was essentially intended to be a demo client that already pushes over a thousand lines is likely to be well-received.

Even if that in particular weren't integrated, it's so trivial for the user to implement on their end that I feel like it isn't that big of a deal.

Yeah color/escape codes have been [fought](https://lists.orbitalfox.eu/archives/gemini/2019/000271.html) [over](https://lists.orbitalfox.eu/archives/gemini/2020/000390.html), and like half of the bigger names on the mailing list are in favor and like half are against, although looking back through the archives it's actually been a while since anyone talked about it, and looking back through, [this](https://lists.orbitalfox.eu/archives/gemini/2020/000373.html) is the most recent thing I've seen solderpunk say about it, and it seems pretty middle-of-the-road. I can only assume that solderpunk intentionally didn't use something like `ansiwrap` b/c he didn't want external dependencies. Thinking back on it, someone at some point in the debate over reflow/soft/hard-wrapping definitely made the argument that wrapping libraries in most languages are pretty bad at handling ansi escape codes, and the fact that external libraries may be required for properly handling wrapping was one of the arguments that seemed like it was going to win the fight for the hard-wrap side because external libraries were considered so problematic. I definitely think that making wrapping optional is really useful, so something like your PR is cool and good and important imo (assuming the bug[s] in it are fixed) but I would be very surprised if the idea of pulling in `ansiwrap` to enhance the functionality of what was essentially intended to be a demo client that already pushes over a thousand lines is likely to be well-received. Even if that in particular weren't integrated, it's so trivial for the user to implement on their end that I feel like it isn't that big of a deal.

solderpunk commented

2020-03-08 14:30:32 +00:00

Hey, thanks for bringing this to my attention!

To be honest, I'm kind of torn on the colour code thing. I like Konpeito as much as anybody else (possibly more!), and in general I don't want to do anything that seems like treading on people's creativity. But some of the arguments for disallowing them do make sense...anyway, certainly the current breakage is not deliberate because I'm staunchly opposed to colours, I just didn't notice this problem when I introduced wrapping.

AV-98 is not really a minimalist demo client (see gemini-demo-1 -2 and -3 for that kind of thing), it's more of a "flagship client" and I'm not totally opposed to bringing in dependencies (probably I'll need to do it eventually anyway to work around the Python stdlib's pretty woeful TLS handling abilities). So, ansiwrap is definitely on the table as a solution to this.

In fact, right now either ansiwrap or some home-spun implementation of some of its logic, would be my preferred fix for this. Right now, the philosophy of having the client wrap long lines to fit its window size or the user's preference has become pretty deeply baked into the Gemini spec. Right now most of the content out here is hard-wrapped, but this will presumably begin to shift and moving forward a terminal client which doesn't do wrapping of its own is going to offer a pretty poor user experience. So it's probably better to fix this some other way.

I just yesterday added support for the preformatted text syntax (```), so it's possible (even likely) that if cat encloses the Koneptio banner within triple backticks then this will just go away. I'll let him know about that...

Hey, thanks for bringing this to my attention! To be honest, I'm kind of torn on the colour code thing. I like Konpeito as much as anybody else (possibly more!), and in general I don't want to do anything that seems like treading on people's creativity. But some of the arguments for disallowing them do make sense...anyway, certainly the current breakage is not deliberate because I'm staunchly opposed to colours, I just didn't notice this problem when I introduced wrapping. AV-98 is not really a minimalist demo client (see gemini-demo-1 -2 and -3 for that kind of thing), it's more of a "flagship client" and I'm not totally opposed to bringing in dependencies (probably I'll need to do it eventually anyway to work around the Python stdlib's pretty woeful TLS handling abilities). So, `ansiwrap` is definitely on the table as a solution to this. In fact, right now either `ansiwrap` or some home-spun implementation of some of its logic, would be my preferred fix for this. Right now, the philosophy of having the client wrap long lines to fit its window size or the user's preference has become pretty deeply baked into the Gemini spec. Right now most of the content out here is hard-wrapped, but this will presumably begin to shift and moving forward a terminal client which doesn't do wrapping of its own is going to offer a pretty poor user experience. So it's probably better to fix this some other way. I just yesterday added support for the preformatted text syntax (```), so it's possible (even likely) that if cat encloses the Koneptio banner within triple backticks then this will just go away. I'll let him know about that...

This pull request has changes conflicting with the target branch.