Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor&Enhance: reduce some spaghetti from TBuffer class... #2133

Merged
merged 11 commits into from
Jan 2, 2019

Conversation

SlySven
Copy link
Member

@SlySven SlySven commented Dec 11, 2018

...additionally, adds OSC P/R (MUD Server set/reset) of standard ANSI 16-colours.

Refactor code to remove (int) TBuffer::mCode[1024] as fixed int array - although name has been reused as a QString to replace (QString) TBuffer::code.

Eliminate the use of several goto DECODE's and indeed, that label entirely from TBuffer::translateToPlainText(...).

Renamed some variables to better explain their meanings and class relationship:

  • (bool) TBuffer::gotESC --> TBuffer::mGotESC
  • (bool) TBuffer::gotHeader --> TBuffer::mGotCSI

Some class members were being shadowed in some of the TBuffer methods as arguments or were effectively duplicates:

  • (int) TBuffer::fgColorR --\
  • (int) TBuffer::mFgColorR --+--> TBuffer::mCurrentFgColorR
  • (int) TBuffer::fgColorG --\
  • (int) TBuffer::mFgColorG -+--> TBuffer::mCurrentFgColorG
  • (int) TBuffer::fgColorB --\
  • (int) TBuffer::mFgColorB --+--> TBuffer::mCurrentFgColorB
  • (int) TBuffer::fgColorLightR --> TBuffer::mCurrentFgColorLightR
  • (int) TBuffer::fgColorLightG --> TBuffer::mCurrentFgColorLightG
  • (int) TBuffer::fgColorLightB --> TBuffer::mCurrentFgColorLightB
  • (int) TBuffer::bgColorR --> TBuffer::mCurrentBgColorR
  • (int) TBuffer::bgColorG --> TBuffer::mCurrentBgColorG
  • (int) TBuffer::bgColorB --> TBuffer::mCurrentBgColorB

Needed to add a per profile configuration option (on "Special options" tab) to handle cases where a SGR (3|4)8;2;...m code using only semicolon as separator either includes or excludes a Colour Space Id as the third (expected to be empty) parameter. This is not needed when colons are used to sub-divide the parameter string.

Also added a per profile configuration option (on "Colour view" tab) so that the user can control whether the MUD Server is allowed to use the OSC P and R sequences to redefine the basic ANSI 16-colours as mentioned above.

Refactoring of the code enabled the elimination of a number of state booleans that were used to carry forward the state of SGR (3|4)8 codes because the text that contains all SGR codes is now split out from the byte-by-byte processing and decoded separately as a sub-string:

  • (bool) TBuffer::mFgColorCode
  • (bool) TBuffer::mBgColorCode
  • (bool) TBuffer::mWaitingForHighColorCode
  • (bool) TBuffer::mWaitingForMillionsColorCode
  • (bool) TBuffer::mIsHighOrMillionsColorMode
  • (bool) TBuffer::mIsHighOrMillionsColorModeForeground
  • (bool) TBuffer::mIsHighOrMillionsColorModeBackground

Also the unused:

  • (TMxpElement) TBuffer::mCurrentElement
  • (std::string) TBuffer::tempLine
  • (int) TBuffer::currentFgColorProperty

And the factored out:

  • (int) TBuffer::codeRet
  • (QCOlor) TBuffer::mFgColor
  • (QCOlor) TBuffer::mBgColor

As part of deducing the behaviour of ANSI ESC sequences I realised that the MXP decoding is considerably short of handling the full MXP specifications as detailed at: https://www.zuggsoft.com/zmud/mxp.htm .

Also removed some unused members of other classes that I found whilst investigating the (int) TBuffer::mCode[1024] member:

  • (bool) Host::mCodeCompletion
  • (bool) Host::mDisableAutoCompletion
  • (int) Host::mMXPMode
  • (int) TConsole::currentFgColorProperty

Whilst working on the profile preference dialogue I spotted that there was a mis-nested layout (one layout inside another one) involving some of the Discord options on the last tab near to where I added the option to handle the optional inclusion of the Colour Space Identifier in 16M colour codes which only uses the semi-colon as a parameter separator. So I have cleaned up that at the same time...

Signed-off-by: Stephen Lyons slysven@virginmedia.com

...additionally, adds OSC P/R (MUD Server set/reset) of standard ANSI
16-colours.

