Add support for telnet #36

sloum · 2019-09-10T15:24:31Z

sloum commented

2019-09-10 15:24:31 +00:00

Some of this is admitedly a little hacky. Due to some earlier design decisions (having the gopher module return a view rather than raw text) I did what would work. I have a branch for a V2 of Bombadillo in the works that keeps a lot of the same methods but drastically restructures the code and simplifies the screen drawing a lot. Until then, I thought this might be cool so that people could visit telnet links.

Let me know if you think anything should be changed.

Some of this is admitedly a little hacky. Due to some earlier design decisions (having the gopher module return a view rather than raw text) I did what would work. I have a branch for a V2 of Bombadillo in the works that keeps a lot of the same methods but drastically restructures the code and simplifies the screen drawing a lot. Until then, I thought this might be cool so that people could visit telnet links. Let me know if you think anything should be changed.

asdf was assigned by sloum

2019-09-10 15:24:31 +00:00

sloum added the

enhancement

label 2019-09-10 15:24:31 +00:00

asdf commented

2019-09-11 09:58:30 +00:00

This is pretty cool! This was a good prompt to try out telnet from gopher, and found gopher://telnetgames.de:70/1/ and played tetris which was actually really impressive.

Transitioning over to the telnet client is pretty seamless and adds an extra dimension to the client. A good addition!

I did note the following issues when playing around:

One of the games, Snake Freeplay (item 6) changes text background to red when it quits. Bombadillo's background is then red until you press a key to redraw the screen.
The first game CCCamp Snake Freeplay (item 2) times out when connecting. The escape key CTRL+] does not do anything, and you have to CTRL+C which quits both telnet and bombadillo. I would have thought that only telnet would close but this might be my own misunderstanding.
When disconnecting, the message about the telnet session being terminated is red. I'm not sure this is the right colour for successfully returning to bombadillo.
The way this is implemented is quite different to the way that external web browser is implemented. It probably doesn't need to have that same level of rigour though - isn't telnet the only client most people would use and is similar on each OS? No need for user or OS customisation.

I'll make some reviews on the code as well.

This is pretty cool! This was a good prompt to try out telnet from gopher, and found gopher://telnetgames.de:70/1/ and played tetris which was actually really impressive. Transitioning over to the telnet client is pretty seamless and adds an extra dimension to the client. A good addition! I did note the following issues when playing around: - One of the games, Snake Freeplay (item 6) changes text background to red when it quits. Bombadillo's background is then red until you press a key to redraw the screen. - The first game CCCamp Snake Freeplay (item 2) times out when connecting. The escape key `CTRL+]` does not do anything, and you have to CTRL+C which quits both telnet and bombadillo. I would have thought that only telnet would close but this might be my own misunderstanding. - When disconnecting, the message about the telnet session being terminated is red. I'm not sure this is the right colour for successfully returning to bombadillo. - The way this is implemented is quite different to the way that external web browser is implemented. It probably doesn't need to have that same level of rigour though - isn't `telnet` the only client most people would use and is similar on each OS? No need for user or OS customisation. I'll make some reviews on the code as well.

sloum commented

2019-09-11 16:36:01 +00:00

Swesome! Your testing and bug reports are always so thorough!

