Adds gmcp support #24
Conversation
LiquidityC
force-pushed
the
add_gmcp_support
branch
3 times, most recently
from
47c0676
to
7278b90
Compare
|
Thanks for the contribution @LiquidityC! The code itself looks solid, although I haven't had a chance to test it myself (I will prioritize that as soon as I get a chance). As we add features, I am curious how everyone feels about using isolated header files (in this case using a |
|
I agree that smaller header files are generally a better idea. It's a balancing act around too many includes and circular includes. |
| gmcp = FALSE; | ||
| dsock->next_command[j] = '\0'; | ||
| gmcpReceived(dsock); | ||
| dsock->next_command[j = 0] = '\0'; |
There was a problem hiding this comment.
Might be. But I think it's one of those, "once you extend it you can clean it up a bit" situations. :D
Didn't want to overthink things now since I don't know how it will be used in the future.
There was a problem hiding this comment.
I found funny doing the assignment inside array reference, never saw it before.
There was a problem hiding this comment.
Never used it before. But I felt it was nicer to keep the line count low here.
There was a problem hiding this comment.
@rogersm Wow, great eye! Definitely a nuance I didn't notice, but I'm okay with the convention here.
|
Want me to separate the gmcp stuff from mud.h to gmcp.h? |
|
@zachflower only for the 3 gmcp functions? I would say keep them in mud.h for now. |
|
@rogersm I'm fine with that for now, we could always separate things out in a separate refactor in the future if we were so inclined. |
src/socket.c
Outdated
| @@ -33,6 +33,7 @@ const unsigned char compress_will [] = { IAC, WILL, TELOPT_COMPRESS, '\0' }; | |||
| const unsigned char compress_will2 [] = { IAC, WILL, TELOPT_COMPRESS2, '\0' }; | |||
| const unsigned char do_echo [] = { IAC, WONT, TELOPT_ECHO, '\0' }; | |||
| const unsigned char dont_echo [] = { IAC, WILL, TELOPT_ECHO, '\0' }; | |||
| const unsigned char gmcp_will [] = { IAC, WILL, TELOPT_GMCP, SE, '\0' }; | |||
There was a problem hiding this comment.
It looks like the SE is unnecessary within the negotiation and causes the client (in my test case Mudlet) to try and render the line:
Would you update the negotiation to { IAC, WILL, TELOPT_GMCP, '\0' };?
There was a problem hiding this comment.
Definitely, weird that I added it in the first place. Must have made an editing mistake or something when I was cleaning up.
LiquidityC
force-pushed
the
add_gmcp_support
branch
from
7278b90
to
fcec312
Compare
|
I added the changes but the commits are squashed so the fixes are in the same commit. |
|
Looks good, thanks so much for the contribution @LiquidityC! If you haven't already joined us on Slack, please pop by (https://slack.mudcoders.com/) we'd love to have you! |
I added an embryo of GMCP support. It's usable and works. I'd love some comments. And if it's approved I think it's up to someone with a bigger perspective on the project to decide how to use it. Some sort of JSON module would likely help.
Please get back with any improvement suggestions etc. I'm not assuming this will go straight into the devel branch ;)