Refactor code to remove `(int) TBuffer::mCode[1024]` as fixed int array
- although name has been reused as a QString to replace
`(QString) TBuffer::code`.

Eliminate the use of several `goto DECODE'` and indeed, that label entirely
from `TBuffer::translateToPlainText(...)`.

Renamed some variables to better explain their meanings and class
relationship:
* (bool) TBuffer::gotESC ==> TBuffer::mGotESC
* (bool) TBuffer::gotHeader ==> TBuffer::mGotCSI

Some class members were being shadowed in some of the `TBuffer` methods as
arguments or were effectively duplicates:
* (int) TBuffer::fgColorR   ==#
* (int) TBuffer::mFgColorR  ==#==> TBuffer::mCurrentFgColorR
* (int) TBuffer::fgColorG   ==#
* (int) TBuffer::mFgColorG  ==#==> TBuffer::mCurrentFgColorG
* (int) TBuffer::fgColorB   ==#
* (int) TBuffer::mFgColorB  ==#==> TBuffer::mCurrentFgColorB
* (int) TBuffer::fgColorLightR ==> TBuffer::mCurrentFgColorLightR
* (int) TBuffer::fgColorLightG ==> TBuffer::mCurrentFgColorLightG
* (int) TBuffer::fgColorLightB ==> TBuffer::mCurrentFgColorLightB
* (int) TBuffer::bgColorR ==> TBuffer::mCurrentBgColorR
* (int) TBuffer::bgColorG ==> TBuffer::mCurrentBgColorG
* (int) TBuffer::bgColorB ==> TBuffer::mCurrentBgColorB

Needed to add a per profile configuration option (on "Special options" tab)
to handle cases where a SGR (3|4)8;2;...m code using only semicolon as
separator either includes or excludes a Colour Space Id as the third
(expected to be empty) parameter. This is not needed when colons are used
to sub-divide the parameter string.

Also added a per profile configuration option (on "Colour view" tab) so
that the user can control whether the MUD Server is allowed to use the
OSC P and R sequences to redefine the basic ANSI 16-colours as mentioned
above.

Refactoring of the code enabled the elimination of a number of state
booleans that were used to carry forward the state of SGR (3|4)8 codes
because the text that contains all SGR codes is now split out from the
byte-by-byte processing and decoded separately as a sub-string:
* (bool) TBuffer::mFgColorCode
* (bool) TBuffer::mBgColorCode
* (bool) TBuffer::mWaitingForHighColorCode
* (bool) TBuffer::mWaitingForMillionsColorCode
* (bool) TBuffer::mIsHighOrMillionsColorMode
* (bool) TBuffer::mIsHighOrMillionsColorModeForeground
* (bool) TBuffer::mIsHighOrMillionsColorModeBackground

Also the unused:
* (TMxpElement) TBuffer::mCurrentElement
* (std::string) TBuffer::tempLine
* (int) TBuffer::currentFgColorProperty

And the factored out:
* (int) TBuffer::codeRet
* (QCOlor) TBuffer::mFgColor
* (QCOlor) TBuffer::mBgColor

As part of deducing the behaviour of ANSI ESC sequences I realised that
the MXP decoding is considerably short of handling the full MXP
specifications as detailed at: https://www.zuggsoft.com/zmud/mxp.htm .

Also removed some unused members of other classes that I found whilst
investigating the `(int) TBuffer::mCode[1024]` member:
* (bool) Host::mCodeCompletion
* (bool) Host::mDisableAutoCompletion
* (int) Host::mMXPMode
* (int) TConsole::currentFgColorProperty

Whilst working on the profile preference dialogue I spotted that there was
a mis-nested layout (one layout inside another one) involving some of the
Discord options on the last tab near to where I added the option to handle
the optional inclusion of the Colour Space Identifier in 16M colour codes
which only uses the semi-colon as a parameter separator. So I have cleaned
up that at the same time...

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Copy link
Member

@vadi2 vadi2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, very big breakage in Achaea somewhere...

image