I thought I set it to autoredraw the screen after exiting telnet, but maybe I messed it up somehow. I'll try to fix that.
The control C thing is because it is running as a subprocess. I may be able to work it so the signal only closes the subprocess. I'll look into it. It is weird that ctl-j didnt work though.
Yeah, this is due to lack of good planning for how different modules would work. V2 of bombadillo (I have a branch you can sneak a peak at, but it doesnt run currently so you'd just get to see how the code is slowly getting reworked) will have completely separate modules for each protocol and will make it easy to follow links from any protocol to any other protocol and provide easy and correct messaging. For the time being, I can probably just stick an escape sequence for blue rather than red for the non-error exit (so there will still be the red, but the blue will come after and take precedence). Working on V1 of this project has taught me a lot of what I do not want to do next time, lol.
I think for the time being it is fine as is. In the long run I would like a user to be able to provide a string with a command to run for telnet (defaulting to telnet). That can be one of the configurable options. That way we dont have to sniff out the os, they can just provide the necessary info.

Swesome! Your testing and bug reports are always so thorough! - I thought I set it to autoredraw the screen after exiting telnet, but maybe I messed it up somehow. I'll try to fix that. - The control C thing is because it is running as a subprocess. I may be able to work it so the signal only closes the subprocess. I'll look into it. It is weird that ctl-j didnt work though. - Yeah, this is due to lack of good planning for how different modules would work. V2 of bombadillo (I have a branch you can sneak a peak at, but it doesnt run currently so you'd just get to see how the code is slowly getting reworked) will have completely separate modules for each protocol and will make it easy to follow links from any protocol to any other protocol and provide easy and correct messaging. For the time being, I can probably just stick an escape sequence for blue rather than red for the non-error exit (so there will still be the red, but the blue will come after and take precedence). Working on V1 of this project has taught me a lot of what I do not want to do next time, lol. - I think for the time being it is fine as is. In the long run I would like a user to be able to provide a string with a command to run for telnet (defaulting to `telnet`). That can be one of the configurable options. That way we dont have to sniff out the os, they can just provide the necessary info.

asdf commented

2019-09-12 01:47:01 +00:00

Thanks for the feedback! Glad I could help.

From the code, it looks like screen redrawing is done before the telnet client runs, but not afterwards.

Further to the telnet colour changes affecting bombadillo, I've seen telnet and bombadillo history being left visible on the terminal, above and below the prompt. One key way to reproduce is to go to the games page, launch nethack (item 10), quit nethack (q), then quit bombadillo (q).

There's other weird behaviour, probably from telnet itself, that affects the way the terminal works:

mouse movements entering text to the console
cursor disappearing

If we can redraw, or reset, after telnet is run, maybe it will address these issues. This would probably be the one thing I'd recommend attempting before merging.

One last thing - I noticed using the gopher client that telnet stays open after you disconnect with some telnet-specific messages. You have to press a key to go back to the gopher client. I'm not sure how important these messages are, but it might be an alternative to having bombadillo try to interpret what the telnet client is doing when it exits.

Thanks for the feedback! Glad I could help. From the code, it looks like screen redrawing is done before the telnet client runs, but not afterwards. Further to the telnet colour changes affecting bombadillo, I've seen telnet and bombadillo history being left visible on the terminal, above and below the prompt. One key way to reproduce is to go to the games page, launch nethack (item 10), quit nethack (q), then quit bombadillo (q). There's other weird behaviour, probably from telnet itself, that affects the way the terminal works: - mouse movements entering text to the console - cursor disappearing If we can redraw, or reset, after telnet is run, maybe it will address these issues. This would probably be the one thing I'd recommend attempting before merging. One last thing - I noticed using the gopher client that telnet stays open after you disconnect with some telnet-specific messages. You have to press a key to go back to the gopher client. I'm not sure how important these messages are, but it might be an alternative to having bombadillo try to interpret what the telnet client is doing when it exits.

asdf commented

2019-09-12 02:47:52 +00:00

Oh and documentation! Don't forget documentation!

Maybe just a small update to your Bombadillo page. In a future PR, I plan to take the information from this page and put it in to bombadillo-info.

I'd suggest amending the section Documentation > Gopher Support as follow (addition as marked in bold):

Gopher is the sole protocol supported by Bombadillo at this time. Gopher types are implemented in a 4 tier fashion:

Gophermaps are supported with all classic/standardized features
Text documents are displayed in the client as scrollable documents
Search is provided by a prompt, and usually returns a gophermap
HTTP, HTTPS and telnet protocols can be display via an external viewer
All other types (images, binaries, etc) are implemented as downloads

Oh and documentation! Don't forget documentation! Maybe just a small update to your Bombadillo page. In a future PR, I plan to take the information from this page and put it in to `bombadillo-info`. I'd suggest amending the section Documentation > Gopher Support as follow (addition as marked in bold): Gopher is the sole protocol supported by Bombadillo at this time. Gopher types are implemented in a 4 tier fashion: 1. Gophermaps are supported with all classic/standardized features 1. Text documents are displayed in the client as scrollable documents 1. Search is provided by a prompt, and usually returns a gophermap 1. <b>HTTP, HTTPS and telnet protocols can be display via an external viewer</b> 1. All other types (images, binaries, etc) are implemented as downloads

sloum commented

2019-09-13 04:22:18 +00:00

Interestingly, I think the issues you were having with redraw and screen artifacts may be platform dependent. Those issues do not happen for me at all. I launched nethack, the screen went black and it started connecting in the top left. It did its thing, I hit q for quit and it exited back to bombadillo exactly as it was width the added messaging for having exited the telnet session.

What system are you running it on? I am on Linux x86_64 using - nope, scratch all that. It all looked right in bombadillo, but it was not. Upon quitting bombadillo my whole terminal was donked up. Telnet must reassign a bunch of tty params that bombadillo is already setting. As such, I suppose I'll have to reset things after exiting a telnet session. I had not thought at all about telnet hijacking the terminal to that degree. I'll work on a fix to that.

Thanks for the doc updates too. I am still trying to find a viable way to distribute and install a man page with bombadillo. Maybe a makefile? I've been thinking that for version two I'd like to put things together as a snap package (mostly for the low barrier to entry. apt, yum, etc would be preferable, but can be tricky to set up.... snappy could be ok though).

Interestingly, I think the issues you were having with redraw and screen artifacts may be platform dependent. Those issues do not happen for me at all. I launched nethack, the screen went black and it started connecting in the top left. It did its thing, I hit q for quit and it exited back to bombadillo exactly as it was width the added messaging for having exited the telnet session. What system are you running it on? I am on Linux x86_64 using - nope, scratch all that. It all looked right in bombadillo, but it was not. Upon quitting bombadillo my whole terminal was donked up. Telnet must reassign a bunch of tty params that bombadillo is already setting. As such, I suppose I'll have to reset things after exiting a telnet session. I had not thought at all about telnet hijacking the terminal to that degree. I'll work on a fix to that. Thanks for the doc updates too. I am still trying to find a viable way to distribute and install a man page with bombadillo. Maybe a makefile? I've been thinking that for version two I'd like to put things together as a snap package (mostly for the low barrier to entry. apt, yum, etc would be preferable, but can be tricky to set up.... snappy could be ok though).

sloum commented

2019-10-23 05:17:19 +00:00

I'm going to close this. We will not really be going into master directly anymore and develop is for 2.0.0. This feature just did not make it into the last pre 2.0.0 version in time. No worries.

sloum closed this pull request

2019-10-23 05:17:19 +00:00