src/Host.cpp Outdated
@@ -169,6 +167,8 @@ Host::Host(int port, const QString& hostname, const QString& login, const QStrin
, mHaveMapperScript(false)
, mAutoAmbigousWidthGlyphsSetting(true)
, mWideAmbigousWidthGlyphs(false)
, mSColonOnlySCRColCodesHaveColSpace(false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oy, this is a difficult variable name to understand and work with

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really wanted something shorter and in this case I am open to suggestions!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would mSGRCodeHasColSpaceId be short enough for your tender sensibilities?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😁 how about mSGRColonSeparator?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what? This role - which means: when handling SGR 38 or 48 codes of type 2 (16M RGB colours) or 3 {16M CMY - not used but passed-over and discarded} or 4 {4.3B CMYK - also not used but passed over} has an extra parameter which is in the third position (but which is expected to an default {i.e. empty, no other characters}) and thus needs to be passed over to get to the colour components - but ONLY when they are delimited only with semicolons ; (and not sub-divided with colons :)...

... so calling it mSGRColonSeparator is confusing because I (and more importantly I guess others) might expect that to be used somehow to flag that SGR sequences have been spotted which use the colon as a (sub-)separator.

src/Host.h Outdated Show resolved Hide resolved
src/TBuffer.cpp Outdated Show resolved Hide resolved
@SlySven
Copy link
Member Author

SlySven commented Dec 12, 2018

Can you record that Achaea as a replay so I can debug what went wrong...?

@SlySven
Copy link
Member Author

SlySven commented Dec 12, 2018

Oh, it seems to be a glitch in processing MXP - which does not surprise me that much. 🤷‍♂️

Achaeaa launched straight into spewing out <ELEMENT ...> tags even before it got a response to the <VERSION> and then <SUPPORT> tags - talk about making assumptions... anyhow it looks like this needs more work.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@SlySven
Copy link
Member Author

SlySven commented Dec 12, 2018

Oh silly me - I had reverted the process of the ? in a \x1b[?z sequence so that I could use the existing (very limited) handling of that as a QString (I had been converting it to a number to drop into a switch (?) {...} chunk of code - but in reverting it I hadn't removed the (bool) isOk from the previous QString::toInt(&isOk) and since there was nothing to set that flag to true all the MXP control sequences were being rejected as invalid (the code thought the ? wasn't a number)).

Anyhow the fix was relatively quick and simple so try it again now...

To cut down spammy qDebug() output when it isn't needed.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@vadi2
Copy link
Member

vadi2 commented Dec 12, 2018

Getting better!

image

I don't want to do a replay since it'll log the password at the moment

@SlySven
Copy link
Member Author

SlySven commented Dec 12, 2018

I tried to log in but my previous Achaea character had been deleted due to inactivity. I recreated a character with the same name but I am not getting the registration email to (re)confirm my account.

Anyhow I do not need the record now!

So is it up to scratch?

@vadi2
Copy link
Member

vadi2 commented Dec 12, 2018

No, it's incorrect in adding the underline and italics everywhere

Here's a replay, I realised they do not contain the password anyway: Uploading 12-12-2018#20-17-15.zip…

@vadi2
Copy link
Member

vadi2 commented Dec 12, 2018

Correct output which you can see on 3.15:

image

Thanks for working on this 😃

@SlySven
Copy link
Member Author

SlySven commented Dec 12, 2018

<aside>In terms of logging passwords I really must get around to adding support for the telnet ECHO (1) sub-option - such that we forcibly disabling the "Show text you type" when the Server sends a IAC WILL ECHO (and re-enabling it if wanted when it follows it up with a WONT) - which is the traditional way of do this to hide password data (because it is accepted practice that if the remote end is echoing our data for us we do NOT do it ourselves so that passwords are hidden by the remote {Server} end offering to echo for us and then NOT doing so)...</aside>

@SlySven
Copy link
Member Author

SlySven commented Dec 12, 2018

⚠️ That upload is not working - it is just a link to this same page!

@SlySven
Copy link
Member Author

SlySven commented Dec 12, 2018

I can't reproduce what you are seeing, would you mind un-commenting the following defines at the top of TBuffer.cpp: and posting the debug output when you run that replay:

  • DEBUG_SGR_PROCESSING
  • DEBUG_OSC_PROCESSING
  • DEBUG_MXP_PROCESSING

and/or try to upload that replay again?

FWIIW I can see that things seem to be going wrong after the "Your moss tattoo tingles slightly." line.

@vadi2
Copy link
Member

vadi2 commented Dec 13, 2018

12-12-2018#20-17-15.zip

... where I copied and pasted more than I should have done.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
This was previously really messing SGR sequence handling up as the colour
index (the third number) in a SGR(3|4)8;5;#m sequence was being interpreted
as a SGR#m code which was most commonly showing up as odd lapses into using
italics and underlines when not expected.

I have also commented out some SGR#m `case #:`s in some `switch () {}`
that did not have any implementations so that they will be reported as
being un-handled rather than being silently dropped even when
`DEBUG_SGR_PROCESSING` is defined during debug builds.  This is so their
use can be spotted in the debug output in the future...

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@SlySven
Copy link
Member Author

SlySven commented Dec 14, 2018

Okay, that recording was a great help in that it enabled me to find an off-by-one error when skipping through SGR(3|4)8;5;??;m cases such that the third parameter (the ?? in there) was not being passed over at the end of processing that set of three parameters and it was being reinterpreted as a SGR???m code as well - so that SGR38;5;1m {dark red} was also being interpreted as turn on bold; SGR38;5;3m {dark yellow} as turn on italics. and SGR38;5;4m {dark blue} as turn on underline...! 😮

It now appears thus (I have tweaked my colours a tiny bit to make them a little more visible:)
screenshot_20181214_010102

src/TBuffer.cpp Outdated Show resolved Hide resolved
When wrangling one of the previous commits it seems I mislaid the line of
code to save the setting about whether to allow the MUD Server to redefine
and/or reset the ANSI 16-colour set with <OSC>P<i><rr><gg><bb><ST> and
<OSC>R<ST> byte sequences!

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@@ -391,6 +391,7 @@ void XMLexport::writeHost(Host* pHost, pugi::xml_node mudletPackage)
host.append_attribute("mRequiredDiscordUserName") = pHost->mRequiredDiscordUserName.toUtf8().constData();
host.append_attribute("mRequiredDiscordUserDiscriminator") = pHost->mRequiredDiscordUserDiscriminator.toUtf8().constData();
host.append_attribute("SemiColonOnlySGRColorCodesHaveColorSpaceId") = pHost->getHaveColorSpaceId() ? "yes" : "no";
host.append_attribute("ServerMayRedefineColors") = pHost->getMayRedefineColors() ? "yes" : "no";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we still do the m prefix tradition? 🙏

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is not helpful IMHO - that string is what appears in the XML file so should I think be comprehensible outside of the application itself - sadly past copying of the variable name even when it is mNOT_THAT_GOOD {er, mUSE_FORCE_LF_AFTER_PROMPT} does not seem helpful to me and it is difficult to clean up past cases. Better I think to have a long, but readable text - unless it is an element that appears very frequently - so that it documents itself (as putting explanatory comments in there is a whole different matter that in a C header file)... 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well - all the rest are - and it's not a practical problem we're facing...

@vadi2
Copy link
Member

vadi2 commented Dec 14, 2018

A bug somewhere that you can see in the image table

image

14-12-2018#08-59-13.zip

@SlySven
Copy link
Member Author

SlySven commented Dec 14, 2018

Ah, that appears to be a packet split issue - at the point of the breakage there is 9201 bytes in the TBuffer::localBuffer - which starts with Available XTerm256 Colours:... and ends with \x1b[38;5;1 all those colour codes takes a lot of bytes and I guess the data got split between two packets. BTW each coloured rectangle is actually printed with %%% but the foreground and background colours are the same - so all that got lost/misinterpreted was the foreground colour code setting for the 199 block. I'll take another look at what happens when there is not enough bytes to complete an SGR code...

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
…tion

Was missing code to do this after a colour was redefined (although
resetting them did cause an update!).

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@SlySven
Copy link
Member Author

SlySven commented Dec 15, 2018

Okay - that seems to work better now, and I also fixed another glitch in that after using a <OSC>Pirrggbb<ST> code the main TConsole instance was not being updated to use the new colour going forward.

I have a supplementary commit which (I will leace for a subsequent, separate, PR and it) will raise an event for the lua sub-system so that user scripts/packages can be informed that the default colours have been changed (by the Server or via the user adjusting the particular colour controls in the preferences) and they may want to copy the changes to sub-consoles and user windows - although it is not quite clear whether redefining the ANSI colours will actually impact upon those (it is not obvious to me whether the details of those colours are available to user scripts/packages anyway)...

@vadi2
Copy link
Member

vadi2 commented Dec 15, 2018

image

15-12-2018#06-50-20.zip

Still seems to occur in this scenario

@SlySven
Copy link
Member Author

SlySven commented Dec 15, 2018

Ah, I was storing the incomplete (SGR code) bytes correctly but failing to prepend them to the next packet because the code to do that was conditional on a multi-byte encoding being used. The fix for this does remove some code put in since this PR was started by #2134 so there will be a merge clash which I will leave a comment for in the next commit so that it can be resolved here afterwards before the merge will be allowable.

…codes

Also makes changes already merged into development branch code by
Mudlet#2134
although that will cause a clash - so also leaves a comment about how to
resolve that.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@SlySven
Copy link
Member Author

SlySven commented Dec 15, 2018

After resolving that merge clash it seems okay now:
screenshot_20181215_170621

Do any of the I.R.E. Muds have 16M colour testers? 😀

@vadi2
Copy link
Member

vadi2 commented Dec 17, 2018

image

17-12-2018#07-03-07.zip

Sorry, still a problem...

IRE muds don't use 16m colours, no.

@vadi2
Copy link
Member

vadi2 commented Dec 17, 2018

@SlySven
Copy link
Member Author

SlySven commented Dec 17, 2018

Strange, that replay worked okay for me:
screenshot_20181217_204309

Just in case it is a factor - what Server encoding are you using?

@vadi2
Copy link
Member

vadi2 commented Dec 18, 2018

ASCII encoding. That replay is working for me as well when I try playing it a few times before logging in - but when I login and check colors, first thing that happened was:

image

It seems it is depending upon the state of the parsing somehow?

18-12-2018#07-46-54.zip

@vadi2 vadi2 assigned SlySven and unassigned vadi2 Dec 20, 2018
@SlySven
Copy link
Member Author

SlySven commented Dec 30, 2018

I could NOT reproduce that bug you report, either with that save file or after logging into Achaea and using the colours command to generate the same test text.

That was with these options checked (:negative_squared_cross_mark:) :

  • Enable GMCP
  • Allow server to install script packages
  • Fix unnecessary linebreaks on GA servers

and with these options unchecked (:black_square_button:) :

  • Enable MSDP
  • Strict UNIX line endings
  • Force compression off
  • Force telnet GA signal interpretation off
  • Force MXP negotiation off

additionally these settings were:

  • Server data encoding ASCII
  • Wrap lines at: 100 characters
  • Indent wrapped lines by 0 characters

Is there any variations between these setting and your setup? FTR I am currently running on a Debian Linux OS.

Copy link
Member

@vadi2 vadi2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't get it anymore either - strange. Sorry about taking the time on this.

Could you just name the xml attributes in consistency with the rest?

Ideally an XML file should be human readable as well as machine readable.
So the names of attributes and elements would, in the best case, make
sense on their own; we have not really gone along with that practice in the
past and instead have used the name of the internal variable which does
mean that some of the details in the XML files are not, perhaps as
comprehensible to those not intimately familiar with the C++ core of the
application. This being the case I hereby convert the names of the
attributes needed for the settings introduced as part of this PR from a
long but descriptive camel case text to the internal variable name - as I
was instructed to! 8-P

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@SlySven
Copy link
Member Author

SlySven commented Jan 2, 2019

Okay, have done that.

@SlySven SlySven merged commit 67909e9 into Mudlet:development Jan 2, 2019
@SlySven SlySven deleted the Bugfix_16M_SGRCodes branch January 2, 2019 07:38
SlySven added a commit to SlySven/Mudlet that referenced this pull request Jan 12, 2019
In Mudlet#2133 I deliberately left some code
untouched so that the diffs were a bit more comprehensible - this was in
the region of the code that identified the CSI sequences associated with
setting the MXP mode.

This commit cleans up and lays out the code correctly to match the
surrounding indentation (and adds some related MXP mode comments), which
might be a preliminary for a solution to:
Mudlet#1623

It should not change the functionality at all.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
SlySven added a commit that referenced this pull request Jan 14, 2019
In #2133 I deliberately left some code
untouched so that the diffs were a bit more comprehensible - this was in
the region of the code that identified the CSI sequences associated with
setting the MXP mode.

This commit cleans up and lays out the code correctly to match the
surrounding indentation (and adds some related MXP mode comments), which
might be a preliminary for a solution to:
#1623

It should not change the functionality at all.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@SlySven SlySven restored the Bugfix_16M_SGRCodes branch June 22, 2020 18:04
@SlySven SlySven deleted the Bugfix_16M_SGRCodes branch June 22, 2020 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants